On Wed, 19 Oct 2011, commits@source.squeak.org wrote:
Chris Muller uploaded a new version of ToolBuilder-Kernel to project The Trunk: http://source.squeak.org/trunk/ToolBuilder-Kernel-cmm.52.mcz
==================== Summary ====================
Name: ToolBuilder-Kernel-cmm.52 Author: cmm Time: 18 October 2011, 8:12:42.168 pm UUID: 54416a65-63c1-4ba4-959e-7f1f7db3aa62 Ancestors: ToolBuilder-Kernel-dtl.51
Fix ability to safely catch Notification so that, if #displayProgressAt:from:to:during: is sent, will actually execute the during block.
I don't like this change, because it breaks existing code and supports a bad pattern. For example ComplexProgressIndicator and Installer assumes that ProgressInitiationException >> #resume just resumes with nil without any side effects, so these are broken now. Catching all Notifications and just resuming them is something that doesn't work, because there may be other handlers which expect to catch them, so you should #pass the notifications that you don't want to swallow. Using Notification instead of specific subclasses is also a bad idea unless you really want to process all notifications. Maybe we should raise an error when someone is trying to signal a Notification, because that class is just a marker and not something that should be used as a Notification (I could say it's abstract, but I don't like this term, because in Smalltalk it doesn't have a well-defined meaning).
Levente
=============== Diff against ToolBuilder-Kernel-dtl.51 ===============
Item was changed: ----- Method: ProgressInitiationException>>defaultAction (in category 'as yet unclassified') ----- defaultAction
- self resume!
- | result |
- result := UIManager default
displayProgress: progressTitleat: aPointfrom: minValto: maxValduring: workBlock.- self resume: result!
Item was added:
- ----- Method: ProgressInitiationException>>defaultResumeValue (in category 'as yet unclassified') -----
- defaultResumeValue
- ^ UIManager default
displayProgress: progressTitleat: aPointfrom: minValto: maxValduring: workBlock!
On 19.10.2011, at 12:45, Levente Uzonyi wrote:
On Wed, 19 Oct 2011, commits@source.squeak.org wrote:
Chris Muller uploaded a new version of ToolBuilder-Kernel to project The Trunk: http://source.squeak.org/trunk/ToolBuilder-Kernel-cmm.52.mcz
==================== Summary ====================
Name: ToolBuilder-Kernel-cmm.52 Author: cmm Time: 18 October 2011, 8:12:42.168 pm UUID: 54416a65-63c1-4ba4-959e-7f1f7db3aa62 Ancestors: ToolBuilder-Kernel-dtl.51
Fix ability to safely catch Notification so that, if #displayProgressAt:from:to:during: is sent, will actually execute the during block.
I don't like this change, because it breaks existing code and supports a bad pattern. For example ComplexProgressIndicator and Installer assumes that ProgressInitiationException >> #resume just resumes with nil without any side effects, so these are broken now. Catching all Notifications and just resuming them is something that doesn't work, because there may be other handlers which expect to catch them, so you should #pass the notifications that you don't want to swallow. Using Notification instead of specific subclasses is also a bad idea unless you really want to process all notifications. Maybe we should raise an error when someone is trying to signal a Notification, because that class is just a marker and not something that should be used as a Notification (I could say it's abstract, but I don't like this term, because in Smalltalk it doesn't have a well-defined meaning).
Levente
+1
- Bert -
=============== Diff against ToolBuilder-Kernel-dtl.51 ===============
Item was changed: ----- Method: ProgressInitiationException>>defaultAction (in category 'as yet unclassified') ----- defaultAction
- self resume!
- | result |
- result := UIManager default
displayProgress: progressTitleat: aPointfrom: minValto: maxValduring: workBlock.- self resume: result!
Item was added:
- ----- Method: ProgressInitiationException>>defaultResumeValue (in category 'as yet unclassified') -----
- defaultResumeValue
- ^ UIManager default
displayProgress: progressTitleat: aPointfrom: minValto: maxValduring: workBlock!
Levente, can you be more clear in your argument? To me, it looks like Installer is broken _without_ this change, not with it. You are referring to Installer class>>#noProgressDuring: yes?
[ block value: self ] on: ProgressInitiationException do: [ :note | note resume ]
Where I see the expression, "note resume" I expect whatever code signaled it to, well, *resume* on the next expression. But "resume" in this case does not "resume with nil" as you said, instead it aborts and returns nil, since that's the defaultAction of Notification. I demonstrated this the other day, in my test case and now here:
|executed | executed:=false. Installer noProgressDuring: [ : InstallerClass | 'block involves a progress bar' displayProgressFrom: 1 to: 10 during: [ : bar | executed:=true ] ]. self assert: executed
To me, silently not running code I expect to be run, is the crime being committed.
Catching all Notifications and just resuming them is something that doesn't work, because there may be other handlers which expect to catch them, so you should #pass the notifications that you don't want to swallow. Using
I want to catch Notifications and Warnings at the highest level, log them and resume. If there was a lower-handler then my high-level handler wouldn't get it. I'm not understanding what point you're making here..
Notification instead of specific subclasses is also a bad idea unless you really want to process all notifications. Maybe we should raise an error when someone is trying to signal a Notification, because that class is just
I'm not signaling Notifications, I'm only catching (subclasses of) them, and only at the very highest level.
a marker and not something that should be used as a Notification (I could say it's abstract, but I don't like this term, because in Smalltalk it doesn't have a well-defined meaning).
Refactoring Engine considers an abstract class in Smalltalk as one that has a method that sends #subclassResponsibility, which Notification does not.
- Chris
Levente
=============== Diff against ToolBuilder-Kernel-dtl.51 ===============
Item was changed: ----- Method: ProgressInitiationException>>defaultAction (in category 'as yet unclassified') ----- defaultAction
- self resume!
- | result |
- result := UIManager default
- displayProgress: progressTitle
- at: aPoint
- from: minVal
- to: maxVal
- during: workBlock.
- self resume: result!
Item was added:
- ----- Method: ProgressInitiationException>>defaultResumeValue (in
category 'as yet unclassified') -----
- defaultResumeValue
- ^ UIManager default
- displayProgress: progressTitle
- at: aPoint
- from: minVal
- to: maxVal
- during: workBlock!
But "resume" in this case does not "resume with nil" as you said, instead it aborts and returns nil, since that's the defaultAction of Notification.
Correction: It's because resume never executes its workBlock.
But this does not change my main complaint.
I said it wrong because I also evaluated changing Notification>>#defaultAction to resume -- I never understood why Notifications defaultAction was to abort the code that signaled the Notification. But this approach seemed less risky, since we're only talking about ProgressInitiationException.
- Chris
On Wed, 19 Oct 2011, Chris Muller wrote:
Levente, can you be more clear in your argument? To me, it looks like Installer is broken _without_ this change, not with it. You are referring to Installer class>>#noProgressDuring: yes?
Right, there seem to be incompatibilities with Installer and progress displaying.
[ block value: self ] on: ProgressInitiationException do: [ :note | note resume ]
Where I see the expression, "note resume" I expect whatever code signaled it to, well, *resume* on the next expression. But "resume" in this case does not "resume with nil" as you said, instead it aborts and returns nil, since that's the defaultAction of Notification. I demonstrated this the other day, in my test case and now here:
|executed | executed:=false. Installer noProgressDuring: [ : InstallerClass | 'block involves a progress bar' displayProgressFrom: 1 to: 10 during: [ : bar | executed:=true ] ]. self assert: executed
To me, silently not running code I expect to be run, is the crime being committed.
It's clearly a bug in Installer. I guess this fixes it:
noProgressDuring: block
[ block value: self ] on: ProgressInitiationException do: [ :note | note sendNotificationsTo: [ :min :max :curr | "ignore" ] ]
Catching all Notifications and just resuming them is something that doesn't work, because there may be other handlers which expect to catch them, so you should #pass the notifications that you don't want to swallow. Using
I want to catch Notifications and Warnings at the highest level, log them and resume. If there was a lower-handler then my high-level
Why?
Warnings are okay, because those only inform the user about some issue that can be ignored at own risk, but Notifications are used for all kind of stuff that involves passing objects up and/or down the stack. Catching them without proper handling can and will break stuff.
I have the feeling that you think about a Notification as an exception that passes some information about the system and swallowing it is harmless, like Warning or an info or notice level message in syslog. Notifications are Exceptions with a well-defined #defaultAction. So it's always safe to throw and not catch them.
handler wouldn't get it. I'm not understanding what point you're making here..
Notification instead of specific subclasses is also a bad idea unless you really want to process all notifications. Maybe we should raise an error when someone is trying to signal a Notification, because that class is just
I'm not signaling Notifications, I'm only catching (subclasses of) them, and only at the very highest level.
I was referring to SUnitToolBuilderTests >> #testHandlingNotification.
a marker and not something that should be used as a Notification (I could say it's abstract, but I don't like this term, because in Smalltalk it doesn't have a well-defined meaning).
Refactoring Engine considers an abstract class in Smalltalk as one that has a method that sends #subclassResponsibility, which Notification does not.
Yes, that's one interpretation.
Levente
- Chris
Levente
=============== Diff against ToolBuilder-Kernel-dtl.51 ===============
Item was changed: ----- Method: ProgressInitiationException>>defaultAction (in category 'as yet unclassified') ----- defaultAction
- self resume!
- | result |
- result := UIManager default
- displayProgress: progressTitle
- at: aPoint
- from: minVal
- to: maxVal
- during: workBlock.
- self resume: result!
Item was added:
- ----- Method: ProgressInitiationException>>defaultResumeValue (in
category 'as yet unclassified') -----
- defaultResumeValue
- ^ UIManager default
- displayProgress: progressTitle
- at: aPoint
- from: minVal
- to: maxVal
- during: workBlock!
squeak-dev@lists.squeakfoundation.org
-
Bert Freudenberg -
Chris Muller -
Levente Uzonyi