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.

Sign up for GitHub

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

Jump to bottom

CheatsManager: Create Action Replay and Gecko code widgets only once #12966

Merged
merged 4 commits into from
Sep 7, 2024
Merged

CheatsManager: Create Action Replay and Gecko code widgets only once #12966

merged 4 commits into from
Sep 7, 2024

Conversation

Dentomologist
Copy link
Contributor

@Dentomologist Dentomologist commented Jul 31, 2024 •
edited
Loading

Create CheatsManager's ARCodeWidget and GeckoCodeWidget once on startup (in CheatsManager's constructor), and trigger updates to their contents when the current game changes.

Previously, ARCodeWidget and GeckoCodeWidget would be recreated every time a game was started or shutdown. If either of these widgets was the visible tab this would cause the tab to switch to CheatSearchFactoryWidget (since the other tabs temporarily didn't exist), and also could cause a crash if the user initiated a game shutdown and then opened a code edit window before the shutdown finished.

Tilka
Tilka approved these changes Aug 7, 2024
View reviewed changes
Copy link
Member

@Tilka Tilka left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Sounds/looks good. Untested.

@mitaclaw
Copy link
Contributor

mitaclaw commented Aug 21, 2024 •
edited
Loading

I can confirm the issue wherein the CheatsManager switches to the CheatSearchFactoryWidget whenever emulation is started or stopped has been fixed.

The issue wherein a code edit window open during shutdown causes a crash (abort due to free(): invalid pointer) has been fixed, but in its place I have found a new issue: attempting to save a cheat code after emulation has stopped will crash (abort due to double free or corruption (out)). Pressing the "Cancel" push button or closing the code edit window does not crash.

Saving a cheat code when no software is running doesn't make much sense in the first place—it would probably write to an INI for the default GameID, 00000000—so I think this action should be made inaccessible in the UI if possible. The modality of the code edit window and the current limitations on CheatsManager push buttons depending on emulation state almost get there.

@Dentomologist Dentomologist force-pushed the cheatsmanager_create_code_widgets_only_once branch 2 times, most recently from b687acf to ded8e3e Compare August 26, 2024 00:03
@Dentomologist
Copy link
Contributor Author

I've pushed an update. The most important change is that when the game changes, the CheatCodeEditor in Cheats Manager's ARCodeWidget and GeckoCodeWidget are rejected and so can't be used to save a code in an invalid context. The widgets in a game's Properties window stay open and continue working correctly.

A few other changes that I made either out of necessity or because they were right there:

  • Removed the calls to SetQWidgetWindowDecorations. As far as my testing has shown they aren't actually necessary for the CheatCodeEditor to respond to themes correctly, and in combination with the changes I had to make to enable rejecting the editor they were causing the Cheats Manager window to be far too small on creation.
  • The Edit and Remove buttons in ARCodeWidget are now disabled when no code is selected.
  • Some minor refactoring and constifying.
  • Removed a call to OnSelectionChange that had no effect in context.

@mitaclaw
Copy link
Contributor

Please restore the SetQWidgetWindowDecorations call before each QDialog exec call. Without it, the title bar will (I think?) be the system default, which may or may not match the current window style if Dolphin is configured to override it with light or dark style. I was unable to reproduce the too-small dialog window you described, unless I just forgot to reinstate a SetQWidgetWindowDecorations call in a key place?

With SetQWidgetWindowDecorations Without SetQWidgetWindowDecorations
Here are screenshots in which the system default style is light mode but Dolphin overrides it with dark mode. On the left is with the SetQWidgetWindowDecorations call, on the right is without.

@Dentomologist
ARWidget: Disable Edit and Remove buttons when no code is selected
ee35aa4 
Also some minor refactoring of nearby/related code:
* Make non-obvious variable types explicit instead of auto.
* Throw some consts around.
* Use setDisabled(empty) instead of setEnabled(!empty).
@Dentomologist
Extract ARCodeWidget/GeckoCodeWidget code loading to functions
232d241
@Dentomologist
CheatsManager: Create ARCodeWidget and GeckoCodeWidget only once.
f4db168 
Create ARCodeWidget and GeckoCodeWidget once on startup rather than
every time a game is launched or shutdown.

In addition to losing focus on the tab (since the previous widget and
tab no longer existed), the behavior prior to this commit could cause a
crash if the user initiated a game shutdown and then opened a code edit
window since the AR/GeckoCodeWidget would get deleted in the meantime.
@Dentomologist
ARCodeWidget: Remove unnecessary call to OnSelectionChanged
9e6a4e9 
Before the call to OnSelectionChange, m_code_edit and m_code_remove are
disabled and UpdateList calls m_code_list->clear(), thereby deselecting
any selected items.

When no items are selected, OnSelectionChange disables m_code_edit and
m_code_remove and then returns. Since that was already done, the call
doesn't change anything and can be removed.
@Dentomologist Dentomologist force-pushed the cheatsmanager_create_code_widgets_only_once branch from ded8e3e to 9e6a4e9 Compare August 26, 2024 06:49
@Dentomologist
Copy link
Contributor Author

I believe the too-small window was a result of my attempts to avoid having to call SetQWidgetWindowDecorations on m_cheat_code_editor every time it called exec despite the editor being created a single time per AR/GeckoCodeWidget per Dolphin session, but if that's what it takes to make everything display correctly then so be it.

My testing of the removal of SetQWidgetWindowDecorations was hampered by the fact that I didn't notice it only affects Windows Light -> Dolphin Dark (while my Windows theme is dark), and also that my Windows version is too old for the function to actually do anything at all (as DWMWINDOWATTRIBUTE::DWMWA_USE_IMMERSIVE_DARK_MODE is unsupported).

Since I can't test it myself, let me know if the theme is being properly applied now.

mitaclaw
mitaclaw approved these changes Aug 26, 2024
View reviewed changes
Copy link
Contributor

@mitaclaw mitaclaw left a comment

Choose a reason for hiding this comment

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

The title bar issue is fixed, and the two resolved reviews I left were my only concerns.

@JMC47
Copy link
Contributor

JMC47 commented Sep 7, 2024

I was just using cheat search a few days ago and these are some welcome improvements.

@JMC47 JMC47 merged commit 2c15d7a into dolphin-emu:master Sep 7, 2024
11 checks passed
@Dentomologist Dentomologist deleted the cheatsmanager_create_code_widgets_only_once branch September 7, 2024 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mitaclaw mitaclaw mitaclaw approved these changes

@Tilka Tilka Tilka approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Dentomologist @mitaclaw @JMC47 @Tilka