From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15788 invoked by alias); 6 Jan 2015 16:03:21 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 15776 invoked by uid 89); 6 Jan 2015 16:03:20 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 06 Jan 2015 16:03:09 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t06G369e010595 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Tue, 6 Jan 2015 11:03:07 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t06G2sMD001966; Tue, 6 Jan 2015 11:02:59 -0500 Message-ID: <54AC072D.9050006@redhat.com> Date: Tue, 06 Jan 2015 16:03:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0 MIME-Version: 1.0 To: Eli Zaretskii CC: gdb-patches@sourceware.org Subject: Re: [PATCHSET] [2/4] Fix various issue in TUI References: <83y4pnbtnc.fsf@gnu.org> <54AAE1D9.9000409@redhat.com> <83h9w5819b.fsf@gnu.org> In-Reply-To: <83h9w5819b.fsf@gnu.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit X-SW-Source: 2015-01/txt/msg00076.txt.bz2 On 01/05/2015 07:40 PM, Eli Zaretskii wrote: >> Date: Mon, 05 Jan 2015 19:11:21 +0000 >> From: Pedro Alves >> >> 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 >>> >>> * 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