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 focus being lost when that was not intended #13359

Merged
merged 1 commit into from Jan 15, 2018

Conversation

@Voyager1
Copy link
Member

commented Jan 14, 2018

Description

see #13356 for description. The initial fix proposed there was symptomatic and somewhat invalid.
This PR addresses the root cause for focus going 'astray', i.e. an obvious missed break statement.

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of change

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

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
@Voyager1 Voyager1 requested a review from FernetMenta Jan 14, 2018
@ghost

This comment has been minimized.

Copy link

commented Jan 14, 2018

was borked in aff8128

@Voyager1

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2018

was borked in aff8128

not sure what you mean... Seems like the code before that had the issue too. That said it's an issue with only very rare negative effect. The screensaver reset and DPMS stuff gets called anyway inside OnAppCommand, it's only the mouse move action (ACTION_MOUSE_MOVE) that has a potential negative effect. I'm surprised we haven't seen more issues because of this.

@ghost

This comment has been minimized.

Copy link

commented Jan 14, 2018

Previous code was missing break but did a return, so it worked. Changed code removed the return but missed the fact that it made up for the missing break. Little surprised that it was not caught in review. Errors like these are hard to track down. I'll bet there are more opps like this lurking.

@Voyager1

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2018

Previous code was missing break but did a return, so it worked.

oh I missed that - you're right :-)

@Voyager1

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2018

@FernetMenta - any objection?

Copy link
Member

left a comment

thanks for fixing it.
btw: APPCOMMAND is win32 only and should be dropped. no need to have platform specific code messing up CApplication

@Voyager1

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2018

APPCOMMAND is win32 only

Win64 as well then? Anyway, I'll merge this for now, and deal with the removal some other time.

@Voyager1 Voyager1 merged commit b6e0913 into xbmc:master Jan 15, 2018
1 check failed
1 check failed
default Sorry, building this PR failed. Please check the logs for the errors.
Details
@Rechi Rechi added this to the L 18.0-alpha1 milestone Jan 15, 2018
@Voyager1 Voyager1 deleted the Voyager1:fix_guifocus branch Jan 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.