tim Rowledge uploaded a new version of ToolBuilder-Morphic to project The Trunk: http://source.squeak.org/trunk/ToolBuilder-Morphic-tpr.196.mcz
==================== Summary ====================
Name: ToolBuilder-Morphic-tpr.196 Author: tpr Time: 2 October 2017, 5:09:16.231117 pm UUID: 7036c368-757d-4824-ac4a-52ac133665f3 Ancestors: ToolBuilder-Morphic-nice.195
Rather than testing an argument for morphnicity, how about jaust making a morph do the right thing (ie nothing)?
Not entirely clear that the 'problem' can ever really arise, but I can't see any way to prove either case.
=============== Diff against ToolBuilder-Morphic-nice.195 ===============
Item was added: + ----- Method: Morph>>buildWith: (in category '*ToolBuilder-Morphic-opening') ----- + buildWith: aToolBuilder + "A Morph is already built, so simply return myself" + ^self!
Item was changed: ----- Method: MorphicToolBuilder>>open: (in category 'opening') ----- open: anObject "Build and open the object. Answer the widget opened." + + ^ (self build: anObject) morphicOpenWith: self! - | morph | - morph := anObject isMorph ifTrue: [anObject] ifFalse: [self build: anObject]. - ^ morph morphicOpenWith: self!
#buildWith: is a method for just ToolBuilders. The clarity of the decision of whether to build or not was articulated by the code of the class responsible for it. A dispatch seems unnecessary here. In fact, introducing Morph>>#buildWith: adds yet another muddle-method to its already-large API, and now inherited by the entire hierarchy. Will it invite people to start overriding it in subclasses as an alternative way to use ToolBuilder? Maybe. I think they'll at least be confused about why it's in Morph.
IMO, this is one of those times when a type check was the simplest and best way to go.
On Mon, Oct 2, 2017 at 7:09 PM, commits@source.squeak.org wrote:
tim Rowledge uploaded a new version of ToolBuilder-Morphic to project The Trunk: http://source.squeak.org/trunk/ToolBuilder-Morphic-tpr.196.mcz
==================== Summary ====================
Name: ToolBuilder-Morphic-tpr.196 Author: tpr Time: 2 October 2017, 5:09:16.231117 pm UUID: 7036c368-757d-4824-ac4a-52ac133665f3 Ancestors: ToolBuilder-Morphic-nice.195
Rather than testing an argument for morphnicity, how about jaust making a morph do the right thing (ie nothing)?
Not entirely clear that the 'problem' can ever really arise, but I can't see any way to prove either case.
=============== Diff against ToolBuilder-Morphic-nice.195 ===============
Item was added:
- ----- Method: Morph>>buildWith: (in category '*ToolBuilder-Morphic-opening') -----
- buildWith: aToolBuilder
"A Morph is already built, so simply return myself"^self!Item was changed: ----- Method: MorphicToolBuilder>>open: (in category 'opening') ----- open: anObject "Build and open the object. Answer the widget opened."
^ (self build: anObject) morphicOpenWith: self!
| morph |morph := anObject isMorph ifTrue: [anObject] ifFalse: [self build: anObject].^ morph morphicOpenWith: self!
Well, since Tobias started this refactoring in April 2017 to clear-up MorphicToolBuilder >> #open, this seems a logical next step to also have a #buildWith: in Morph.
Just my two cents. :)
Best, Marcel Am 05.10.2017 04:40:46 schrieb Chris Muller asqueaker@gmail.com: #buildWith: is a method for just ToolBuilders. The clarity of the decision of whether to build or not was articulated by the code of the class responsible for it. A dispatch seems unnecessary here. In fact, introducing Morph>>#buildWith: adds yet another muddle-method to its already-large API, and now inherited by the entire hierarchy. Will it invite people to start overriding it in subclasses as an alternative way to use ToolBuilder? Maybe. I think they'll at least be confused about why it's in Morph.
IMO, this is one of those times when a type check was the simplest and best way to go.
On Mon, Oct 2, 2017 at 7:09 PM, wrote:
tim Rowledge uploaded a new version of ToolBuilder-Morphic to project The Trunk: http://source.squeak.org/trunk/ToolBuilder-Morphic-tpr.196.mcz
==================== Summary ====================
Name: ToolBuilder-Morphic-tpr.196 Author: tpr Time: 2 October 2017, 5:09:16.231117 pm UUID: 7036c368-757d-4824-ac4a-52ac133665f3 Ancestors: ToolBuilder-Morphic-nice.195
Rather than testing an argument for morphnicity, how about jaust making a morph do the right thing (ie nothing)?
Not entirely clear that the 'problem' can ever really arise, but I can't see any way to prove either case.
=============== Diff against ToolBuilder-Morphic-nice.195 ===============
Item was added:
- ----- Method: Morph>>buildWith: (in category '*ToolBuilder-Morphic-opening') -----
- buildWith: aToolBuilder
- "A Morph is already built, so simply return myself"
- ^self!
Item was changed: ----- Method: MorphicToolBuilder>>open: (in category 'opening') ----- open: anObject "Build and open the object. Answer the widget opened."
- ^ (self build: anObject) morphicOpenWith: self!
- | morph |
- morph := anObject isMorph ifTrue: [anObject] ifFalse: [self build: anObject].
- ^ morph morphicOpenWith: self!
Note I would rather rename #morphicOpenWith: to #openAsTool or similar because (1) to not confuse with the rather old pattern "morphicOpen..." and "mvcOpen..." and (2) it already is in the scope of Morphic. No need to encode this in the selector, too.
Best, Marcel Am 05.10.2017 08:11:58 schrieb Marcel Taeumel marcel.taeumel@hpi.de: Well, since Tobias started this refactoring in April 2017 to clear-up MorphicToolBuilder >> #open, this seems a logical next step to also have a #buildWith: in Morph.
Just my two cents. :)
Best, Marcel Am 05.10.2017 04:40:46 schrieb Chris Muller asqueaker@gmail.com: #buildWith: is a method for just ToolBuilders. The clarity of the decision of whether to build or not was articulated by the code of the class responsible for it. A dispatch seems unnecessary here. In fact, introducing Morph>>#buildWith: adds yet another muddle-method to its already-large API, and now inherited by the entire hierarchy. Will it invite people to start overriding it in subclasses as an alternative way to use ToolBuilder? Maybe. I think they'll at least be confused about why it's in Morph.
IMO, this is one of those times when a type check was the simplest and best way to go.
On Mon, Oct 2, 2017 at 7:09 PM, wrote:
tim Rowledge uploaded a new version of ToolBuilder-Morphic to project The Trunk: http://source.squeak.org/trunk/ToolBuilder-Morphic-tpr.196.mcz
==================== Summary ====================
Name: ToolBuilder-Morphic-tpr.196 Author: tpr Time: 2 October 2017, 5:09:16.231117 pm UUID: 7036c368-757d-4824-ac4a-52ac133665f3 Ancestors: ToolBuilder-Morphic-nice.195
Rather than testing an argument for morphnicity, how about jaust making a morph do the right thing (ie nothing)?
Not entirely clear that the 'problem' can ever really arise, but I can't see any way to prove either case.
=============== Diff against ToolBuilder-Morphic-nice.195 ===============
Item was added:
- ----- Method: Morph>>buildWith: (in category '*ToolBuilder-Morphic-opening') -----
- buildWith: aToolBuilder
- "A Morph is already built, so simply return myself"
- ^self!
Item was changed: ----- Method: MorphicToolBuilder>>open: (in category 'opening') ----- open: anObject "Build and open the object. Answer the widget opened."
- ^ (self build: anObject) morphicOpenWith: self!
- | morph |
- morph := anObject isMorph ifTrue: [anObject] ifFalse: [self build: anObject].
- ^ morph morphicOpenWith: self!
People will think #buildWith: is a hook method, are you saying it is now? I don't think you are, but as it was even put into the 'ToolBuilder-Morphic-opening' category, people will think it is (!), and I can see that being incredibly confusing for them trying to understand how the system is intended to be programmed.
This shifting of the responsibility for "building" from the ToolBuilder to Morph's requires a great more consideration than "betcha can't prove I broke it...." This was an emotional reaction to a type-check, not a refactoring. What about sending the right clues to people reading the code? What about proper delegation of responsibility? What about good API design?
Please don't take this personally Tim, but it tends to be better to take your frustrations with the code to your peers on the list first (as you sometimes do, thank you) than publish these same-day, reactionary commits.
On Thu, Oct 5, 2017 at 1:11 AM, Marcel Taeumel marcel.taeumel@hpi.de wrote:
Well, since Tobias started this refactoring in April 2017 to clear-up MorphicToolBuilder >> #open, this seems a logical next step to also have a #buildWith: in Morph.
Just my two cents. :)
Best, Marcel
Am 05.10.2017 04:40:46 schrieb Chris Muller asqueaker@gmail.com:
#buildWith: is a method for just ToolBuilders. The clarity of the decision of whether to build or not was articulated by the code of the class responsible for it. A dispatch seems unnecessary here. In fact, introducing Morph>>#buildWith: adds yet another muddle-method to its already-large API, and now inherited by the entire hierarchy. Will it invite people to start overriding it in subclasses as an alternative way to use ToolBuilder? Maybe. I think they'll at least be confused about why it's in Morph.
IMO, this is one of those times when a type check was the simplest and best way to go.
On Mon, Oct 2, 2017 at 7:09 PM, wrote:
tim Rowledge uploaded a new version of ToolBuilder-Morphic to project The Trunk: http://source.squeak.org/trunk/ToolBuilder-Morphic-tpr.196.mcz
==================== Summary ====================
Name: ToolBuilder-Morphic-tpr.196 Author: tpr Time: 2 October 2017, 5:09:16.231117 pm UUID: 7036c368-757d-4824-ac4a-52ac133665f3 Ancestors: ToolBuilder-Morphic-nice.195
Rather than testing an argument for morphnicity, how about jaust making a morph do the right thing (ie nothing)?
Not entirely clear that the 'problem' can ever really arise, but I can't see any way to prove either case.
=============== Diff against ToolBuilder-Morphic-nice.195 ===============
Item was added:
- ----- Method: Morph>>buildWith: (in category
'*ToolBuilder-Morphic-opening') -----
- buildWith: aToolBuilder
- "A Morph is already built, so simply return myself"
- ^self!
Item was changed: ----- Method: MorphicToolBuilder>>open: (in category 'opening') ----- open: anObject "Build and open the object. Answer the widget opened."
- ^ (self build: anObject) morphicOpenWith: self!
- | morph |
- morph := anObject isMorph ifTrue: [anObject] ifFalse: [self build:
anObject].
- ^ morph morphicOpenWith: self!
The proper answer to this particular use or misuse (depending upon one’s tastes) would be to find out if the problematic case - the toolbuilder ever being asked to open a Morph - could ever actually occur. If it can’t then no problem at all. If it can, it would be best to fix it in the probably poorly designed sender.
Part of the generic aspect of the problem is a tendency to use very generic message names that end up giving far too little information to the reader unless they’re fresh off writing the code. For example ‘builder build: aSpec’ has a clear simple meaning in some sense, at least when you first think about creating the code. But does the genericity imply that this builder can build something other than a spec? I’d suggest that for later readers it might have been better if it were ‘builder buildFromSpec: aSpec’ since that really makes it clear.
It’s hard though; finding good names for things is probably the most difficult part of making good software (as opposed to just whacking stuff together that does the job but really has no value beyond right now).
Specifically on theMorph>buildWith: thing I’d actually prefer to go with at least opening a notification, in the hope we might end up feeling comfortable that it never happens. My guess is that the type test was simply a response to a bug that never got properly cleaned up. Similarly, take a look at SUnitToolBuilder>>#buildPluggableWindow: - now how did we ever end up with it needing a test for ‘children isSymbol’ ? That seems nuts.
tim -- tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Strange Opcodes: ZZZZZZZZZZZZ: enter sleep mode
I think the code reads better with Tim's change, but Chris has a point.
And Morph>>buildWith: seems jarring to me because implementors of #buildWith: are all model things, while I expect Morph to be more of a view, and so it seems out of place. Which it is, since it is really just trying to handle an error condition.
I just deleted Morph>>buildWith: in my working image, and if nothing bad happens for a few days maybe that is a sign we can just get rid of it.
Some of these kinds of error checks were clearly done for purposes of getting Morphic or MVC or ToolBuilder things working in the first place, and may no longer be needed now that the system works and is stable. But keep in mind that failures in these areas are the reason that we still have an emergency evaluator, so a few extra safety checks may not be as horrible as they seem at first glance.
So here's my suggestion: Everyone just delete Morph>>buildWith: from your trunk image, and if nobody crashes, we can all be happy.
Dave
On Thu, Oct 05, 2017 at 04:48:48PM -0700, tim Rowledge wrote:
The proper answer to this particular use or misuse (depending upon one???s tastes) would be to find out if the problematic case - the toolbuilder ever being asked to open a Morph - could ever actually occur. If it can???t then no problem at all. If it can, it would be best to fix it in the probably poorly designed sender.
Part of the generic aspect of the problem is a tendency to use very generic message names that end up giving far too little information to the reader unless they???re fresh off writing the code. For example ???builder build: aSpec??? has a clear simple meaning in some sense, at least when you first think about creating the code. But does the genericity imply that this builder can build something other than a spec? I???d suggest that for later readers it might have been better if it were ???builder buildFromSpec: aSpec??? since that really makes it clear.
It???s hard though; finding good names for things is probably the most difficult part of making good software (as opposed to just whacking stuff together that does the job but really has no value beyond right now).
Specifically on theMorph>buildWith: thing I???d actually prefer to go with at least opening a notification, in the hope we might end up feeling comfortable that it never happens. My guess is that the type test was simply a response to a bug that never got properly cleaned up. Similarly, take a look at SUnitToolBuilder>>#buildPluggableWindow: - now how did we ever end up with it needing a test for ???children isSymbol??? ? That seems nuts.
tim
tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Strange Opcodes: ZZZZZZZZZZZZ: enter sleep mode
Part of the generic aspect of the problem is a tendency to use very generic message names that end up giving far too little information to the reader unless they’re fresh off writing the code. For example ‘builder build: aSpec’ has a clear simple meaning in some sense, at least when you first think about creating the code. But does the genericity imply that this builder can build something other than a spec? I’d suggest that for later readers it might have been better if it were ‘builder buildFromSpec: aSpec’ since that really makes it clear.
I think methods that are the most central, key abstracton of a class or framework are fine candidates for a beautifully terse name and comment.
Specifically on theMorph>buildWith: thing I’d actually prefer to go with at least opening a notification, in the hope we might end up feeling comfortable that it never happens. My guess is that the type test was simply a response to a bug that never got properly cleaned up. Similarly, take a look at SUnitToolBuilder>>#buildPluggableWindow: - now how did we ever end up with it needing a test for ‘children isSymbol’ ? That seems nuts.
It's not nuts. It expands the usage of this or any framework to allow specification of selector names, instead of only objects, and imparts a late-bound, leveraged, power-of-expression to the user.
Symbol is a base type! Maybe consider relaxing your aversion to type-checking for the base types at least. If you ever use #isNil or #ifTrue:, then you must already agree these type checks are sometimes necessary. Numbers, Strings, Arrays and, most certainly, Symbols are also okay to type-check in rare, key places where it provides a lot of leverage.
From there, #isMorph does not seem like a huge leap. Just sayin' because,
you know, there are a lot more of them. ;)
- Chris
On 05-10-2017, at 8:00 PM, Chris Muller asqueaker@gmail.com wrote: [snip] I think methods that are the most central, key abstracton of a class or framework are fine candidates for a beautifully terse name and comment.
Well chosen terse names and explanations are fine where you are familiar with the subject, and where the potential for clashing with other areas using the same terse phrases is not an issue. There are two practical issues I see a) if you work in an area and use terseness as your watchword, then move off to work on another area for some time, then when you return … it’s gone. All that internalised context that made the terseness work is gone. Unless you have truly epic memory, which some people do and some don’t. I seem to work in a bunch of disparate areas on a schedule that tends to be six months here, four months there, back to here.. and it takes time to get back into the milieu every time b) terseness without some serious explanations somewhere easily discoverable from this terse code is inimical to newcomers. Oh, and looking at it another way - terseness reduces the number of bits of information offered, which is another aspect of being rude to newcomers. Even in software we need some level of social lubrication.
Symbol is a base type! Maybe consider relaxing your aversion to type-checking for the base types at least.
I disagree that there is any concept of base types here. That’s one of the great things about Smalltalk; everything is objects.
If you ever use #isNil or #ifTrue:, then you must already agree these type checks are sometimes necessary. Numbers, Strings, Arrays and, most certainly, Symbols are also okay to type-check in rare, key places where it provides a lot of leverage.
isNil/ifTrue are not type checks, they’re value checks. true/false testing is part of the way we see control flow and we have true/false for exactly that. nil checking is a bit of a cop-out that we use because nobody has thought of a better option for handling the "if this thing hasn't been initialised yet, do some special code”. I dare say there are sophisticated things one might imagine with classes for uninitialisedFooVariableForMyPackage and .. well, who cares about details for a daft thought experiment.
I don’t like stuff that smacks of type checking because it is almost always a sign pointing to poor factoring in code that never got thoroughly reviewed and fixed up. Amazingly often it’s also a place where performance got gobbled up - the archetypical nasty variety of type checking is #isKindOf: and I got somewhere around a factor 10 speed improvement in Scratch just by removing the ‘need’ for half-a-dozen of them in important places. Not that I am going to claim success in being pure in this - there are still dozens or more places in the Scratch code with exactly these #isGumby checks, that will never have much hope of being removed.
tim -- tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim "How many Teela Browns does it take to change a lightbulb?” "Stupid question."
On Fri, Oct 6, 2017 at 1:44 PM, tim Rowledge tim@rowledge.org wrote:
On 05-10-2017, at 8:00 PM, Chris Muller asqueaker@gmail.com wrote: [snip] I think methods that are the most central, key abstracton of a class or framework are fine candidates for a beautifully terse name and comment.
Well chosen terse names and explanations are fine where you are familiar with the subject, and where the potential for clashing with other areas using the same terse phrases is not an issue. There are two practical issues I see a) if you work in an area and use terseness as your watchword, then move off to work on another area for some time, then when you return … it’s gone. All that internalised context that made the terseness work is gone. Unless you have truly epic memory, which some people do and some don’t. I seem to work in a bunch of disparate areas on a schedule that tends to be six months here, four months there, back to here.. and it takes time to get back into the milieu every time b) terseness without some serious explanations somewhere easily discoverable from this terse code is inimical to newcomers. Oh, and looking at it another way - terseness reduces the number of bits of information offered, which is another aspect of being rude to newcomers. Even in software we need some level of social lubrication.
For example, by ignoring and even clipping the *payoff* aspect of type-checking I conveyed, you made this reply more "terse." :(
I think we both agree that imparting the intent of the code with low friction is the goal. We just have different philosophies about whether the code, the apis, and categories, collectively, should BE the main conduit of understanding for the user.
I'm sure you're glad that Collection>>#do: isn't named Collection>>#doBlock:. That's what I meant by employing terseness sparingly, for key methods -- something short and sweet will stand out in the word-salad and be duly noticed.
Symbol is a base type! Maybe consider relaxing your aversion to type-checking for the base types at least.
I disagree that there is any concept of base types here. That’s one of the great things about Smalltalk; everything is objects.
Okay, but zooming out from the "what's-great-about-Smalltalk" for a moment to consider that real world data points have consistent (meta)properties, is all you need to have a "group" of something. As an example, I'm sure you acknowledge the existence of "value" types. Raw data types.
If you ever use #isNil or #ifTrue:, then you must already agree these type checks are sometimes necessary. Numbers, Strings, Arrays and, most certainly, Symbols are also okay to type-check in rare, key places where it provides a lot of leverage.
isNil/ifTrue are not type checks, they’re value checks. true/false testing is part of the way we see control flow and we have true/false for exactly that. nil checking is a bit of a cop-out that we use because nobody has thought of a better option for handling the "if this thing hasn't been initialised yet, do some special code”.
Tim, obviously we're never getting rid of nil but.. just to challenge your thought process...
Different, custom initializations for every var of every object? How would should this be controlled by the user? "Nil" is a real world concept anyone can understand. How about initializing all variables to nil, and let the user write:
thisThing isNil ifTrue: [ self doSomeSpecialCode ]
For anthing outside the lab, nil checks and then occasional type checks are equally essential as control flow, IMO.
I dare say there are sophisticated things one might imagine with classes for uninitialisedFooVariableForMyPackage and .. well, who cares about details for a daft thought experiment.
I don’t like stuff that smacks of type checking because it is almost always a sign pointing to poor factoring in code that never got thoroughly reviewed and fixed up.
"Almost always" depends on the code base. I remember you mentioning that in the Scratch code.
Amazingly often it’s also a place where performance got gobbled up - the archetypical nasty variety of type checking is #isKindOf: and I got somewhere around a factor 10 speed improvement in Scratch just by removing the ‘need’ for half-a-dozen of them in important places. Not that I am going to claim success in being pure in this - there are still dozens or more places in the Scratch code with exactly these #isGumby checks, that will never have much hope of being removed.
Yeah, #isKindOf: is almost never justified.
- Chris
+1 for Morph>>#buildWith: (because it is in the *toolbuilder catergory..) -1 for the manual dispatch.
2ct.
On 05.10.2017, at 08:11, Marcel Taeumel marcel.taeumel@hpi.de wrote:
Well, since Tobias started this refactoring in April 2017 to clear-up MorphicToolBuilder >> #open, this seems a logical next step to also have a #buildWith: in Morph.
Just my two cents. :)
Best, Marcel
Am 05.10.2017 04:40:46 schrieb Chris Muller asqueaker@gmail.com:
#buildWith: is a method for just ToolBuilders. The clarity of the decision of whether to build or not was articulated by the code of the class responsible for it. A dispatch seems unnecessary here. In fact, introducing Morph>>#buildWith: adds yet another muddle-method to its already-large API, and now inherited by the entire hierarchy. Will it invite people to start overriding it in subclasses as an alternative way to use ToolBuilder? Maybe. I think they'll at least be confused about why it's in Morph.
IMO, this is one of those times when a type check was the simplest and best way to go.
On Mon, Oct 2, 2017 at 7:09 PM, wrote:
tim Rowledge uploaded a new version of ToolBuilder-Morphic to project The Trunk: http://source.squeak.org/trunk/ToolBuilder-Morphic-tpr.196.mcz
==================== Summary ====================
Name: ToolBuilder-Morphic-tpr.196 Author: tpr Time: 2 October 2017, 5:09:16.231117 pm UUID: 7036c368-757d-4824-ac4a-52ac133665f3 Ancestors: ToolBuilder-Morphic-nice.195
Rather than testing an argument for morphnicity, how about jaust making a morph do the right thing (ie nothing)?
Not entirely clear that the 'problem' can ever really arise, but I can't see any way to prove either case.
=============== Diff against ToolBuilder-Morphic-nice.195 ===============
Item was added:
- ----- Method: Morph>>buildWith: (in category '*ToolBuilder-Morphic-opening') -----
- buildWith: aToolBuilder
- "A Morph is already built, so simply return myself"
- ^self!
Item was changed: ----- Method: MorphicToolBuilder>>open: (in category 'opening') ----- open: anObject "Build and open the object. Answer the widget opened."
- ^ (self build: anObject) morphicOpenWith: self!
- | morph |
- morph := anObject isMorph ifTrue: [anObject] ifFalse: [self build: anObject].
- ^ morph morphicOpenWith: self!
squeak-dev@lists.squeakfoundation.org
-
Chris Muller -
Chris Muller -
commits＠source.squeak.org -
David T. Lewis -
Marcel Taeumel -
tim Rowledge -
Tobias Pape