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

[Christian Franke <Christian.Franke@t-online.de>]

[-- Attachment #1: Type: text/plain, Size: 4556 bytes --]

Jon Turney wrote:
> 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.

Thanks - revised patch is attached.


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

Handling these in the popup menu is possibly tricky. According to 
documentation of TrackPopupMenu(), hWndListView "receives all messages 
from the menu" which is apparently not the case.


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

Done, see "enum Action".


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

Done.


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

Done, see 'struct ModifierKeys'.


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

Why should Ctrl+Alt+I have the same effect as Ctrl+I ?
The new patch also checks for shift.


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

Otherwise a strange state change would occur at least in the GUI when an 
install request is undone:

"Skip" == Ctrl+I ==> "3.2-1" == Ctrl+U ==> "Uninstall"

The new patch includes another addition which prevents this on installs 
from local directory when the current default version is not yet downloaded:

"Skip" == Ctrl+I ==> "" (empty)


BTW, I didn't understand this line:

   void
   packagemeta::set_action (...)
   {
     ...
     else if (action == Install_action)
       {
         desired = default_version;
         if (desired)
           {
             if (desired != installed)
               if (desired.accessible ())
                 {
                   ...
                   pick (true);
                   srcpick (false);
                 }
               else
                 {
                   pick (false);
                   srcpick (true); <== why true? ==
                 }
               ...


[-- Attachment #2: 0001-Keyboard-accelerators-for-install-reinstall-uninstal.patch --]
[-- Type: text/plain, Size: 9308 bytes --]

From 895f5510731bec0b161ba9b651b5a77dd8cb96a4 Mon Sep 17 00:00:00 2001
From: Christian Franke <christian.franke@t-online.de>
Date: Mon, 22 Aug 2022 16:39:21 +0200
Subject: [PATCH] Keyboard accelerators for install/reinstall/uninstall

Ctrl+I/R/U select install/reinstall/uninstall and then move selection
to next row.
---
 ListView.cc         | 64 +++++++++++++++++++++++++++++++--------------
 ListView.h          | 10 ++++++-
 PickCategoryLine.cc | 21 +++++++--------
 PickCategoryLine.h  |  2 +-
 PickPackageLine.cc  | 27 ++++++++++++++-----
 PickPackageLine.h   |  2 +-
 package_meta.cc     | 11 ++++++++
 7 files changed, 97 insertions(+), 40 deletions(-)

diff --git a/ListView.cc b/ListView.cc
index 62a37ab..7cc7f0f 100644
--- a/ListView.cc
+++ b/ListView.cc
@@ -24,6 +24,18 @@
 // ListView Common Control
 // ---------------------------------------------------------------------------
 
+int ModifierKeys::get()
+{
+  int keys = 0;
+  if (GetKeyState(VK_SHIFT) & 0x8000)
+    keys |= Shift;
+  if (GetKeyState(VK_CONTROL) & 0x8000)
+    keys |= Control;
+  if (GetKeyState(VK_MENU) & 0x8000)
+    keys |= Alt;
+  return keys;
+}
+
 void
 ListView::init(HWND parent, int id, HeaderList headers)
 {
@@ -563,33 +575,47 @@ ListView::OnNotify (NMHDR *pNmHdr, LRESULT *pResult)
     {
       NMLVKEYDOWN *pNmLvKeyDown = (NMLVKEYDOWN *)pNmHdr;
       int iRow = ListView_GetSelectionMark(hWndListView);
+      int modkeys = ModifierKeys::get();
 #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
+                      << " Shift:" << !!(modkeys & ModifierKeys::Shift)
+                      << " Ctrl:" << !!(modkeys & ModifierKeys::Control)
+                      << " Alt:" << !!(modkeys & ModifierKeys::Alt) << endLog;
 #endif
 
       if (contents && iRow >= 0)
         {
-          int col_num;
-          int action_id;
-          if ((*contents)[iRow]->map_key_to_action(pNmLvKeyDown->wVKey, &col_num, &action_id))
+          int col_num = 0;
+          int action_id = 0;
+          int todo = (*contents)[iRow]->map_key_to_action(pNmLvKeyDown->wVKey, modkeys,
+                                                          col_num, action_id);
+          int update = 0;
+          if (todo & ListViewLine::Action::Direct)
+            update = (*contents)[iRow]->do_action(col_num, action_id);
+          else if (todo & ListViewLine::Action::PopUp)
             {
-              int update;
-              if (action_id >= 0)
-                update = (*contents)[iRow]->do_action(col_num, action_id);
-              else
-                {
-                  POINT p;
-                  RECT r;
-                  ListView_GetSubItemRect(hWndListView, iRow, col_num, LVIR_BOUNDS, &r);
-                  p.x = r.left;
-                  p.y = r.top;
-                  ClientToScreen(hWndListView, &p);
+              POINT p;
+              RECT r;
+              ListView_GetSubItemRect(hWndListView, iRow, col_num, LVIR_BOUNDS, &r);
+              p.x = r.left;
+              p.y = r.top;
+              ClientToScreen(hWndListView, &p);
+
+              update = popup_menu(iRow, col_num, p);
+            }
 
-                  update = popup_menu(iRow, col_num, p);
-                }
+          if (update > 0)
+            ListView_RedrawItems(hWndListView, iRow, iRow + update -1);
 
-              if (update > 0)
-                ListView_RedrawItems(hWndListView, iRow, iRow + update -1);
+          if ((todo & ListViewLine::Action::NextRow)
+              && iRow + 1 < ListView_GetItemCount(hWndListView))
+            {
+              // move selection to next row
+              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);
             }
         }
     }
diff --git a/ListView.h b/ListView.h
index 95dd9ee..6a1be0b 100644
--- a/ListView.h
+++ b/ListView.h
@@ -25,10 +25,17 @@
 // ListView Common Control
 // ---------------------------------------------------------------------------
 
+struct ModifierKeys
+{
+  enum { Shift = 0x01, Control = 0x02, Alt = 0x04 };
+  static int get(); // get bitmask of currently pressed keys
+};
+
 class ListViewLine
 {
  public:
   enum class State { collapsed, expanded, nothing=-1 };
+  enum Action { None = 0x00, Direct = 0x01, PopUp = 0x02, NextRow = 0x04 };
 
   virtual ~ListViewLine() {};
   virtual const std::wstring get_text(int col) const = 0;
@@ -38,7 +45,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 int map_key_to_action(WORD vkey, int modkeys, int & col_num,
+                                int & action_id) const = 0;
 };
 
 typedef std::vector<ListViewLine *> ListViewContents;
diff --git a/PickCategoryLine.cc b/PickCategoryLine.cc
index d2ac899..b13dbe4 100644
--- a/PickCategoryLine.cc
+++ b/PickCategoryLine.cc
@@ -96,20 +96,19 @@ PickCategoryLine::get_tooltip(int col_num) const
   return "";
 }
 
-bool
-PickCategoryLine::map_key_to_action(WORD vkey, int *col_num, int *action_id) const
+int
+PickCategoryLine::map_key_to_action(WORD vkey, int modkeys, int & col_num,
+                                    int & action_id) const
 {
   switch (vkey)
     {
-    case VK_SPACE:
-      *col_num = pkgname_col;
-      *action_id = 0;
-      return true;
-    case VK_APPS:
-      *col_num = new_col;
-      *action_id = -1;
-      return true;
+    case VK_SPACE: // expand <> collapse category
+      col_num = pkgname_col;
+      return Action::Direct;
+    case VK_APPS: // install/reinstall/uninstall context menu for category
+      col_num = new_col;
+      return Action::PopUp;
     }
 
-  return false;
+  return Action::None;
 }
diff --git a/PickCategoryLine.h b/PickCategoryLine.h
index 6a7321d..7616b15 100644
--- a/PickCategoryLine.h
+++ b/PickCategoryLine.h
@@ -41,7 +41,7 @@ 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;
+  int map_key_to_action(WORD vkey, int modkeys, int & col_num, int & action_id) const;
 
 private:
   CategoryTree * cat_tree;
diff --git a/PickPackageLine.cc b/PickPackageLine.cc
index ae1e520..c1e2a15 100644
--- a/PickPackageLine.cc
+++ b/PickPackageLine.cc
@@ -144,17 +144,30 @@ PickPackageLine::get_indent() const
   return indent;
 }
 
-bool
-PickPackageLine::map_key_to_action(WORD vkey, int *col_num, int *action_id) const
+int
+PickPackageLine::map_key_to_action(WORD vkey, int modkeys, int & col_num,
+                                   int & action_id) const
 {
   switch (vkey)
     {
-    case VK_SPACE:
+    case VK_SPACE: // install/reinstall/uninstall context menu for package
     case VK_APPS:
-      *col_num = new_col;
-      *action_id = -1;
-      return true;
+      col_num = new_col;
+      return Action::PopUp;
+    case 'I': // Ctrl+I: select install default version and move to next row
+    case 'R': // Ctrl+R: select reinstall and move to next row
+    case 'U': // Ctrl+U: select uninstall and move to next row
+      if (modkeys != ModifierKeys::Control)
+        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;
+        }
+      return Action::Direct | Action::NextRow;
     }
 
-  return false;
+  return Action::None;
 }
diff --git a/PickPackageLine.h b/PickPackageLine.h
index 2c59e90..0bf4ae6 100644
--- a/PickPackageLine.h
+++ b/PickPackageLine.h
@@ -37,7 +37,7 @@ 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;
+  int map_key_to_action(WORD vkey, int modkeys, int & col_num, int & action_id) const;
 private:
   packagemeta & pkg;
   PickView & theView;
diff --git a/package_meta.cc b/package_meta.cc
index 8a69525..a5dc436 100644
--- a/package_meta.cc
+++ b/package_meta.cc
@@ -651,6 +651,13 @@ packagemeta::set_action (_actions action, packageversion const &default_version,
 	      srcpick (false);
 	    }
 	}
+      else
+	{
+	  action = NoChange_action;
+	  desired = installed;
+	  pick (false);
+	  srcpick (false);
+	}
     }
   else if (action == Reinstall_action)
     {
@@ -670,6 +677,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;
     }
 
   _action = action;
-- 
2.37.1