HeroicGamesLauncher icon indicating copy to clipboard operation
HeroicGamesLauncher copied to clipboard

[Refactor] Game Status handling

Open Nocccer opened this issue 1 year ago • 5 comments

  • Moving the handling of game status to the backend and use electron-store to store progress.
  • Adapt and renamed hasProgress to hasGameStatus to also listen to status and not only to progress
  • Added a function to init gameStatus on start of heroic. This is helpful to restore the previous progress on heroic crash

Use the following Checklist if you have changed something on the Backend or Frontend:

  • [x] Tested the feature and it's working on a current and clean install.
  • [x] Tested the main App features and they are still working on a current and clean install. (Login, Install, Play, Uninstall, Move games, etc.)
  • [ ] Created / Updated Tests (If necessary)
  • [ ] Created / Updated documentation (If necessary)

Nocccer avatar Sep 30 '22 23:09 Nocccer

Ok, so far the issues I found:

  • It is not possible to stop the download from the game page. Probably the button is not calling the method to install again.
  • When continuing a download it doesn't check the previous progress. Should be easy to fix just check for the game status inside the store and if it is the same install folder just show the remaining size to download and the current progress.

Other than that it is working fine.

flavioislima avatar Oct 01 '22 16:10 flavioislima

Ok, now it is possible to stop the download from gamepage. Another bug now is that the after install/uninstall the status on the gamepage and gamecard is not changing without a manual library refresh. I think the refreshLibrary is not being called after the installation.

flavioislima avatar Oct 03 '22 14:10 flavioislima

Ok, now it is possible to stop the download from gamepage. Another bug now is that the after install/uninstall the status on the gamepage and gamecard is not changing without a manual library refresh. I think the refreshLibrary is not being called after the installation.

This is the bug i thought you fixed on main? Because the same thing happens for me on beta aswell. Don't know if this is a problem of this changes.

Nocccer avatar Oct 03 '22 14:10 Nocccer

Ok, now it is possible to stop the download from gamepage. Another bug now is that the after install/uninstall the status on the gamepage and gamecard is not changing without a manual library refresh. I think the refreshLibrary is not being called after the installation.

This is the bug i thought you fixed on main? Because the same thing happens for me on beta aswell. Don't know if this is a problem of this changes.

No, on this PR on the latest commit stopping now is possible. On my first test it wasn't.

flavioislima avatar Oct 03 '22 14:10 flavioislima

Ok, now it is possible to stop the download from gamepage. Another bug now is that the after install/uninstall the status on the gamepage and gamecard is not changing without a manual library refresh. I think the refreshLibrary is not being called after the installation.

This is the bug i thought you fixed on main? Because the same thing happens for me on beta aswell. Don't know if this is a problem of this changes.

No, on this PR on the latest commit stopping now is possible. On my first test it wasn't.

Sorry was talking about that games are not shown as installed or uninstalled. Everything else was really bugs i introduced.

Nocccer avatar Oct 03 '22 14:10 Nocccer

Some changes I added:

  • store a {} for the game statuses
  • moved all status changes to the backend and the backend pushes updates to the frontend
  • I was able to remove the previousProgress info, and the continue download feature is working fine
  • I replaced the sendKill when cancelling an installation with a new cancelInstallation api method, with this I was able to also move the removeFolder to the backend instead of calling window.api.removeFolder (which I think was a security risk to expose a method to remove any arbitrary folder)

This is ready for review

arielj avatar Oct 25 '22 04:10 arielj

I've been reading through the code and testing a bit. My main question is why do we need to write this to file? It seems to me that it is ephemeral state. What happens if the app crashes? Do we use this game status data to restart the download or for some other purpose?

Right now these changes (in my one test I did) incur an additional ~100 reads and writes to disk while downloading a game too (get status and set status with updated % downloaded).

I think it would be simpler and more performant if we just cached transient data like % downloaded instead of writing to disk. We could do this with an object in gamestatushandler. What do you think @Nocccer?

BrettCleary avatar Oct 26 '22 02:10 BrettCleary

It is used to restore the progress when restarting a download, we currently store that in the browser's local storage (not sure how that works, maybe it writes to disk with some buffer and hides the amount of writes?).

Actually, the approach of storing the % is flawed, it's not accurate and it gets lost when changing installation folders and back (if you start installing the game in one folder, stop, start in another folder, stop, try to continue the installation in the first folder, there will be files but 0%. There's a way to fix that and not have to store anything, letting legendary calculate the remaining files on demand. I want to implement that eventually but we need the same feature for gogdl for that.

Maybe an option is to put the writes in a buffer and store the value once when the status changes from installing to anything else? that would help with the number of writes (we have the risk of heroic crashing and not storing that value anywhere, but maybe it's good enough until I can implement the other idea)

arielj avatar Oct 26 '22 03:10 arielj

I've been reading through the code and testing a bit. My main question is why do we need to write this to file? It seems to me that it is ephemeral state. What happens if the app crashes? Do we use this game status data to restart the download or for some other purpose?

Right now these changes (in my one test I did) incur an additional ~100 reads and writes to disk while downloading a game too (get status and set status with updated % downloaded).

I think it would be simpler and more performant if we just cached transient data like % downloaded instead of writing to disk. We could do this with an object in gamestatushandler. What do you think @Nocccer?

Before we wrote the old progress only on correct closing of heroic into the file. Now we always have the correct percentage, evevn if heroic crashes.

Nocccer avatar Oct 26 '22 17:10 Nocccer

It is used to restore the progress when restarting a download, we currently store that in the browser's local storage (not sure how that works, maybe it writes to disk with some buffer and hides the amount of writes?).

Actually, the approach of storing the % is flawed, it's not accurate and it gets lost when changing installation folders and back (if you start installing the game in one folder, stop, start in another folder, stop, try to continue the installation in the first folder, there will be files but 0%. There's a way to fix that and not have to store anything, letting legendary calculate the remaining files on demand. I want to implement that eventually but we need the same feature for gogdl for that.

Maybe an option is to put the writes in a buffer and store the value once when the status changes from installing to anything else? that would help with the number of writes (we have the risk of heroic crashing and not storing that value anywhere, but maybe it's good enough until I can implement the other idea)

This makes a lot of sense. I didn't know that about legendary and gogdl. Sounds good. Let's keep writing the % to file each tick. Performance is probably fine as is without a buffer too.

BrettCleary avatar Oct 26 '22 18:10 BrettCleary

@Nocccer @arielj what are your thoughts on having ipcMain events for each app with topic name = 'handleGameStatus' + appName?

I know before there were performance issues but I believe this was due to having >15 listeners to the same topic name. This is because electron calls these listeners synchronously if they have the same name so it'll be linear time to execute. But with the topic name being app specific, it stays constant time for each call since the keys are unique and it just hashes to it.

In this branch currently, we have the same listener hooked up to the LibraryContext so every game card rerenders if one of them changes state. This isn't optimal.

For things like the sidebar that aren't listening to a specific appName, we could keep the existing handleGameStatus general event topic.

BrettCleary avatar Oct 26 '22 18:10 BrettCleary

Ok! finally got this updated with beta and also added the refactor to prevent extra re-renders. Now each card listens for updates on it's own event name, same for the download progress in the game page, sidebar, and download manager.

Would be great if you @Nocccer and @BrettCleary can test this again.

EDIT: don't test yet, I found some issues when opening heroic with things in the queue, progress is not reported correctly

arielj avatar Oct 27 '22 03:10 arielj

Tested here and all status are working just fine. Tested installing, moving and repairing. Also installing with items on the queue.

flavioislima avatar Oct 27 '22 09:10 flavioislima

I'm marking this as WIP. The changes from the download manager broke a few things (there are multiple issues when restarting heroic during a download or after stopping a download).

arielj avatar Oct 27 '22 22:10 arielj

I started rebasing this against the current main but it got too out of sync, so I decided to start extracting this changes and applying them again but in different steps,

This is one of those https://github.com/Heroic-Games-Launcher/HeroicGamesLauncher/pull/2208

I'll keep this PR open so I don't forget to continue with this, I'll wait until that one is merge to extract a new one.

arielj avatar Dec 16 '22 03:12 arielj

Maybe we can close this one since I added the hasStatus hook to deal with that.

flavioislima avatar Feb 16 '23 21:02 flavioislima

I think we can close this, yes, there are some things that this PR was doing that could be re-implemented in the future against the current codebase (moving the progress handling/storing completely to the backend instead of the local storage of the browser) but I don't think I'll have time for it in the near future (I want to prioritize other things that have a more clear UX impact instead, I did extract some things from this already in other PRs)

arielj avatar Feb 16 '23 22:02 arielj