FS#11081 - Adds user programmable hotkey to context menus

Attached to Project: Rockbox
Opened by Jeffrey Goode (Blue_Dude) - Saturday, 06 March 2010, 17:33 GMT
Last edited by Jeffrey Goode (Blue_Dude) - Thursday, 01 April 2010, 03:21 GMT
Task Type Patches
Category User Interface
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Release 3.4
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No


This is a work in progress. This first patch shows how the interface would work on the e200 target only. The hotkey selected for the e200 is short-REC, which was the key used in the bookmark menu to specify the delete function. So far, the hotkey doesn't do anything except show a splash message, except in the bookmark menu, which works as before.

The goal is to allow the user to drill down to the context menu with the context menu button, scroll to the function intended for the hotkey, then press the context menu button again to assign that function to the hotkey. Then, the user could press the hotkey button on an item rather than the context menu button to perform that function. The hotkey assignment would persist between session, and would vary by context menu.

Comments are appreciated.
This task depends upon

Closed by  Jeffrey Goode (Blue_Dude)
Thursday, 01 April 2010, 03:21 GMT
Reason for closing:  Accepted
Additional comments about closing:  Committed as r25414
Comment by Thomas Martitz (kugel.) - Sunday, 07 March 2010, 02:04 GMT
I think we've rejected this kind of work before (see  FS#5555 ) but maybe opinions have changed since then.
Comment by Jeffrey Goode (Blue_Dude) - Sunday, 07 March 2010, 02:50 GMT
Hm. I read some of the comments on  FS#5555 . I sure would like to know why it was ultimately rejected.
Comment by Jeffrey Goode (Blue_Dude) - Monday, 22 March 2010, 15:48 GMT
Here's a working version with keymapping for the e200. So far the only hotkey functions that can be assigned are delete file/dir and playlist insert. It's easily expandable though.
Comment by Jeffrey Goode (Blue_Dude) - Tuesday, 23 March 2010, 04:20 GMT
This one is ready for general use. I've included LANG strings and a way to disable the hotkey: just try to assign the hotkey to a function that's not supported.
Comment by Jeffrey Goode (Blue_Dude) - Tuesday, 23 March 2010, 19:22 GMT
Adds WPS hotkey function, co-opting the current playlist view hotkey. The WPS hotkey defaults to playlist view, so unless the user changes it, operation is completely transparent. Some code cleanup to make it easier to read and change.
Comment by Jeffrey Goode (Blue_Dude) - Wednesday, 24 March 2010, 16:55 GMT
This is another code cleanup, with better build optimizations and a smaller binsize. Non-hotkey targets have no binsize increase at all and even hotkey targets can be built by commenting out the HOTKEY define in its config file without any binsize increase.

I forgot to mention that a couple of versions back I made the hotkey assignment key the same as the hotkey itself. So to assign a hotkey, go into the tree or WPS context menu, scroll to the function you want to assign and press the hotkey. If it's an acceptable assignment, the splash will confirm the new assignment. If not, the splash will say that the hotkey is Off and the assignment will be cleared. Assignments are permanently saved.
Comment by Jeffrey Goode (Blue_Dude) - Thursday, 25 March 2010, 04:11 GMT
Rewrote the set and execute hotkey functions to much more easily incorporate additional function assignments. Assignments are now in a simple struct that has all the info needed to assign and run hotkeys. Bin size is about the same as before, but adding more functions in the future will take much less room going forward than adding in all that else-if and case code. For instance, I enabled the existing delete function in the WPS screen and it didn't add any size at all since it was a matter of setting a flag. Each new function adds 20 bytes.
Comment by Jonathan Gordon (jdgordon) - Thursday, 25 March 2010, 04:18 GMT
there shold be a way to see the hotkeys value (or action or whatever) and a proper way to clear it instead of trying to assign to an illegal value.
Also, I dont tihnk the assignemnt should persist over boots.
Comment by Jeffrey Goode (Blue_Dude) - Friday, 26 March 2010, 02:22 GMT
Added a menu under Settings->General Settings->System->Hotkey that has the following items: Hotkey Persistence, View Hotkey Settings, and Reset Settings. Persistence is boolean, with yes meaning keep settings between sessions. If no, then settings are reset to default during init. View Hotkey Settings shows the functions associated with those keys and Reset Settings resets to defaults. Note that if the default is anything other than OFF, there's no way to completely disable that hotkey. However, this mimics current hard coded key behavior.

These additions cost quite a bit of bin size though: about 600 bytes.
Comment by Jeffrey Goode (Blue_Dude) - Saturday, 27 March 2010, 23:27 GMT
Got rid of the persistence menu item. With a method of resetting the hotkeys manually, it's not necessary. Hotkey settings are now always persistent. If you've got to have it, though, I've included a patch that puts it back in.
Comment by Jeffrey Goode (Blue_Dude) - Monday, 29 March 2010, 02:27 GMT
Added a confirmation dialog for setting hotkeys, so they should never be set by accident. Moved the hotkey menu from the system menu to the general settings menu, which isn't quite so deep and makes more sense.
Comment by Jeffrey Goode (Blue_Dude) - Monday, 29 March 2010, 15:20 GMT
Code cleanup, minor changes. Ready to go!
Comment by Jeffrey Goode (Blue_Dude) - Tuesday, 30 March 2010, 02:28 GMT
Key assignments for the tree screen and WPS screen can be different. This should make creating keymaps far easier. Many fewer conditional compile statements, which is much cleaner but causes a bit of code bloat (< 100 bytes). Keymaps are included for e200 and c200, but hotkeys are disabled by default. Uncomment the define in the config files to switch them on. Took out code in bookmark.c - remapping the delete key to the hotkey just isn't going to work for some targets. Maybe it'll be included later.
Comment by Jeffrey Goode (Blue_Dude) - Wednesday, 31 March 2010, 01:47 GMT
OK, got a lot of work done on different keymaps. There are quite a few targets that can be switched on for hotkeys without any more keymapping. But there are also many that need work. All the targets that can support hotkeys out of the box are switched on. The others have placeholder lines that show where work is needed, and are switched off. Some look like they won't ever support hotkeys.

To assign hotkeys, I used the existing keypress in WPS for View Playlist, which the hotkey replaces. If that keypress could be assigned to the tree hotkey without conflict, I copied it. If not, I put in a commented out placeholder for future assignment. If both the WPS and tree hotkeys were valid, I switched on the hotkey in the associated config file. If not, I put in the lines but commented out the define.

There are several targets that can support hotkeys but there was some kind of conflict that prevented me from making a valid assignment. Changing existing key assignments to clear the conflict should be a group effort. And there were several targets that didn't support View Playlist and I didn't try to go further without more target specific information. Those keymaps were unchanged and their configs switched off.
Comment by Thomas Martitz (kugel.) - Wednesday, 31 March 2010, 11:23 GMT
Maybe simply remove ACTION_WPS_VIEW_PLAYLIST and move the #ifdef'd part into the ACTION_TREE_HOTKEY case (removing the need to #ifdef around in the keymap files?
Comment by Jeffrey Goode (Blue_Dude) - Wednesday, 31 March 2010, 12:56 GMT
I kept the ifdef's in there in case you might want to switch off the hotkey for any reason and get current behavior back. Right now while it's still being rolled out that's prudent. I intend to remove the ifdefs for fully supported targets later. I can't assign both TREE and WPS in the same place because they're in different contexts, so they will always be separate definitions. And if TREE isn't currently defined that means there's a keymap conflict that needs to be resolved by someone with good target knowledge. Once both TREE and WPS are defined without conflict, you can switch on HOTKEY in the config and remove the ifdefs in the keymap without any issue.
Comment by Thomas Martitz (kugel.) - Wednesday, 31 March 2010, 12:59 GMT
I'm not sure if you understood me correctly.

My idea was to ACTION_WPS_VIEW_PLAYLIST (and the case in wps.c) and to put the code of the ACTION_WPS_VIEW_PLAYLIST case into the ACTION_TREE_HOTKEY case. Basically let ACTION_TREE_HOTKEY replace ACTION_WPS_VIEW_PLAYLIST no matter of HAVE_HOTKEY being defined or not.
Comment by Jeffrey Goode (Blue_Dude) - Wednesday, 31 March 2010, 23:50 GMT
Removed conditional compile in wps.c. This makes assigning the keypress for WPS hotkey more flexible in some targets. Also added Show Track Info and Open With... to potential assignments. Tweaked the function code for delete to take into account that not all selections are valid for deletion (replaces menu callback logic).
Comment by Jeffrey Goode (Blue_Dude) - Thursday, 01 April 2010, 01:31 GMT
Cleaned up, changed defines.
Comment by Jeffrey Goode (Blue_Dude) - Thursday, 01 April 2010, 01:38 GMT
Bug in features.txt. Use this instead.