JSON-RPC SendKey, ExecuteAction, ShowCodec and ShowOSD in Input namespace #1056

Merged
merged 7 commits into from Jul 11, 2012

Conversation

Projects
None yet
4 participants
Owner

Montellese commented Jun 8, 2012

This PR adds the methods Input.ShowCodec and Input.ShowOSD on joethefox's request for the official remote app. Furthermore I've added the generic methods Input.SendKey and Input.ExecuteAction. The former takes a string (because a unicode character can be more than a single (ASCII) character) and picks/processes the first unicode character and can therefore be used to send (almost) any key to XBMC e.g. to emulate a keyboard in a remote control app. The latter takes an action and executes it. More sophisticated actions like ExecBuiltIn() are not supported because they can't be validated against a static string in JSON-RPC.

There seems to be quite an interest in Input.SendKey because it would allow clients to do all the input through JSON-RPC and they can drop the EventServer implementation. I've just added Input.ExecuteAction because it belongs in the same area as Input.SendKey but I have to admit that I wouldn't object to removing it from the PR again.

Member

jmarshallnz commented Jun 8, 2012

I'm not sure about Input.SendKey - the others seem reasonable enough.

Are folk wanting SendKey so that it runs through the keyboard keymap? This seems to be non-deterministic to me - they should send actions instead. If they're wanting to send keys to the virtual keyboard then I have no problem with it.

Owner

Montellese commented Jun 8, 2012

So you are against providing SendKey to e.g. open the Context Menu (by sending the "c" key) instead of using Input.ContextMenu or Input.ExecuteAction? I agree that Input.SendKey should primarily be used for general text input but maybe it would make more sense to implement an Input.SendText method in that case as it would cover both sending single characters and texts.

Member

jmarshallnz commented Jun 8, 2012

The problem is that the user changes their keymap and the remote breaks. So yes, ExecuteAction or ContextMenu if we're wanting to be more restrictive make sense. Personally I'd go for the latter, or at least validate what we're allowing through ExecuteAction to make sure we're not sending things we don't want to (I don't think there'd be anything - the builtins are the worrisome ones). As long as they're validated, having a general ExecuteAction is fine.

So yeah, SendText() could be done, which could be restricted to input in text fields (it should hit only the edit control/virtual keyboard path).

Owner

Montellese commented Jun 8, 2012

Currently ExecuteAction just allows all the actions that are specified in the list in ButtonTranslator.cpp (i.e. https://github.com/Montellese/xbmc/blob/fc4ad0857b43a7f4a28cff1a1ce359d2eebd4a4e/xbmc/input/ButtonTranslator.cpp#L47 ) but that's the only "validation" that takes place in JSON-RPC.

I'll see about replacing Input.SendKey with Input.SendText.

Member

topfs2 commented Jun 9, 2012

I agree with jmarshall, sent text sounds like a better suggestion. Personally I'd prefer more explicit than sendaction, i.e. input.contextmenu (or perhaps even gui.contextmenu?) would be better IMO

Owner

Montellese commented Jun 9, 2012

There already is Input.ContextMenu but obviously it's a bit of a pain to add a new method everytime someone comes up with a new input action that is missing.

Contributor

RobertMe commented Jul 2, 2012

Hi,

As contributor to the xbmcremote app for MeeGo I think it's a good idea to add a SendKey/SendText method. And I have a request in these. Would it be possible to add an Application.RequestKeyboard (or something along those lines) notification? So when the user selects a text input field in XBMC and the (virtual) keyboard pops up it also sends a notification to the clients, so a client would be able to open the virtual keyboard(/show a text input field, when the device has a hardware keyboard) so the user could enter the text in there and the entered text could be send to XBMC (using SendText, would probably be better than SendKey) and the virtual keyboard in XBMC maybe automatically closes when it receives the SendText command. This keyboard integration between XBMC and a remote app could further smoothen the user experience in entering text. If possible I think a "title" of the input field (as displayed on XBMCs virtual keyboard) and a type (text, number) should be the parameters of this notification.

Owner

Montellese commented Jul 8, 2012

Okay I finally found the time to look into this. I have replaced Input.SendKey with Input.SendText which takes any text (with a minimum length of 1 character) and sends it via a new GUI message GUI_MSG_SET_TEXT to the currently active control. If that control is a CGUIEditControl or the dialog containing the focused control is CGUIDialogKeyboard the provided text will be set as the new text. I tested this mainly with the "Filter" functionality in the video library (which can be both an edit control or a virtual keyboard).

Furthermore (as requested) I have added two new notifications GUI.OnKeyboardOpened/OnKeyboardClosed but I have to admit I'm not very happy with the naming. Better ideas are welcome :-)

Member

jmarshallnz commented Jul 9, 2012

So the client would currently need to SendText and then send an OK event to get the virtual keyboard to close?

I wonder if it makes sense for SendText to close the dialog automatically?

I don't have any insight into alternative naming.

Contributor

RobertMe commented Jul 9, 2012

Great. But having to send OK afterwards indeed seems strange. Would it also be possible to send the heading (m_strHeading) as parameter of Opened? So the client can also display the title. And the same support in, I think, CGUIDialogNumeric would be awesome (and pass through the mode). So the client could decide to display a numeric keyboard instead of the qwerty keyboard, or a date and/or timer picker.

Looking at how easy you added the notification I might even try to implement this myself.

Owner

Montellese commented Jul 9, 2012

I thought about the auto-close as well but wasn't sure what the best way was. One way would be to check what kind of dialog we currently have in CInputOperations::SendKey and execute a close on it but I don't like that because it will need extra work in CInputOperations everytime a new dialog is extended with this functionality (yeah this will probably not happen often). Another approach would be to do the close in the handling of GUI_MSG_SET_TEXT so every dialog/control can decide for itself whether it should auto-close or not.

Owner

Montellese commented Jul 9, 2012

Forgot one thing: I didn't extend CGUIDialogNumeric with this functionality yet because it is far more complex than CGUIDialogKeyboard. It doesn't just simply take a string, it can take a datetime (in seconds) or an ip-address (in four number blocks) or a number (as a string).

Contributor

RobertMe commented Jul 9, 2012

Closing would be sending "OK" right? So SendText could execute OK afterwards automatically. Instead of doing some specialized close handling. And it could be added as optional bool ok = true parameter to SendText.

And the numerics could also be send as GUI_MSG_SET_TEXT and parsed internally by DialogNumeric. So the JSON handling could keep sending the SET_TEXT message, and doesn't need to know about which mode the dialog is in. But of course the SET_TEXT handling should be written first. The most basic handling would be to loop over the input string (one char at a time) and call OnNumeric one by one.

Owner

Montellese commented Jul 9, 2012

OK I've made the following changes:

  • CGUIDialogNumeric now handles the GUI_MSG_SET_TEXT message as well. It loops through every character in the string and if it finds one character that is not a number it will fail. If all characters are numbers it will replace the current input value with the one provided with the GUI message.
  • Input.SendText now takes an optional parameter "done" (which defaults to true) and closes the CGUIDialogKeyboard and CGUIDialogNumeric if set to true.
  • I renamed the notifications to Input.OnInputRequested and Input.OnInputFinished (much better than the old ones IMO) and they provide a "type" parameter which can be "keyboard", "time", "date", "ip", "password", "number" or "seconds". Furthermore for CGUIDialogKeyboard it will also contain a "title" parameter with the heading of the dialog.
Contributor

RobertMe commented Jul 9, 2012

Great :) Will try to get it implemented in the MeeGo xbmcremote app and report back on how the API works. I think only date, time and ip will be difficult as they need to be send as one long string with the numbers without delimiters. But shouldn't be a to big issue.

But why aren't you sending "title" in CGUIDialogNumber? As heading seems to be present there also. Is Init and thus the notification getting called before the heading is set?

Owner

Montellese commented Jul 9, 2012

Because in CGUIDialogNumber the heading is not directly available. In SetHeading() it is passed on to the CONTROL_HEADING_LABEL. I'll see if I can retrieve it from that CGUIControl.

Owner

Montellese commented Jul 9, 2012

Added "title" for the notifications from CGUIDialogNumeric as well (if available) but the implementation isn't very nice :-/

Member

jmarshallnz commented Jul 9, 2012

The GUIDialogNumeric is a bit messy - you can't just send 13:45 for the time, 13/5/2012 for the date, or 123.4.5.6 for the IP which seems the most natural.

It'd be much nicer if SetMode() was changed to always take a string - it's only not that way for the date/time stuff.

I suggest leaving this out and pushing the rest in this window perhaps?

Owner

Montellese commented Jul 10, 2012

Yeah you are right. I'll remove the handling of GUI_MSG_SET_TEXT from CGUIDialogNumeric and will come up with a better approach for the next merge window.

Owner

Montellese commented Jul 10, 2012

OK I've removed all the changes done to CGUIDialogNumeric (both handling GUI_MSG_SET_TEXT and the notifications) and will come up with a better approach for that later on.

@ghost ghost assigned Montellese Jul 10, 2012

Owner

Montellese commented Jul 10, 2012

I added two more commits. The first one implements a new CGUIDialogNumeric::SetMode which takes a CStdString instead of a void*. Only the date and time modes need extra handling in that new implementation and I use CDateTime methods to convert the string parameter into the appropriate format.
The second commit adds the notifications back to CGUIDialogNumeric.

You could probably use dateTime = StringUtils::TimeStringToSeconds(initial); perhaps?

Owner

Montellese replied Jul 11, 2012

I looked at StringUtils::TimeStringToSeconds() but the problem is that in case of INPUT_TIME we look for HH:MM whereas TimeStringToSeconds assumes that if the string is XX:YY that it's MM:SS and only handles hours if the string is HH:MM:SS.

Member

jmarshallnz commented Jul 10, 2012

Looks good once that's done.

Montellese added a commit that referenced this pull request Jul 11, 2012

Merge pull request #1056 from Montellese/jsonrpc_sendkey
JSON-RPC SendKey, ExecuteAction, ShowCodec and ShowOSD in Input namespace

@Montellese Montellese merged commit c924204 into xbmc:master Jul 11, 2012

Contributor

RobertMe commented Jul 11, 2012

Don't know if I'm to late with this question as it's already merged, but I started implementing it on the client side and have a small extra request. Would it be possible to also send the value in the OnInputRequested announcement? For example some/most settings already have a value set and when the virtual keyboard opens this value is also already set. So it would be great if this value could also be preset on the client which receives the notification. So the user is able to edit the value, instead of having to type it over and only changing a small part.

Edit:
DialogKeyboard-s hiddenInput flag (for passwords) would also be appreciated as being part of OnInputRequested.

Owner

Montellese commented Jul 12, 2012

Makes sense to me to provide the initial value of the input dialogs. Will add later.

Concerening the hiddenInput flag, would it be enough to change the "type" value to "password" as that's already available or would you need some way of being able to distinguish between the password mode of CGUIDialogKeyboard and CGUIDialogNumeric?

Contributor

RobertMe commented Jul 12, 2012

Distinguishing between keyboard and numeric can be used to set the the input method hint on the mobile phone (which validates the input and also changes the keyboard between qwerty and numeric), and on a text field it's also possible to set the echo mode to normal or password. Haven't tested if they could be combined but don't see a reason why it shouldn't be possible.

I'm also wondering if DialogNumeric distinguishes integers from decimals. So if only digits can be inserted or also a decimal dot. As the input method hints also has digits only and numeric.

I also discussed SendText with the main dev. of xbmcremote for MeeGo. A good point he made in still using the old method of sending text (which in MeeGos app is a legacy command) and possibility to send only a char is fast scrolling through a list. So when typing for example a "p" the active list scrolls to the "p". So maybe the list control should also be able to handle the SET_TEXT message and scroll to the first letter of the send text (or maybe even the best match of the send text, so when sending "abc" and there isn't an item starting with abc it selects "abb", so the last item before where the match would be).

tru added a commit to plexinc/plex-home-theater-public that referenced this pull request Mar 14, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment