Add tab completion for TUI's "focus" command

  The implementation is pretty straightforward, though there's the
question of what the completion list should show when the TUI has not
yet been initalized.  At that point no TUI window is considered visible
thus the completion list will be composed of only the "next" and "prev"
keywords (which are always added to the completion list).  But
technically the commands "focus cmd" and "focus src" will work even when
the TUI is not yet initialized because the initialization done in the
meantime will make these windows visible by default.  So should the
entries "cmd" and "src" be added to the completion list as special
cases when the TUI is not yet initialized?

gdb/ChangeLog:

	* tui/tui-win.c (focus_completer): New static function.
	(_initialize_tui_win): Set the completion function of the
	"focus" command to focus_completer.

gdb/testsuite/ChangeLog:

	* gdb.base/completion.exp: Test the completion of the "focus"
	command.
---
 gdb/testsuite/gdb.base/completion.exp | 18 +++++++++++
 gdb/tui/tui-win.c                     | 59 ++++++++++++++++++++++++++++++++++-
 2 files changed, 76 insertions(+), 1 deletion(-)
  

On Sun, Jun 21, 2015 at 10:30 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> The implementation is pretty straightforward, though there's the
> question of what the completion list should show when the TUI has not
> yet been initalized.

Hi.
What does "not yet been initialized" mean here?

> At that point no TUI window is considered visible
> thus the completion list will be composed of only the "next" and "prev"
> keywords (which are always added to the completion list).  But
> technically the commands "focus cmd" and "focus src" will work even when
> the TUI is not yet initialized because the initialization done in the
> meantime will make these windows visible by default.  So should the
> entries "cmd" and "src" be added to the completion list as special
> cases when the TUI is not yet initialized?
>
> gdb/ChangeLog:
>
>         * tui/tui-win.c (focus_completer): New static function.
>         (_initialize_tui_win): Set the completion function of the
>         "focus" command to focus_completer.
>
> gdb/testsuite/ChangeLog:
>
>         * gdb.base/completion.exp: Test the completion of the "focus"
>         command.
> ---
>  gdb/testsuite/gdb.base/completion.exp | 18 +++++++++++
>  gdb/tui/tui-win.c                     | 59 ++++++++++++++++++++++++++++++++++-
>  2 files changed, 76 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/testsuite/gdb.base/completion.exp b/gdb/testsuite/gdb.base/completion.exp
> index 4c31bfc..5c20f26 100644
> --- a/gdb/testsuite/gdb.base/completion.exp
> +++ b/gdb/testsuite/gdb.base/completion.exp
> @@ -878,3 +878,21 @@ if {![skip_tui_tests]} {
>         }
>      }
>  }
> +if {![skip_tui_tests]} {
> +    with_test_prefix "focus command" {
> +       set test "test completion"
> +       send_gdb "focus\t\t\t"

I think I understand the purpose of three tabs here,
but I'm wondering what happens if we add a "focus-foo" command.
Seems like it'd be more robust to replace the first tab with a space.

> +       gdb_test_multiple "" "$test" {
> +           -re "next *prev *\r\n$gdb_prompt focus $" {
> +               pass "$test"
> +           }
> +       }
> +       send_gdb "\003"
> +       set test "quit command input after testing completion"
> +       gdb_test_multiple "" "$test" {
> +           -re "$gdb_prompt $" {
> +               pass "$test"
> +           }
> +       }
> +    }
> +}
> diff --git a/gdb/tui/tui-win.c b/gdb/tui/tui-win.c
> index 8c0685b..8860ca7 100644
> --- a/gdb/tui/tui-win.c
> +++ b/gdb/tui/tui-win.c
> @@ -354,6 +354,61 @@ tui_set_var_cmd (char *null_args, int from_tty, struct cmd_list_element *c)
>      tui_rehighlight_all ();
>  }
>
> +/* Complete possible window names to focus on.  TEXT is the complete text
> +   entered so far, WORD is the word currently being completed.  */
> +
> +static VEC (char_ptr) *
> +focus_completer (struct cmd_list_element *ignore,
> +                 const char *text, const char *word)
> +{
> +  VEC (const_char_ptr) *completion_name_vec = NULL;
> +  VEC (char_ptr) *matches_vec;
> +  int win_type;
> +
> +  for (win_type = SRC_WIN; win_type < MAX_MAJOR_WINDOWS; win_type++)
> +    {
> +      const char *completion_name = NULL;
> +
> +      /* We can't focus on an invisible window.  */
> +      if (tui_win_list[win_type] == NULL
> +         || !tui_win_list[win_type]->generic.is_visible)
> +       continue;
> +
> +      switch (win_type)
> +       {
> +       case SRC_WIN:
> +         completion_name = "src";
> +         break;
> +       case DISASSEM_WIN:
> +         completion_name = "asm";
> +         break;
> +       case DATA_WIN:
> +         completion_name = "regs";
> +         break;
> +       case CMD_WIN:
> +         completion_name = "cmd";
> +         break;
> +       default:
> +         break;
> +       }
> +
> +      if (completion_name != NULL)
> +       VEC_safe_push (const_char_ptr, completion_name_vec, completion_name);
> +    }
> +
> +  VEC_safe_push (const_char_ptr, completion_name_vec, "next");
> +  VEC_safe_push (const_char_ptr, completion_name_vec, "prev");
> +  VEC_safe_push (const_char_ptr, completion_name_vec, NULL);
> +
> +  matches_vec
> +    = complete_on_enum (VEC_address (const_char_ptr, completion_name_vec),
> +                       text, word);
> +
> +  VEC_free (const_char_ptr, completion_name_vec);
> +
> +  return matches_vec;
> +}
> +
>  /* Function to initialize gdb commands, for tui window
>     manipulation.  */
>
> @@ -365,6 +420,7 @@ _initialize_tui_win (void)
>  {
>    static struct cmd_list_element *tui_setlist;
>    static struct cmd_list_element *tui_showlist;
> +  struct cmd_list_element *focus_cmd;
>
>    /* Define the classes of commands.
>       They will appear in the help list in the reverse of this order.  */
> @@ -393,7 +449,7 @@ regs : the register display\n"));
>    add_com_alias ("wh", "winheight", class_tui, 0);
>    add_info ("win", tui_all_windows_info,
>             _("List of all displayed windows.\n"));
> -  add_com ("focus", class_tui, tui_set_focus_command, _("\
> +  focus_cmd = add_com ("focus", class_tui, tui_set_focus_command, _("\
>  Set focus to named window or next/prev window.\n\
>  Usage: focus {<win> | next | prev}\n\
>  Valid Window names are:\n\
> @@ -402,6 +458,7 @@ asm  : the disassembly window\n\
>  regs : the register display\n\
>  cmd  : the command window\n"));
>    add_com_alias ("fs", "focus", class_tui, 0);
> +  set_cmd_completer (focus_cmd, focus_completer);
>    add_com ("+", class_tui, tui_scroll_forward_command, _("\
>  Scroll window forward.\n\
>  Usage: + [win] [n]\n"));
> --
> 2.4.4.410.g43ed522.dirty
>
  

On Tue, Jun 23, 2015 at 10:01 AM, Doug Evans <dje@google.com> wrote:
> On Sun, Jun 21, 2015 at 10:30 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
>> The implementation is pretty straightforward, though there's the
>> question of what the completion list should show when the TUI has not
>> yet been initalized.
>
> Hi.
> What does "not yet been initialized" mean here?

I mean that the user never switched to the TUI during the session, so
the windows and stuff have not yet been created.

>
>> At that point no TUI window is considered visible
>> thus the completion list will be composed of only the "next" and "prev"
>> keywords (which are always added to the completion list).  But
>> technically the commands "focus cmd" and "focus src" will work even when
>> the TUI is not yet initialized because the initialization done in the
>> meantime will make these windows visible by default.  So should the
>> entries "cmd" and "src" be added to the completion list as special
>> cases when the TUI is not yet initialized?
>>
>> gdb/ChangeLog:
>>
>>         * tui/tui-win.c (focus_completer): New static function.
>>         (_initialize_tui_win): Set the completion function of the
>>         "focus" command to focus_completer.
>>
>> gdb/testsuite/ChangeLog:
>>
>>         * gdb.base/completion.exp: Test the completion of the "focus"
>>         command.
>> ---
>>  gdb/testsuite/gdb.base/completion.exp | 18 +++++++++++
>>  gdb/tui/tui-win.c                     | 59 ++++++++++++++++++++++++++++++++++-
>>  2 files changed, 76 insertions(+), 1 deletion(-)
>>
>> diff --git a/gdb/testsuite/gdb.base/completion.exp b/gdb/testsuite/gdb.base/completion.exp
>> index 4c31bfc..5c20f26 100644
>> --- a/gdb/testsuite/gdb.base/completion.exp
>> +++ b/gdb/testsuite/gdb.base/completion.exp
>> @@ -878,3 +878,21 @@ if {![skip_tui_tests]} {
>>         }
>>      }
>>  }
>> +if {![skip_tui_tests]} {
>> +    with_test_prefix "focus command" {
>> +       set test "test completion"
>> +       send_gdb "focus\t\t\t"
>
> I think I understand the purpose of three tabs here,
> but I'm wondering what happens if we add a "focus-foo" command.
> Seems like it'd be more robust to replace the first tab with a space.

Good point.  Then we'll need to emit only 2 tabs I think.

>
>> +       gdb_test_multiple "" "$test" {
>> +           -re "next *prev *\r\n$gdb_prompt focus $" {
>> +               pass "$test"
>> +           }
>> +       }
>> +       send_gdb "\003"
>> +       set test "quit command input after testing completion"
>> +       gdb_test_multiple "" "$test" {
>> +           -re "$gdb_prompt $" {
>> +               pass "$test"
>> +           }
>> +       }
>> +    }
>> +}
>> diff --git a/gdb/tui/tui-win.c b/gdb/tui/tui-win.c
>> index 8c0685b..8860ca7 100644
>> --- a/gdb/tui/tui-win.c
>> +++ b/gdb/tui/tui-win.c
>> @@ -354,6 +354,61 @@ tui_set_var_cmd (char *null_args, int from_tty, struct cmd_list_element *c)
>>      tui_rehighlight_all ();
>>  }
>>
>> +/* Complete possible window names to focus on.  TEXT is the complete text
>> +   entered so far, WORD is the word currently being completed.  */
>> +
>> +static VEC (char_ptr) *
>> +focus_completer (struct cmd_list_element *ignore,
>> +                 const char *text, const char *word)
>> +{
>> +  VEC (const_char_ptr) *completion_name_vec = NULL;
>> +  VEC (char_ptr) *matches_vec;
>> +  int win_type;
>> +
>> +  for (win_type = SRC_WIN; win_type < MAX_MAJOR_WINDOWS; win_type++)
>> +    {
>> +      const char *completion_name = NULL;
>> +
>> +      /* We can't focus on an invisible window.  */
>> +      if (tui_win_list[win_type] == NULL
>> +         || !tui_win_list[win_type]->generic.is_visible)
>> +       continue;
>> +
>> +      switch (win_type)
>> +       {
>> +       case SRC_WIN:
>> +         completion_name = "src";
>> +         break;
>> +       case DISASSEM_WIN:
>> +         completion_name = "asm";
>> +         break;
>> +       case DATA_WIN:
>> +         completion_name = "regs";
>> +         break;
>> +       case CMD_WIN:
>> +         completion_name = "cmd";
>> +         break;
>> +       default:
>> +         break;
>> +       }
>> +
>> +      if (completion_name != NULL)
>> +       VEC_safe_push (const_char_ptr, completion_name_vec, completion_name);
>> +    }
>> +
>> +  VEC_safe_push (const_char_ptr, completion_name_vec, "next");
>> +  VEC_safe_push (const_char_ptr, completion_name_vec, "prev");
>> +  VEC_safe_push (const_char_ptr, completion_name_vec, NULL);
>> +
>> +  matches_vec
>> +    = complete_on_enum (VEC_address (const_char_ptr, completion_name_vec),
>> +                       text, word);
>> +
>> +  VEC_free (const_char_ptr, completion_name_vec);
>> +
>> +  return matches_vec;
>> +}
>> +
>>  /* Function to initialize gdb commands, for tui window
>>     manipulation.  */
>>
>> @@ -365,6 +420,7 @@ _initialize_tui_win (void)
>>  {
>>    static struct cmd_list_element *tui_setlist;
>>    static struct cmd_list_element *tui_showlist;
>> +  struct cmd_list_element *focus_cmd;
>>
>>    /* Define the classes of commands.
>>       They will appear in the help list in the reverse of this order.  */
>> @@ -393,7 +449,7 @@ regs : the register display\n"));
>>    add_com_alias ("wh", "winheight", class_tui, 0);
>>    add_info ("win", tui_all_windows_info,
>>             _("List of all displayed windows.\n"));
>> -  add_com ("focus", class_tui, tui_set_focus_command, _("\
>> +  focus_cmd = add_com ("focus", class_tui, tui_set_focus_command, _("\
>>  Set focus to named window or next/prev window.\n\
>>  Usage: focus {<win> | next | prev}\n\
>>  Valid Window names are:\n\
>> @@ -402,6 +458,7 @@ asm  : the disassembly window\n\
>>  regs : the register display\n\
>>  cmd  : the command window\n"));
>>    add_com_alias ("fs", "focus", class_tui, 0);
>> +  set_cmd_completer (focus_cmd, focus_completer);
>>    add_com ("+", class_tui, tui_scroll_forward_command, _("\
>>  Scroll window forward.\n\
>>  Usage: + [win] [n]\n"));
>> --
>> 2.4.4.410.g43ed522.dirty
>>
  

On Tue, Jun 23, 2015 at 9:37 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> On Tue, Jun 23, 2015 at 10:01 AM, Doug Evans <dje@google.com> wrote:
>> On Sun, Jun 21, 2015 at 10:30 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
>>> The implementation is pretty straightforward, though there's the
>>> question of what the completion list should show when the TUI has not
>>> yet been initalized.
>>
>> Hi.
>> What does "not yet been initialized" mean here?
>
> I mean that the user never switched to the TUI during the session, so
> the windows and stuff have not yet been created.

Thanks.
I tried "focus cmd" and "focus src" at the start of a gdb session and
they worked.
I also tried "focus asm" and got a complaint of an invalid window.
So I'd say sure, add cmd and src to the completion list
even if TUI isn't initialized, since the command will work
as expected. I'd add a comment to the code describing
why they're being unconditionally added.

Adding next and prev when TUI hasn't been initialized
are kinda the odd ones out, conceptually, but they
too "work" (so I'm not suggesting removing them).
  

On 06/23/2015 03:54 PM, Doug Evans wrote:

> I tried "focus cmd" and "focus src" at the start of a gdb session and
> they worked.
> I also tried "focus asm" and got a complaint of an invalid window.
> So I'd say sure, add cmd and src to the completion list
> even if TUI isn't initialized, since the command will work
> as expected. I'd add a comment to the code describing
> why they're being unconditionally added.
> 
> Adding next and prev when TUI hasn't been initialized
> are kinda the odd ones out, conceptually, but they
> too "work" (so I'm not suggesting removing them).

Fully agreed.

Thanks,
Pedro Alves