Opened 8 years ago

Closed 6 years ago

Last modified 6 years ago

#12753 closed Patch - Feature (Won't Fix)

PR for Services API Adding GetKeyBindList and PutKeyBind to Myth Services PR #120

Reported by: Mitch Capper <mitch.capper@…> Owned by: Bill Meek
Priority: minor Milestone: 0.28.2
Component: MythTV - Services API - Backend Version: Master Head
Severity: medium Keywords:
Cc: Ticket locked: no

Description

https://github.com/MythTV/mythtv/pull/120

This adds two service API calls GetKeyBindList? and PutKeyBind?. The code here is largely modeled after the existing GetSettingList/PutSetting? and storageGroupDir / storageGroupDirList code. It was done after some discussion in #12738.

This is to make working with the new Frontend/SendKey? function easier, so you can determine what key is mapped to what, and adjust key bindings. It doesn't notify/reload the frontend (but neither does MythWeb).

There is at least two major items for review/comment: -PutSetting? only returns true if it has changed the key binding. It returns false if the key binding is invalid, or if it is already set to that key. I could just have it always return true without checking if any rows are updated, or if we want to be very correct I could first do a select on the key to make sure it exists, and return true as long as it exists.

-I merged jumppoints and key bindings together (from a function point of view). Jumppoints now show up as Context=JumpPoint?. Its a hack but it avoids having to make two more functions and more round trips for a client trying to get all the current binds.

-Also I debated added an IsAlive? type option to only return keybindings for hosts that are active (when hosts are not specified) I don't feel this is really needed however.

Change History (9)

comment:1 Changed 8 years ago by Bill Meek

Status: newinfoneeded_new

Mitch,

Did you consider, for example: KeyBindings::ReplaceActionKey rather to doing the UPDATE keybindings in Myth::PutKeyBind?

On a different note, for getting rid of trailing blanks,this may be of value if you use vim (from .vimrc):

autocmd FileType c,cpp,h,python autocmd BufWritePre <buffer> :%s/\s\+$//e

I don't see trailing spaces mentioned in the Coding Standards, but I have seen commits removing them.

comment:2 Changed 8 years ago by Mitch Capper <mitch.capper@…>

I have updated the code to remove the trailing spaces, will try to pay attention to that in the future not sure where I am picking them up from.

Will wait to hear back about if using the FE classes for this is the best way to go before trying to update to the KeyBindings? class.

comment:3 Changed 8 years ago by Bill Meek

Status: infoneeded_newassigned

comment:4 Changed 8 years ago by sphery

If this is going to allow changing bindings, it should also (somehow) tell any running frontend on the host for which the bindings are changed to reload its keys, similar to what's done in KeyBindings::CommitAction?() and KeyBindings::CommitJumppoint?() or a blanket reload as in mythfrontend's static ReloadKeys?() (in main.cpp). Without that, the frontend would be running with different keys until a restart. I'd guess this would entail adding a frontend services call to ask the frontend to reload keys, then making use of that call after modifying the keys.

comment:5 Changed 8 years ago by Mitch Capper <mitch.capper@…>

Sure, mainly this replicates the current MythWeb implementation. stuartm was debating in the channel if we want to continue to allow remote keybind modification or not.

We may want to (if we are to notify the frontends) figure out a way to rate limit the reload requests, incase someone is changing a lot of bindings a once rather than sending dozens of reloads all at the same time.

comment:6 Changed 8 years ago by Mitch Capper <mitch.capper@…>

In thinking about this a bit more I find it would be useful to be able to get and edit key bindings via the services API. Maintenance for the code should be fairly low, it allows for easier configuration of remotes like http://www.amazon.com/Ortek-Windows-Infrared-Receiver-Ultimate/dp/B00224ZDFY/ too, and the ability to do things like duplicate control setup (or partial setup) between frontends quite easily.

I think there is something of a goal to have 3rd party clients able to be first class members of mythtv without having to resort to mythprotocol or direct SQL manipulation.

It sounds like using this in concert with SendKey? is not really recommended, to which (assuming others agree) that means we need to fix SendAction? to make sure it works for all actions possible (reachable via SendKey? that is). If others agree I can try to fix that and submit a separate PR for it.

comment:7 Changed 7 years ago by Stuart Auchterlonie

Milestone: 0.28.10.28.2

Moving remaining open 0.28.1 tickets to 0.28.2

comment:8 Changed 6 years ago by Stuart Auchterlonie

Resolution: Won't Fix
Status: assignedclosed

Closing any remaining tickets for 0.28, if the issue persists, feel free to reopen and align to v29 or master

comment:9 Changed 6 years ago by Bill Meek

Owner: changed from Bill Meek to Bill Meek
Note: See TracTickets for help on using tickets.