HeroicGamesLauncher
HeroicGamesLauncher copied to clipboard
[Refactor] Game Status handling
- Moving the handling of game status to the backend and use electron-store to store progress.
- Adapt and renamed
hasProgress
tohasGameStatus
to also listen tostatus
and not only toprogress
- 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)
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.
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.
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.
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.
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.
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 thecontinue download
feature is working fine - I replaced the
sendKill
when cancelling an installation with a newcancelInstallation
api method, with this I was able to also move theremoveFolder
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
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?
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)
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.
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'sgood 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.
@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.
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
Tested here and all status are working just fine. Tested installing, moving and repairing. Also installing with items on the queue.
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).
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.
Maybe we can close this one since I added the hasStatus
hook to deal with that.
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)