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: Make MAD process cancellable #815

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

Conversation

Alystrasz
Copy link
Contributor

@Alystrasz Alystrasz commented Sep 4, 2024

A long time ago, during the initial development of the feature, somebody (Jan maybe?) suggested making the mod install process cancellable, in case it takes too long (slow Internet speed, for instance).

This PR does just that, implementing the ModDownloader::CancelDownload function (already present in the header btw), that can stop MAD process at two moments:

Must be merged with mods counterpart (R2Northstar/NorthstarMods#865)!

Media

mod_download_cancelling.webm

Here, mod install process is interrupted twice, by successively stopping archive downloading and archive extraction; note that this does not prevent mod install from completing successfully when ran a third time.

Testing

  1. Install both launcher and mods PRs;
  2. Launch game and go to server browser;
  3. Enable MAD (allow_mod_auto_download 1);
  4. Try to join Space battle server (this server requires a mod that's big enough to leave you enough time to click the new "cancel" button);
  5. Enjoy the new "cancel" button!

@github-actions github-actions bot added needs testing Changes from the PR still need to be tested needs code review Changes from PR still need to be reviewed in code labels Sep 4, 2024
Copy link
Contributor

@ASpoonPlaysGames ASpoonPlaysGames left a comment

Choose a reason for hiding this comment

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

Code looks good

What happens if a server requires two mods, and the first one completes but the second is aborted? Do we clean up the already-downloaded mod properly?

@ASpoonPlaysGames ASpoonPlaysGames added almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge and removed needs code review Changes from PR still need to be reviewed in code labels Sep 5, 2024
@Alystrasz
Copy link
Contributor Author

What happens if a server requires two mods, and the first one completes but the second is aborted? Do we clean up the already-downloaded mod properly?

The download method is triggered once per mod: this means first mod would be installed, and second mod would be cleaned up.

@ASpoonPlaysGames
Copy link
Contributor

The download method is triggered once per mod: this means first mod would be installed, and second mod would be cleaned up.

Do we enable the mods as soon as they are downloaded? Could this potentially cause issues?

@Alystrasz
Copy link
Contributor Author

Alystrasz commented Sep 5, 2024

Do we enable the mods as soon as they are downloaded? Could this potentially cause issues?

Nope, mods enabling happens after all mods have been downloaded (meaning no mod reloading happens if cancel button is pressed) and only has an effect on RequiredOnClient mods.

@github-actions github-actions bot added the merge conflicts Blocked by merge conflicts, waiting on the author to resolve label Sep 7, 2024
@github-actions github-actions bot removed the merge conflicts Blocked by merge conflicts, waiting on the author to resolve label Sep 7, 2024
@GeckoEidechse GeckoEidechse added the MAD Related to mod-auto-download label Sep 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge MAD Related to mod-auto-download needs testing Changes from the PR still need to be tested
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants