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

[input] fix stack overflow in HasLongpressMapping #7846

Merged
merged 1 commit into from Aug 27, 2015

Conversation

xhaggi
Copy link
Member

@xhaggi xhaggi commented Aug 18, 2015

@FernetMenta @koying let's move the discussion to this PR.

@xhaggi xhaggi force-pushed the fix-longpress-fallback-handling branch from 441582e to e53a099 Compare August 18, 2015 12:40
@xhaggi xhaggi added the Type: Fix non-breaking change which fixes an issue label Aug 18, 2015
@xhaggi
Copy link
Member Author

xhaggi commented Aug 18, 2015

@koying i've added a separate commit for what you've suggested, return -1 in case GetFallbackWindow is called with WINDOW_ADDON_START

@xhaggi xhaggi force-pushed the fix-longpress-fallback-handling branch from 60fdd94 to a091ba1 Compare August 18, 2015 12:56
@koying
Copy link
Contributor

koying commented Aug 18, 2015

If you go my route, you also have to change

int id = WINDOW_ADDON_START;
to "+1" and you might need to rework
int fallbackWindow = GetFallbackWindow(window);

But that probably deserves a separate PR. I suggest to do a plain fix here and think further about making GetFallbackWindow more coherent.

@xhaggi
Copy link
Member Author

xhaggi commented Aug 18, 2015

I've squashed both commits together because the fix only works if both are applied.

// because id is dynamic
if (windowID >= WINDOW_ADDON_START && windowID <= WINDOW_ADDON_END)
// for addon windows use WINDOW_ADDON_START because id is dynamic
if (windowID > WINDOW_ADDON_START && windowID <= WINDOW_ADDON_END)

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@FernetMenta
Copy link
Contributor

@xhaggi I only have a MacBook with me and can't test until coming Monday. Thanks for looking into this.

@xhaggi
Copy link
Member Author

xhaggi commented Aug 18, 2015

@FernetMenta does it crash if you open an addon window and press a key?

@FernetMenta
Copy link
Contributor

@xhaggi yes, it crashed as soon as I pressed a key when an addon window was opened.

@xhaggi
Copy link
Member Author

xhaggi commented Aug 19, 2015

Thought i can test it but don't find an addon window 😄 and do not have vdr ready.

@FernetMenta
Copy link
Contributor

ADSP uses addon windows too. @AchimTuran are there any ADSP addons ready for testing?

@xhaggi
Copy link
Member Author

xhaggi commented Aug 20, 2015

@FernetMenta do you know a reason why we do this special handling in GetFallbackWindow for addon window ids but not for our python addon window ids?

@FernetMenta
Copy link
Contributor

@xhaggi I think python windows have not had the requirement so far. I implemented it for addon windows because I had the requirement to define key-action mappings for those.

@xhaggi
Copy link
Member Author

xhaggi commented Aug 20, 2015

Thanks for the explanation :)

@FernetMenta
Copy link
Contributor

@xhaggi I am sure there is much room for improvement. At the time I did this I resurrected addon windows that were decayed and not usable.
Ideally an addon should be able to bring its own key-action mapping like it can bring its own skin files. What do you think?

@AchimTuran
Copy link
Member

@FernetMenta

ADSP uses addon windows too. @AchimTuran are there any ADSP addons ready for testing?
You can use adsp.basic, adsp.freesurround or adsp.biquad.filters.

Hopefully they will compile. But in two weeks I will have some time to fix them.

@FernetMenta
Copy link
Contributor

@xhaggi seems to fix the issue

@xhaggi
Copy link
Member Author

xhaggi commented Aug 26, 2015

jenkins build and merge

@koying
Copy link
Contributor

koying commented Aug 27, 2015

Addons building is failing. unrelated.

koying added a commit that referenced this pull request Aug 27, 2015
[input] fix stack overflow in HasLongpressMapping
@koying koying merged commit 443f692 into xbmc:master Aug 27, 2015
@hudokkow hudokkow added this to the Jarvis 16.0-alpha2 milestone Aug 27, 2015
@stefansaraev
Copy link
Contributor

@xhaggi well.. this completely breaks longpress for me.

@xhaggi
Copy link
Member Author

xhaggi commented Sep 1, 2015

could you please explain what's broken?

@stefansaraev
Copy link
Contributor

impossible to use long press. short press action immediately executed, log spammed http://sprunge.us/YOZg.

after reverting: http://sprunge.us/CdKc

xhaggi added a commit to xhaggi/xbmc that referenced this pull request Sep 1, 2015
xhaggi added a commit that referenced this pull request Sep 2, 2015
[input] fix long press fallback handling after #7846
sportica pushed a commit to sportica/xbmc that referenced this pull request Sep 21, 2015
sportica pushed a commit to sportica/xbmc that referenced this pull request Sep 21, 2015
stevegal pushed a commit to stevegal/xbmc that referenced this pull request Jan 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Fix non-breaking change which fixes an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants