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.

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

Improve keyboard handling during button mapping #11645

Merged
merged 3 commits into from Feb 11, 2017

Conversation

@garbear
Copy link
Member

commented Feb 9, 2017

Right now, if Kodi receives a keyboard press while mapping a button, it will unconditionally abort the prompt. Empirically, I've observed users becoming confused when they press a direction but the focus doesn't move. Also, I've seen users press A and become confused as to why a "select" button caused a "cancel" action.

It feels like a sin including Key.h every time I want a couple actions, so I also included a commit separating CAction from CKey.

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
@garbear

This comment has been minimized.

Copy link
Member Author

commented Feb 9, 2017

@MartijnKaijser would you consider UX improvements a fix to be backported?

@Paxxi

This comment has been minimized.

Copy link
Member

commented Feb 9, 2017

Can't use line comments on the phone.
Looks good overall.
Namespace KEYBOARD should be in namespace KODI, many older don't do this but new namespace should so we get a proper hierarchy.

Cpp file shouldn't use namespace KEYBOARD but also be wrapped, same as header file.

@garbear

This comment has been minimized.

Copy link
Member Author

commented Feb 9, 2017

Cpp file shouldn't use namespace KEYBOARD but also be wrapped, same as header file.

No symbols are declared in the .cpp file, so this shouldn't matter, right?

@Paxxi

This comment has been minimized.

Copy link
Member

commented Feb 9, 2017

No I don't know of a compile time reason to require it really. Part style and afaik it can mess with static analysis tooling.

But we specifically require no namespace indentation to make this scenario simple. I leave it up to you to decide.

Sticking it into the Kodi namespace is a requirement though 😀

@garbear garbear force-pushed the garbear:fix-keyboard branch from 6cf4875 to 5bff7b5 Feb 9, 2017
@garbear

This comment has been minimized.

Copy link
Member Author

commented Feb 9, 2017

KODI namespace now has a few new friends to play with

@garbear garbear force-pushed the garbear:fix-keyboard branch from 5bff7b5 to 86edbd9 Feb 9, 2017
@garbear garbear referenced this pull request Feb 9, 2017
96 of 123 tasks complete
@garbear garbear referenced this pull request Feb 10, 2017
1 of 3 tasks complete
garbear added 2 commits Feb 9, 2017
Right now, if Kodi receives a keyboard press while mapping a button, it will
unconditionally abort the prompt.

Empirically, I've observed users becoming confused when they press a
direction but the focus doesn't move.

Also, I've seen users press A and become confused as to why a "select"
button caused a "cancel" action.
@garbear garbear force-pushed the garbear:fix-keyboard branch from 86edbd9 to 65f60ab Feb 10, 2017
@Paxxi

This comment has been minimized.

Copy link
Member

commented Feb 10, 2017

Thanks, I'm happy :)

@garbear

This comment has been minimized.

Copy link
Member Author

commented Feb 10, 2017

jenkins build this please

@garbear garbear merged commit cbfa964 into xbmc:master Feb 11, 2017
2 of 3 checks passed
2 of 3 checks passed
jenkins4kodi I've found some spare time so building this now
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@garbear garbear deleted the garbear:fix-keyboard branch Feb 11, 2017
@MartijnKaijser MartijnKaijser added this to the L 18.0-alpha1 milestone Feb 14, 2017
@LS80

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2017

@garbear This appears to have inadvertently removed the action ids from the Python API xbmcgui module (e.g. xbmcgui.ACTION_MOVE_DOWN).

I think the new input/ActionIDs.h needs to be included in interfaces/swig/AddonModuleXbmcgui.i.

@Rechi Rechi added the v18 Leia label Mar 19, 2017
@LS80 LS80 referenced this pull request Mar 22, 2017
1 of 9 tasks complete
@garbear garbear referenced this pull request Jun 8, 2017
3 of 6 tasks complete
@garbear garbear referenced this pull request Apr 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.