Re: [Abook-devel] Patch: mouse support

[Thorsten W. <ed...@th...>]

On Sun, Apr 29, 2012 at 07:42:53PM +0200, Raphaël Droz wrote:
> On Sun, Apr 29, 2012 at 03:52:07PM +0200, Thorsten Wißmann wrote:
> 
> > I also fixed the annoying lag between a click and the reaction by abook.
> > Now the mouse should just work as expected™. Can someone test it and
> > give feedback? (Then we could enable it by default)
> 
> One visual problem I found is that editing a field immediately makes
> some garbage appear in the bottom editing bar ( #X* ).

Thanks for fixing that.

> I also found some problem with wheel-down handling. It appears that
> ncurses before 2005 didn't support it (at least it didn't #define
> corresponding events). Then it comes for --with-ext-mouse ncurses build
> (which is still not the default for gentoo in 5.9), see:
> $ grep -m1 NCURSES_MOUSE_VERSION /usr/include/ncursesw/curses.h
> 
> AFAICT button2 represents the middle click rather than the wheel but the
> patch appeared to work for me because I'm using a newer version of
> rxvt-unicode which embed built-in support for mouse wheel.
> 
> The attached patch should apply on top of yours.

I've also changed your patch, added it to my "Fix lag..."-commit, pushed
it and attached it to this message.

> * It does not attach to BUTTON1_PRESS event anymore as it's not needed,
>   unless I missed something.

I personally prefer the selection on BUTTON1_PRESS rather than on
BUTTON1_CLICKED (press + release), because it feels faster and because
there's no need to wait for the mouse button to be released.

> * Conditionally set mousemask() according to ncurses headers and removed
>   the use of BUTTON2_PRESS which AFAICT is not involved in mouse-wheel-down,
>   whatever NCURSES_MOUSE_VERSION is.
>   There may be more elegant function to do this in the ncurses API but I
>   didn't take the time to look at them.
> 
> * detach from mouse signal before we start editing a field (in
>   edit_field()) and restore before we return.
> 
> * removed some debug statements
> 
> So it's a bit crappy (4x #if NCURSES_MOUSE_VERSION) but tries to play
> nice with ncurses headers. Let me know if you have any idea in order to
> make the whole better.

I reduced that to 2x: once in ui_enable_mouse(bool) and once in ui.h
where I define BUTTON5_PRESSED for old ncurses versions. But i *DON'T*
have any test-system with an old enough ncurses version.

> Finally, I've a bad feeling about was_double_click() / custom handling
> of mouse interval ? do we really need this ? isn't what
> mouseinterval(200) would do ?

No, because mouseinterval(200) buffers the click event for 200ms. I.e.
it buffers a single click event until it definitely can tell it can't be
a double-click. But that's only important if there's a difference
between "single click on item X and then double-click on X" and "only
double-click on X". In our case there's no difference so we don't want
to wait for 200ms for the single click event.

> It may also be a bug in ncurses, or a bad use of the mouse handling API ?
> Could you ensure that this is the right way to fix the lag (I didn't
> find myself the UI laggy) ?

I couldn't find documentation about the "right way" and I don't know any
other ncurses-application that handles double-clicks.

> or that it still happens when we stop listening
> to BUTTON1_PRESSED ? ...

If we stop listening to BUTTON1_PRESS, the lag still is there.

> PS: I also attached the colors-related patches rebased/merged, as they
> will appear in the upstream/master repository once pushed; unless you have some
> additional remarks. (I would personally have preferred to see 2/2
> Fabio's patch merged into Thorsten's one but either way wouldn't hurt).

Feel free to push them upstream.

Thorsten