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

Merged
merged 19 commits into from
Nov 22, 2024

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.

@Alystrasz Alystrasz mentioned this pull request Sep 7, 2024
2 tasks
@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
Copy link

@NachosChipeados NachosChipeados left a comment

Choose a reason for hiding this comment

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

@Alystrasz Alystrasz added READY TO MERGE This mergeable right now and removed needs testing Changes from the PR still need to be tested almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge labels Nov 20, 2024
@GeckoEidechse
Copy link
Member

Thanks for testing. I'll push out the current release-candidate as a full patch release, give it a day or two to stabilise and then merge this and prepare the next minor release ^^

@wolf109909
Copy link
Contributor

Tested on NorthstarCN but it works fine!

@GeckoEidechse GeckoEidechse merged commit 6585d62 into R2Northstar:main Nov 22, 2024
4 checks passed
@Alystrasz Alystrasz deleted the feat/mad-abort branch November 22, 2024 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MAD Related to mod-auto-download READY TO MERGE This mergeable right now
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants