Re: [Help-smalltalk] Gtk event loop

[Paolo Bonzini]

Despite the apparent harshness of the review, it's really good stuff. 
Thanks!

Go through the comments in the order I mention (see asterisked numbers; 
each number refers to the text before) and commit the solution to each 
part (which may not be exactly what I write of course!) separately so 
it's easy to understand when things wrong.

So let's start... first: where is the ChangeLog? :-)

> +    handleInnerEvent [
> +
> +       | event |
> +        event := GTK.Gtk events.
> +        event ifNotNil: [ | args |
> +            args := Array new: event n_params value.
> +            1 to: args size do: [ :i | args at: i put: (event data value at: i 
> - 1) ].
> +            [ ^ (event receiver value perform: event selector value 
> withArguments: args) ] on: Exception do: [ :exception |
> +               Behavior debuggerClass open ] ]
> +    ]

Why the handler?  It shouldn't be necessary as the inner event runs in 
its own Smalltalk process. *10*

> +    insideLoop [
> +
> +       | event |
> +        eventSem wait.
> +        event := GTK.Gtk events.
> +        event ifNotNil: [ | args |
> +            args := Array new: event n_params value.
> +            1 to: args size do: [ :i | args at: i put: (event data value at: i 
> - 1) ].
> +            [ GTK.Gtk result: (event receiver value perform: event selector 
> value withArguments: args) mutex: event mutex value ] on: Exception do: [ 
> :exception |
> +               event status value: 0.
> +               Behavior debuggerClass open.
> +               GTK.Gtk result: nil mutex: event mutex value ] ]

What about something like this instead, trapping Exception is almost 
always wrong (you trap Notification and Warning as well):

[ [ result := event receiver value perform: event selector value 
withArguments: args ]
   on: System.UnhandledException do: [ :ex |
       event status value: 0.
       ex pass ] ]
           ensure: [ GTK.Gtk result: result mutex: event mutex value ]

*11*

> -       sem := Semaphore new.
> -       GTK.Gtk main: sem.

> +       eventSem:= Semaphore new.
> +       GTK.Gtk eventSemaphore: eventSem.
> +      GTK.Gtk mainLoop.
> +       [ self insideLoop.

What about instead keeping more of the old code: *3*

   sem := Semaphore new.
   GTK.Gtk mainLoop: sem.
   [ self insideLoop: sem

?

(BTW I'd rename insideLoop to #processEvents or something like that). *2*

> +          true ] whileTrue: [ "DOES NOT WORK ::::::  Processor yield" ]

Is it needed at all, since we yield when waiting on sem? *1*  Just do

    [ self insideLoop ] repeat

(or the similar thing with sem if you accept my suggestion above).

> diff --git a/packages/gtk/MoreFuncs.st b/packages/gtk/MoreFuncs.st
> index a07ae29..d16210d 100644
> --- a/packages/gtk/MoreFuncs.st
> +++ b/packages/gtk/MoreFuncs.st
> @@ -12,9 +12,9 @@ Gtk class extend [
>         
>       ]
>
> -    main: aSemaphore [
> +    mainLoop [
>         <category: 'C call-outs'>
> -       <asyncCCall: 'gstGtkMain' args: #(#smalltalk )>
> +       <asyncCCall: 'gstGtkMain' args: #()>

Doesn't need to be asynchronous anymore.  In fact, I'd like to deprecate 
#asyncCCall:, I'm not even sure I understand their implications.

Just make it return void. *4*

> +static OOP result = NULL;

This is never read?!?

Also, maybe you want to move it to SmalltalkClosureStruct. *5*

> +typedef struct SmalltalkClosureStruct {
> +  OOP receiver;
> +  OOP selector;
> +  OOP *data;
> +  OOP widget;
> +  int n_params;
> +  GMutex *mutex;
> +  int status
> +} SmalltalkClosureStruct;
> +
> +static SmalltalkClosureStruct *event;

Does not need to be static. *6*

> -static void my_gtk_main (OOP semaphore);
> +static void my_gtk_main ();

As above, consider keeping the parameter. *3*

> +  event = malloc (sizeof (*event));

Nowhere freed. *7*

> +  event->receiver = stc->receiver;
> +  event->selector = stc->selector;

registerOOP on these and args.  Even better, construct a DirectedMessage 
object so that #insideLoop and #handleInnerEvent can just do "event 
message value send".  Right now they have duplicated code! *7*

> +  if (g_thread_self () != main_thread) {

g_thread_equal. *8*

> +    event->mutex = g_mutex_new ();
> +    g_mutex_lock (event->mutex);
> +
> +    _gst_vm_proxy->syncSignal (event_sem, true);
> +    _gst_vm_proxy->wakeUp ();
> +
> +    g_mutex_lock (event->mutex);

Locking twice is surely wrong? *9*

> +    // Smalltalk event handling raises an exception launch an inner
> +    // event loop
> +    //
> +    if (event->status == 0)
> +      gtk_main ();

Longer longer longer explanation needed? (And no // comments). Actually, 
let's postpone this to after the next iteration.

> +void
> +set_event_semaphore (OOP semaphore)
> +{
> +  event_sem = semaphore;
> +  _gst_vm_proxy->registerOOP (semaphore);
> +}

Unnecessary if you pass the semaphore to my_gtk_main. *3*

> +void
> +set_result (OOP anObject, GMutex *mutex)
> +{
> +  result = anObject;
> +  g_mutex_unlock (mutex);
> +}

Pass back the entire events struct so that you can also call 
unregisterOOP, free the args, and free the struct itself.  If you make 
it like this:

void
set_event_result (SmalltalkClosureStruct *event, OOP anObject)
{
  event->result = anObject;
  g_mutex_unlock (event->mutex);
}

you can even declare it as an instance-side method in 
SmalltalkClosureStruct.  Much nicer that way! *5*

> -       self
> +       "self
>             connectSignal: 'paste-clipboard' to: self selector: #paste 
> userData: nil;
>               connectSignal: 'cut-clipboard' to: self selector: #cut userData: 
> nil.
>
> @@ -24,7 +24,7 @@ GTK.GtkTextView subclass: GtkTextWidget [
>               connectSignal: 'begin-user-action' to: self selector: 
> #'beginUserAction' userData: nil;
>               connectSignal: 'end-user-action' to: self selector: 
> #'endUserAction' userData: nil;
>               connectSignal: 'insert-text' to: self selector: 
> #'insert:at:text:size:' userData: nil;
> -            connectSignal: 'delete-range' to: self selector: 
> #'delete:from:to:' userData: nil
> +            connectSignal: 'delete-range' to: self selector: #'delete:from:to:' 
> userData: nil"

What's the problem?  Things like this should not be there for the new 
event loop to go in.


> @@ -218,10 +221,7 @@ THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE 
> SOFTWARE.';
>         <category: 'file events'>
>
>         (GTK.GtkFileChooserDialog save: 'Save image as...' parent: window)
> -           runNonBlockingAndDo: [ :dlg :res |
> -               imageName := dlg getFilename.
> -               dlg destroy.
> -               res = GTK.Gtk gtkResponseAccept ifTrue: [ self saveImage: [ 
> ObjectMemory snapshot: imageName ] ] ]
> +           run

Same here of course.

> -<start>VisualGST.VisualGST open.
> -    GTK.Gtk main</start>
> +<start>
> +        | a |
> +       VisualGST.VisualGST open.
> +       [ GTK.Gtk main ] fork.
> +       a := Semaphore new.
> +       a wait.
> +       'byebye' printNl.
> +</start>

This is just debugging code I guess?

Paolo