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
_______________________________________________
help-smalltalk mailing list
[hidden email]
http://lists.gnu.org/mailman/listinfo/help-smalltalk
Locked
gtk-event-loop.patch (15K)