-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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.
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
Fix JSON ButtonEvent #18302
Fix JSON ButtonEvent #18302
Conversation
@garbear What's your thoughts on this? |
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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any problems with this approach. hold time calculation certainly belongs in |
I wasn't sure about this. |
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. |
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. |
Thx for your comments @garbear I'll check out about a new approach. |
…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.
dc2cbb9
to
ecc5870
Compare
@garbear that's the new version. |
xbmc/input/ButtonStat.cpp
Outdated
if (key.GetHeld() > HOLD_TRESHOLD) | ||
buttonCode |= CKey::MODIFIER_LONG; | ||
|
||
CKey translatedKey(buttonCode, key.GetHeld()); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this 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/?
There was a problem hiding this 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
ecc5870
to
2ea0aaa
Compare
Alright I've moved the ButtonStat file into a new subdirectory inside input. Shows OSD:
Pauses playback:
If jenkins is happy, I think it's good to go. |
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:
Types of change
Checklist: