Re: [PATCH setup] Keyboard accelerators for install/reinstall/uninstall

[Jon Turney <jon.turney@dronecode.org.uk>]

On 14/08/2022 12:57, Christian Franke wrote:
> This eases state changes of a selected sequence of packages.

Nice!  The keyboard control of the package chooser was a bit of an 
after-thought, which it really shouldn't be.

> Ctrl+U is in particular useful to cleanup installations in conjunction 
> with "unneeded" view:
> https://sourceware.org/pipermail/cygwin-apps/2022-August/042185.html
> 
> Open issue: Add some visual clue (tooltip?) to make this functionality 
> more obvious.

Yeah.  These shortcuts should also be accelerators for the package 
action selection popup menu, which would make them more discoverable?

> 
> 0001-Keyboard-accelerators-for-install-reinstall-uninstal.patch
> 
>  From 71da4fbc68022b5083eba0cbdf2c0a4a541ddf1c Mon Sep 17 00:00:00 2001
> From: Christian Franke<christian.franke@t-online.de>
> Date: Sun, 14 Aug 2022 13:43:48 +0200
> Subject: [PATCH] Keyboard accelerators for install/reinstall/uninstall
> 
> Ctrl+I/R/U select install/reinstall/uninstall and then move selection
> to next list item.
> ---
>   ListView.cc         | 17 +++++++++++++++--
>   ListView.h          |  3 ++-
>   PickCategoryLine.cc |  3 ++-
>   PickCategoryLine.h  |  3 ++-
>   PickPackageLine.cc  | 17 ++++++++++++++++-
>   PickPackageLine.h   |  3 ++-
>   package_meta.cc     |  4 ++++
>   7 files changed, 43 insertions(+), 7 deletions(-)
> 
> diff --git a/ListView.cc b/ListView.cc
> index 62a37ab..22009ba 100644
> --- a/ListView.cc
> +++ b/ListView.cc
> @@ -564,14 +564,20 @@ ListView::OnNotify (NMHDR *pNmHdr, LRESULT *pResult)
>         NMLVKEYDOWN *pNmLvKeyDown = (NMLVKEYDOWN *)pNmHdr;
>         int iRow = ListView_GetSelectionMark(hWndListView);
>   #if DEBUG
> -      Log (LOG_PLAIN) << "LVN_KEYDOWN vkey " << pNmLvKeyDown->wVKey << " on row " << iRow << endLog;
> +      Log (LOG_PLAIN) << "LVN_KEYDOWN vkey " << pNmLvKeyDown->wVKey << " on row " << iRow
> +                      << " Ctrl:" << !!(GetKeyState(VK_CONTROL) & 0x8000)
> +                      <<  " Alt:" << !!(GetKeyState(VK_MENU) & 0x8000) << endLog;
>   #endif
>   
>         if (contents && iRow >= 0)
>           {
> +          bool ctrl = !!(GetKeyState(VK_CONTROL) & 0x8000);
> +          bool alt = !!(GetKeyState(VK_MENU) & 0x8000);
>             int col_num;
>             int action_id;
> -          if ((*contents)[iRow]->map_key_to_action(pNmLvKeyDown->wVKey, &col_num, &action_id))
> +          bool down = false;
> +          if ((*contents)[iRow]->map_key_to_action(pNmLvKeyDown->wVKey, ctrl, alt, &col_num,
> +                                                   &action_id, &down))
>               {
>                 int update;
>                 if (action_id >= 0)
> @@ -591,6 +597,13 @@ ListView::OnNotify (NMHDR *pNmHdr, LRESULT *pResult)
>                 if (update > 0)
>                   ListView_RedrawItems(hWndListView, iRow, iRow + update -1);
>               }
> +          if (down && iRow + 1 < ListView_GetItemCount(hWndListView)) {

Again as a stylistic thing, I'd suggest perhaps changing 
map_key_to_action() to return a set of flags which could include 
"ACTION", "POPUP" and "DOWN", rather than adding another flag parameter 
to it...

> +            ListView_SetItemState(hWndListView, -1, 0, LVIS_SELECTED | LVIS_FOCUSED);
> +            ListView_SetItemState(hWndListView, iRow + 1, LVIS_SELECTED | LVIS_FOCUSED,
> +                                  LVIS_SELECTED | LVIS_FOCUSED);
> +            ListView_SetSelectionMark(hWndListView, iRow + 1);
> +            ListView_EnsureVisible(hWndListView, iRow + 1, false);

A comment here saying "and move selection to next row".

> +          }
>           }
>       }
>       break;
> diff --git a/ListView.h b/ListView.h
> index 95dd9ee..a2ecdef 100644
> --- a/ListView.h
> +++ b/ListView.h
> @@ -38,7 +38,8 @@ class ListViewLine
>     virtual ActionList *get_actions(int col) const = 0;
>     virtual int do_action(int col, int id) = 0;
>     virtual int do_default_action(int col) = 0;
> -  virtual bool map_key_to_action(WORD vkey, int *col_num, int *action_id) const = 0;
> +  virtual bool map_key_to_action(WORD vkey, bool ctrl, bool alt, int *col_num,
> +                                 int *action_id, bool *down) const = 0;
>   };
>   
>   typedef std::vector<ListViewLine *> ListViewContents;
> diff --git a/PickCategoryLine.cc b/PickCategoryLine.cc
> index d2ac899..9872a2e 100644
> --- a/PickCategoryLine.cc
> +++ b/PickCategoryLine.cc
> @@ -97,7 +97,8 @@ PickCategoryLine::get_tooltip(int col_num) const
>   }
>   
>   bool
> -PickCategoryLine::map_key_to_action(WORD vkey, int *col_num, int *action_id) const
> +PickCategoryLine::map_key_to_action(WORD vkey, bool ctrl, bool alt, int *col_num,
> +                                    int *action_id, bool *down) const
>   {
>     switch (vkey)
>       {
> diff --git a/PickCategoryLine.h b/PickCategoryLine.h
> index 6a7321d..0bfba33 100644
> --- a/PickCategoryLine.h
> +++ b/PickCategoryLine.h
> @@ -41,7 +41,8 @@ public:
>     ActionList *get_actions(int col) const;
>     int do_action(int col, int action_id);
>     int do_default_action(int col);
> -  bool map_key_to_action(WORD vkey, int *col_num, int *action_id) const;
> +  bool map_key_to_action(WORD vkey, bool ctrl, bool alt, int *col_num,
> +                         int *action_id, bool *down) const;
>   
>   private:
>     CategoryTree * cat_tree;
> diff --git a/PickPackageLine.cc b/PickPackageLine.cc
> index ae1e520..ba47e1f 100644
> --- a/PickPackageLine.cc
> +++ b/PickPackageLine.cc
> @@ -145,7 +145,8 @@ PickPackageLine::get_indent() const
>   }
>   
>   bool
> -PickPackageLine::map_key_to_action(WORD vkey, int *col_num, int *action_id) const
> +PickPackageLine::map_key_to_action(WORD vkey, bool ctrl, bool alt, int *col_num,
> +                                   int *action_id, bool *down) const
>   {
>     switch (vkey)
>       {
> @@ -154,6 +155,20 @@ PickPackageLine::map_key_to_action(WORD vkey, int *col_num, int *action_id) cons
>         *col_num = new_col;
>         *action_id = -1;
>         return true;
> +    case 'I':
> +    case 'R':
> +    case 'U':
> +      if (!(ctrl && !alt))

As a stylistic thing, I'd perhaps rather combine ctrl and alt flags as a 
modifier bitmap.

What is the reasoning for selecting the "ctrl but not alt" modifier 
state here?

> +        break;
> +      *col_num = new_col;
> +      switch (vkey)
> +        {
> +        case 'I': *action_id = packagemeta::Install_action; break;
> +        case 'R': *action_id = packagemeta::Reinstall_action; break;
> +        default:  *action_id = packagemeta::Uninstall_action; break;
> +        }
> +      *down = true;
> +      return true;
>       }
>   
>     return false;
> diff --git a/PickPackageLine.h b/PickPackageLine.h
> index 2c59e90..9eb09db 100644
> --- a/PickPackageLine.h
> +++ b/PickPackageLine.h
> @@ -37,7 +37,8 @@ public:
>     ActionList *get_actions(int col_num) const;
>     int do_action(int col, int action_id);
>     int do_default_action(int col);
> -  bool map_key_to_action(WORD vkey, int *col_num, int *action_id) const;
> +  bool map_key_to_action(WORD vkey, bool ctrl, bool alt, int *col_num,
> +                         int *action_id, bool *down) const;
>   private:
>     packagemeta & pkg;
>     PickView & theView;
> diff --git a/package_meta.cc b/package_meta.cc
> index 8a69525..05b8946 100644
> --- a/package_meta.cc
> +++ b/package_meta.cc
> @@ -670,6 +670,10 @@ packagemeta::set_action (_actions action, packageversion const &default_version,
>     else if (action == Uninstall_action)
>       {
>         desired = packageversion ();
> +      pick (false);
> +      srcpick (false);
> +      if (!installed)
> +	action = NoChange_action;

Hmm... why is adding this needed?

>       }
>   
>     _action = action;