On community development of the VM's platform code...

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

On community development of the VM's platform code...

Eliot Miranda-2
 
Hi All,

    this morning I've looked at some platform even t code to address an issue with scrolling that Chris Muller was bitten by.  In reading the code I've tried to make the code comprehensible to others who may come to read it. In particular I changed this code in platforms/iOS/vm/OSX/sqSqueakOSXApplication+events.m

        evt.charCode = keyCode;
        evt.utf32Code = 0;
        evt.reserved1 = 0;
        evt.modifiers = (CtrlKeyBit | OptionKeyBit | CommandKeyBit | ShiftKeyBit);
        evt.windowIndex = windowIndex;
        [self pushEventToQueue:(sqInputEvent *) &evt];

to read
        evt.charCode = keyCode;
        evt.utf32Code = 0;
        evt.reserved1 = 0;
       /* Set every meta bit to distinguish the fake event from a real right/left
        * arrow.
        */
        evt.modifiers = (CtrlKeyBit | OptionKeyBit | CommandKeyBit | ShiftKeyBit);
        evt.windowIndex = windowIndex;
        [self pushEventToQueue:(sqInputEvent *) &evt];

After all, what the code does is obvious, but /why/ the code does what it does is not at all obvious.

In reading similar code on the other platforms I see things like this, an example from platforms/win32/vm/sqWin32Window.c:

      evt->modifiers = CtrlKeyBit;
#ifdef PharoVM
     evt->utf32Code = evt->charCode;
#else
      evt->utf32Code = 0;
#endif
      evt->reserved1 = 0;

Again, *why* the code sets utf32Code but not on Squeak, Newspeak, etc, is not at all obvious and it would be really helpful if it were commented so that the decision as to whether the ifdef might be eliminated and the code always set6 (surely preferable) would be made easier.

The lack of such comments makes managing this code very hard for those of us that are not platform experts but must maintain the code across all platforms because that's our lot in life.  The absence of such comments, when one is struggling to figure out what the hell is going on, feels like passive aggression ;-)  So if you've made some m modification to the platform code in the past few years and you haven't commented it to explain the *why*, please feel free to comment the code and commit or submit a pull request; it will be warmly appreciated.

Thank you!

_,,,^..^,,,_
best, Eliot
Reply | Threaded
Open this post in threaded view
|

Re: On community development of the VM's platform code...

johnmci
 
Ok, so in reviewing GitHub, this change comes from 
https://github.com/pharo-project/pharo-vm/commit/e80301237d8f23d0da0db80fe1ebbcc5ee049c2d

"Fake mouse wheel events with all modifier keys pressed (not just ctrl)"

What made it unclear was I hand merged the Pharo & Pyonkee changes in early 2015, thus losing the historical Git information found in the pharo git repository. In this case I'll suspect there is changes to the event handling in Pharo which perhaps are not in Squeak Smalltalk base?

JMM - integrate Pyonkee & Pharo changes to cocoa os-x and iPhone code base 
git-svn-id: http://squeakvm.org/svn/squeak/trunk@3324 fa1542d4-bde8-0310-ad64-8ed1123d492a
Former-commit-id: e11f52c96c104466f3636f9cfb8f1d8bc4219b56
Commit:346a75a0cbf554bfe5aed0281283fd19f8a38fef [346a75a0c]
Parents:839962918e
Author:John McIntosh <[hidden email]>
Date:May 7, 2015 at 11:23:28 AM PDT 

....
John M. McIntosh. Corporate Smalltalk Consulting Ltd https://www.linkedin.com/in/smalltalk


‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Wednesday, November 7, 2018 7:32 AM, Eliot Miranda <[hidden email]> wrote:

Hi All,

    this morning I've looked at some platform even t code to address an issue with scrolling that Chris Muller was bitten by.  In reading the code I've tried to make the code comprehensible to others who may come to read it. In particular I changed this code in platforms/iOS/vm/OSX/sqSqueakOSXApplication+events.m

        evt.charCode = keyCode;
        evt.utf32Code = 0;
        evt.reserved1 = 0;
        evt.modifiers = (CtrlKeyBit | OptionKeyBit | CommandKeyBit | ShiftKeyBit);
        evt.windowIndex = windowIndex;
        [self pushEventToQueue:(sqInputEvent *) &evt];

to read
        evt.charCode = keyCode;
        evt.utf32Code = 0;
        evt.reserved1 = 0;
       /* Set every meta bit to distinguish the fake event from a real right/left
        * arrow.
        */
        evt.modifiers = (CtrlKeyBit | OptionKeyBit | CommandKeyBit | ShiftKeyBit);
        evt.windowIndex = windowIndex;
        [self pushEventToQueue:(sqInputEvent *) &evt];

After all, what the code does is obvious, but /why/ the code does what it does is not at all obvious.

In reading similar code on the other platforms I see things like this, an example from platforms/win32/vm/sqWin32Window.c:

      evt->modifiers = CtrlKeyBit;
#ifdef PharoVM
     evt->utf32Code = evt->charCode;
#else
      evt->utf32Code = 0;
#endif
      evt->reserved1 = 0;

Again, *why* the code sets utf32Code but not on Squeak, Newspeak, etc, is not at all obvious and it would be really helpful if it were commented so that the decision as to whether the ifdef might be eliminated and the code always set6 (surely preferable) would be made easier.

The lack of such comments makes managing this code very hard for those of us that are not platform experts but must maintain the code across all platforms because that's our lot in life.  The absence of such comments, when one is struggling to figure out what the hell is going on, feels like passive aggression ;-)  So if you've made some m modification to the platform code in the past few years and you haven't commented it to explain the *why*, please feel free to comment the code and commit or submit a pull request; it will be warmly appreciated.

Thank you!

_,,,^..^,,,_
best, Eliot

Reply | Threaded
Open this post in threaded view
|

Re: On community development of the VM's platform code...

Sean P. DeNigris
Administrator
 
johnmci wrote
> "Fake mouse wheel events with all modifier keys pressed (not just ctrl)"

I thought that looked familiar. I needed that change IIRC to enable
horizontal scrolling in Pharo because there was a shortcut conflict.
However, I got bogged down when I went to make the change in Windows and
then got distracted. The reason I chose a more complex hack instead of a
clean fix is because the solution seemed backward compatible so it didn't
break Squeak et al.



-----
Cheers,
Sean
--
Sent from: http://forum.world.st/Squeak-VM-f104410.html
Cheers,
Sean
Reply | Threaded
Open this post in threaded view
|

Re: On community development of the VM's platform code...

Eliot Miranda-2
 
Hi John, Hi Sean,

On Wed, Nov 7, 2018 at 2:16 PM Sean P. DeNigris <[hidden email]> wrote:
 
johnmci wrote
> "Fake mouse wheel events with all modifier keys pressed (not just ctrl)"

I thought that looked familiar. I needed that change IIRC to enable
horizontal scrolling in Pharo because there was a shortcut conflict.
However, I got bogged down when I went to make the change in Windows and
then got distracted. The reason I chose a more complex hack instead of a
clean fix is because the solution seemed backward compatible so it didn't
break Squeak et al.

I'm sure I speak for everyone in being very grateful for the code!  My point is not to call either of you out on this specific issue & commit.  It is rather to ask people to put more effort into commenting the platform code.  It belongs to all of us but is not Smalltalk, is often platform-specific, and crosses the boundary between platform and Smalltalk, so it is complex and mysterious, and more commentary is very helpful.

Cheers,
Sean

_,,,^..^,,,_
best, Eliot
Reply | Threaded
Open this post in threaded view
|

Re: On community development of the VM's platform code...

timrowledge
 


> On 2018-11-08, at 11:07 AM, Eliot Miranda <[hidden email]> wrote:
>
> I'm sure I speak for everyone in being very grateful for the code!  My point is not to call either of you out on this specific issue & commit.  It is rather to ask people to put more effort into commenting the platform code.  It belongs to all of us but is not Smalltalk, is often platform-specific, and crosses the boundary between platform and Smalltalk, so it is complex and mysterious, and more commentary is very helpful.

Exactly. Comments need to explain  why, why not, what is done, what is still needed someday, obscure points not necessarily obvious to a non-platform expert and so on. If possible point to the doc that explains it too. Yes, the code says what actually happens but anyone that thinks that 'actual' == 'needed' is likely to be very disappointed, very often.

tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Useful random insult:- Not much to show for four billion years of evolution.