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