Re: mouse.c: protocol error in Windows for bind

[Ethan M. <merritt@u.washington.edu>]

On Thursday 19 March 2009 15:32:41 Petr Mikulik wrote:
> > > Can somebody else on Windows reproduce the bug below? I would like this gets 
> > > fixed before 4.2.5 is released.
> > > 
> > > In Windows, there is only one open and active graph window. 
> > > I think this could be the reason while mouse.c:event_keypress() reports 
> > > "protocol error" if you try e.g.:
> > > 
> > > bind 's' 'print "Hello";'
> > > bind 'd' 'set grid;'
> > > bind 'f' 'set time'
> > > bind 'g' 'plot x;'
> > > bind 'x' 'test;'
> > > plot x*x
> > > 
> > > and then press "s" hotkey (or the others) several times. 
> > > 
> > > The code in mouse.c after
> > >    if (ptr->allwindows && ptr->command) {
> > > 
> > > is testing "allwindows" and "active" windows but it could somehow miss the 
> > > case of a single-window graph. I'm not sure what should be the correct fix 
> > > among these several if .. else if ... This well handles case of multiple 
> > > windows (x11, wxt), but what should it do for single-window cases such as 
> > > windows or pm terminals?
> > 
> > It must recognize the current window correctly, or it would have exited the
> > loop before reaching that protocol error message.
> > 1387:	} else if (!current) break;
> > 
> > I think it is more likely that some spurious event is being generated
> > in addition to the desired keypress event, and we should just ignore it.
> > Is the behaviour acceptable if you change the fprintf() to FPRINTF(())?
> 
> Correct behaviour is achieved by setting
> 	ptr->allwindows = 1
> or by
> 	par2 = 0
> With
> 	current = 1
> some events are lost and "protocol error" is shown.
> 
> What (and where) should be set for correct behaviour?

OK, with that set of clues I conclude that the failure happens whenever
the GE_keypress event notification is sent with a non-zero par2.
The only place that can happen is here, and only when
thisTimestamp != lastTimestamp.   Suggested patch is shown below.

%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%

--- gnuplot42/src/win/wgraph.c  2008-10-10 10:21:07.000000000 -0700
+++ gnuplot-4.2.5/src/win/wgraph.c      2009-03-19 16:59:30.000000000 -0700
@@ -1973,9 +1973,13 @@ Wnd_exec_event(LPGW lpgw, LPARAM lparam,
     int mx, my;
     static unsigned long lastTimestamp = 0;
     unsigned long thisTimestamp = GetMessageTime();
+    int par2 = thisTimestamp - lastTimestamp;
+
+    if (type == GE_keypress)
+       par2 = 0;

     GetMousePosViewport(lpgw, &mx, &my);
-    gp_exec_event(type, mx, my, par1, thisTimestamp - lastTimestamp, 0);
+    gp_exec_event(type, mx, my, par1, par2, 0);
     lastTimestamp = thisTimestamp;
 }

%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%

I do not know what this Timestamp test is intended to do.
No equivalent test exists in the x11 or wxt drivers for keypress events.
Perhaps it is intended to handle double-click?  If so it should only
apply to buttonrelease events, not keypress events.

It might also be safe to simply remove the test in mouse.c, like this:
%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
--- gnuplot42/src/mouse.c       2009-02-08 20:43:44.000000000 -0800
+++ gnuplot-4.2.5/src/mouse.c   2009-03-19 16:51:07.000000000 -0700
@@ -1387,7 +1387,7 @@ event_keypress(struct gp_event_t *ge, TB
            } else if (!current) {
                break;
            /* Let user defined bindings overwrite the builtin bindings */
-           } else if ((par2 & 1) == 0 && ptr->command) {
+           } else if (ptr->command) {
                do_string(ptr->command);
                break;
            } else if (ptr->builtin) {
%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
but I am a little reluctant to do that for 4.2.5 because I haven't figured
out what case it was supposed to handle.  We could make the change in 4.3
and see if anyone reports problems with x11 or wxt or os2.

-- 
Ethan A Merritt