Mouse button handling

[Damien Guibouret]

Hello,

Loading the ncurses 5.9 version last week to try it in replacement to the old
5.4 I am still using, I've found some differences into mouse management that
breaks the behaviour in some cases. I take a look at the sources and did some
changes that I think necessary. All the changes I did are into
ncurses/base/lib_mouse.c file (I used 5.9 released version as base) and are
into the attached file with the following explanations.

The first change is to add a variable keeping the state of the buttons (which
ones are pressed) between calls to _nc_mouse_inline. In origin version this
relies in using the bstate of the last recorded mouse event. But in case the
getmouse function has been called, the last recorded mouse event does no more
reflect button state as it was discarded and an older one is used which is
certainly wrong considering button state. The best way to see this behaviour is
by using the 'a' test of test/ncurses program pressing the right button, hold it
enough time for ncurses to not consider it is a click (basically until the press
event message is printed) and release it. The release is not seen as a release
event but as a position event.
So I added a pressedButtons local variable (NOTE: that's certainly wrong in case
there is several pointing device, so it is certainly better to add it into
SCREEN structure, but I do not know to which extent this one could be modified
without breaking anything) that is used to keep button state between calls to
_nc_mouse_inline.
Most of the changes are to take this variable into account.
I still see some open points on this change:
- for the USE_EMX_MOUSE case, I do not know the EMX protocol and cannot test it,
but is the fact that it sets button as pressed and released in same event the
correct interpretation. If so, shouldn't the click event be set also?
- for the release case, I do not understand why prev is checked against
BUTTON_PRESSED | BUTTON_RELEASED instead of only BUTTON_PRESSED (which I used in 
the modified version)?

The second change (the one that lead to the encountered different behaviour) is
due to changes that were done to handle mouse wheel. Documents I found about
xterm control sequence, describes mouse wheel events as being reported with
adding 64 to button press code. As this code is already 32 (+ pressed button),
this lead to having a 96 (+ pressed button) value. So checking mouse wheel
button should not rely on a 64 value mask but on a 96 value (moreover as the 64
value can be obtained when using motion events (in 1002 mode) that are reported
as adding 32 to button press code).
For the button2/button5 case, the event shall be treated even for the
NCURSES_MOUSE_VERSION != 2 case, but with being discarded, else this will
generate a button2 press event that will never be followed by a release event as
wheel does not generate such events.

The next change is into _nc_mouse_parse to limit runcount to effective events
that could remain into circular buffer (for those that are too stressed on the
mouse buttons :-).
I think there could be more change here (handling this into the loop that calls
_mouse_parse to avoid loosing events, setting the runcount as a pointer such as
_nc_mouse_parse could report how many events trully remains into circular buffer
at end) but it is to manage very corner cases (less perhaps for wheel events
that can be quick and numerous).

And the last one is for the getmouse function: it moves the _mouse_eventp
reference one events more than needed so discards a potentially recorded event
(as prev is already the event previous the first free event and has been
discarded, it becomes the first free event, so there is no need to go to the
previous one more). This change is perhaps wrong with considering the
_nc_mouse_parse code as this one merges press/release in a click event that is
set in place of the release one with setting the press one to invalid. So going
backward twice allows reaching a potential event before press/release pair when
with this change, all events before click are lost. To use this change, without
getting this problem, would need to modify _nc_mouse_parse such as it groups all
events that remain at end into adjacent cells (but that means also that a
complete handling of runcount shall be done to know how many events are really
recorded, so some not such simple changes).

I have also a question regarding the wide character handling: the get_wch
function returns a OK code for standard key and a KEY_CODE_YES for KEY_ codes.
But the inverse function (unget_wch) does not allow performing such a
distinction. Does this mean that ungetting KEY_ codes is not allowed (or could
work but will depend on if it enters in conflict with a valid standard key) ?

I hope that at least some of these changes will be usefull,

Regards,

Damien


diff -ur -w ncurses-5.9-orig/ncurses/base/lib_mouse.c 
ncurses-5.9/ncurses/base/lib_mouse.c
--- ncurses-5.9-orig/ncurses/base/lib_mouse.c   2011-01-22 20:47:47.000000000 
+0100
+++ ncurses-5.9/ncurses/base/lib_mouse.c        2011-08-28 23:57:36.000000000 
+0200
@@ -351,6 +351,8 @@
 #ifndef USE_TERM_DRIVER
 #define xterm_kmous "\033[M"
 
+static mmask_t pressedButtons = 0;
+
 static void
 init_xterm_mouse(SCREEN *sp)
 {
@@ -900,30 +902,46 @@
 
 #if USE_EMX_MOUSE
 #define PRESS_POSITION(n) \
+       do { \
        eventp->bstate = MASK_PRESS(n); \
-       if (kbuf[0] & 0x40) \
-           eventp->bstate = MASK_RELEASE(n)
+               pressedButtons |= MASK_PRESS(n); \
+               if (kbuf[0] & 0x40) { \
+                       eventp->bstate = MASK_RELEASE(n); \
+                       pressedButtons &= ~MASK_PRESS(n); \
+               } \
+       } while (0)
 #else
 #define PRESS_POSITION(n) \
-       eventp->bstate = (mmask_t) (prev & MASK_PRESS(n) \
+       do { \
+               eventp->bstate = (mmask_t) (pressedButtons & MASK_PRESS(n) \
                                    ? REPORT_MOUSE_POSITION \
-                                   : MASK_PRESS(n))
+                                       : MASK_PRESS(n)); \
+               pressedButtons |= MASK_PRESS(n); \
+       } while (0)
 #endif
 
        switch (kbuf[0] & 0x3) {
        case 0x0:
-           if (kbuf[0] & 64)
+           if ((kbuf[0] & 96) == 96) {
                eventp->bstate = MASK_PRESS(4);
+               /* Do not memorize this into pressedButtons as there is no 
corresponding
+                * release event */
+           }
            else
                PRESS_POSITION(1);
            break;
 
        case 0x1:
+           if ((kbuf[0] & 96) == 96) {
 #if NCURSES_MOUSE_VERSION == 2
-           if (kbuf[0] & 64)
                eventp->bstate = MASK_PRESS(5);
-           else
+               /* See comment above for button 4 */
+#else
+               /* Ignore this event as it is not a true press of the button */
+               eventp->bstate = REPORT_MOUSE_POSITION;
 #endif
+           }
+           else
                PRESS_POSITION(2);
            break;
 
@@ -939,12 +957,13 @@
             * release, we can infer the button actually released by looking at
             * the previous event.
             */
-           if (prev & (BUTTON_PRESSED | BUTTON_RELEASED)) {
+           if (pressedButtons & BUTTON_PRESSED) {
                eventp->bstate = BUTTON_RELEASED;
                for (b = 1; b <= MAX_BUTTONS; ++b) {
-                   if (!(prev & MASK_PRESS(b)))
+                   if (!(pressedButtons & MASK_PRESS(b)))
                        eventp->bstate &= ~MASK_RELEASE(b);
                }
+               pressedButtons = 0;
            } else {
                /*
                 * XFree86 xterm will return a stream of release-events to
@@ -994,7 +1013,7 @@
        return;
 
     if (on) {
-
+       pressedButtons = 0;
        switch (sp->_mouse_type) {
        case M_XTERM:
 #if NCURSES_EXT_FUNCS
@@ -1113,6 +1132,9 @@
     }
 
     /* find the start of the run */
+    if (runcount > EV_MAX) {
+      runcount = EV_MAX;
+    }
     runp = eventp;
     for (n = runcount; n > 0; n--) {
        runp = PREV(runp);
@@ -1368,7 +1390,7 @@
                              (long) IndexEV(SP_PARM, prev)));
 
            prev->id = INVALID_EVENT;   /* so the queue slot becomes free */
-           SP_PARM->_mouse_eventp = PREV(prev);
+           SP_PARM->_mouse_eventp = prev;
            result = OK;
        }
     }