diff mbox

Improve/fix the TUI's current source line highlight

Message ID 186859064.9289078.1552667917410@mail.yahoo.com
State New, archived
Headers

Commit Message

Terekhov, Mikhail via Gdb-patches March 15, 2019, 4:38 p.m. UTC 
  Am Donnerstag, 14. März 2019, 18:47:56 MEZ hat Pedro Alves <palves@redhat.com> Folgendes geschrieben: 

> WDYT?

The result looks very nice.


> +/* If true, we're highlighting the current source line in reverse
> +  video mode.  */
> +static bool reverse_mode_p = true;

Shouldn't this variable be initialized to false?
Weird things happen on my end with true (sometimes the cmd window contents are reversed).


Also, I had to add the following to make it work on windows
(but beware, I'm using pdcurses, not ncurses, and I don't know if ncurses
for windows needs this as well):

Comments

Pedro Alves March 15, 2019, 4:45 p.m. UTC | #1 
On 03/15/2019 04:38 PM, Hannes Domani via gdb-patches wrote:
> Am Donnerstag, 14. März 2019, 18:47:56 MEZ hat Pedro Alves <palves@redhat.com> Folgendes geschrieben: 
> 
>> WDYT?
> 
> The result looks very nice.
> 
> 
>> +/* If true, we're highlighting the current source line in reverse
>> +  video mode.  */
>> +static bool reverse_mode_p = true;
> 
> Shouldn't this variable be initialized to false?
> Weird things happen on my end with true (sometimes the cmd window contents are reversed).
> 

Wow, of course.  I have no idea how I missed that.

> 
> Also, I had to add the following to make it work on windows
> (but beware, I'm using pdcurses, not ncurses, and I don't know if ncurses
> for windows needs this as well):

Thanks, but can you explain it?  It is not obvious to me.

Thanks,
Pedro Alves
Eli Zaretskii March 17, 2019, 4:07 p.m. UTC | #2 
> Date: Fri, 15 Mar 2019 16:38:37 +0000 (UTC)
> From: "Hannes Domani via gdb-patches" <gdb-patches@sourceware.org>
> 
> Also, I had to add the following to make it work on windows
> (but beware, I'm using pdcurses, not ncurses, and I don't know if ncurses
> for windows needs this as well):

Ncurses doesn't need this.

> --- a/gdb/tui/tui-io.c
> +++ b/gdb/tui/tui-io.c
> @@ -369,6 +386,17 @@ apply_ansi_escape (WINDOW *w, const char *buf)
> 
>    if (reverse_mode_p)
>      {
> +#if defined(__MINGW32__)
> +      if (style.get_foreground ().is_basic ()
> +         && style.get_foreground ().get_value ()
> +         == (ncurses_norm_attr & 15))
> +       style.set_fg (ui_file_style::NONE);
> +      if (style.get_background ().is_basic ()
> +         && style.get_background ().get_value ()
> +         == ((ncurses_norm_attr >> 4) & 15))
> +       style.set_bg (ui_file_style::NONE);
> +#endif
> +

Could you describe what happens with pdcurses if you don't make this
change?  It's strange that pdcurses cannot use explicit color
specification if the color is the default one.
Terekhov, Mikhail via Gdb-patches March 17, 2019, 4:46 p.m. UTC | #3 
Am Sonntag, 17. März 2019, 17:07:46 MEZ hat Eli Zaretskii <eliz@gnu.org> Folgendes geschrieben: 
> > Date: Fri, 15 Mar 2019 16:38:37 +0000 (UTC)> > From: "Hannes Domani via gdb-patches" <gdb-patches@sourceware.org>> > > > Also, I had to add the following to make it work on windows> > (but beware, I'm using pdcurses, not ncurses, and I don't know if ncurses> > for windows needs this as well):>
> Ncurses doesn't need this.>
>
> > --- a/gdb/tui/tui-io.c> > +++ b/gdb/tui/tui-io.c> > @@ -369,6 +386,17 @@ apply_ansi_escape (WINDOW *w, const char *buf)> > > >    if (reverse_mode_p)> >      {> > +#if defined(__MINGW32__)> > +      if (style.get_foreground ().is_basic ()> > +         && style.get_foreground ().get_value ()> > +         == (ncurses_norm_attr & 15))> > +       style.set_fg (ui_file_style::NONE);> > +      if (style.get_background ().is_basic ()> > +         && style.get_background ().get_value ()> > +         == ((ncurses_norm_attr >> 4) & 15))> > +       style.set_bg (ui_file_style::NONE);> > +#endif> > +>
> Could you describe what happens with pdcurses if you don't make this> change?  It's strange that pdcurses cannot use explicit color> specification if the color is the default one.
Actually, you can disregard that.
Before you added that fix for windows, I found a different workaround,it was to set the "normal" color in esc.style to gray.
I forgot that I did that, and it backfired now.So I removed that again, and everything is fine now even without that above change.
Sorry for the noise.


RegardsHannes Domani
Eli Zaretskii March 17, 2019, 4:52 p.m. UTC | #4 
> Date: Sun, 17 Mar 2019 16:46:37 +0000 (UTC)
> From: "Hannes Domani via gdb-patches" <gdb-patches@sourceware.org>
> 
> Actually, you can disregard that.
> Before you added that fix for windows, I found a different workaround,it was to set the "normal" color in esc.style to gray.
> I forgot that I did that, and it backfired now.So I removed that again, and everything is fine now even without that above change.

Great, thanks for unlocking this mystery.

So Pedro, I think the way is clear for you to go ahead and push.
Thanks.
Pedro Alves March 18, 2019, 2:45 p.m. UTC | #5 
On 03/17/2019 04:46 PM, Hannes Domani via gdb-patches wrote:
> Am Sonntag, 17. März 2019, 17:07:46 MEZ hat Eli Zaretskii <eliz@gnu.org> Folgendes geschrieben: 
>>> Date: Fri, 15 Mar 2019 16:38:37 +0000 (UTC)> > From: "Hannes Domani via gdb-patches" <gdb-patches@sourceware.org>> > > > Also, I had to add the following to make it work on windows> > (but beware, I'm using pdcurses, not ncurses, and I don't know if ncurses> > for windows needs this as well):>
>> Ncurses doesn't need this.>
>>
>>> --- a/gdb/tui/tui-io.c> > +++ b/gdb/tui/tui-io.c> > @@ -369,6 +386,17 @@ apply_ansi_escape (WINDOW *w, const char *buf)> > > >    if (reverse_mode_p)> >      {> > +#if defined(__MINGW32__)> > +      if (style.get_foreground ().is_basic ()> > +         && style.get_foreground ().get_value ()> > +         == (ncurses_norm_attr & 15))> > +       style.set_fg (ui_file_style::NONE);> > +      if (style.get_background ().is_basic ()> > +         && style.get_background ().get_value ()> > +         == ((ncurses_norm_attr >> 4) & 15))> > +       style.set_bg (ui_file_style::NONE);> > +#endif> > +>
>> Could you describe what happens with pdcurses if you don't make this> change?  It's strange that pdcurses cannot use explicit color> specification if the color is the default one.
> Actually, you can disregard that.
> Before you added that fix for windows, I found a different workaround,it was to set the "normal" color in esc.style to gray.
> I forgot that I did that, and it backfired now.So I removed that again, and everything is fine now even without that above change.
> Sorry for the noise.

No worries, thanks for following through and for testing.
The patch is in master and 8.3 branch now.

Thanks,
Pedro Alves
diff mbox

Patch

--- a/gdb/tui/tui-io.c
+++ b/gdb/tui/tui-io.c
@@ -369,6 +386,17 @@  apply_ansi_escape (WINDOW *w, const char *buf)

   if (reverse_mode_p)
     {
+#if defined(__MINGW32__)
+      if (style.get_foreground ().is_basic ()
+         && style.get_foreground ().get_value ()
+         == (ncurses_norm_attr & 15))
+       style.set_fg (ui_file_style::NONE);
+      if (style.get_background ().is_basic ()
+         && style.get_background ().get_value ()
+         == ((ncurses_norm_attr >> 4) & 15))
+       style.set_bg (ui_file_style::NONE);
+#endif
+
       /* We want to reverse _only_ the default foreground/background
         colors.  If the foreground color is not the default (because
         the text was styled), we want to leave it as is.  If e.g.,