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

Resolution: Use v17 behaviour if whitelist is not present #14701

Merged
merged 2 commits into from Oct 28, 2018

Conversation

Projects
None yet
4 participants
@fritsch
Member

fritsch commented Oct 24, 2018

Description

V18 combines the old Adjust Refreshrate Feature with a whitelist
This changed behaviour as users don't expect that they need to alter two settings, e.g. enable Adjust Refreshrate and also fill the Whitelist which is in a different settings menu

Motivation and Context

Restore out of the box usability

How Has This Been Tested?

It was not tested at all - I will do this on the weekend

Screenshots (if appropriate):

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
@MartijnKaijser

This comment has been minimized.

Member

MartijnKaijser commented Oct 24, 2018

Installed v17 with refreshrate enabled, upgraded to this build and checked the whitelist. None of the resolutions was selected.
If that was supposed to happen then it didn't work. If it was not suppose to select any resolution in white list can we please do that cause that's what i was expecting to happen.

Here's my debuglog for my laptop:
https://paste.ubuntu.com/p/jdwyWP5c5v/

@fritsch

This comment has been minimized.

Member

fritsch commented Oct 25, 2018

@graham8

This comment has been minimized.

Contributor

graham8 commented Oct 25, 2018

I'm with Martijn. On install/upgrade whitelist should be populated with desktop and greater resolutions. That would make it easier for users to understand what a whitelist does.

And obviates the need for special behaviour when whitelist is empty.

@da-anda

This comment has been minimized.

Member

da-anda commented Oct 25, 2018

I disagree. Keeping it empty will ensure that this feature will always work as expected, even if you attach your KODI device to another screen. Once there is a whitelist, what should happen to it if you attach a different screen? So IMO empty = v17 behavior is good. And only when filled it will be limited to those resolutions. Yes, once manually filled we'd still have the same situation when you attach a different screen, but since the whitelist is considered an expert setting (IMO), the people messing around with it should also be able to deal with this scenario, while the average user that never touched the whitelist wouldn't if it's pre-populated

@graham8

This comment has been minimized.

Contributor

graham8 commented Oct 25, 2018

So you are saying with whitelist empty we get v17 behaviour, with one or more whitelist entries, only those resolutions (??and the desktop resolution) get used. And how does the Adjust refresh rate setting work with all that?

Just trying to understand the intention.

@fritsch

This comment has been minimized.

Member

fritsch commented Oct 25, 2018

First sentence: fully correct IF adjust refreshrate is not off, which answers your seconds question.

The reasoning (if there is one): This makes whitelist a feature "on top". Means a user that wants to switch to other resolutions or to some resolutions only, edits his whitelist.

@fritsch

This comment has been minimized.

Member

fritsch commented Oct 27, 2018

Fixed up - and tested.

@MartijnKaijser

This comment has been minimized.

Member

MartijnKaijser commented Oct 27, 2018

@MartijnKaijser

Works as advertised

@MartijnKaijser MartijnKaijser added this to the Leia 18.0-beta5 milestone Oct 27, 2018

@MartijnKaijser MartijnKaijser merged commit 77bafbd into xbmc:master Oct 28, 2018

1 check passed

default You're awesome. Have a cookie
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment