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

[Feat] Improve the "Move Game" experience a bit #3648

Merged
merged 2 commits into from
Mar 26, 2024
Merged

Conversation

CommandMC
Copy link
Collaborator

Nothing too significant, just 3 quick improvements to the "Move Game" functionality:

  • Progress reports are dispatched to the Frontend more frequently
    • There were multiple if (percent) checks, which erroneously filtered out the case where percent might be 0 (aka we just started copying a new file)
  • On Linux, we now tell rsync to delete source/old files after they're copied to the new location
    • This should help in scenarios with very little disk space being available. Before, we first copy and then delete, now we delete files while copying, and then the entire directory (which should be using virtually no space anymore) at the end
  • The types for the progress property of the progress update are now correct (percent should be a number instead of a string, and eta and bytes were missing entirely)
    • Note that eta and bytes still aren't populated on Windows (robocopy simply doesn't give us this info), so I've not displayed them anywhere yet
    • This issue was found by type-checking sendFrontendMessage, a PR for that should land alongside this one

I'm still not entirely happy with this function (from a user's point of view I'd expect it to give me a progress bar for the entire move operation, knowing that one specific file is being moved is not too helpful), but that'd be a more complex refactor


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

  • Tested the feature and it's working on a current and clean install.
  • 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)

@CommandMC CommandMC added the pr:ready-for-review Feature-complete, ready for the grind! :P label Mar 24, 2024
@CommandMC CommandMC requested a review from a team March 24, 2024 20:49
@CommandMC CommandMC self-assigned this Mar 24, 2024
@CommandMC CommandMC requested review from arielj, flavioislima, Etaash-mathamsetty, Nocccer and imLinguin and removed request for a team March 24, 2024 20:49
src/backend/utils.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@arielj arielj left a comment

Choose a reason for hiding this comment

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

I left some comments, I couldn't test the windows one cause I don't have windows

src/backend/utils.ts Outdated Show resolved Hide resolved
src/backend/utils.ts Outdated Show resolved Hide resolved
src/backend/utils.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@arielj arielj 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 to me!

I can't test the windows one though, and I feel like we are doing more work than needed sending so many updates to the frontend but I think that's something to refactor in another PR anyway (I imagine something like capturing the last file and progress for every data but only send messages to the frontend every X milliseconds? something like that)

@CommandMC
Copy link
Collaborator Author

CommandMC commented Mar 26, 2024

We should definitely modify how the progress is presented, I agree. I think going for a general "Here's how far along the entire move process is" progress bar (as Steam does it, for example), and switching to per-file progress if one takes more than a couple seconds to transfer, would be the best option

I can't test the windows one though

I've tested this in a Windows VM (actually worked out the Windows changes before the Linux ones) and it works fine. Filenames are a little weird sometimes (it sometimes shows a relative path and sometimes an absolute one), but that's robocopy's fault as far as I can tell

@CommandMC CommandMC merged commit c62820d into main Mar 26, 2024
9 checks passed
@CommandMC CommandMC deleted the feat/better-move branch March 26, 2024 22:19
@Heroic-Games-Launcher Heroic-Games-Launcher locked and limited conversation to collaborators Mar 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr:ready-for-review Feature-complete, ready for the grind! :P
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants