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/WatchWidget: Don't update if not paused. #11623

Merged

Conversation

AdmiralCurtiss
Copy link
Contributor

This is a workaround for https://bugs.dolphin-emu.org/issues/13190.

There's definitely a better way to solve this but I'm really unsure what the proper way to deal with it is, so this should at least resolve the pseudo-infinite-loop in the meantime.

@JosJuice
Copy link
Member

JosJuice commented Mar 5, 2023

Is it fine to leave the table as it is when not paused, or should we clear it or something to make it clear to the user that the values are not up to date?

LGTM other than that.

@AdmiralCurtiss
Copy link
Contributor Author

Probably better like this?

@JosJuice
Copy link
Member

JosJuice commented Mar 7, 2023

I just hope this won't make people think their watches have been deleted... I don't know, though. I'll leave the decision to someone who uses the debugging UI more than me.

@Pokechu22
Copy link
Contributor

I don't think clearing the table is a good approach. It'd be better to gray out the values (perhaps using setDisabled) but leave the addresses and labels visible.

This also introduces a minor inconsistency: when the watch widget first appears, it has 0 rows, but after starting and then pausing a game, it gains 1 blank row (which is usable for creating new watches). Before, the 1 blank row was always visible (even when emulation hadn't started - though also you can use that blank row to create new watches when emulation hasn't started even though the menu options are disabled... it's fairly jank).

@AdmiralCurtiss
Copy link
Contributor Author

Yes, this entire watch widget is super janky... feel free to fix it, but that wasn't my goal, I just wanted to stop Dolphin freezing and/or lagging when it's open.

@Pokechu22
Copy link
Contributor

Pokechu22 commented Mar 7, 2023

Now before the game starts, it looks like this:

image

which is really jank looking. That can probably be solved by moving the setHorizontalHeaderLabels call into CreateWidgets, and using clearContents() instead of clear(). (The empty row could also be created in CreateWidgets by duplicating this code there.)

This does fix the infinite loop though.

@AdmiralCurtiss
Copy link
Contributor Author

There, yeah that's definitely better.

Copy link
Contributor

@Pokechu22 Pokechu22 left a comment

Choose a reason for hiding this comment

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

Looks good enough to me.

There are still a few oddities:

  • The "address" field doesn't get greyed out properly because we use a special brush for it to turn it red if it's not a valid address:
    image
    I'm not sure how to properly combine disabled and custom brushes, and we might want to adjust the logic for this anyways in the future.
  • The watch table doesn't get cleared when stopping emulation.

These don't need to be addressed now, and can be fixed in a future PR that makes other changes to the watch widget.

@AdmiralCurtiss
Copy link
Contributor Author

The watch table doesn't get cleared when stopping emulation.

Honestly it probably should actually remain saved across sessions (at least for the same game)! But yeah, that's probably a more significant change.

@AdmiralCurtiss AdmiralCurtiss merged commit ff265b6 into dolphin-emu:master Mar 7, 2023
@AdmiralCurtiss AdmiralCurtiss deleted the watch-window-workaround branch March 7, 2023 19:45
AdmiralCurtiss added a commit to AdmiralCurtiss/dolphin that referenced this pull request Apr 23, 2023
This is similar to dolphin-emu#11623 where the Core state change invoked by the CPUThreadGuard does indirectly cause another Update() call.
Medard22 pushed a commit to Medard22/Dolphin-MMJR2-VBI that referenced this pull request Mar 10, 2024
This is similar to dolphin-emu/dolphin#11623 where the Core state change invoked by the CPUThreadGuard does indirectly cause another Update() call.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants