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

Android: seekbar.setMin requires API level 26 #9112

Merged
merged 1 commit into from Sep 27, 2020

Conversation

Ebola16
Copy link
Member

@Ebola16 Ebola16 commented Sep 26, 2020

Apparently there's no good alternative before setMin was implemented for dealing with minimums less than 0.

@JosJuice
Copy link
Member

JosJuice commented Sep 26, 2020

Oh, indeed. Weird that it didn't exist before then, but good catch.

Could you add some clamping to SliderSetting.setSelectedValue (i.e. changing selection to Math.max(selection, getMin())) so that we don't write invalid values to the SYSCONF, preferably with a comment mentioning the API 26 problem? I can merge it after that.

@JosJuice
Copy link
Member

JosJuice commented Sep 26, 2020

On second thought, it's probably better to clamp when calling setSelectedValue here, so that the workaround for the issue is in the same part of the code as the issue:

IntSliderSetting sliderSetting = (IntSliderSetting) mClickedItem;
if (sliderSetting.getSelectedValue(getSettings()) != mSeekbarProgress)
mView.onSettingChanged();
sliderSetting.setSelectedValue(getSettings(), mSeekbarProgress);

@Ebola16
Copy link
Member Author

Ebola16 commented Sep 26, 2020

Like this? Since we're now clamping, I'm assuming it's cleaner to remove setMin completely.

@JosJuice
Copy link
Member

JosJuice commented Sep 26, 2020

Please keep calling setMin on API 26 and above. That way we can avoid subpar UX for users who are on API 26 or higher. Being able to set the slider to 0 but then having it revert to 1 isn't very clean from the user's perspective.

Alternatively... Maybe the solution which would be the cleanest for all users (but would require slightly more code) would be to not call setMin at all, and instead call getMax with item.getMax() - item.getMin(), and adjust for the discrepancy in onSliderClick and onProgressChanged so that the text shown on the screen and the value stored to the settings go from item.getMin() to item.getMax().

@Ebola16
Copy link
Member Author

Ebola16 commented Sep 27, 2020

The problem with adjusting for item.getMin in onProgressChanged is that multiple presses of the slider dialog without changing the value will apply the adjustment multiple times. Is the current state of this PR acceptable instead?

@JosJuice
Copy link
Member

I'll merge this as-is so that we'll have it fixed before tagging the next beta. I might make another PR later to refine the logic.

@JosJuice JosJuice merged commit 9607500 into dolphin-emu:master Sep 27, 2020
10 checks passed
@Ebola16 Ebola16 deleted the setmin branch September 27, 2020 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants