Skip to content

Download failure error.#21460

Merged
PunkPun merged 1 commit into
OpenRA:bleedfrom
darkademic:pr/download-failure-error
Oct 12, 2024
Merged

Download failure error.#21460
PunkPun merged 1 commit into
OpenRA:bleedfrom
darkademic:pr/download-failure-error

Conversation

@darkademic

Copy link
Copy Markdown
Contributor

Currently when downloading mod content, if a download fails (e.g. due to 404), there's no error message or indication that anything failed, and it just returns to the downloads list. This adds an error message so it's clear something went wrong and allows retrying.

@PunkPun PunkPun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there an easy way to test this?

@RoosterDragon

Copy link
Copy Markdown
Member

Invert the condition to == HttpStatusCode.OK - to pop the error in the success case.

@anvilvapre

Copy link
Copy Markdown
Contributor

Wouldn't it be practical then to also show the status code and message and even log the URL?

@Mailaender

Copy link
Copy Markdown
Member

OnError already has logging:

Log.Write("install", $"Download from {host} failed: " + s);

@Mailaender

Copy link
Copy Markdown
Member

However you are combining a localized string with a non-localized one:

getStatusText = () => $"{host}: Error: {s}";

Would be nice if you could complete it.

@PunkPun PunkPun force-pushed the pr/download-failure-error branch from 4b0a546 to 20131fe Compare October 12, 2024 11:25
@PunkPun

PunkPun commented Oct 12, 2024

Copy link
Copy Markdown
Member

that's scope creep, it's an issue that's already present and not introduced by this PR

@PunkPun PunkPun merged commit d450ef4 into OpenRA:bleed Oct 12, 2024
@PunkPun

PunkPun commented Oct 12, 2024

Copy link
Copy Markdown
Member

changelog

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants