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

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

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!