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

CtInfo: Update Games List after Batch Update #350

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

sonic2kk
Copy link
Contributor

Overview

This PR fixes the CtInfo dialog's game list not updating after a batch update. If you perform a batch update, the game list still shows the old values. The used/unused and game count indicators will update correctly on the main menu, and re-opening or refreshing the CtInfo dialog will update the values. But they are not immediately updated after a batch update.

When we do a batch update to move the games in use by one compatibility tool to another, aside from actually doing this, there are some other steps:

  • In the Batch Update dialog class, we emit a signal to say that the batch update is complete
  • In the CtInfo dialog, we connect to this signal and call update_game_list_steam
  • In update_game_list_steam, at the end, we call emit a signal to notify that the batch update is complete. This is received by the main window and updates athe compatibility tool usage information (i.e. immediately update to reflect if a tool is unused after a batch update)

Problem

Currently though, the call to update_game_list_steam is incorrect, for two reasons:

  1. The main reason, we're still using a cached game list. So calling the method doesn't really do anything.
  2. If we ever enabled batch update for other launchers, it wouldn't update their game lists, since it would always try to call update_game_list_steam.
    a. By extension, we were only calling batch_update_complete.emit inside of update_games_list_steam, when it should've been called in update_game_list so that the signal is correctly emitted if we ever implement batch update for other launchers.

Solution

The fix for this is to first, call btn_refresh_games_clicked, which is the method connected on click of the CtInfo dialog and calls update_game_list with cached=False. Then to fix the last issue, batch_update_complete.emit was moved to the bottom of update_game_list. I wasn't sure if it should go in update_game_list or update_game_list_ui, but I figured keeping it more "top-level" in update_game_list is fine. It does technically have the job of performing a Qt-related action, and updating the UI on the main menu, so if you'd prefer it elsewhere that's fine. 🙂

This does mean there is a bit of delay after pressing the "Batch Update" button, as it will "hang" a bit before the dialog actually closes while the CtInfo dialog updates. Once this is updated, the Batch Update dialog will close and the user will be returned to the updated

Concerns

Two minor concerns here:

We're calling batch_update_complete.emit(True) unconditionally. We were doing this before already, but only with Steam, but now we're doing it for every launcher when we open a CtInfo dialog. I have not noticed any performance impact to this. We could possibly add batch_update_complete=False as a default parameter to update_game_list, and then we could change the emit call to self.batch_update_complete.emit(batch_update_complete). Then for the Batch Update dialog's signal, we can to change to connecting like this: ctbu_dialog.batch_update_complete.connect(lambda: self.update_game_list(cached=False, batch_update_complete=True).

My other concern is, after a Batch Update, the CtInfo dialog is always going to be empty since we're moving every game. There may not be much reason to keep the dialog open. Maybe we should close the CtInfo dialog after a batch update? We'd have to make sure we still call self.batch_update_complete.emit(True) in CtInfo so that the main menu information updates, but that should be fine to do. It would invalidate this PR but it may be better UX - or unexpected, depending on your point of view 😅


Thanks! :-)

@sonic2kk sonic2kk changed the title Update ctinfo after batch update CtInfo: Update Games List after Batch Update Jan 29, 2024
@DavidoTek
Copy link
Owner

Thanks!

  1. The main reason, we're still using a cached game list.
  2. If we ever enabled batch update for other launchers, it wouldn't update their game lists, since it would always try to call update_game_list_steam. [...]

Good catch!

Maybe we should close the CtInfo dialog after a batch update?
better UX - or unexpected

Interesting idea. It would make sense to do that as you're done with that compatibility tool after a batch update.
I guess users should also notice the (unused) flag behind the tool in the main window then. If they are unsure, they can check and will learn that it is the intended behavior.
Feel free to do it this way if you want to.

ctbu_dialog.batch_update_complete.connect(lambda: self.update_game_list(cached=False, batch_update_complete=True)

The boolean parameter of batch_update_complete is currently not used for anything, so we could do that.
I case we do it that way, we can omit cached=False in pupgui2.py#MainWindow#update_ui as we already refreshed the cache then:

steam_app_list = get_steam_app_list(install_loc.get('vdf_dir'), cached=False) # update app list cache

That only makes sense if we leave this PR as is. If you decide to not update the ctinfo game list, but instead close the ctinfo dialog after a batch update, we still need to have cached=False after a batch update.

ctbu_dialog.batch_update_complete.connect(self.btn_refresh_games_clicked)
self.batch_update_complete.emit(True)

(only if we close the UI after a batch update)
I feel like the signal won't be single purpose anymore, but we could do something like this to improve performance:

  • When a batch update is completed:
    • Close the CtInfo dialog
    • self.batch_update_complete.emit(True)
    • This will trigger an update_ui in the MainWindow and call get_steam_app_list with cached=False to reload the steam app list
  • When clicking refresh games:
    • self.batch_update_complete.emit(False)
    • This will trigger an update_ui in the MainWindow and call get_steam_app_list with cached=True, as we already reloaded the steam app list cache in btn_refresh_games_clicked

That also means we need to add cached: bool to pupgui2.py#MainWindow#update_ui.
On the CtInfo side, we can simply add another utility function like this:

# When a batch update is completed
ctbu_dialog.batch_update_complete.connect(self.on_batch_update_complete)
def on_batch_update_complete(self):
    self.batch_update_complete.emit(True)  # call update_ui and reload steam app list
    self.ui.close()

# When clicking refresh games
self.update_game_list(cached=False)
def update_game_list(self, cached=True):
    ...  # code which reloads the cached app list
    self.batch_update_complete.emit(False)  # call update_ui but use cached app list there


# pupgui2.py#MainWindow
def update_ui(self, cached=False):
    ...
    steam_app_list = get_steam_app_list(..., cached=cached)

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

Successfully merging this pull request may close these issues.

2 participants