| Message ID | 83y4pnbtnc.fsf@gnu.org |
|---|---|
| State | New |
| Headers | show |
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.
--- 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 @@ /* 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 --- 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 (); +} + /* 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); --- gdb/tui/tui-win.c~0 2014-10-29 21:45:50 +0200 +++ gdb/tui/tui-win.c 2014-12-31 13:55:32 +0200 @@ -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) {
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.