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/Controllers: Refresh GUI on settings change. #11075

Merged

Conversation

AdmiralCurtiss
Copy link
Contributor

@AdmiralCurtiss AdmiralCurtiss commented Sep 20, 2022

(without triggering signals)

Prep work for allowing opening the Controller settings mid-Netplay without everything breaking. See #11070 (comment).

To be clear, the intent of this PR is having the Controller GUI properly reflect the current state if it's changed programmatically by Netplay (or anything else). The eventual target (not in this PR) is that during Netplay, the dropdowns for the GC controller ports and Wiimotes will be greyed out (because we don't support them changing mid-session), but the Configure buttons next to them will still work so you can change your mappings, along with harmless-to-Netplay settings like Background Input.

Copy link
Contributor

@iwubcode iwubcode left a comment

Choose a reason for hiding this comment

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

Untested but code looks good. Nice work!

@iwubcode
Copy link
Contributor

Ok, did a bit of testing to complete this. Not noticing any regressions or other issues.

@AdmiralCurtiss
Copy link
Contributor Author

Alright then.

@AdmiralCurtiss AdmiralCurtiss merged commit 3fa9fdf into dolphin-emu:master Sep 23, 2022
11 checks passed
@AdmiralCurtiss AdmiralCurtiss deleted the controller-gui-refresh branch September 23, 2022 22:14
@shuffle2
Copy link
Contributor

shuffle2 commented Jan 8, 2023

Supposedly this causes a crash: https://bugs.dolphin-emu.org/issues/13141

@shuffle2
Copy link
Contributor

shuffle2 commented Jan 12, 2023

@AdmiralCurtiss do you know if this PR caused the threading issues seen in that issue? Or does this PR just happen to make it more obvious? If the former, maybe this PR should be reverted (and Config Add/Remove should be noted as not thread safe).

@shuffle2
Copy link
Contributor

note however, that the Config stuff seems to be broken even without threading issues: since Add/Remove can be called recursively, you either wind up with iterators into s_callbacks being invalidated, or recursive calls getting silently dropped (the current behavior has OnConfigChanged returning early). I'm not sure dropping the recursion is actually good? It means that if a Config change occurs while walking s_callbacks, the previously invoked callbacks won't see the new change...right?

@AdmiralCurtiss
Copy link
Contributor Author

I'll have to look at it in detail, but yeah the entire system is a bit sketchy...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants