[00/66] Clean up the TUI

Message ID 20190623224329.16060-1-tom@tromey.com
Headers show
Series
  • Clean up the TUI
Related show

Message

Tom Tromey June 23, 2019, 10:42 p.m.
My first real encounter with the TUI code was last year, when I added
syntax styling to it.

On the one hand, this experience convinced me that this code was some
of the ugliest in gdb.  It uses two different unions at different
spots in the conceptual class hierarchy, leading to very obscure code.
It also hard-codes both the possible window types but also the
qpossible layout types in a brittle and unextensible way.

On the other hand, working on the TUI changed my mind about the TUI
generally.  I've become a fan.  I think it would be good to improve
the TUI for users, but to do that I think we first have to improve its
code.

This series is a start at this.  It's already quite long,
unfortunately, and yet is still incomplete.  Still, I think it is
progress.  This series:

* Splits tui_win_info into subclasses, one per window type.

* Adds virtual methods, particularly in an attempt to remove all the
  code that switches based on the type of the window.  This is
  important to make it simpler to add new window types.

* Changes tui_gen_win_info into a real base class for tui_win_info.

* Removes the uses of unions from the window class hierarchy.

* Simplifies and C++-ifies various other minor things along the way.

There is still a ways to go, I only stopped here in the interest of
review sanity.  In particular:

* Window layouts should be first-class entities that do not involve
  hard-coding the possible layouts in the C++.

* tui_win_list should be changed to allow multiple types of windows.

* The few remaining checks of the various *_WIN constants should be
  removed.

I think once these things are done it should be possible to move on to
making useful user-visible changes, like allowing custom layouts, or
allowing custom windows to be written in Python.

I tested this by trying the various TUI features by hand.  However,
I'm not sure I managed to test them all.  I've also run the gdb.tui
part of the test suite, though that does not really test very much.

This series fixes one latent bug that I found with valgrind.  I found
two other existing bugs as well, but those I simply filed.

Tom

Comments

Pedro Alves June 24, 2019, 2:23 p.m. | #1
On 6/23/19 11:42 PM, Tom Tromey wrote:

> On the other hand, working on the TUI changed my mind about the TUI

> generally.  I've become a fan.  I think it would be good to improve

> the TUI for users, but to do that I think we first have to improve its

> code.


Ahah, welcome on board.  :-)

> 

> This series is a start at this.  It's already quite long,

> unfortunately, and yet is still incomplete.  Still, I think it is

> progress.  This series:

> 

> * Splits tui_win_info into subclasses, one per window type.

> 

> * Adds virtual methods, particularly in an attempt to remove all the

>   code that switches based on the type of the window.  This is

>   important to make it simpler to add new window types.

> 

> * Changes tui_gen_win_info into a real base class for tui_win_info.

> 

> * Removes the uses of unions from the window class hierarchy.

> 

> * Simplifies and C++-ifies various other minor things along the way.

> 


Hurray!

As I think you know, I attempted something like this over 7 years ago,
but doing it in C wasn't nearly as nice and I just gave up.  Being
a TUI user myself, I was so happy to see this series that I couldn't
resist and started reading it straight away.  Thanks so much for
working on this.

Looks like only patches up to #49 made it to the list (and I haven't
received a copy of the missing ones either).

Anyway, I skimmed patches #1-#49, and I sent a few responses
for some very minor things, but overall all of it seems good to me.
Looking forward to see this merged.

> I tested this by trying the various TUI features by hand.  However,

> I'm not sure I managed to test them all.


If you have this on a branch, I volunteer to test a bit.

Thanks,
Pedro Alves
Tom Tromey June 24, 2019, 4:47 p.m. | #2
Pedro> As I think you know, I attempted something like this over 7 years ago,
Pedro> but doing it in C wasn't nearly as nice and I just gave up.

Yeah.  I looked at that branch but it seemed better to just redo it,
since we have C++ now.

Pedro> Looks like only patches up to #49 made it to the list (and I haven't
Pedro> received a copy of the missing ones either).

I don't know why :( I will send the remaining patches separately.

>> I tested this by trying the various TUI features by hand.  However,

>> I'm not sure I managed to test them all.


Pedro> If you have this on a branch, I volunteer to test a bit.

In my github, branch submit/tui-rewrite.

Tom
Pedro Alves June 24, 2019, 5:46 p.m. | #3
On 6/24/19 5:47 PM, Tom Tromey wrote:

> Pedro> If you have this on a branch, I volunteer to test a bit.

> 

> In my github, branch submit/tui-rewrite.


I played a little, and didn't spot any behavior change so far.

Thanks,
Pedro Alves
Tom Tromey June 24, 2019, 6:54 p.m. | #4
Tom> I don't know why :( I will send the remaining patches separately.

I managed to do this, though somewhat clumsily as I forgot to use
--no-thread.

Tom
Pedro Alves June 24, 2019, 10:23 p.m. | #5
On 6/24/19 7:54 PM, Tom Tromey wrote:
> Tom> I don't know why :( I will send the remaining patches separately.

> 

> I managed to do this, though somewhat clumsily as I forgot to use

> --no-thread.

> 


Skimmed the patches out, and nothing stood out to me.  It all
looked fine.

Thanks again for doing all this, and I have to say I'm impressed
by the neat patch splitting.

Thanks,
Pedro Alves
Tom Tromey June 25, 2019, 1:51 p.m. | #6
Pedro> Skimmed the patches out, and nothing stood out to me.  It all
Pedro> looked fine.

I'm going to check it in now.

Pedro> Thanks again for doing all this, and I have to say I'm impressed
Pedro> by the neat patch splitting.

Thanks!

Tom