Re: [PATCHv2 1/2] Initial TUI mouse support

[Tom Tromey <tom@tromey.com>]

>>>>> "Hannes" == Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> writes:

Hannes> Implements an overridable tui_win_info::click method whose arguments
Hannes> are the mouse coordinates inside the specific window, and the mouse
Hannes> button clicked.

Hannes> And if the curses implementation supports 5 buttons, the 4th and 5th
Hannes> buttons are used for scrolling.

Thank you for doing this.  I have a few minor comments.

Hannes> Should there be a configure option for this at build time?
Hannes> I don't know if the mousemask/getmouse functions are available for
Hannes> all curses variants, so I would imagine an option --enable-mouse:
Hannes> - yes -> error if getmouse can't be linked
Hannes> - no -> just disable
Hannes> - auto -> enable if getmouse can be linked

I think it's fine to move forward, and if we have problems with it, we
can add a configure check for 'mousemask' and use that to disable the
new code.

The ncurses man page says we could check NCURSES_MOUSE_VERSION to see if
this is available.  So maybe new configure code isn't even needed.

       The feature macro NCURSES_MOUSE_VERSION is provided so the preprocessor
       can be used to test whether these features are present.

Hannes> +  /* Called for each mouse click inside this window.  */
Hannes> +  virtual void click (int mouse_x, int mouse_y, int mouse_button)
Hannes> +  {

I think it would be nice if this comment documented the meaning of x/y
(are they window- or terminal-relative?  And 0- or 1-based?) and also
the possible/typical values of mouse_button (0- or 1-based?).

Hannes> diff --git a/gdb/tui/tui-io.c b/gdb/tui/tui-io.c
Hannes> index a2be4d4353e..7787789f0c7 100644
Hannes> --- a/gdb/tui/tui-io.c
Hannes> +++ b/gdb/tui/tui-io.c
Hannes> @@ -33,6 +33,7 @@
Hannes>  #include "tui/tui-wingeneral.h"
Hannes>  #include "tui/tui-file.h"
Hannes>  #include "tui/tui-out.h"
Hannes> +#include "tui/tui-source.h"

Why was this needed?

Hannes> +		if ((mev.bstate & BUTTON1_CLICKED)
Hannes> +		    || (mev.bstate & BUTTON2_CLICKED)
Hannes> +		    || (mev.bstate & BUTTON3_CLICKED))

gdb style would use a '!= 0' after each test.

Hannes> +		  {
Hannes> +		    int button = (mev.bstate & BUTTON1_CLICKED) ? 1
Hannes> +		      : (mev.bstate & BUTTON2_CLICKED) ? 2
Hannes> +		      : 3;

The right-hand-side here would normally have an extra layer of () so
that the continuation lines are indented.  Also it would be good to
reformat that so the "?..:" parts line up more clearly.

thanks,
Tom