Re: GDB 9 TUI mode broken

Re: GDB 9 TUI mode broken

[Eli Zaretskii]

> 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.

--- 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))

Re: GDB 9 TUI mode broken

[Simon Marchi]

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

Re: GDB 9 TUI mode broken

[Eli Zaretskii]

> 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.

Re: GDB 9 TUI mode broken

[Eli Zaretskii]

> 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.

Re: GDB 9 TUI mode broken

[Tom Tromey]

>>>>> "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

Re: GDB 9 TUI mode broken

[Tom Tromey]

>>>>> "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

Re: GDB 9 TUI mode broken

[Eli Zaretskii]

> 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.

Re: GDB 9 TUI mode broken

[Eli Zaretskii]

> Date: Sat, 22 Feb 2020 20:23:29 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> CC: midenok@gmail.com, gdb-patches@sourceware.org
> 
> > Your patch looks reasonable to me.
> 
> Thanks, I will finish it off as suggested, and submit a full patch for
> review.

The below seems to DTRT.  OK to commit?

2020-02-22  Eli Zaretskii  <eliz@gnu.org>

	* tui/tui-win.c (new_height_ok, tui_adjust_win_heights): Rename
	'locator' to 'status_line', to better match terminology in the
	manual.
	(tui_adjust_win_heights): Resize the status_line window to match
	the new dimensions of the command and source/disassembly windows.
	Reported by Aleksey Midenkov <midenok@gmail.com>.

--- gdb/tui/tui-win.c~0	2020-02-08 14:50:14.000000000 +0200
+++ gdb/tui/tui-win.c	2020-02-22 20:39:24.847375000 +0200
@@ -1067,7 +1067,7 @@ tui_adjust_win_heights (struct tui_win_i
 	{
 	  int diff;
 	  struct tui_win_info *win_info;
-	  struct tui_locator_window *locator = tui_locator_win_info_ptr ();
+	  struct tui_locator_window *status_line = tui_locator_win_info_ptr ();
 	  enum tui_layout_type cur_layout = tui_current_layout ();
 	  int width = tui_term_width ();
 
@@ -1077,21 +1077,30 @@ 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);
+		  status_line->resize (1, width, 0,
+				       win_info->origin.y + win_info->height);
 		}
 	      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);
-	      TUI_CMD_WIN->origin.y = locator->origin.y + 1;
+				    0, win_info->origin.y - diff);
+		  status_line->resize (1, width, 0,
+				       primary_win_info->origin.y
+				       + primary_win_info->height);
+		}
 	      if ((src_win_info->type == SRC_WIN
 		   || src_win_info->type == DISASSEM_WIN))
 		{
@@ -1122,7 +1131,7 @@ tui_adjust_win_heights (struct tui_win_i
 	      if (primary_win_info == TUI_CMD_WIN)
 		{ /* Split the change in height across the 1st & 2nd
 		     windows, adjusting them as well.  */
-		  /* Subtract the locator.  */
+		  /* Subtract the status_line.  */
 		  int first_split_diff = diff / 2;
 		  int second_split_diff = first_split_diff;
 
@@ -1160,12 +1169,12 @@ tui_adjust_win_heights (struct tui_win_i
 		  second_win->resize (second_win->height + second_split_diff,
 				      width,
 				      0, first_win->height - 1);
-		  locator->resize (1, width,
+		  status_line->resize (1, width,
 				   0, (second_win->origin.y
 				       + second_win->height + 1));
 
 		  TUI_CMD_WIN->resize (new_height, width,
-				       0, locator->origin.y + 1);
+				       0, status_line->origin.y + 1);
 		}
 	      else
 		{
@@ -1196,15 +1205,16 @@ tui_adjust_win_heights (struct tui_win_i
 		  else
 		    second_win->resize (second_win->height, width,
 					0, first_win->height - 1);
-		  locator->resize (1, width,
+		  status_line->resize (1, width,
 				   0, (second_win->origin.y
 				       + second_win->height + 1));
-		  TUI_CMD_WIN->origin.y = locator->origin.y + 1;
+		  TUI_CMD_WIN->origin.y = status_line->origin.y + 1;
 		  if ((TUI_CMD_WIN->height + diff) < 1)
-		    TUI_CMD_WIN->resize (1, width, 0, locator->origin.y + 1);
+		    TUI_CMD_WIN->resize (1, width, 0,
+					 status_line->origin.y + 1);
 		  else
 		    TUI_CMD_WIN->resize (TUI_CMD_WIN->height + diff, width,
-					 0, locator->origin.y + 1);
+					 0, status_line->origin.y + 1);
 		}
 	      if (src1 != nullptr && src1->content.empty ())
 		src1->erase_source_content ();
@@ -1272,13 +1282,14 @@ new_height_ok (struct tui_win_info *prim
 	  /* We could simply add all the heights to obtain the same
 	     result but below is more explicit since we subtract 1 for
 	     the line that the first and second windows share, and add
-	     one for the locator.  */
+	     one for the status_line.  */
 	  total_height = cur_total_height =
 	    (first_win->height + second_win->height - 1)
-	    + TUI_CMD_WIN->height + 1;	/* Locator. */
+	    + TUI_CMD_WIN->height + 1;	/* Status_line. */
 	  if (primary_win_info == TUI_CMD_WIN)
 	    {
-	      /* Locator included since first & second win share a line.  */
+	      /* Status_line included since first and second window
+		 share a line.  */
 	      ok = ((first_win->height +
 		     second_win->height + diff) >=
 		    (MIN_WIN_HEIGHT * 2) 

Re: GDB 9 TUI mode broken

[Tom Tromey]

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

Re: GDB 9 TUI mode broken

[Tom Tromey]

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

Eli> 2020-02-22  Eli Zaretskii  <eliz@gnu.org>

Eli> 	* tui/tui-win.c (new_height_ok, tui_adjust_win_heights): Rename
Eli> 	'locator' to 'status_line', to better match terminology in the
Eli> 	manual.
Eli> 	(tui_adjust_win_heights): Resize the status_line window to match
Eli> 	the new dimensions of the command and source/disassembly windows.
Eli> 	Reported by Aleksey Midenkov <midenok@gmail.com>.

Looks good to me, thanks.

Tom

Re: GDB 9 TUI mode broken

[Eli Zaretskii]

> From: Tom Tromey <tom@tromey.com>
> Cc: tom@tromey.com,  midenok@gmail.com,  gdb-patches@sourceware.org
> Date: Sat, 22 Feb 2020 12:09:50 -0700
> 
> >>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:
> 
> Eli> 2020-02-22  Eli Zaretskii  <eliz@gnu.org>
> 
> Eli> 	* tui/tui-win.c (new_height_ok, tui_adjust_win_heights): Rename
> Eli> 	'locator' to 'status_line', to better match terminology in the
> Eli> 	manual.
> Eli> 	(tui_adjust_win_heights): Resize the status_line window to match
> Eli> 	the new dimensions of the command and source/disassembly windows.
> Eli> 	Reported by Aleksey Midenkov <midenok@gmail.com>.
> 
> Looks good to me, thanks.

Thanks, pushed to gdb-9-branch, after creating a Bugzilla PR for it.