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

[fix] move virtual window handling to translators to solve issue with remote mappings #13444

Merged
merged 1 commit into from
Jan 27, 2018

Conversation

xhaggi
Copy link
Member

@xhaggi xhaggi commented Jan 26, 2018

@MilhouseVH hope this will fix the reported issue #13433 (comment)

Runtime tested with keyboard mappings for virtual windows and works fine.
I can't test remote mappings because I don't have a remote control anymore (using FLIRC).

@xhaggi xhaggi requested a review from garbear January 26, 2018 21:51
@xhaggi xhaggi added Type: Fix non-breaking change which fixes an issue Component: Input v18 Leia labels Jan 26, 2018
@garbear
Copy link
Member

garbear commented Jan 26, 2018

Gotta run but I can review this tmrw. I noticed, when looking at these lines:

window = CWindowTranslator::GetVirtualWindow(window);

this would be easier to reason about if all virtual window IDs had "virtual" in the variable name, e.g.

int virtualWindow = CWindowTranslator::GetVirtualWindow(window);

similar to how we include units in the variable name.

Then we could verify that:

  • All keymap lookups are by virtual window ID
  • Actual -> virtual translation isn't done twice (although here this isn't an issue as GetVirtualWindow() is idempotent)

@xhaggi
Copy link
Member Author

xhaggi commented Jan 26, 2018

Sure can adjust the variables.

@xhaggi
Copy link
Member Author

xhaggi commented Jan 26, 2018

@garbear I thought about it again and it doesn't make much sense to change the variable, because it's not necessarily a virtual window ID.

@MilhouseVH
Copy link
Contributor

@xhaggi many thanks, I've posted a test build on the forum - I'll let you know the feedback.

@MilhouseVH
Copy link
Contributor

@HiassofT gave the new test build with this PR a quick test and says it's looking good but will test more tomorrow (Saturday). Hopefully forum users are also able to confirm similar results.

@xhaggi
Copy link
Member Author

xhaggi commented Jan 27, 2018

@MilhouseVH thanks for the fast response and test build.

@xhaggi
Copy link
Member Author

xhaggi commented Jan 27, 2018

Forum users reported it's fixed now.

@HiassofT
Copy link
Contributor

With this commit OK button is correctly mapped to OSD action again (previously it was Select) during livetv:

10:17:24.003 T:1941955584   DEBUG: LIRC: Update - NEW at 49526:160 0 KEY_OK devinput (KEY_OK)
10:17:24.003 T:1941955584   DEBUG: HandleKey: 11 (0x0b, obc244) pressed, action is OSD
10:17:24.004 T:1941955584   DEBUG: ------ Window Init (VideoOSD.xml) ------

Other remote buttons work fine during livetv as well.

Thanks a lot for the quick fix, it looks good to me!

@xhaggi
Copy link
Member Author

xhaggi commented Jan 27, 2018

@garbear any objections?

@xhaggi xhaggi merged commit ef94c6b into xbmc:master Jan 27, 2018
@Rechi Rechi added this to the L 18.0-alpha1 milestone Jan 27, 2018
@xhaggi xhaggi deleted the fix-virtual-window branch February 4, 2018 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Input Type: Fix non-breaking change which fixes an issue v18 Leia
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants