Fwd: Issue 1129 in pharo: UIManager #request: and #multilineRequest do not distinguish between empty and cancel

> From: [hidden email]
> Date: September 29, 2009 6:31:41 PM GMT+02:00
> To: [hidden email]
> Subject: Issue 1129 in pharo: UIManager #request: and  
> #multilineRequest do not distinguish between empty and cancel
>
>
> Comment #22 on issue 1129 by andreas.raab: UIManager #request: and  
> #multilineRequest do not distinguish between empty and cancel
> http://code.google.com/p/pharo/issues/detail?id=1129
>
> I'll give it one last shot: From my perspective, the problem is not  
> how difficult it
> is to fix it, the problem is that there is a change in semantics of  
> a widely deployed
> method that affects every single place that uses it. I could follow  
> your reasoning
> more easily if the situation where such that for 80% of the users  
> everything stays
> the same but the change you are proposing necessitates that every  
> single place that
> uses these methods needs to be changed. And not being able to tell  
> whether a
> particular place has been fixed or not is just bad engineering since  
> you are making
> it strictly harder for your users to determine whether they still  
> need to do
> something for addressing this issue or not. That's what deprecations  
> are for -
> telling users that there is something they need to take care of.
>
> I think that the change you are proposing is a decidedly bad choice  
> and urge you to
> reconsider for the following reasons:
>
> 1) It is subtle breakage that creates a major and completely  
> unnecessary PITA for
> anyone who needs to write UIManager code that works across Pharo,  
> Squeak, Croquet etc.
>
> 2) It weakens the framework. Toolbuilder is based on having the same  
> semantics across
> platforms and UIs and this change means that the entire family of  
> input requests is
> no longer reliable as it will behave differently between Pharo and  
> anything else in
> existence.
>
> 3) The alternatives are every bit as good. The protocol complication  
> of using
> onCancel: are minimal to non-existent (since in many cases the  
> ability to provide a
> cancel action is what you'll want anyway) and even if that were the  
> case, a new
> protocol that over time can be supported by other UIs would solve  
> that completely and
> in a proper evolutionary way.
>
> 4) With the choice you've made, you need to touch every user of that  
> method anyway.
> If that's true, there is no penalty for changing the protocol and it  
> will make
> migration easier since you can simply browse for all senders of  
> #request: and find
> those that haven't been converted yet.
>
> Please do consider these issues carefully. I really don't think  
> there is any
> advantage for you or your users by introducing such a subtle breakage.
>
> --
> You received this message because you are listed in the owner
> or CC fields of this issue, or because you starred this issue.
> You may adjust your issue notification preferences at:
> http://code.google.com/hosting/settings

> On Wed, Sep 30, 2009 at 12:07 PM, Stéphane Ducasse
> <[hidden email]> wrote:
>> I agree with andreas point. :)
>
> Hi Andreas,
>
> this is an interesting discussion and I think I understand your point
> : you see the problem of the current API but want to avoid impacting
> immediately all the code base. To do that, you propose to introduce
> the following 4 new methods: #request:onCancel:,
> #request:initialAnswer:onCancel:,
> #multiLineRequest:centerAt:initialAnswer:answerHeight:onCancel: and
> #requestPassword:onCancel:. Am I correct?
>
> To me, this is introducing needless complexity.  The nil value is here
> exactly to indicate the absence of a value. When the first
> implementation of the #request: method was created, I have no idea why
> they didn't choose to return nil upon cancel. I really have no idea
> how users of this library never had the need to distinguish an empty
> answer from a cancel. I can see so many examples where it is crucial
> to distinguish. Do you agree with me that the current API is broken?
> I'm not talking about adding a feature, I'm talking about fixing a
> bug.

>
> Please let me comment on your reasons:
>
> On Tue, Sep 29, 2009 at 6:31 PM,  <[hidden email]> wrote:
>> 1) It is subtle breakage that creates a major and completely  
>> unnecessary
>> PITA for anyone who needs to write UIManager code that works across  
>> Pharo, Squeak,
>> Croquet etc.
>
> I do not agree. Squeak doesn't have to change its implementation of
> #request:. Clients can just test for the nil value without wondering
> if it is going to be returned (in Pharo) or not (in Squeak).
>
> This has an immediate consequence : you can apply the two changesets
> which replace #isEmpty by #isEmptyOrNil after each call to #request:.
> You don't have to change #request: for this.
>
>
>> 2) It weakens the framework. Toolbuilder is based on having the same
>> semantics across
>> platforms and UIs and this change means that the entire family of  
>> input
>> requests is
>> no longer reliable as it will behave differently between Pharo and  
>> anything
>> else in
>> existence.
>
>
> To me, UIManager is broken. I don't understand why fixing it would
> weaken anything.
> Moreover, I'm quite sure all UIs can perfectly distinguish between the
> 2 buttons ok and cancel. For example, the Polymorph library returns
> nil by default and nil was translated to the empty string in
> PSUIManager to follow the behavior of UIManager.
>
>> 3) The alternatives are every bit as good. The protocol  
>> complication of
>> using
>> onCancel: are minimal to non-existent (since in many cases the  
>> ability to
>> provide a
>> cancel action is what you'll want anyway) and even if that were the  
>> case, a
>> new
>> protocol that over time can be supported by other UIs would solve  
>> that
>> completely and
>> in a proper evolutionary way.
>
> I agree that #onCancel: can be interesting on its own. However, I see
> it more as a new interesting feature whereas I'm talking about bug
> fixing.
>
>
>> 4) With the choice you've made, you need to touch every user of  
>> that method
>> anyway.
>
> True
>
>> If that's true, there is no penalty for changing the protocol
>
> here I don't agree. Changing the protocol means adding complexity. My
> solution does not complicate the protocol, it just fixes its meaning.
>
>> and it will make
>> migration easier since you can simply browse for all senders of  
>> #request:
>> and find
>> those that haven't been converted yet.
>
> I agree.
>
>> Please do consider these issues carefully. I really don't think  
>> there is any
>> advantage for you or your users by introducing such a subtle  
>> breakage.
>
> I can see pros and cons of each approach. Yours is more pragmatic I
> think while mine is more long-term.
>
>
> --
> Damien Cassou
> http://damiencassou.seasidehosting.st
>
> "Lambdas are relegated to relative obscurity until Java makes them
> popular by not having them." James Iry
>
> _______________________________________________
> Pharo-project mailing list
> [hidden email]
> http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project

> +1 to fix it one and for all.
>
> Remember, from the pharo-project.org page:
>
> "We want to create a better Smalltalk and be free to enhance it  
> without
> fear of backwards compatibility to Squeak."
>
> This discussion is trying to achieve backwards compatibility even if
> that means not fixing this bug.
>
> So, lets fix it and as the packages are going ported to Pharo, make  
> the
> necessary changes to them. When a lot of packages (if any) are using  
> the
> new semantics, the Squeak guys might consider to use the Pharo fix.
>
> No more discussion using the backwards compatibility argument.
>
> --
> Miguel Cobá
> http://miguel.leugim.com.mx
>
>
> _______________________________________________
> Pharo-project mailing list
> [hidden email]
> http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project

>>> I see two options:
>>>
>>> - yours is to complicate the API in order to keep backward
>>> compatibility with the code base and thus avoiding bugs.
>>> - mine is to fix the problem once and for all. Yes, it also means  
>>> bugs
>>> will appear during the period of fixing which can take some time
>>
>> the point andreas is to deprecate request:* so that when you see  
>> where
>> to change instead
>> of getting bitten.
>>
>> Stef
>
> Deprecation sounds like a bad solution in this case to me.
> It's useful when functionality has either been
> - Moved
> - Removed
> - Changed in a way that results in a silent failure
>
> In this case, either:
> -It's changed in both Squeak and Pharo, and you get MNU's everywhere  
> for the old ifEmpty. None of the 3 cases above is satisfied, so I  
> don't really see the benefit of deprecation here. You still have to  
> change your app, replacing all calls to the now deprecated to the  
> new one, plus make sure the code at each spot handles the nil case.
> - It's deprecated just one place, leading to extra porting overhead  
> if you have to change both the method name and the answer handling.  
> (plus the other dialect would have to add a deprecated ask: method  
> telling you how to conform to semantics).
>
>>
>>>
>>> Please let me comment on your reasons:
>>>
>>> On Tue, Sep 29, 2009 at 6:31 PM,  <[hidden email]>  
>>> wrote:
>>>> 1) It is subtle breakage that creates a major and completely
>>>> unnecessary
>>>> PITA for anyone who needs to write UIManager code that works across
>>>> Pharo, Squeak,
>>>> Croquet etc.
>>>
>>> I do not agree. Squeak doesn't have to change its implementation of
>>> #request:. Clients can just test for the nil value without wondering
>>> if it is going to be returned (in Pharo) or not (in Squeak).
>>>
>>> This has an immediate consequence : you can apply the two changesets
>>> which replace #isEmpty by #isEmptyOrNil after each call to  
>>> #request:.
>>> You don't have to change #request: for this.
>
> That's false.
> I suspect you only considered the case of porting Squeak code to  
> Pharo (which doesn't fail silently anyways, which is the major PITA,  
> you get an UndefinedObject >> MNU).
> Porting code written in Pharo, f.ex. using ifNotNil: to handle an  
> action where empty input is allowed, WILL fail silently in Squeak if  
> request:* is not modified, resulting in a cancel action replacing  
> the previous value with an empty string.
>
>>>
>>>
>>>> 2) It weakens the framework. Toolbuilder is based on having the  
>>>> same
>>>> semantics across
>>>> platforms and UIs and this change means that the entire family of
>>>> input
>>>> requests is
>>>> no longer reliable as it will behave differently between Pharo and
>>>> anything
>>>> else in
>>>> existence.
>>>
>>>
>>> To me, UIManager is broken.
>
> I agree, the issue is whether the default behaviour of handling  
> empty input and cancel the same way is what's broken, or the fact  
> that no way of handling such a case at all is provided.
>
> Considering the amount of times I've had to add ifEmpty: clauses  
> doing the same as ifNil: to request: call return values in VW (which  
> returns nil), I'd say the latter.
>
>>>
>>> I agree that #onCancel: can be interesting on its own. However, I  
>>> see
>>> it more as a new interesting feature whereas I'm talking about bug
>>> fixing.
>
> Andreas has a good point, changing the current behaviour will either:
> a) Lead to a lot of changes (albeit simple ones) for everyone if  
> adopted everywhere.
> b) Lead to subtle errors when porting between dialects if adopted  
> just in one place.
>
> The fact it's been the way it is without getting changed so far, and  
> my anecdotal experiences in VW implies to me that the case where you  
> want to distinguish between an empty response and a cancel is the  
> exception rather than the rule.
> Therefore, I support the option of adding explicit onCancel: aBlock  
> versions, and use those in the cases it's needed, rather than  
> changing the underlying behaviour, leading to either situation a) or  
> b) above.
>
> Cheers,
> Henry

> Thanks henry
> Now we will to make a synthesis of all the options and pros and cons.
> I'm too tired to get a clear idea.n but what you are saying that this
> is better
> to keep resquest funky behavior and introduce request:onCancel:
>
> Stef
>
>    
>>>> I see two options:
>>>>
>>>> - yours is to complicate the API in order to keep backward
>>>> compatibility with the code base and thus avoiding bugs.
>>>> - mine is to fix the problem once and for all. Yes, it also means
>>>> bugs
>>>> will appear during the period of fixing which can take some time
>>>>          
>>> the point andreas is to deprecate request:* so that when you see
>>> where
>>> to change instead
>>> of getting bitten.
>>>
>>> Stef
>>>        
>> Deprecation sounds like a bad solution in this case to me.
>> It's useful when functionality has either been
>> - Moved
>> - Removed
>> - Changed in a way that results in a silent failure
>>
>> In this case, either:
>> -It's changed in both Squeak and Pharo, and you get MNU's everywhere
>> for the old ifEmpty. None of the 3 cases above is satisfied, so I
>> don't really see the benefit of deprecation here. You still have to
>> change your app, replacing all calls to the now deprecated to the
>> new one, plus make sure the code at each spot handles the nil case.
>> - It's deprecated just one place, leading to extra porting overhead
>> if you have to change both the method name and the answer handling.
>> (plus the other dialect would have to add a deprecated ask: method
>> telling you how to conform to semantics).
>>
>>      
>>>        
>>>> Please let me comment on your reasons:
>>>>
>>>> On Tue, Sep 29, 2009 at 6:31 PM,<[hidden email]>
>>>> wrote:
>>>>          
>>>>> 1) It is subtle breakage that creates a major and completely
>>>>> unnecessary
>>>>> PITA for anyone who needs to write UIManager code that works across
>>>>> Pharo, Squeak,
>>>>> Croquet etc.
>>>>>            
>>>> I do not agree. Squeak doesn't have to change its implementation of
>>>> #request:. Clients can just test for the nil value without wondering
>>>> if it is going to be returned (in Pharo) or not (in Squeak).
>>>>
>>>> This has an immediate consequence : you can apply the two changesets
>>>> which replace #isEmpty by #isEmptyOrNil after each call to
>>>> #request:.
>>>> You don't have to change #request: for this.
>>>>          
>> That's false.
>> I suspect you only considered the case of porting Squeak code to
>> Pharo (which doesn't fail silently anyways, which is the major PITA,
>> you get an UndefinedObject>>  MNU).
>> Porting code written in Pharo, f.ex. using ifNotNil: to handle an
>> action where empty input is allowed, WILL fail silently in Squeak if
>> request:* is not modified, resulting in a cancel action replacing
>> the previous value with an empty string.
>>
>>      
>>>>
>>>>          
>>>>> 2) It weakens the framework. Toolbuilder is based on having the
>>>>> same
>>>>> semantics across
>>>>> platforms and UIs and this change means that the entire family of
>>>>> input
>>>>> requests is
>>>>> no longer reliable as it will behave differently between Pharo and
>>>>> anything
>>>>> else in
>>>>> existence.
>>>>>            
>>>>
>>>> To me, UIManager is broken.
>>>>          
>> I agree, the issue is whether the default behaviour of handling
>> empty input and cancel the same way is what's broken, or the fact
>> that no way of handling such a case at all is provided.
>>
>> Considering the amount of times I've had to add ifEmpty: clauses
>> doing the same as ifNil: to request: call return values in VW (which
>> returns nil), I'd say the latter.
>>
>>      
>>>> I agree that #onCancel: can be interesting on its own. However, I
>>>> see
>>>> it more as a new interesting feature whereas I'm talking about bug
>>>> fixing.
>>>>          
>> Andreas has a good point, changing the current behaviour will either:
>> a) Lead to a lot of changes (albeit simple ones) for everyone if
>> adopted everywhere.
>> b) Lead to subtle errors when porting between dialects if adopted
>> just in one place.
>>
>> The fact it's been the way it is without getting changed so far, and
>> my anecdotal experiences in VW implies to me that the case where you
>> want to distinguish between an empty response and a cancel is the
>> exception rather than the rule.
>> Therefore, I support the option of adding explicit onCancel: aBlock
>> versions, and use those in the cases it's needed, rather than
>> changing the underlying behaviour, leading to either situation a) or
>> b) above.
>>
>> Cheers,
>> Henry
>>      
> _______________________________________________
> Pharo-project mailing list
> [hidden email]
> http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
>
>
>    

> I'm saying that in my eyes, it seems like the best option because:
> - It keeps the current behaviour of handling empty input and cancel the
> same way. At least in my experience, this is the common case.
> - It corrects the deficiency that there's currently no way to handle the
> two differently (which in my eyes is the real bug).
> - Using a default where you're likely to have to handle nil and empty in
> the same way seem (to me at least) in some ways just as bad as
> implementing do: on Object.
> - The new method request:onCancel: will show up right below request:, so
> it's easy to spot if you find yourself in the exceptional situation
> where you DO need to handle these cases differently. (Especially with a
> method comment explaining that's exactly what it's intended for)
> - As an added benefit, it also keeps compatability with Squeak :)

>
> Cheers,
> Henry
>
> On 01.10.2009 19:07, Stéphane Ducasse wrote:
> > Thanks henry
> > Now we will to make a synthesis of all the options and pros and cons.
> > I'm too tired to get a clear idea.n but what you are saying that this
> > is better
> > to keep resquest funky behavior and introduce request:onCancel:
> >
> > Stef
> >
> >    
> >>>> I see two options:
> >>>>
> >>>> - yours is to complicate the API in order to keep backward
> >>>> compatibility with the code base and thus avoiding bugs.
> >>>> - mine is to fix the problem once and for all. Yes, it also means
> >>>> bugs
> >>>> will appear during the period of fixing which can take some time
> >>>>          
> >>> the point andreas is to deprecate request:* so that when you see
> >>> where
> >>> to change instead
> >>> of getting bitten.
> >>>
> >>> Stef
> >>>        
> >> Deprecation sounds like a bad solution in this case to me.
> >> It's useful when functionality has either been
> >> - Moved
> >> - Removed
> >> - Changed in a way that results in a silent failure
> >>
> >> In this case, either:
> >> -It's changed in both Squeak and Pharo, and you get MNU's everywhere
> >> for the old ifEmpty. None of the 3 cases above is satisfied, so I
> >> don't really see the benefit of deprecation here. You still have to
> >> change your app, replacing all calls to the now deprecated to the
> >> new one, plus make sure the code at each spot handles the nil case.
> >> - It's deprecated just one place, leading to extra porting overhead
> >> if you have to change both the method name and the answer handling.
> >> (plus the other dialect would have to add a deprecated ask: method
> >> telling you how to conform to semantics).
> >>
> >>      
> >>>        
> >>>> Please let me comment on your reasons:
> >>>>
> >>>> On Tue, Sep 29, 2009 at 6:31 PM,<[hidden email]>
> >>>> wrote:
> >>>>          
> >>>>> 1) It is subtle breakage that creates a major and completely
> >>>>> unnecessary
> >>>>> PITA for anyone who needs to write UIManager code that works across
> >>>>> Pharo, Squeak,
> >>>>> Croquet etc.
> >>>>>            
> >>>> I do not agree. Squeak doesn't have to change its implementation of
> >>>> #request:. Clients can just test for the nil value without wondering
> >>>> if it is going to be returned (in Pharo) or not (in Squeak).
> >>>>
> >>>> This has an immediate consequence : you can apply the two changesets
> >>>> which replace #isEmpty by #isEmptyOrNil after each call to
> >>>> #request:.
> >>>> You don't have to change #request: for this.
> >>>>          
> >> That's false.
> >> I suspect you only considered the case of porting Squeak code to
> >> Pharo (which doesn't fail silently anyways, which is the major PITA,
> >> you get an UndefinedObject>>  MNU).
> >> Porting code written in Pharo, f.ex. using ifNotNil: to handle an
> >> action where empty input is allowed, WILL fail silently in Squeak if
> >> request:* is not modified, resulting in a cancel action replacing
> >> the previous value with an empty string.
> >>
> >>      
> >>>>
> >>>>          
> >>>>> 2) It weakens the framework. Toolbuilder is based on having the
> >>>>> same
> >>>>> semantics across
> >>>>> platforms and UIs and this change means that the entire family of
> >>>>> input
> >>>>> requests is
> >>>>> no longer reliable as it will behave differently between Pharo and
> >>>>> anything
> >>>>> else in
> >>>>> existence.
> >>>>>            
> >>>>
> >>>> To me, UIManager is broken.
> >>>>          
> >> I agree, the issue is whether the default behaviour of handling
> >> empty input and cancel the same way is what's broken, or the fact
> >> that no way of handling such a case at all is provided.
> >>
> >> Considering the amount of times I've had to add ifEmpty: clauses
> >> doing the same as ifNil: to request: call return values in VW (which
> >> returns nil), I'd say the latter.
> >>
> >>      
> >>>> I agree that #onCancel: can be interesting on its own. However, I
> >>>> see
> >>>> it more as a new interesting feature whereas I'm talking about bug
> >>>> fixing.
> >>>>          
> >> Andreas has a good point, changing the current behaviour will either:
> >> a) Lead to a lot of changes (albeit simple ones) for everyone if
> >> adopted everywhere.
> >> b) Lead to subtle errors when porting between dialects if adopted
> >> just in one place.
> >>
> >> The fact it's been the way it is without getting changed so far, and
> >> my anecdotal experiences in VW implies to me that the case where you
> >> want to distinguish between an empty response and a cancel is the
> >> exception rather than the rule.
> >> Therefore, I support the option of adding explicit onCancel: aBlock
> >> versions, and use those in the cases it's needed, rather than
> >> changing the underlying behaviour, leading to either situation a) or
> >> b) above.
> >>
> >> Cheers,
> >> Henry
> >>      
> > _______________________________________________
> > Pharo-project mailing list
> > [hidden email]
> > http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
> >
> >
> >    
>
>
> _______________________________________________
> Pharo-project mailing list
> [hidden email]
> http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project