[mythtv] Key mapping for BOOKMARK

[Michael T. Dean]

On 03/13/2018 02:44 PM, Peter Bennett wrote:
> During playback, the select key assigned in global key mapping is used 
> for setting and removing bookmarks.

Yes, it's an action "override" (where you tell MythTV to do 
something--SELECT--and it does something else, though in this case the 
something else is at least reasonable as it's selecting that location 
for the bookmark) and is modified via a setting:

Alternate clear and save bookmark
During playback the SELECT key (Enter or Space) will alternate between 
"Bookmark Saved" and "Bookmark Cleared". If disabled, the SELECT key 
will save the current position for each keypress.

You'll need to handle this, also, when you modify.  Since a setting 
shouldn't modify key behavior (so that users can map both behaviors, if 
desired), we really should have a separate action for SET_BOOKMARK 
(always sets the current location as the bookmark) and TOGGLE_BOOKMARK 
(which alternates between setting and clearing) rather than just keeping 
the setting and changing a generic action (BOOKMARK) to set or toggle 
based on the setting--which is a problem for users who want both actions 
available.

> To allow users to set a different key for bookmarks without having to 
> change global select, I have create a new key mapping for bookmarks.

Creating a new action for setting bookmarks is good.  All actions should 
be separately modifiable, exactly for the reason you describe.

> I can default this to the same keys as SELECT (i.e. space, enter, 
> return) and it should not affect anybody unless they have changed 
> their global select settings. (see 
> https://code.mythtv.org/trac/attachment/ticket/13234/20180313_1414_bookmark.patch). 
> It is not committed yet.

This--the "unless they have changed their global settings" part--is one 
reason why...

> Personally I never set a bookmark and the fact that it is on the most 
> prominent keyboard keys seems inappropriate.
>
> On the Nvidia Shield there is only one button (enter), and it should 
> do something useful that leads to other interactions, rather than set 
> a bookmark. Other remotes also have a prominent enter button, which 
> should be used for something more important.
>
> I propose defaulting the new BOOKMARK setting to Ctrl+K (since K is 
> "jump to bookmark").

...our approach in the past has generally been to not provide a default 
key binding for any new actions added to MythTV because the chosen 
key(s) may conflict with a user-mapped binding.  See 
https://lists.gt.net/mythtv/dev/266100#266100 .  Based on the responses 
in this thread--suggesting few people use SELECT to set a bookmark--I 
would consider this to be a not significant case. Therefore, having an 
empty default keylist for SET_BOOKMARK and for TOGGLE_BOOKMARK would 
make sense since there are other approaches by which the user can set a 
bookmark (even by just exiting playback).

However, some existing users are using the functionality. Therefore, my 
recommendation is to do it the hard way, but the way that will work for 
everyone.  You'll create the new actions (one for SET_ and one for 
TOGGLE_BOOKMARK), and default each to an empty keylist.  So, new users 
will have to manually set a key binding if they want to use the actions 
(but based on the responses to this showing little use of the SELECT 
override and my assumption that more people more often set bookmarks by 
exiting playback than by using our SELECT override, that shouldn't be a 
problem).  And, to ensure that existing users get the behavior they're 
used to, you can then do a DB update that:

for each host in the keybindings table with a Global/SELECT mapping:
   get the keylist for Global/SELECT
   check the setting AltClearSavedPosition
     if enabled, map the Global/SELECT keylist to 
TV_Playback/TOGGLE_BOOKMARK
     if disabled, map the Global/SELECT keylist to TV_Playback/SET_BOOKMARK

This will be similar to what was done in DB update 1280.

However, I think mapping a key that's already mapped to a Global action 
to an action in another context will result in a warning every time you 
start up mythfrontend.  See 
https://github.com/MythTV/mythtv/commit/629db590 and test by looking for:

QString("Key %1 is bound to multiple actions in context 
%2.").arg(key).arg(context));
(from MythMainWindow::BindKey() )

in your mythfrontend log. I'm OK with the warning showing up for 
existing users who "inherit" the SELECT key list for SET_ or 
TOGGLE_BOOKMARK, but it's quite likely you may get a lot of 
discussion/confusion on the lists because of the warning just showing up 
after a user upgrades to the new version which no longer uses SELECT for 
the bookmark actions.

Mike