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

[Petr M. <mi...@ph...>]

> > > > 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;
>  }


This patch works OK, thanks!


> %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
> 
> 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.

There are doubleclicks, see "help mouse".
MB1 copies mouse position to clipboard. This needs to be a doubleclick 
because single click focuses/selects the window.

> 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.

This patch also fixes the problem (patch 1 is not applied).

I think the patch 1 should be put to cvs.
I don't know why there is "(par2 & 1) == 0".

---
PM