[PATCHSET,2/4] Fix various issue in TUI
Commit Message
This patch fixes the problem whereby setting the border attributes had
no effect whatsoever. Turns out no one was calling the functions that
detected changes in these settings and applied them to windows.
While at that, this also fixes a copy/paste error in a comment to a
function.
OK?
2014-12-31 Eli Zaretskii <eliz@gnu.org>
* tui/tui-win.c (tui_rehighlight_all): New function.
* tui/tui-win.h: Add prototype for tui_rehighlight_all.
* tui/tui-command.c (tui_dispatch_ctrl_char): When Ctrl-L was
pressed, call tui_rehighlight_all if tui_update_variables
indicates it should be.
* tui/tui-win.c (tui_refresh_all_command): Call
tui_rehighlight_all if tui_update_variables indicates it
should be.
* tui/tui-hooks.c (tui_note_setting_change): New function.
(tui_install_hooks): Install an observer for "command
parameter changed".
(tui_remove_hooks): Uninstall the observer.
Comments
On 12/31/2014 05:45 PM, Eli Zaretskii wrote:
> This patch fixes the problem whereby setting the border attributes had
> no effect whatsoever.
Rather, you need to switch out of the TUI and back, with c-x,a.
> Turns out no one was calling the functions that
> detected changes in these settings and applied them to windows.
Thanks.
> While at that, this also fixes a copy/paste error in a comment to a
> function.
Could you please push this part separately?
>
> OK?
>
> 2014-12-31 Eli Zaretskii <eliz@gnu.org>
>
> * tui/tui-win.c (tui_rehighlight_all): New function.
>
No empty lines between ChangeLog entries, please.
> * tui/tui-win.h: Add prototype for tui_rehighlight_all.
>
> * tui/tui-command.c (tui_dispatch_ctrl_char): When Ctrl-L was
> pressed, call tui_rehighlight_all if tui_update_variables
> indicates it should be.
tabs vs spaces.
>
> * tui/tui-win.c (tui_refresh_all_command): Call
> tui_rehighlight_all if tui_update_variables indicates it
> should be.
>
> * tui/tui-hooks.c (tui_note_setting_change): New function.
> (tui_install_hooks): Install an observer for "command
> parameter changed".
> (tui_remove_hooks): Uninstall the observer.
>
>
> --- gdb/tui/tui-win.h~0 2014-06-11 18:34:41 +0300
> +++ gdb/tui/tui-win.h 2014-12-31 13:38:18 +0200
> @@ -35,6 +35,7 @@
> extern void tui_set_win_focus_to (struct tui_win_info *);
> extern void tui_resize_all (void);
> extern void tui_refresh_all_win (void);
> +extern void tui_rehighlight_all (void);
>
> extern chtype tui_border_ulcorner;
> extern chtype tui_border_urcorner;
>
>
> --- gdb/tui/tui-command.c~0 2014-10-29 21:45:50 +0200
> +++ gdb/tui/tui-command.c 2014-12-31 13:49:43 +0200
> @@ -50,7 +50,12 @@
Seems like you're not using git diff to generate the diffs
for some reason. If using GNU diff, could you make sure
to use the "-p" switch? It makes review a lot easier.
>
> --- gdb/tui/tui-command.c~0 2014-10-29 21:45:50 +0200
> +++ gdb/tui/tui-command.c 2014-12-31 13:49:43 +0200
> @@ -50,7 +50,12 @@
>
> /* Handle the CTRL-L refresh for each window. */
> if (ch == '\f')
> - tui_refresh_all_win ();
> + {
> + if (tui_update_variables ())
> + tui_rehighlight_all ();
> +
> + tui_refresh_all_win ();
tui_refresh_all_win already calls tui_check_and_display_highlight_if_needed,
so why do we need to call tui_rehighlight_all? Is it the tui_update_variables
call that is really missing? But why would we need to call
tui_update_variables on CTRL-L, if we already call it when the user
changes the variables?
> + }
>
> /* If the command window has the logical focus, or no-one does
> assume it is the command window; in this case, pass the character
>
>
> --- gdb/tui/tui-hooks.c~0 2014-06-11 18:34:41 +0300
> +++ gdb/tui/tui-hooks.c 2014-12-31 14:41:11 +0200
> @@ -247,12 +247,23 @@
> tui_display_main ();
> }
>
> +/* Refresh the display when settings important to us change. */
> +static void
> +tui_note_setting_change (const char *param, const char *value)
> +{
> + if (tui_active
> + && strncmp (param, "tui ", sizeof ("tui ") - 1) == 0
> + && tui_update_variables ())
> + tui_rehighlight_all ();
> +}
> +
Please do this from the "set" hook of the relevant commands instead.
IOW, replace NULL below (and in the other commands):
add_setshow_enum_cmd ("active-border-mode", no_class, tui_border_mode_enums,
&tui_active_border_mode, _("\
...
NULL,
show_tui_active_border_mode,
&tui_setlist, &tui_showlist);
> @@ -985,11 +995,14 @@
> /* Make sure the curses mode is enabled. */
> tui_enable ();
>
> + if (tui_update_variables ())
> + tui_rehighlight_all ();
> +
> tui_refresh_all_win ();
> }
>
I don't understand this one. When is it ever necessary?
Thanks,
Pedro Alves
> Date: Mon, 05 Jan 2015 19:11:21 +0000
> From: Pedro Alves <palves@redhat.com>
>
> On 12/31/2014 05:45 PM, Eli Zaretskii wrote:
> > This patch fixes the problem whereby setting the border attributes had
> > no effect whatsoever.
>
> Rather, you need to switch out of the TUI and back, with c-x,a.
Yeah, well... not really friendly.
> > While at that, this also fixes a copy/paste error in a comment to a
> > function.
>
> Could you please push this part separately?
Will do.
> > 2014-12-31 Eli Zaretskii <eliz@gnu.org>
> >
> > * tui/tui-win.c (tui_rehighlight_all): New function.
> >
>
> No empty lines between ChangeLog entries, please.
Why are we using a format that is different from how Emacs formats
ChangeLog entries? It's just annoying extra manual work.
> > --- gdb/tui/tui-command.c~0 2014-10-29 21:45:50 +0200
> > +++ gdb/tui/tui-command.c 2014-12-31 13:49:43 +0200
> > @@ -50,7 +50,12 @@
>
> Seems like you're not using git diff to generate the diffs
> for some reason.
This wasn't done in the git repo, I did it with the GDB 7.8.1 sources.
> If using GNU diff, could you make sure to use the "-p" switch? It
> makes review a lot easier.
I'll try to remember. (The ChangeLog entries state the function
names, don't they?)
> > --- gdb/tui/tui-command.c~0 2014-10-29 21:45:50 +0200
> > +++ gdb/tui/tui-command.c 2014-12-31 13:49:43 +0200
> > @@ -50,7 +50,12 @@
> >
> > /* Handle the CTRL-L refresh for each window. */
> > if (ch == '\f')
> > - tui_refresh_all_win ();
> > + {
> > + if (tui_update_variables ())
> > + tui_rehighlight_all ();
> > +
> > + tui_refresh_all_win ();
>
>
> tui_refresh_all_win already calls tui_check_and_display_highlight_if_needed,
> so why do we need to call tui_rehighlight_all? Is it the tui_update_variables
> call that is really missing?
Yes, the call to tui_update_variables is required to take notice of
the changes. Also, I'm not sure tui_refresh_all_win does that for all
windows, it has a switch that handles on 3 types.
> But why would we need to call tui_update_variables on CTRL-L, if we
> already call it when the user changes the variables?
I thought it'd be better to ensure this is called directly on refresh,
since C-l can be used in all kinds of weird situations where the
screen is messed up e.g. by the program being debugged.
> > --- gdb/tui/tui-hooks.c~0 2014-06-11 18:34:41 +0300
> > +++ gdb/tui/tui-hooks.c 2014-12-31 14:41:11 +0200
> > @@ -247,12 +247,23 @@
> > tui_display_main ();
> > }
> >
> > +/* Refresh the display when settings important to us change. */
> > +static void
> > +tui_note_setting_change (const char *param, const char *value)
> > +{
> > + if (tui_active
> > + && strncmp (param, "tui ", sizeof ("tui ") - 1) == 0
> > + && tui_update_variables ())
> > + tui_rehighlight_all ();
> > +}
> > +
>
> Please do this from the "set" hook of the relevant commands instead.
> IOW, replace NULL below (and in the other commands):
>
> add_setshow_enum_cmd ("active-border-mode", no_class, tui_border_mode_enums,
> &tui_active_border_mode, _("\
> ...
> NULL,
> show_tui_active_border_mode,
> &tui_setlist, &tui_showlist);
OK.
> > @@ -985,11 +995,14 @@
> > /* Make sure the curses mode is enabled. */
> > tui_enable ();
> >
> > + if (tui_update_variables ())
> > + tui_rehighlight_all ();
> > +
> > tui_refresh_all_win ();
> > }
> >
>
> I don't understand this one. When is it ever necessary?
Same as Ctrl-l: this is the "refresh" command.
On 01/05/2015 07:40 PM, Eli Zaretskii wrote:
>> Date: Mon, 05 Jan 2015 19:11:21 +0000
>> From: Pedro Alves <palves@redhat.com>
>>
>> On 12/31/2014 05:45 PM, Eli Zaretskii wrote:
>>> This patch fixes the problem whereby setting the border attributes had
>>> no effect whatsoever.
>>
>> Rather, you need to switch out of the TUI and back, with c-x,a.
>
> Yeah, well... not really friendly.
Yeah, definitely agreed.
>>> 2014-12-31 Eli Zaretskii <eliz@gnu.org>
>>>
>>> * tui/tui-win.c (tui_rehighlight_all): New function.
>>>
>>
>> No empty lines between ChangeLog entries, please.
>
> Why are we using a format that is different from how Emacs formats
> ChangeLog entries? It's just annoying extra manual work.
That's a bit exaggerated, given you always have to edit
the ChangeLog entry anyway. Both the emacs, and the gnu coding
conventions manuals say that when changes are related, they get no line
between:
http://www.gnu.org/software/emacs/manual/html_node/emacs/Format-of-ChangeLog.html
"One entry can describe several changes; each change should have
its own item, or its own line in an item. Normally there should be a blank
line between items. When items are related (parts of the same change,
in different places), group them by leaving no blank line between them."
https://www.gnu.org/prep/standards/html_node/Style-of-Change-Logs.html#Style-of-Change-Logs
"Separate unrelated change log entries with blank lines. Don’t put blank lines
between individual changes of an entry. You can omit the file name and the asterisk
when successive individual changes are in the same file."
Given we prefer that unrelated changes are split into separate patches,
it follows that the norm is to not include the empty line.
emacs can't guess whether the changes are related or not, but IMO a much
better default would be to assume yes.
>
>>> --- gdb/tui/tui-command.c~0 2014-10-29 21:45:50 +0200
>>> +++ gdb/tui/tui-command.c 2014-12-31 13:49:43 +0200
>>> @@ -50,7 +50,12 @@
>>
>> Seems like you're not using git diff to generate the diffs
>> for some reason.
>
> This wasn't done in the git repo, I did it with the GDB 7.8.1 sources.
>
>> If using GNU diff, could you make sure to use the "-p" switch? It
>> makes review a lot easier.
>
> I'll try to remember.
Thanks.
> (The ChangeLog entries state the function
> names, don't they?)
They do, but then without -p it's harder to map a change to the
corresponding ChangeLog entry, exactly because the hunk is missing
the function name that -p gives you.
>
>>> --- gdb/tui/tui-command.c~0 2014-10-29 21:45:50 +0200
>>> +++ gdb/tui/tui-command.c 2014-12-31 13:49:43 +0200
>>> @@ -50,7 +50,12 @@
>>>
>>> /* Handle the CTRL-L refresh for each window. */
>>> if (ch == '\f')
>>> - tui_refresh_all_win ();
>>> + {
>>> + if (tui_update_variables ())
>>> + tui_rehighlight_all ();
>>> +
>>> + tui_refresh_all_win ();
>>
>>
>> tui_refresh_all_win already calls tui_check_and_display_highlight_if_needed,
>> so why do we need to call tui_rehighlight_all? Is it the tui_update_variables
>> call that is really missing?
>
> Yes, the call to tui_update_variables is required to take notice of
> the changes. Also, I'm not sure tui_refresh_all_win does that for all
> windows, it has a switch that handles on 3 types.
If there's a window type that is mishandled in tui_refresh_all_win,
then we should fix it there. I'd prefer to start out with the minimal
that that just fixes what we know is broken.
>
>> But why would we need to call tui_update_variables on CTRL-L, if we
>> already call it when the user changes the variables?
>
> I thought it'd be better to ensure this is called directly on refresh,
> since C-l can be used in all kinds of weird situations where the
> screen is messed up e.g. by the program being debugged.
If the screen is messed up, then we should always unconditionally
redraw everything, not do it only when the user has changed tui
settings, which is what tui_update_variables concerns about. But
that's what tui_refresh_all_win does already, so I don't think that
change really does anything. Unless there's a real reason, I'd rather
drop those hunks.
>
>>> --- gdb/tui/tui-hooks.c~0 2014-06-11 18:34:41 +0300
>>> +++ gdb/tui/tui-hooks.c 2014-12-31 14:41:11 +0200
>>> @@ -247,12 +247,23 @@
>>> tui_display_main ();
>>> }
>>>
>>> +/* Refresh the display when settings important to us change. */
>>> +static void
>>> +tui_note_setting_change (const char *param, const char *value)
>>> +{
>>> + if (tui_active
>>> + && strncmp (param, "tui ", sizeof ("tui ") - 1) == 0
>>> + && tui_update_variables ())
>>> + tui_rehighlight_all ();
>>> +}
>>> +
>>
>> Please do this from the "set" hook of the relevant commands instead.
>> IOW, replace NULL below (and in the other commands):
>>
>> add_setshow_enum_cmd ("active-border-mode", no_class, tui_border_mode_enums,
>> &tui_active_border_mode, _("\
>> ...
>> NULL,
>> show_tui_active_border_mode,
>> &tui_setlist, &tui_showlist);
>
> OK.
>
>>> @@ -985,11 +995,14 @@
>>> /* Make sure the curses mode is enabled. */
>>> tui_enable ();
>>>
>>> + if (tui_update_variables ())
>>> + tui_rehighlight_all ();
>>> +
>>> tui_refresh_all_win ();
>>> }
>>>
>>
>> I don't understand this one. When is it ever necessary?
>
> Same as Ctrl-l: this is the "refresh" command.
Then I don't think this is necessary.
It may also be simpler to just call tui_refresh_all_win
from the settings' set hook(s) instead of adding
the new tui_rehighlight_all function. I don't imagine
a full redraw being a performance issue here?
Thanks,
Pedro Alves
> Date: Mon, 05 Jan 2015 19:11:21 +0000
> From: Pedro Alves <palves@redhat.com>
>
> > While at that, this also fixes a copy/paste error in a comment to a
> > function.
>
> Could you please push this part separately?
Done.
@@ -35,6 +35,7 @@
extern void tui_set_win_focus_to (struct tui_win_info *);
extern void tui_resize_all (void);
extern void tui_refresh_all_win (void);
+extern void tui_rehighlight_all (void);
extern chtype tui_border_ulcorner;
extern chtype tui_border_urcorner;
@@ -50,7 +50,12 @@
/* Handle the CTRL-L refresh for each window. */
if (ch == '\f')
- tui_refresh_all_win ();
+ {
+ if (tui_update_variables ())
+ tui_rehighlight_all ();
+
+ tui_refresh_all_win ();
+ }
/* If the command window has the logical focus, or no-one does
assume it is the command window; in this case, pass the character
@@ -247,12 +247,23 @@
tui_display_main ();
}
+/* Refresh the display when settings important to us change. */
+static void
+tui_note_setting_change (const char *param, const char *value)
+{
+ if (tui_active
+ && strncmp (param, "tui ", sizeof ("tui ") - 1) == 0
+ && tui_update_variables ())
+ tui_rehighlight_all ();
+}
+
/* Observers created when installing TUI hooks. */
static struct observer *tui_bp_created_observer;
static struct observer *tui_bp_deleted_observer;
static struct observer *tui_bp_modified_observer;
static struct observer *tui_inferior_exit_observer;
static struct observer *tui_about_to_proceed_observer;
+static struct observer *tui_setting_changed_observer;
/* Install the TUI specific hooks. */
void
@@ -278,6 +289,9 @@
= observer_attach_about_to_proceed (tui_about_to_proceed);
deprecated_register_changed_hook = tui_register_changed_hook;
+
+ tui_setting_changed_observer
+ = observer_attach_command_param_changed (tui_note_setting_change);
}
/* Remove the TUI specific hooks. */
@@ -300,6 +314,8 @@
tui_inferior_exit_observer = NULL;
observer_detach_about_to_proceed (tui_about_to_proceed_observer);
tui_about_to_proceed_observer = NULL;
+ observer_detach_command_param_changed (tui_setting_changed_observer);
+ tui_setting_changed_observer = NULL;
}
void _initialize_tui_hooks (void);
@@ -649,6 +649,16 @@
}
+void
+tui_rehighlight_all (void)
+{
+ enum tui_win_type type;
+
+ for (type = SRC_WIN; type < MAX_MAJOR_WINDOWS; type++)
+ tui_check_and_display_highlight_if_needed (tui_win_list[type]);
+}
+
+
/* Resize all the windows based on the terminal size. This function
gets called from within the readline sinwinch handler. */
void
@@ -985,11 +995,14 @@
/* Make sure the curses mode is enabled. */
tui_enable ();
+ if (tui_update_variables ())
+ tui_rehighlight_all ();
+
tui_refresh_all_win ();
}
-/* Set the height of the specified window. */
+/* Set the tab width of the specified window. */
static void
tui_set_tab_width_command (char *arg, int from_tty)
{