[squeak-dev] The Trunk: Tools-mt.1074.mcz
Christoph.Thiede at student.hpi.uni-potsdam.de
Mon Nov 29 20:38:40 UTC 2021
> 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.
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! :-)
Von: Squeak-dev <squeak-dev-bounces at lists.squeakfoundation.org> im Auftrag von Taeumel, Marcel
Gesendet: Montag, 29. November 2021 17:41:44
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. :-)
Am 27.11.2021 21:46:44 schrieb Thiede, Christoph <christoph.thiede at student.hpi.uni-potsdam.de>:
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?
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:
==================== Summary ====================
Time: 25 November 2021, 11:44:01.944354 am
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') -----
+ bindings := Dictionary new.
- self initializeBindings.
acceptDroppedMorphs := false.
mustDeclareVariables := self class declareVariablesAutomatically not.
environment := Environment current!
Item was removed:
- ----- Method: Workspace>>initializeBindings (in category 'binding') -----
- 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') -----
+ "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!
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the Squeak-dev