[M3devel] small design problem in ButtonVBT?

[Tony Hosking]

Yes, exactly!  I don't think the change is needed and goes against the  
grain of a pervasive design principle.  We might question that  
decision, but I hesitate to apply a piecemeal tweak that violates the  
principle.

On 28 Aug 2009, at 05:11, hendrik at topoi.pooq.com wrote:

> On Thu, Aug 27, 2009 at 11:53:27PM -0400, Tony Hosking wrote:
>> Right.  Plus, overriding copies the entire method suite for the new
>> type.
>
> Now that makes sense.  I'll re-evaluate my suggestion in this light.
>
>>
>> On 27 Aug 2009, at 23:28, Jay K wrote:
>>
>>>
>>> I think I know the reason it is this way.
>>> It is very common to override action and nothing else.
>>> It is "inconvenient" to derive a new type, just to change one
>>> function.
>
> It's more a matter of letting the action procedure or method have  
> clean
> access to application data in the subclassed function.  But your code
> suggests a simpler way to do this than either your code or mine.  It
> takes a run-time type check, which is conceptually messy.  But the  
> code
> is smaller, and seems to have a smaller memory footprint.
>
> See below.
>
>>>
>>> - Jay
>>>
>>> ----------------------------------------
>>>> Date: Thu, 27 Aug 2009 23:17:23 -0400
>>>> From: hendrik at topoi.pooq.com
>>>> To: m3devel at elegosoft.com
>>>> Subject: Re: [M3devel] small design problem in ButtonVBT?
>>>>
>>>> On Thu, Aug 27, 2009 at 03:15:41PM -0400, Tony Hosking wrote:
>>>>> I am nervous about having the two alternative entries to the  
>>>>> action
>>>>> procedure that other code might not be aware of. Yes, you've
>>>>> modified
>>>>> the default mouse method to call the action method instead of the
>>>>> procedure, but what about other places where the mouse method has
>>>>> been
>>>>> overridden.
>
> Yes.  That could be an issue.  But only if act is also overridden,  
> which
> won't happen in legacy code, since the defauly act behaves identically
> to the action procedure.
>
>>>>>
>>>>> If you want to be smart about it in your own code you can simply
>>>>> invert your suggestion and do the following:
>>>>>
>>>>> TYPE T = ButtonVBT.T OBJECT
>>>>> METHODS action(cd: VBT.MouseRec) := Action;
>>>>>
>>>>> PROCEDURE Action(v: T; cd: VBT.MouseRec) = ...
>>>>>
>>>>> PROCEDURE ActionProc(v: ButtonVBT.T; cd: VBT.MouseRec) =
>>>>> BEGIN
>>>>> NARROW(v, T).action(cd);
>>>>> END ActionProc;
>>>>>
>>>>> and initialize your T thingy with ButtonVBT.New(..., action :=
>>>>> ActionProc; ...)
>>>>>
>>>>> Then in your code you can go ahead and override the action  
>>>>> method to
>>>>> your hearts content.
>
> But I can simplify this further.
>
> TYPE T = ButtonVBT.T OBJECT (* as you propose. *)
>
> For each action that I had wanted a subclass for, write
>
> PROCEDURE Action(vv: T; cd: VBT.MouseRec) = ...
> VAR v : T;
> BEGIN
> v := NARROW(vv, T);
> ... normal code for performing the action
> END Action;
>
> and initialize my T thingy using this Action.
>
> So instead of multiple subclasses to get the various actions, I get  
> two
> extra lines of code in each action procedure.  And I don't have to use
> GetProp or PutProp (which probably involve some kind of search).  The
> run-time cost is the type-check in the NARROW(vv, T), which is  
> probably
> cheaper than a method-dispatch on act.
>
> Hmmm.  I'm starting to think you're right, and my change is  
> unnecessary.
>
> I'll have a look in my applicatin code, and see if there are further
> glitches.  I'm not expecting any.
>
> -- hendrik
>
>>>>
>>>> This kind of thing would work. But, like the existing semantics of
>>>> ButtonVBT, it seems to be fighting the language instead of working
>>>> with
>>>> it. I'd be happiest to get rid of the action variable altogether,  
>>>> but
>>>> that would be a massively incompatible change, and would require
>>>> change
>>>> to a lot of existing code. What I proposed is compatible with
>>>> existing
>>>> code, and also has the advantage that someone who subclasses
>>>> ButtonVBT.T
>>>> and overrides act can completely ignore the action variable
>>>> altogether.
>>>>
>>>> You say
>>>>
>>>>> and initialize your T thingy with ButtonVBT.New(..., action :=
>>>>> ActionProc; ...)
>>>>
>>>> But that requires every object I make of type T or any sibclass  
>>>> to be
>>>> explicitly initialised with ActionProc, certainly a nuisance, and
>>>> certainly error-prone.
>>>>
>>>> The inelegance I see with my approach is that there are two
>>>> mechanisms
>>>> for specifying the action to be taken. Either will work without the
>>>> programmer paying any attention to the other, but if you use both,
>>>> one
>>>> will override the other. This is not a problem with legacy  
>>>> software.
>>>> I'll have to come up with some documentation that makes this clear.
>>>> It's the inelegance that comes from backward compatibility, and I
>>>> think
>>>> my solution is less inelegant, and easier to use, than yours.  
>>>> Yours,
>>>> however, involves no change in existing code.
>>>>
>>>>>
>>>>> Am I just being paranoid?
>>>>
>>>> No. Perhaps just paranoid about different things.
>>>>
>>>> I had hoped for someone to come up with a solid technical reason
>>>> for the
>>>> action variable. The original designers were clever people, and I'd
>>>> *still* like to know why they thought it was a good idea. It's
>>>> awkward
>>>> and error-prone to use, and they had to use the PutProp/GetProp
>>>> kludge
>>>> just to approximate what the language provided for free with
>>>> inheritance.
>>>>
>>>> I'm tempted to go over the entire code-base and change the  
>>>> buttons to
>>>> use act instead of action, but I have to admit I'm not tempted
>>>> enough to
>>>> actually do it...
>>>>
>>>> -- hendrik
>>>>
>>>>> On 27 Aug 2009, at 08:17, Olaf Wagner wrote:
>>>>>
>>>>>> Though I promised to do it, I have lost sight of this task while
>>>>>> trying to build some RC3 archives on Windows. As I'm not at home
>>>>>> this
>>>>>> evening, can somebody else please commit Hendrik's extension?
>>>>>>
>>>>>> Is this something we should should have in the release branch,  
>>>>>> too?
>>>>>> Probably not necessary?
>>>>>>
>>>>>> Olaf
>>>>>>
>>>>>> Quoting hendrik at topoi.pooq.com:
>>>>>>
>>>>>>> On Wed, Apr 01, 2009 at 12:03:47PM +0200, Olaf Wagner wrote:
>>>>>>>> Quoting hendrik at topoi.pooq.com:
>>>>>>>>
>>>>>>>>> The BottonVBT contains an action, which is a procedure rather
>>>>>>>> than a
>>>>>>>>> method. b This makes it awkward to subclass ButtonVBT becaue  
>>>>>>>>> the
>>>>>>>> action
>>>>>>>>> will still expect its first parameter to be a ButtonVBT
>>>>>>>>> instead of
>>>>>>>>> belonging to the subclass, and an explicit downcast of some  
>>>>>>>>> sort
>>>>>>>> will be
>>>>>>>>> needed to acess the subclass's members.
>>>>>>>>>
>>>>>>>>> The Trestle manual says this was a deliberate decision:
>>>>>>>>>
>>>>>>>>> : The action procedure is a field rather than a method in  
>>>>>>>>> order
>>>>>>>> to allow
>>>>>>>>> : buttons with different action procedures to share their  
>>>>>>>>> method
>>>>>>>> suites.
>>>>>>>>>
>>>>>>>>> I, however, find it massively inconvenient because it mans I
>>>>>>>>> have
>>>>>>>> to
>>>>>>>>> resort to downcasting or the very kludgey VBT.GetProp to  
>>>>>>>>> access
>>>>>>>> what
>>>>>>>>> should be just there. I'd rather the action procedure was a
>>>>>>>> method.
>>>>>>>>>
>>>>>>>>> But it should be possible to have the best of both worlds.
>>>>>>>>> Have an
>>>>>>>>> action2 (better name wanted here) method that is available for
>>>>>>>>> overriding, and by default calls the procedure in the existing
>>>>>>>> action.
>>>>>>>>> field. That action2 method wounl then be called by Trestle
>>>>>>>> wherever
>>>>>>>>> action is now called.
>>>>>>>>>
>>>>>>>>> Does anyone see problems with this?
>>>>>>>>
>>>>>>>> Sounds fine to me offhand. You should test your extensions with
>>>>>>>> some of the existing larger applications, like mentor and  
>>>>>>>> Juno-2,
>>>>>>>> of course.
>>>>>>>
>>>>>>> OK. Here's the patch. Would it be possible for someone to  
>>>>>>> check it
>>>>>>> in
>>>>>>> for me?
>>>>>>>
>>>>>>> mentor runs fine with this change; I watched it animate a heap-
>>>>>>> sort.
>>>>>>>
>>>>>>> I have no idea what Juno is supposed to do, but it did respond  
>>>>>>> to
>>>>>>> button-pushes.
>>>>>>>
>>>>>>>
>>>>>>> hendrik at lovesong:~/cm3/love-sq/cm3/m3-ui/ui/src/split$ cvs diff
>>>>>>> cvs diff: Diffing .
>>>>>>> Index: ButtonVBT.i3
>>>>>>> =
>>>>>>> = 
>>>>>>> = 
>>>>>>> ================================================================
>>>>>>> RCS file: /usr/cvs/cm3/m3-ui/ui/src/split/ButtonVBT.i3,v
>>>>>>> retrieving revision 1.2
>>>>>>> diff -r1.2 ButtonVBT.i3
>>>>>>> 39a40
>>>>>>>> act(READONLY cd: VBT.MouseRec);
>>>>>>> Index: ButtonVBT.m3
>>>>>>> =
>>>>>>> = 
>>>>>>> = 
>>>>>>> ================================================================
>>>>>>> RCS file: /usr/cvs/cm3/m3-ui/ui/src/split/ButtonVBT.m3,v
>>>>>>> retrieving revision 1.2
>>>>>>> diff -r1.2 ButtonVBT.m3
>>>>>>> 28c28,29
>>>>>>> < init := Be
>>>>>>> ---
>>>>>>>> init := Be;
>>>>>>>> act := act
>>>>>>> 46a48,52
>>>>>>>> PROCEDURE act(v : T; READONLY cd: VBT.MouseRec) =
>>>>>>>> BEGIN
>>>>>>>> v.action(v, cd);
>>>>>>>> END act;
>>>>>>>>
>>>>>>> 59c65
>>>>>>> < v.action(v, cd);
>>>>>>> ---
>>>>>>>> v.act(cd);
>>>>>>> hendrik at lovesong:~/cm3/love-sq/cm3/m3-ui/ui/src/split$
>>>>>>>
>>>>>>> -- hendrik
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Olaf Wagner -- elego Software Solutions GmbH
>>>>>> Gustav-Meyer-Allee 25 / Gebäude 12, 13355 Berlin,
>>>>>> Germany
>>>>>> phone: +49 30 23 45 86 96 mobile: +49 177 2345 869 fax: +49 30 23
>>>>>> 45 86 95
>>>>>> http://www.elegosoft.com | Geschäftsführer: Olaf Wagner | Sitz:
>>>>>> Berlin
>>>>>> Handelregister: Amtsgericht Charlottenburg HRB 77719 | USt-IdNr:
>>>>>> DE163214194
>>>>>>
>>>>>
>>