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

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

On Monday 23 March 2009 15:17:33 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;
> >  }
> 
> 
> This patch works OK, thanks!

Great.  I have added it to cvs.  Please check that I didn't mess it up somehow,
since I can't test for a successful Windows build here.

Any other issues to be resolved before pushing out 4.2.5 ?







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



-- 
Ethan A Merritt
Biomolecular Structure Center
University of Washington, Seattle 98195-7742