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 disable mouse buttons setting not showing default indicator when using keybind #12015

Merged
merged 3 commits into from
Mar 16, 2021

Conversation

Joehuu
Copy link
Member

@Joehuu Joehuu commented Mar 15, 2021

The problem is that ConfigManager's Set changes the default value to the changed one.

https://github.com/ppy/osu-framework/blob/1a0a270c9c933fdd469398b2db4086bef56b7101/osu.Framework/Configuration/ConfigManager.cs#L108-L125

Before After
f5QmEntIlj 4XNiyeY1HD

@peppy
Copy link
Member

peppy commented Mar 15, 2021

This looks like a pretty large gotcha with the Set method...

@smoogipoo any issues with fixing this on the framework side? something like this https://github.com/ppy/osu-framework/compare/master...peppy:fix-config-manager-set-default?expand=1

Copy link
Member

@frenzibyte frenzibyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, was gonna suggest separating setting default value of setting to another method (SetWithDefault) but yours sounds much better.

osu.Game/OsuGame.cs Outdated Show resolved Hide resolved
@smoogipoo
Copy link
Contributor

smoogipoo commented Mar 15, 2021

I'm fine with a framework change, but it setting the default should be the... default. Most usages of Set are expecting to set defaults (e.g. sensitivity: min: 0.1, default/value: 1, max: 6).
So something like Set(..., bool isDefaultValue = true) would be fine imo.

I'm also not really against the change in this PR if we disagree on that.

@peppy
Copy link
Member

peppy commented Mar 15, 2021

I was planning on just having the first Set call ever set the default (since the general pattern is to always call Set in InitialiseDefaults in order to use a specific configuration enum). Is that too haphazard?

@smoogipoo
Copy link
Contributor

Hmmm... It feels weird in terms of the overrides accepting min/max values, no? I'm actually not sure if it should even be public in the first place, and that this PR is what we want in the end.

@peppy
Copy link
Member

peppy commented Mar 15, 2021

Can agree to making the Set methods protected, for sure.

@smoogipoo smoogipoo merged commit 891e7aa into ppy:master Mar 16, 2021
@Joehuu Joehuu deleted the fix-mouse-disable-default-indicator branch March 16, 2021 03:00
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