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

Qt: Fix some options not changing enabled status on game start. #13259

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TryTwo
Copy link
Contributor

@TryTwo TryTwo commented Jan 2, 2025

When a game starts and the game config is loaded, the signal block on ConfigChanged will prevent options from updating properly, such as triggering an enable/disable for some options that are linked. The signal block was used to prevent triggering a Save.

Programmatic updates that change the value to something different from the config need to be saved. "VBI skip" triggering "skip duplicate XFBs" is one example.
ConfigChanged updates change the value of an option to be consistent with the new config, and should not trigger a Save, because that triggers another ConfigChanged.

I dropped the SignalBlocker in favor of an m_updating bool, to only block the re-saving.

A different solution would be to just shove every enable/disable/etc check under the ConfigChanged signal. I don't really like how bloated it's getting though.

@TryTwo TryTwo force-pushed the PR_Config_signals branch 2 times, most recently from 10cb848 to b07170d Compare January 4, 2025 22:39
@TryTwo
Copy link
Contributor Author

TryTwo commented Jan 4, 2025

@JosJuice Could you check this bugfix? I think it's safe, because the m_updating is set and removed in the same function.

@JosJuice
Copy link
Member

JosJuice commented Jan 5, 2025

Let's say that, for instance, we're executing ConfigBool::OnConfigChanged, and it calls setChecked with the same value that the QCheckBox already was set to. Does this trigger the QCheckBox::toggled signal? If yes, you should add checks in the classes that inherit from ConfigControl so that they don't emit any signal in this scenario, to avoid sending "useless" signals to other objects that just waste time. (Or am I overestimating how many other objects there are listening to these signals?) If no, then the comment here appears to describe a scenario that cannot happen:

// Avoid OnConfigChanged -> option changed to current config's value -> unnecessary save ->
// ConfigChanged.

@JosJuice
Copy link
Member

JosJuice commented Jan 5, 2025

Also, it would be helpful to put the PR description in the commit. Currently, the commit message lacks a lot of the details that you've taken the time to write out for the PR description.

@TryTwo
Copy link
Contributor Author

TryTwo commented Jan 6, 2025

toggled and others like currentIndexChanged only signal if the value changes. The time it matters is when loading a new config, such as when starting a game up. The config will have all the correct values, but some options will have their values changed to match that. We don't want to re-save, but we do want enabled status to be updated, which also uses the toggled signal.

I could also check that the value being saved is actually a new one every time Save is called, and return early if it's the same.

There are not that many options that check for enabled status and the like. There are a few that need to change themselves based on other options too. It's just important to avoid unnecessary ConfigChanged signals because that hits a ton of stuff.

Copy link
Member

@JosJuice JosJuice left a comment

Choose a reason for hiding this comment

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

I think this is fine as it is then.

…h the enabled status of certain options when a new config is loaded (such as on starting a game). Blocks previously unwanted behavior with a new safety check.

QCheckBox::toggled and other similar signals are used to save changes and to update widget status (such as enabled).. OnConfigChanged needs to load new values and trigger widget updates, but the new value shouldn't trigger a save. A save is unnecessary (the config has the correct values and the UI is being updated to those values) and it'd trigger another ConfigChanged signal.   This commit blocks the save without blocking the signal entirely.
@TryTwo TryTwo force-pushed the PR_Config_signals branch from b07170d to 5395f21 Compare January 7, 2025 10:02
@TryTwo
Copy link
Contributor Author

TryTwo commented Jan 7, 2025

Updated commit message

@AdmiralCurtiss AdmiralCurtiss changed the title Qt. Fix some options not changing enabled status on game start. Qt: Fix some options not changing enabled status on game start. Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants