Hi Christoph, Hi Marcel, Hi Ron, Hi All,

On Mon, Nov 29, 2021 at 12:38 PM Thiede, Christoph <
Christoph.Thiede at student.hpi.uni-potsdam.de> wrote:

> Hi Marcel,
>
>
> > We might want to have this for all kinds of models.
>
>
> Sorry, but I'm not convinced. :-) The pattern is cool and very flexible,
> but as a downside, boilerplate code increases while readability and code
> exploration become harder - for instance, we do not have the proper tooling
> to find all "assignments" to a property or to enumerate all properties in a
> class. And implementing this tooling does not look straightforward to me
> because programmers could easily use a complex expression (self
> setProperty: self fileDirectorySelector toValue: ...) rather than a literal
> symbol for a property key so that a simple string/AST search would be
> insufficient - the hurdle for programmers is lower in this case than for
> instance variables, where they would be required to explicitly use the
> metaprogramming construct #instVarNamed:. Also, it becomes even harder to
> achieve encapsulation of an object's state.
>
>
> I very much agree with Kent Beck's arguments about this pattern:
>
>
> > Warning! This is not a pattern you should use often, if ever. It is
> here because you will see code written with it, and you will have to
> understand what is going on
>
>
> > The problem with code like this is that you can’t read it and
> understand what is going on. The programming tools don’t support this style
> of programming well. There is no equivalent of “browse instance variable
> references” for code that stores all its state in Dictionaries. It is
> certainly legitimate to build models where state is sometimes present and
> sometimes not, even in instances of the same class. You wouldn’t want to
> declare a hundred variables almost all of which are nil in almost all
> instances, just because a few instances will need them.
>

Twe thing is, he's only half right.  One *can* have support in the tools.
In fact, in Tweak, part of Andreas Raab's Croquet kernel, CObject and
TObject both define myProperties to hold instance state added in
sumclasses.  Consequently we have added support to the tools for finding
references to state via messages.

Now this raises an interesting point.  Right now there is somewhat of a
confusion between policy and mechanism in the searching the tools do.  If
we resolve this confusion I think we can improve our tools significantly.
Let me give an example.

Right now we search for inst var reads and writes by using wrappers around
CompiledCode's readsField: and writesField: methods respectively.  This is
appropriate where inst vars are implemented as normal Smalltalk nst vars,
but it means that when one searches for inst var references the tools
*don't*
- reveal mentions of a given inst var in a class comment
- reveal mentions of an inst var in class side methods that might
introspect using the inst var name

If SystemNavigation were to separate the mechanism
(CompiledCode>>readsField:/writesField:) from the policy ("show me all
references to this inst var within this class hierarchy; and I don't just
mean references in code, I also mean references in class and method
comments, and anywhere else you can think to look"), then we could do a
better job of helping a programmer find all effective references, while
still preserving the useful concrete semantics of methods such as
readsField: & writesField:, and we could also easily accomodate useful
patterns such as "state in property dictionaries".  Indeed a class could
provide class-sode metadata to inform a tool such as SystemNavigation of
the representational choices it makes, etc.

To allow the tools to display the results of results that intermix
references in code with references in text we need meta handle objects that
allow MethodReference (the standard meta handle object for a method) to
live alongside, say, ClassCommentReference, and SharedPoolReference, etc.

Is this compelling or not?

> So I think models that really need it should implement a variable state
> themselves, but we should not explicitly promote this
> (sometimes anti-)pattern. Hypothetically, provide a trait (TVariableState?)
> for it, maybe. I'm not even sure whether it was a good idea to introduce
> this on Morph. As a general solution for "complex optional state", I would
> myself prefer classical composition via a nullable variable for a
> MyOptionalState instance.
>
>
> > Why do you see this as a "crazy hack"? It's rather interesting to have
> a tool that is able to reflect on its internals the same way as it does on
> external (user specified) information. ;-) Feels like a the textual version
> of a halo for a command line (or text buffer).
>
> I see that point for self-inspection and I am not against such a hook in
> general. But code evaluated in a workspace should just work like outside a
> workspace (with the only exception of automatic variables), so to address
> the name collision issue, you could at least use a prefix that is not
> allowed in Smalltalk variable names ($#, $ , $:, whatever). Still, IMO one
> variable should serve one purpose, which is why I consider this a crazy
> hack - the rule of separation of concerns would tell me to use a separate
> dictionary for properties than for bindings. The only legitimate reason to
> reuse the first dict that I could think of would be storage optimization -
> but I don't think we need this in this case (and if yes, lazy
> initialization of the separate dict might be sufficient, too).
>
> Coming back to the self-inspection "feature": I don't actually see where
> you would need this - in particular, given the very limited set of
> properties that are currently available -, but if we want to pursue this
> idea, from a user perspective, I would consider something like
> "thisWorkspace fileDirectory", "thisWorkspace containingWindow", etc. less
> surprising, but more clear and powerful. Still, we should discuss whether
> it is a good idea to risk possible naming conflicts by this. In this
> example, maybe "thisWorkspace" should be a default entry in the normal
> bindings directory rather than a special syntax feature.
>
> > > I only need to print "_fileDirectory" in a workspace and the menu
> will break.
> > That's a bug. Why should a read operation change the bindings
> dictionary? I did not know that. Should be an easy fix. :-)
>
> I have relied on this behavior a few times to see what parts of a script I
> have evaluated already. But that's arguably not a legitimate use case, so
> thank you for the fix! :-)
>
> Best,
> Christoph
>
> ------------------------------
> *Von:* Squeak-dev <squeak-dev-bounces at lists.squeakfoundation.org> im
> Auftrag von Taeumel, Marcel
> *Gesendet:* Montag, 29. November 2021 17:41:44
> *An:* squeak-dev
> *Betreff:* Re: [squeak-dev] The Trunk: Tools-mt.1074.mcz
>
> Hi Christoph --
>
> Given that I needed to mimic the flexibility of morph's properties for the
> sake of robustness, we can add an extra "properties" instVar if necessary.
> Actually adding instVars for a non-essential extra in the Workspace tool
> felt overkill. We might want to have this for all kinds of models. Would
> make extensions easier. So, it's not a "new thing" per se, but an idea
> present in morphs applied to models.
>
> Why do you see this as a "crazy hack"? It's rather interesting to have a
> tool that is able to reflect on its internals the same way as it does on
> external (user specified) information. ;-) Feels like a the textual version
> of a halo for a command line (or text buffer).
>
> If you think that the risk of a name collision in bindings is too high, we
> can choose a different prefix or add an extra dictionary instVar. But then
> we would loose that cool self-reflection feature. :-O
>
> > I only need to print "_fileDirectory" in a workspace and the menu will
> break.
>
> That's a bug. Why should a read operation change the bindings dictionary?
> I did not know that. Should be an easy fix. :-)
>
> Best,
> Marcel
>
> Am 27.11.2021 21:46:44 schrieb Thiede, Christoph <
> christoph.thiede at student.hpi.uni-potsdam.de>:
>
> Hi Marcel,
>
>
> crazy hack, but it bites with the fact that storing underscore variables
> in a Workspace is a legal operation. I only need to print "_fileDirectory"
> in a workspace and the menu will break. Could we please just use a separate
> dictionary for the properties? :-)
>
>
> Furthermore, how many different properties can actually be stored in a
> Workspace? So far I only counted 3 different keys - does this really
> justify the use of the Variable State pattern?
>
>
> Best,
>
> Christoph
> ------------------------------
> *Von:* Squeak-dev <squeak-dev-bounces at lists.squeakfoundation.org> im
> Auftrag von commits at source.squeak.org <commits at source.squeak.org>
> *Gesendet:* Donnerstag, 25. November 2021 11:44:04
> *An:* squeak-dev at lists.squeakfoundation.org;
> packages at lists.squeakfoundation.org
> *Betreff:* [squeak-dev] The Trunk: Tools-mt.1074.mcz
>
> Marcel Taeumel uploaded a new version of Tools to project The Trunk:
> http://source.squeak.org/trunk/Tools-mt.1074.mcz
>
> ==================== Summary ====================
>
> Name: Tools-mt.1074
> Author: mt
> Time: 25 November 2021, 11:44:01.944354 am
> UUID: 39954290-b416-f242-a5b0-c7aea0e3c6a6
> Ancestors: Tools-mt.1073
>
> Adds a property protocol to Workspace to avoid having to go through
> #containingWindow to store extra stuff, which does not even work for MVC,
> just for morphs (i.e., SystemWindow, not StandardSystemView). Reuses a
> workspace's "bindings" with each property being prefixed with a "_" to not
> interfere with regular bindings.
>
> Note that such extra properties have already been added to
> #containingWindow in the past.
>
> =============== Diff against Tools-mt.1073 ===============
>
> Item was added:
> + ----- Method: Workspace>>hasProperty: (in category 'binding -
> properties') -----
> + hasProperty: aSymbol
> +
> +        | propertyValue |
> +        propertyValue := self valueOfProperty: aSymbol.
> +        propertyValue ifNil: [^ false].
> +        propertyValue == false ifTrue: [^ false].
> +        ^ true!
>
> Item was changed:
>   ----- Method: Workspace>>initialize (in category 'initialize-release')
> -----
>   initialize
>
>          super initialize.
> +        bindings := Dictionary new.
> -        self initializeBindings.
>          acceptDroppedMorphs := false.
>          mustDeclareVariables := self class declareVariablesAutomatically
> not.
>          environment := Environment current!
>
> Item was removed:
> - ----- Method: Workspace>>initializeBindings (in category 'binding') -----
> - initializeBindings
> -
> -        bindings := Dictionary new!
>
> Item was added:
> + ----- Method: Workspace>>removeProperty: (in category 'binding -
> properties') -----
> + removeProperty: aSymbol
> +
> +        bindings removeKey: ('_', aSymbol) asSymbol ifAbsent: [].!
>
> Item was added:
> + ----- Method: Workspace>>resetBindings (in category 'binding') -----
> + resetBindings
> +        "Remove all bindings that are not prefixed with an $_. See
> #setProperty:toValue:."
> +
> +        bindings keysAndValuesRemove: [:key :value | key first ~= $_]!
>
> Item was added:
> + ----- Method: Workspace>>setProperty:toValue: (in category 'binding -
> properties') -----
> + setProperty: aSymbol toValue: anObject
> +
> +        anObject ifNil: [^ self removeProperty: aSymbol].
> +        bindings at: ('_', aSymbol) asSymbol put: anObject.!
>
> Item was added:
> + ----- Method: Workspace>>valueOfProperty: (in category 'binding -
> properties') -----
> + valueOfProperty: aSymbol
> +
> +        ^ self valueOfProperty: aSymbol ifAbsent: nil!
>
> Item was added:
> + ----- Method: Workspace>>valueOfProperty:ifAbsent: (in category 'binding
> - properties') -----
> + valueOfProperty: aSymbol ifAbsent: aBlock
> +
> +        ^ bindings at: ('_', aSymbol) asSymbol ifAbsent: aBlock!
>
>
>
>

-- 
_,,,^..^,,,_
best, Eliot
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20211130/a9d5e3e1/attachment.html>