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

Fix: Display progress message properly when download size missing (#1) #15588

Merged
merged 1 commit into from Sep 13, 2018

Conversation

Projects
None yet
4 participants
@dmsnell
Copy link
Contributor

dmsnell commented Sep 5, 2018

When I downloaded the assets for Red Alert through the Quick Install I noticed the progress bar proceed and display a recognizable message: Downloading from … 1.47/12 MB (12%). This was fine.

When I downloaded the assets for one of the other games, maybe Dune 2000, there was obviously no total download size available. I saw an unexpected message: Downloading from … 1.47/NaN (NaN%)

The code handling network progress events seems to be aware of the possibility that no full download size exists but it doesn't update the message. In this patch I'm proposing that we display a separate message indicating that we don't know how much more we have to download for these cases.

Of the alternative ways to implement this I chose to move the reassignment of getStatusText into the conditional structures to preserve the existing choice. The message was qualitatively different and so I felt it worthwhile to create entirely different closures vs. doing something like this…

getStatusText = () => ( Double.isNaN( dataTotal ) ? "Downloading {1} of unknown amount" : "Downloading {1}/{2}" ).F( … );

Status

  • I have not tested these changes. I don't have the dev environment setup and for this change I didn't want to attempt all that work. Sorry 🤷‍♂️ - this bug doesn't bother me; I just thought I'd push the change up in case anyone wanted it.
  • Wasn't sure if we should address progressBar.Percentage = i.ProgressPercentage; at the same time. I'm guessing that this too will be NaN but in the display I think it results in an empty progress bar so maybe it's not a problem.

Closes #15489.
Closes #15493.

@matjaeck

This comment has been minimized.

Copy link
Contributor

matjaeck commented Sep 5, 2018

Hi, thanks for making the PR.

I have not tested these changes.

Please note that OpenRA is very short on reviewers so it is a great help if authors test their changes. Could you confirm that the changes work and do what you intend with them, i.e. repeat the download with your changes? You should be able to test this by deleting the downloaded files, run make all on the PR branch and then try to download again.

@@ -85,21 +85,24 @@ void ShowDownloadDialog()
dataTotal = float.NaN;
dataReceived = i.BytesReceived;
dataSuffix = SizeSuffixes[0];

This comment has been minimized.

@matjaeck

matjaeck Sep 5, 2018

Contributor

Style error: Invalid spacing at the end of the line.

@dmsnell

This comment has been minimized.

Copy link
Contributor

dmsnell commented Sep 5, 2018

Could you confirm that the changes work and do what you intend with them, i.e. repeat the download with your changes? You should be able to test this by deleting the downloaded files, run make all on the PR branch and then try to download again.

Realistically not soon but I can see if I can slate some time for it. I have installed openra through homebrew as a binary and I'll have to see if I have the build environment readily available to build the source.

(I'll keep an eye on Travis to make sure it's happy too - thanks for the additional comments though!)

@matjaeck

This comment has been minimized.

Copy link
Contributor

matjaeck commented Sep 5, 2018

Thanks, if you find the time for it, https://github.com/OpenRA/OpenRA/wiki/Contributing contains useful information (it should cover all required steps) to get started!

@dmsnell https://github.com/OpenRA/OpenRA/wiki/Compiling for compiling from source, this is probably what you want.

@dmsnell

This comment has been minimized.

Copy link
Contributor

dmsnell commented Sep 5, 2018

@matjaeck this team deserves some serious props because this was a breeze to compile and test locally. 👏

tested the changes, updated the patch, working as expected.

there are a couple things this doesn't address:

  • the number of bytes flying by is a big list; it would be better to have a fallback human-time format to switch to KB or MB etc…
  • combined with listing the full byte count the unknown size string gets pushed out of the display on my computer - the dialog is too narrow. not a big deal
  • the dialog reappears after successfully downloading the assets for a moment before the game loads. I haven't investigated this at all
@dmsnell

This comment has been minimized.

Copy link
Contributor

dmsnell commented Sep 10, 2018

@matjaeck was there anything else you are waiting on me for with this PR?

@abcdefg30
Copy link
Member

abcdefg30 left a comment

Code looks good to me (not tested ingame). Can you squash the commits, please?

Fix: Display progress message properly when download size missing (#1)
When I downloaded the assets for Red Alert through the Quick Install I noticed the progress bar proceed and display a recognizable message: `Downloading from … 1.47/12 MB (12%)`. This was fine.

When I downloaded the assets for one of the other games, maybe Dune 2000, there was obviously no total download size available. I was an unexpected message: `Downloading from … 1.47/NaN (NaN%)`

The code handling network progress events seems to be aware of the possibility that no full download size exists but it doesn't update the message. In this path I'm proposing that we display a separate messaging indicating that we don't know how much more we have to download for these cases.

Of the alternative ways to implement this I chose to move the reassignment of `getStatusText` into the conditional structures to preserve the existing choice. The message was qualitatively different and so I felt it worthwhile to create entirely different closures vs. doing something like this…

```cs
getStatusText = () => ( Double.isNaN( dataTotal ) ? "Downloading {1} of unknown amount" : "Downloading {1}/{2}" ).F( … );
```

@dmsnell dmsnell force-pushed the dmsnell:bleed branch from 2a74c5e to b1f3f38 Sep 10, 2018

@dmsnell

This comment has been minimized.

Copy link
Contributor

dmsnell commented Sep 10, 2018

@abcdefg30 squashed and pushed!

@Mailaender Mailaender merged commit 95558a3 into OpenRA:bleed Sep 13, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Mailaender

This comment has been minimized.

Copy link
Member

Mailaender commented Sep 13, 2018

Note: this is almost identical to #15493.

@Mailaender

This comment has been minimized.

Copy link
Member

Mailaender commented Sep 13, 2018

Changelog

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