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

Controller dialog: Update controllers after controller addon installation #15763

Merged
merged 3 commits into from Apr 15, 2019

Conversation

@garbear
Copy link
Member

garbear commented Mar 17, 2019

Description

This PR is a continuation of the UX fix in #15754.

The first commit adds the missing add-on events that caused the list to not refresh when an add-on is installed.

However, this led to the following problem:

Screen Shot 2019-03-16 at 5 18 30 PM

The SNES controller was selected, but installing a 3DO controller updated the controller list indices and caused the wrong one to be highlighted. To fix this, I made the controller dialog focus the newly installed controller.

How Has This Been Tested?

Installed a controller and observed it being focused.

Clicked "Get all", and controllers were updated as they were installed. Unfortunately, the image is blocked by the modal dialog. I would like to make it a toast, but then how would the user cancel? Closing the dialog?

Screen Shot 2019-03-17 at 9 40 06 AM

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)
@garbear

This comment has been minimized.

Copy link
Member Author

garbear commented Mar 17, 2019

@enen92 can you test?

@enen92

This comment has been minimized.

Copy link
Member

enen92 commented Mar 18, 2019

Sorry for the delay @garbear. Tested it and working as expected 👍
One small glitch though: suppose you install all the available controllers. The "Get More" button is still enabled which leads the user to think there are more controllers to install. However if you click the button you'll only find out that "You already have installed all the controllers". Supposedly, the UpdateButtons() call in GUIControllerWindow.cpp is there to prevent this situation I guess. Which leads me to think that the subscription of addon install events I previously added in #15754 would still make some sense.

@garbear

This comment has been minimized.

Copy link
Member Author

garbear commented Apr 14, 2019

Which leads me to think that the subscription of addon install events I previously added in #15754 would still make some sense.

Agreed. I haven't had time to include that fix in this PR, so I think it's best if we merge as-is and follow up with a fix to the problem that #15754 tries to solved. Can you rebase on master after this merge so we can re-evaluate your patch?

jenkins build this please

@garbear garbear merged commit c67eb3a into xbmc:master Apr 15, 2019
1 check passed
1 check passed
default You're awesome. Have a cookie
Details
@garbear garbear deleted the garbear:controller-fixes branch Apr 15, 2019
@ksooo

This comment has been minimized.

Copy link
Member

ksooo commented Apr 16, 2019

@garbear we are just in the middle of branching Leia. In case you want this fix in v18 you need to backport it to Leia branch.

@Rechi Rechi added the v19 Matrix label Apr 16, 2019
@Rechi Rechi added this to the M** 19.0-alpha 1 milestone Apr 16, 2019
@garbear

This comment has been minimized.

Copy link
Member Author

garbear commented Apr 16, 2019

@ksooo I saw the branching discussion in slack too late. Do you recommend backporting?

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.