diff mbox

GDB 9 TUI mode broken

Message ID 83mu9aj1pj.fsf@gnu.org
State New, archived
Headers

Commit Message

Eli Zaretskii Feb. 22, 2020, 11:33 a.m. UTC 
  > Date: Mon, 17 Feb 2020 19:46:36 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> CC: gdb@sourceware.org
> 
> > From: Aleksey Midenkov <midenok@gmail.com>
> > Date: Mon, 17 Feb 2020 19:26:07 +0300
> > 
> > Now the window molders away after `winheight src +8` command.
> 
> This looks like a bug, and is probably unrelated to colors.  Suggest
> to make a bug report.

The patch below seems to fix some of the problem, but not all of it:
the position of the status line (the one that shows the process
information) is not updated correctly.

Tom, where's the position update of the status line supposed to be
handled?

And btw, the meaning of the 'locator' object in tui_adjust_win_heights
is unclear: to which window does it pertain, and what is the
significance of this line:

	      TUI_CMD_WIN->origin.y = locator->origin.y + 1;

I think we could use some additional comments in tui-stack.c or
tui-stack.h, to explain what is the meaning and expected use of
tui_locator_window.

Comments

Simon Marchi Feb. 22, 2020, 3:55 p.m. UTC | #1 
On 2020-02-22 6:33 a.m., Eli Zaretskii wrote:
>> Date: Mon, 17 Feb 2020 19:46:36 +0200
>> From: Eli Zaretskii <eliz@gnu.org>
>> CC: gdb@sourceware.org
>>
>>> From: Aleksey Midenkov <midenok@gmail.com>
>>> Date: Mon, 17 Feb 2020 19:26:07 +0300
>>>
>>> Now the window molders away after `winheight src +8` command.
>>
>> This looks like a bug, and is probably unrelated to colors.  Suggest
>> to make a bug report.
> 
> The patch below seems to fix some of the problem, but not all of it:
> the position of the status line (the one that shows the process
> information) is not updated correctly.
> 
> Tom, where's the position update of the status line supposed to be
> handled?
> 
> And btw, the meaning of the 'locator' object in tui_adjust_win_heights
> is unclear: to which window does it pertain, and what is the
> significance of this line:
> 
> 	      TUI_CMD_WIN->origin.y = locator->origin.y + 1;
> 
> I think we could use some additional comments in tui-stack.c or
> tui-stack.h, to explain what is the meaning and expected use of
> tui_locator_window.

The locator window is the line that sits between the source windows and the command
line's window (in the TUI's source layout).  It displays information such as current
thread id, current line and current PC.

I learned this watching Tom's excellent FOSDEM presentation, here :)

  https://fosdem.org/2020/schedule/event/debugging_gdb_tui/

Simon
Eli Zaretskii Feb. 22, 2020, 4:04 p.m. UTC | #2 
> Cc: midenok@gmail.com, gdb@sourceware.org, gdb-patches@sourceware.org
> From: Simon Marchi <simark@simark.ca>
> Date: Sat, 22 Feb 2020 10:55:19 -0500
> 
> > And btw, the meaning of the 'locator' object in tui_adjust_win_heights
> > is unclear: to which window does it pertain, and what is the
> > significance of this line:
> > 
> > 	      TUI_CMD_WIN->origin.y = locator->origin.y + 1;
> > 
> > I think we could use some additional comments in tui-stack.c or
> > tui-stack.h, to explain what is the meaning and expected use of
> > tui_locator_window.
> 
> The locator window is the line that sits between the source windows and the command
> line's window (in the TUI's source layout).  It displays information such as current
> thread id, current line and current PC.

Thanks; I wish this was spelled out in our comments.

The next question is: what code readjusts the location of the locator
window when the source or the command window is resized, and where is
that code called?  because the preceding code in
tui_adjust_win_heights modifies only 2 windows: the one whose height
was directly changed by the winheight command, and the other one,
which is displayed above or below it.
Eli Zaretskii Feb. 22, 2020, 4:36 p.m. UTC | #3 
> Date: Sat, 22 Feb 2020 18:04:16 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> CC: tom@tromey.com, midenok@gmail.com, gdb-patches@sourceware.org
> 
> > The locator window is the line that sits between the source windows and the command
> > line's window (in the TUI's source layout).  It displays information such as current
> > thread id, current line and current PC.
> 
> Thanks; I wish this was spelled out in our comments.

Btw, this window is called "status line" in the manual, and the
different name doesn't help in code reading.
Tom Tromey Feb. 22, 2020, 6:04 p.m. UTC | #4 
>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

Eli> Tom, where's the position update of the status line supposed to be
Eli> handled?

I looked at the gdb-9 branch.  In tui_adjust_win_heights, it is code
like this:

		  locator->resize (1, width,
				   0, (second_win->origin.y
				       + second_win->height + 1));

Eli> And btw, the meaning of the 'locator' object in tui_adjust_win_heights
Eli> is unclear: to which window does it pertain, and what is the
Eli> significance of this line:

You and Simon discussed this down-thread.  FWIW the name "locator" has
been used in the source since the beginning; but I also don't
particularly care for it (for a long time I could never remember the
name and had to search to find it whenever I needed it).  So, I would
appreciate a rename in the code here as well.

Also, thanks for pointing out that this is called "status" in the docs.
That affects my other series to make user-specified layouts.  In that
series I called the window "locator" -- but I'm going to go rename it.

Eli> TUI_CMD_WIN-> origin.y = locator->origin.y + 1;

This is a leftover and should be removed.  The resize method is the way
to modify a window's size and position.

This code is all different on git master.

Your patch looks reasonable to me.

Tom
Tom Tromey Feb. 22, 2020, 6:06 p.m. UTC | #5 
>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

Eli> The next question is: what code readjusts the location of the locator
Eli> window when the source or the command window is resized, and where is
Eli> that code called?  because the preceding code in
Eli> tui_adjust_win_heights modifies only 2 windows: the one whose height
Eli> was directly changed by the winheight command, and the other one,
Eli> which is displayed above or below it.

Other cases in tui_adjust_win_heights move the locator explicitly:

		  locator->resize (1, width,
				   0, (second_win->origin.y
				       + second_win->height + 1));

This just needs to be added to the first case, that you already
modified:

	  if (cur_layout == SRC_COMMAND 
	      || cur_layout == DISASSEM_COMMAND)
...

Tom
Eli Zaretskii Feb. 22, 2020, 6:23 p.m. UTC | #6 
> From: Tom Tromey <tom@tromey.com>
> Cc: Tom Tromey <tom@tromey.com>,  midenok@gmail.com,  gdb@sourceware.org,
>   gdb-patches@sourceware.org
> Date: Sat, 22 Feb 2020 11:04:59 -0700
> 
> This code is all different on git master.

I don't have the master built here: can you seed if the original
recipe, i.e. "winheight src +8", works on master as expected?  If so,
only the GDB 9 branch needs fixing.

> Your patch looks reasonable to me.

Thanks, I will finish it off as suggested, and submit a full patch for
review.
Tom Tromey Feb. 22, 2020, 7:02 p.m. UTC | #7 
Eli> I don't have the master built here: can you seed if the original
Eli> recipe, i.e. "winheight src +8", works on master as expected?  If so,
Eli> only the GDB 9 branch needs fixing.

I tried it and it seems fine to me.

>> Your patch looks reasonable to me.

Eli> Thanks, I will finish it off as suggested, and submit a full patch for
Eli> review.

Thank you.

Tom
diff mbox

Patch

--- gdb/tui/tui-win.c~0	2020-02-08 14:50:14.000000000 +0200
+++ gdb/tui/tui-win.c	2020-02-22 13:28:37.003625000 +0200
@@ -1077,20 +1077,25 @@  tui_adjust_win_heights (struct tui_win_i
 	    {
 	      struct tui_win_info *src_win_info;
 
-	      primary_win_info->resize (new_height, width,
-					0, primary_win_info->origin.y);
 	      if (primary_win_info->type == CMD_WIN)
 		{
 		  win_info = *(tui_source_windows ().begin ());
 		  src_win_info = win_info;
+		  primary_win_info->resize (new_height, width,
+					    0,
+					    primary_win_info->origin.y + diff);
+		  win_info->resize (win_info->height + diff, width,
+				    0, win_info->origin.y);
 		}
 	      else
 		{
 		  win_info = tui_win_list[CMD_WIN];
 		  src_win_info = primary_win_info;
-		}
+		  primary_win_info->resize (new_height, width,
+					    0, primary_win_info->origin.y);
 	      win_info->resize (win_info->height + diff, width,
-				0, win_info->origin.y);
+				    0, win_info->origin.y - diff);
+		}
 	      TUI_CMD_WIN->origin.y = locator->origin.y + 1;
 	      if ((src_win_info->type == SRC_WIN
 		   || src_win_info->type == DISASSEM_WIN))