[PATCHSET] [2/4] Fix various issue in TUI

[Pedro Alves]

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