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 clashing XBMCK_EJECT and XBMCK_FAVORITES #16899

Merged
merged 1 commit into from
Nov 24, 2019
Merged

fix clashing XBMCK_EJECT and XBMCK_FAVORITES #16899

merged 1 commit into from
Nov 24, 2019

Conversation

bam80
Copy link
Contributor

@bam80 bam80 commented Nov 12, 2019

The clash:
XBMCK_FAVORITES = 0x14d,
XBMCK_EJECT = 333,
0x14d == 333

Description

Motivation and Context

XBMCK_EJECT wrongly maps to XBMCVK_FAVORITES

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

Copy link
Member

@Rechi Rechi left a comment

Choose a reason for hiding this comment

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

  • Please follow the code guidelines.
  • The PR title indicates that only the clashing XBMCK_EJECT and XBMCK_FAVORITES was fixed, but there is also a new key (XBMCK_SUBTITLE) added.
  • The commit message of your second commit has to be adjusted, because Update XBMC_keysym.h doesn't describe what change was done.

xbmc/input/XBMC_keysym.h Outdated Show resolved Hide resolved
xbmc/input/XBMC_keysym.h Outdated Show resolved Hide resolved
@bam80 bam80 force-pushed the master branch 2 times, most recently from 5cebe33 to 777b4f5 Compare November 12, 2019 21:48
@bam80
Copy link
Contributor Author

bam80 commented Nov 12, 2019

  • The PR title indicates that only the clashing XBMCK_EJECT and XBMCK_FAVORITES was fixed, but there is also a new key (XBMCK_SUBTITLE) added.

  • The commit message of your second commit has to be adjusted, because Update XBMC_keysym.h doesn't describe what change was done.

Sorry, it occurred here accidentally. Fixed

@bam80 bam80 requested a review from Rechi November 12, 2019 23:37
The clash:
  XBMCK_FAVORITES   = 0x14d,
  XBMCK_EJECT           = 333,
0x14d == 333
Copy link
Member

@Rechi Rechi left a comment

Choose a reason for hiding this comment

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

code style is now correct

@bam80
Copy link
Contributor Author

bam80 commented Nov 16, 2019

@Rechi anything left?

@Rechi
Copy link
Member

Rechi commented Nov 16, 2019

All my objections are fixed.

@bam80
Copy link
Contributor Author

bam80 commented Nov 16, 2019

Can we merge it then?

@bam80 bam80 requested a review from Rechi November 17, 2019 14:26
@Rechi Rechi removed their request for review November 17, 2019 14:27
@bam80
Copy link
Contributor Author

bam80 commented Nov 17, 2019

@Rechi sorry I didn't understand why the patch was not merged? Please explain what else needed.

@bam80
Copy link
Contributor Author

bam80 commented Nov 19, 2019

@ronie @lrusak Could you guys help me to merge this?
I'm waiting for a week and still no go, I think it's too much for such a simple patch.
Please tell me if there is a reason.
@garbear @ksooo

Copy link
Member

@yol yol left a comment

Choose a reason for hiding this comment

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

lgtm, verified that XBMCK_EJECT is not a original SDL scancode (important for OS X which still uses SDL and 1:1 keycode mapping)

@yol
Copy link
Member

yol commented Nov 24, 2019

I'm waiting for a week and still no go, I think it's too much for such a simple patch. Please tell me if there is a reason.

The reason is that we are all volunteers hacking on Kodi in their spare time. Please understand that such comments only serve to decrease the motivation for any developer to look at it. Besides, nothing in Kodi is simple ;-) I'm actually not 100% sure this will not break something, but I checked the cases I could think of.

@yol yol added this to the Matrix 19.0-alpha 1 milestone Nov 24, 2019
@yol yol added Component: Input Type: Fix non-breaking change which fixes an issue v19 Matrix labels Nov 24, 2019
@yol
Copy link
Member

yol commented Nov 24, 2019

UWP build failure unrelated

@yol yol merged commit c53cf33 into xbmc:master Nov 24, 2019
@bam80
Copy link
Contributor Author

bam80 commented Nov 24, 2019

@yol I was just asking for any reply that devs are aware of the patch and it's not forgotten.
We are all the volunteers and it has nothing to do with it.
I'm new to making PR to this project so maybe I just don't have a notion yet how things are doing.

Thanks for merging.

Maven85 pushed a commit to Maven85/kodi that referenced this pull request Jan 21, 2020
fix clashing XBMCK_EJECT and XBMCK_FAVORITES
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 v19 Matrix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants