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 Settings::FadeButton / initial scrollbar display #12424

Merged
merged 1 commit into from
Jul 5, 2017
Merged

Fix Settings::FadeButton / initial scrollbar display #12424

merged 1 commit into from
Jul 5, 2017

Conversation

peak3d
Copy link
Contributor

@peak3d peak3d commented Jul 4, 2017

Fix Settings::FadeButton / initial scrollbar display

Description

  • Moving focus in settings from menu into settings::grouplist shoud fade the category button
  • Selecting new category in settings should update the scrollbar

Both issues broke with PR 12213, this PR fixes them

Types of change

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

@peak3d
Copy link
Contributor Author

peak3d commented Jul 4, 2017

@HitcherUK <- hope it solves the issues you tracked.

@da-anda
Copy link
Member

da-anda commented Jul 5, 2017

wouldn't it actually be better if skins could define how the "active but unfocused" state looks like? It's odd that the core is dictating how this should look like

@peak3d
Copy link
Contributor Author

peak3d commented Jul 5, 2017

agree 1000% @da-anda, that would mean a triple state, but maybe something like this is already implemented. What I was searching for is that a control has beside "focus" also a "selected" state. and skinner can design a layout for this.

Edit: But not on this round, I'll keep it in mind for later. This PR at least removes the dirrty rect on every Process() because the focus(true) focus false sequence was removed.

@peak3d peak3d merged commit 2a5d882 into xbmc:master Jul 5, 2017
@Hitcher
Copy link
Contributor

Hitcher commented Jul 5, 2017

@da-anda we can by using variables for the focus texture but it's a pain to do it for each different grouplist across a whole skin. This [fading] has been the default behaviour since forever.

@Rechi Rechi added this to the L 18.0-alpha1 milestone Jul 5, 2017
@peak3d
Copy link
Contributor Author

peak3d commented Jul 5, 2017

@HitcherUK but wouldn't be a good idea to have a third state in the future?
The fade effect could be also make sense not only in Settings (where it is implemented right now) but as well in home menu. Simply define a layout anywhere global for the 3rd state and you are done..

@Hitcher
Copy link
Contributor

Hitcher commented Jul 5, 2017

Yes it would be a nice addition in the future for sure.
Note: A fade could easily be added to the Estuary home menu (it's actually a list control in this case) but I just assumed this was a design decision.

@Hitcher
Copy link
Contributor

Hitcher commented Jul 5, 2017

@HitcherUK <- hope it solves the issues you tracked.

All fixed, thanks.

@peak3d peak3d deleted the guidirty branch July 13, 2017 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants