Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix JSON ButtonEvent #18302

Merged
merged 2 commits into from
Oct 6, 2020
Merged

Fix JSON ButtonEvent #18302

merged 2 commits into from
Oct 6, 2020

Conversation

a1rwulf
Copy link
Member

@a1rwulf a1rwulf commented Aug 19, 2020 •
edited
Loading

RFC

This is an RFC on how to fix the issue.
I see multiple ways to do it, for example:
The CKey constructor could use holdtime and check if it exceeds
the treshold and apply the modifier by itself.
I'm not sure where the longpress translation belongs to from an
architectural pov.

Description

The current ButtonEvent implementation has 2 problems:

  • If any key has a longpress mapping it isn't handled at all
  • No matter what holdtime you set, it's not respected

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

@a1rwulf a1rwulf added Type: Fix non-breaking change which fixes an issue Component: Input v19 Matrix labels Aug 19, 2020
@a1rwulf a1rwulf added this to the Matrix 19.0-alpha 2 milestone Aug 19, 2020
@a1rwulf a1rwulf requested a review from garbear August 19, 2020 04:55
@a1rwulf a1rwulf changed the title Fix JSON ButtonEvent WIP: Fix JSON ButtonEvent Aug 19, 2020
@a1rwulf
Copy link
Member Author

a1rwulf commented Aug 20, 2020

@garbear What's your thoughts on this?

garbear
garbear reviewed Aug 21, 2020
View reviewed changes
xbmc/input/KeyboardTranslator.h Outdated
static uint32_t TranslateString(const std::string& szButton);
static uint32_t TranslateString(const std::string& szButton, int holdTime);

static const int HOLD_TRESHOLD = 250;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the same value as other hold time calculations? should it be declared somewhere more central?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can see there are no other hold time calcs, besides the one in KeboardStats cpp which does the exact same thing for real keyboard input, so I'd prefer to get rid of it somehow.
But I think a more central place would be good.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure there's no keyboard hold threshold constants elsewhere? The joystick constant is declared here: https://github.com/xbmc/xbmc/blob/master/system/keymaps/joystick.xml#L175

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found it: https://github.com/xbmc/xbmc/blob/master/xbmc/input/KeyboardStat.cpp#L25

@garbear
Copy link
Member

garbear commented Aug 21, 2020

I don't see any problems with this approach. hold time calculation certainly belongs in input/, I would expect to find it either in InputManager.cpp, input/keyboard/ or a file with "Keyboard" in the name. KeyboardTranslator.cpp satisfies this.

@a1rwulf
Copy link
Member Author

a1rwulf commented Aug 21, 2020

I don't see any problems with this approach. hold time calculation certainly belongs in input/, I would expect to find it either in InputManager.cpp, input/keyboard/ or a file with "Keyboard" in the name. KeyboardTranslator.cpp satisfies this.

I wasn't sure about this.
How does a button hold on an IR remote or gamepad differ from a keyboard hold?

@garbear
Copy link
Member

garbear commented Aug 22, 2020

How does a button hold on an IR remote or gamepad differ from a keyboard hold?

I use the term "input morphology", how the input data looks and what events it arrives for. I've adopted "keyboard" to mean input that looks like how an OS delivers keys, with only key down events, and "button" to mean how input looks from a controller, with a down and up event.

Because IR remotes don't send up events, I consider them closer to operating system keys. Unfortunately, while IR remote buttoms looks like keys to the app, they look like gamepad buttons to humans. So this complicates things. Maybe we should change things architecturally.

@garbear
Copy link
Member

garbear commented Aug 22, 2020

OK, I found the existing keyboard input logic in KeyboardStat.cpp. Now I think that input logic doesn't belong in KeyboardTranslator.cpp, this file must be for only translation, with no input logic. The input logic in KeyboardStat.cpp should be the basis of all thresholding, and possibly refactored to somewhere else.

@a1rwulf
Copy link
Member Author

a1rwulf commented Aug 24, 2020

Thx for your comments @garbear I'll check out about a new approach.

@a1rwulf a1rwulf mentioned this pull request Aug 28, 2020
Merged
13 tasks
@a1rwulf
InputManager: Fix BUTTON_EVENT not working when button has longpress …
e86866b 
…mapping

OnKey is supposed to be used in combination with OnKeyUp, if the
OnKeyUp event isn't also fired, all keys that have a longpress mapping
don't work at all.
We can bypass this limitation, by directly using HandleKey.
@a1rwulf
Copy link
Member Author

a1rwulf commented Sep 29, 2020

@garbear that's the new version.
I tried to check how to handle it and ended up adding a new class that handles longpresses for the XBMC_BUTTON event.
I moved the TRESHOLD constant to InputTypes so that it can be used for XBMC_BUTTONS and for XBMC_KEYDOWN/XBMC_KEYUP events.

eigendude
eigendude reviewed Oct 2, 2020
View reviewed changes
xbmc/input/ButtonStat.cpp Outdated
if (key.GetHeld() > HOLD_TRESHOLD)
buttonCode |= CKey::MODIFIER_LONG;

CKey translatedKey(buttonCode, key.GetHeld());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not do key.SetButtonCode();? Is it ok to drop non-code or held CKey properties?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, this class is called CButtonState, and buttons are scoped to only have a code and held duration.

eigendude
eigendude approved these changes Oct 2, 2020
View reviewed changes
Copy link

@eigendude eigendude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approval, with one request. Can you move CButtonState into a subdirectory of input/?

garbear
garbear approved these changes Oct 2, 2020
View reviewed changes
Copy link
Member

@garbear garbear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approval, that was my work account

@a1rwulf
Copy link
Member Author

a1rwulf commented Oct 5, 2020

Alright I've moved the ButtonStat file into a new subdirectory inside input.
Gave it a short runtime test, seems to work as I expect, if I play a movie:

Shows OSD:

curl --data-binary '{"jsonrpc": "2.0", "method": "Input.ButtonEvent", "id": 1, "params": { "button": "enter", "keymap": "KB"}}' -H 'content-type: application/json;' https://localhost:8080/jsonrpc

Pauses playback:

curl --data-binary '{"jsonrpc": "2.0", "method": "Input.ButtonEvent", "id": 1, "params": { "button": "enter", "keymap": "KB", "holdtime": 500}}' -H 'content-type: application/json;' https://localhost:8080/jsonrpc

If jenkins is happy, I think it's good to go.

@a1rwulf a1rwulf changed the title WIP: Fix JSON ButtonEvent Fix JSON ButtonEvent Oct 5, 2020
@a1rwulf a1rwulf merged commit 449d534 into xbmc:master Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garbear garbear garbear approved these changes

@eigendude eigendude eigendude approved these changes

Assignees
No one assigned
Labels
Component: Input Type: Fix non-breaking change which fixes an issue v19 Matrix
Projects
None yet
Milestone
Matrix 19.0-alpha 3
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@a1rwulf @garbear @eigendude