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

Don't report an unknown error if it is known #2995

Merged
merged 1 commit into from
Feb 10, 2020

Conversation

DasSkelett
Copy link
Member

Problem

When there's an exception thrown during the installation of mods, we catch the usual ones and log an explanation message on the WaitTabPage.
However in PostInstallMods() which is called when the background worker completes InstallMods(), the message An unknown error occurred, please try again! is written to the "log".
This message is actually not true most of the time, because e.g. download errors or inconsistency problems are all catched and a nice explaining error message has already been printed out.

Changes

Now PostInstallMods() distinguishes catched and uncatched errors (the second one will result in RunWorkerCompletedEventArgs.Error being set).
It keeps the old message for uncatched errors, but displays a new one for known ones:

If the above message indicates a download error, please try again. Otherwise, please open a issue for us to investigate.
If you suspect a metadata problem: https://github.com/KSP-CKAN/NetKAN/issues/new/choose
If you suspect a bug in the client: https://github.com/KSP-CKAN/CKAN/issues/new/choose

(Suggestions to change this message are welcome)

This way we don't contradict ourselves, and also encourage the users to report metadata/client problems.

@DasSkelett DasSkelett added Bug Something is not working as intended Easy This is easy to fix GUI Issues affecting the interactive GUI labels Feb 8, 2020
@DasSkelett
Copy link
Member Author

Easy way to test this PR: Block github.com and archive.org in your hosts file (works on Windows too).

GUI/Main/MainInstall.cs Outdated Show resolved Hide resolved
Copy link
Member

@HebaruSan HebaruSan left a comment

Choose a reason for hiding this comment

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

Took me a while to figure out exactly when we fail without e.Error set; it's in all the catch blocks of InstallMods that print an error and return. But this makes sense!

@HebaruSan HebaruSan merged commit 841a55e into KSP-CKAN:master Feb 10, 2020
@DasSkelett DasSkelett deleted the fix/unknown-error branch February 10, 2020 21:10
@DasSkelett
Copy link
Member Author

Took me a while to figure out exactly when we fail without e.Error set; it's in all the catch blocks of InstallMods that print an error and return. But this makes sense!

Yes indeed. e.Error is set by the runtime when an exception inside the worker hasn't been catched.
I was thinking about refactoring the InstallMods to not catch the exceptions and let PostInstallMods handle it, but I didn't want to mess with it and potentially introduce bugs just for this little output change.

@HebaruSan
Copy link
Member

That would make more sense, arguably more in line with how BackgroundWorker is designed.

@DasSkelett DasSkelett added the i18n Issues regarding the internationalization of CKAN and PRs that need translating label Jan 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is not working as intended Easy This is easy to fix GUI Issues affecting the interactive GUI i18n Issues regarding the internationalization of CKAN and PRs that need translating
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants