|
From: Ethan M. <merritt@u.washington.edu> - 2009-03-20 00:01:52
|
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
|