public inbox for cygwin-apps@cygwin.com
 help / color / mirror / Atom feed
* [PATCH setup] Keyboard accelerators for install/reinstall/uninstall
@ 2022-08-14 11:57 Christian Franke
  2022-08-15 17:49 ` Jon Turney
  0 siblings, 1 reply; 6+ messages in thread
From: Christian Franke @ 2022-08-14 11:57 UTC (permalink / raw)
  To: cygwin-apps

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

This eases state changes of a selected sequence of packages.

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.

-- 
Regards,
Christian


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

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)) {
+            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);
+          }
         }
     }
     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))
+        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;
     }
 
   _action = action;
-- 
2.37.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH setup] Keyboard accelerators for install/reinstall/uninstall
  2022-08-14 11:57 [PATCH setup] Keyboard accelerators for install/reinstall/uninstall Christian Franke
@ 2022-08-15 17:49 ` Jon Turney
  2022-08-22 15:29   ` Christian Franke
  0 siblings, 1 reply; 6+ messages in thread
From: Jon Turney @ 2022-08-15 17:49 UTC (permalink / raw)
  To: Christian Franke, cygwin-apps

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;
	

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH setup] Keyboard accelerators for install/reinstall/uninstall
  2022-08-15 17:49 ` Jon Turney
@ 2022-08-22 15:29   ` Christian Franke
  2022-08-23 15:16     ` Jon Turney
  0 siblings, 1 reply; 6+ messages in thread
From: Christian Franke @ 2022-08-22 15:29 UTC (permalink / raw)
  To: Jon Turney, cygwin-apps

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH setup] Keyboard accelerators for install/reinstall/uninstall
  2022-08-22 15:29   ` Christian Franke
@ 2022-08-23 15:16     ` Jon Turney
  2022-08-23 16:44       ` Christian Franke
  0 siblings, 1 reply; 6+ messages in thread
From: Jon Turney @ 2022-08-23 15:16 UTC (permalink / raw)
  To: Christian Franke, cygwin-apps

On 22/08/2022 16:29, Christian Franke wrote:
> 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.
> 

Thanks.  This looks good.  Please apply.

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

Confused. I don't think that matters (we could probably be using 
TPM_NONOTIFY), because we use TPM_RETURNCMD.

If we mark accelerators in the menu:

--- a/res/en/res.rc
+++ b/res/en/res.rc
@@ -573,11 +573,11 @@ BEGIN
      IDS_PROGRESS_POSTINSTALL "Running..."
      IDS_PROGRESS_SOLVING "Solving dependencies..."
      IDS_ACTION_DEFAULT "Default"
-    IDS_ACTION_INSTALL "Install"
-    IDS_ACTION_UNINSTALL "Uninstall"
+    IDS_ACTION_INSTALL "&Install"
+    IDS_ACTION_UNINSTALL "&Uninstall"
      IDS_ACTION_SKIP "Skip"
      IDS_ACTION_KEEP "Keep"
-    IDS_ACTION_REINSTALL "Reinstall"
+    IDS_ACTION_REINSTALL "&Reinstall"
      IDS_ACTION_RETRIEVE "Retrieve"
      IDS_ACTION_UNKNOWN "Unknown"
      IDS_ACTION_SOURCE "Source"

They appear when the menu is opened by pressing the menu key (or always, 
if "Underline access keys when available" is on in ease-of-access 
settings), and menu items can be chosen using them.

It's not quite that straightforward because we need to remove the '&' 
when those strings are used elsewhere (e.g. in the action column), but I 
think it can be done...

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

I see.  But these work correctly when chosen via the action menu 
dropdown?  I forget how this all works, and there probably are separate 
paths which duplicate stuff, but this suggests to me that this is 
increasing that duplication, which isn't the desired direction of travel...

> 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? ==
>                  }
>                ...
> 

Yeah, I don't understand why that's there either.  It seems like it's 
going to give you the source package if you've asked for the package to 
be installed, but it's not available.

I think It's some ancient cruft which nobody would notice if we removed :)

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH setup] Keyboard accelerators for install/reinstall/uninstall
  2022-08-23 15:16     ` Jon Turney
@ 2022-08-23 16:44       ` Christian Franke
  2022-08-26 13:33         ` Jon Turney
  0 siblings, 1 reply; 6+ messages in thread
From: Christian Franke @ 2022-08-23 16:44 UTC (permalink / raw)
  To: Jon Turney, cygwin-apps

Jon Turney wrote:
> On 22/08/2022 16:29, Christian Franke wrote:
>> 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.
>>
>
> Thanks.  This looks good.  Please apply.

Thanks. Applied.


>
>>>> 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.
>
> Confused. I don't think that matters (we could probably be using 
> TPM_NONOTIFY), because we use TPM_RETURNCMD.
>
> If we mark accelerators in the menu:
>
> --- a/res/en/res.rc
> +++ b/res/en/res.rc
> @@ -573,11 +573,11 @@ BEGIN
>      IDS_PROGRESS_POSTINSTALL "Running..."
>      IDS_PROGRESS_SOLVING "Solving dependencies..."
>      IDS_ACTION_DEFAULT "Default"
> -    IDS_ACTION_INSTALL "Install"
> -    IDS_ACTION_UNINSTALL "Uninstall"
> +    IDS_ACTION_INSTALL "&Install"
> +    IDS_ACTION_UNINSTALL "&Uninstall"
>      IDS_ACTION_SKIP "Skip"
>      IDS_ACTION_KEEP "Keep"
> -    IDS_ACTION_REINSTALL "Reinstall"
> +    IDS_ACTION_REINSTALL "&Reinstall"
>      IDS_ACTION_RETRIEVE "Retrieve"
>      IDS_ACTION_UNKNOWN "Unknown"
>      IDS_ACTION_SOURCE "Source"
>
> They appear when the menu is opened by pressing the menu key (or 
> always, if "Underline access keys when available" is on in 
> ease-of-access settings), and menu items can be chosen using them.
>
> It's not quite that straightforward because we need to remove the '&' 
> when those strings are used elsewhere (e.g. in the action column), but 
> I think it can be done...

Using plain letters (without Ctrl) or first digit of version number 
already work in the popup menu. But it is possibly tricky to also 
interpret Ctrl+I/R/U in the popup menu.


>
>>>> @@ -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)
>
> I see.  But these work correctly when chosen via the action menu dropdown?

These state changes are not offered by the popup menu. For example for 
not installed packages the popup menu does not contain a selection which 
returns Uninstall_action. Undoing an install request is done via 
re-selection of "Skip" which returns NoChange_action.

The design alternatives were either to emulate this in 
PickPackageLine::map_key_to_action() or to complete the (hidden) state 
machine in packagemeta::set_action(). I decided to use the latter.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH setup] Keyboard accelerators for install/reinstall/uninstall
  2022-08-23 16:44       ` Christian Franke
@ 2022-08-26 13:33         ` Jon Turney
  0 siblings, 0 replies; 6+ messages in thread
From: Jon Turney @ 2022-08-26 13:33 UTC (permalink / raw)
  To: cygwin-apps, Christian Franke

On 23/08/2022 17:44, Christian Franke wrote:
> Jon Turney wrote:
>> On 22/08/2022 16:29, Christian Franke wrote:
>>> 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.
>>>
>>
>> Thanks.  This looks good.  Please apply.
> 
> Thanks. Applied.
> 
> 
>>
>>>>> 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.
>>
>> Confused. I don't think that matters (we could probably be using 
>> TPM_NONOTIFY), because we use TPM_RETURNCMD.
>>
>> If we mark accelerators in the menu:
>>
>> --- a/res/en/res.rc
>> +++ b/res/en/res.rc
>> @@ -573,11 +573,11 @@ BEGIN
>>      IDS_PROGRESS_POSTINSTALL "Running..."
>>      IDS_PROGRESS_SOLVING "Solving dependencies..."
>>      IDS_ACTION_DEFAULT "Default"
>> -    IDS_ACTION_INSTALL "Install"
>> -    IDS_ACTION_UNINSTALL "Uninstall"
>> +    IDS_ACTION_INSTALL "&Install"
>> +    IDS_ACTION_UNINSTALL "&Uninstall"
>>      IDS_ACTION_SKIP "Skip"
>>      IDS_ACTION_KEEP "Keep"
>> -    IDS_ACTION_REINSTALL "Reinstall"
>> +    IDS_ACTION_REINSTALL "&Reinstall"
>>      IDS_ACTION_RETRIEVE "Retrieve"
>>      IDS_ACTION_UNKNOWN "Unknown"
>>      IDS_ACTION_SOURCE "Source"
>>
>> They appear when the menu is opened by pressing the menu key (or 
>> always, if "Underline access keys when available" is on in 
>> ease-of-access settings), and menu items can be chosen using them.
>>
>> It's not quite that straightforward because we need to remove the '&' 
>> when those strings are used elsewhere (e.g. in the action column), but 
>> I think it can be done...
> 
> Using plain letters (without Ctrl) or first digit of version number 
> already work in the popup menu. But it is possibly tricky to also 
> interpret Ctrl+I/R/U in the popup menu.
>

I think we might be talking at cross-purposes here.

I don't think we need to do anything specific to make accelerators work 
in that menu, because TrackMenuPopup() is running it's own modal message 
loop which does TranslateAccelerator() for it...

>>>>> @@ -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)
>>
>> I see.  But these work correctly when chosen via the action menu 
>> dropdown?
> 
> These state changes are not offered by the popup menu. For example for 
> not installed packages the popup menu does not contain a selection which 
> returns Uninstall_action. Undoing an install request is done via 
> re-selection of "Skip" which returns NoChange_action.
> 
> The design alternatives were either to emulate this in 
> PickPackageLine::map_key_to_action() or to complete the (hidden) state 
> machine in packagemeta::set_action(). I decided to use the latter.

Ok, that makes perfect sense.  Thanks!





^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-08-26 13:33 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-14 11:57 [PATCH setup] Keyboard accelerators for install/reinstall/uninstall Christian Franke
2022-08-15 17:49 ` Jon Turney
2022-08-22 15:29   ` Christian Franke
2022-08-23 15:16     ` Jon Turney
2022-08-23 16:44       ` Christian Franke
2022-08-26 13:33         ` Jon Turney

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).