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

Activity indicator for file download / decryption. #702

Merged
merged 7 commits into from
Jan 23, 2020

Conversation

ntoll
Copy link
Contributor

@ntoll ntoll commented Jan 15, 2020

Description

Fixes #288.

Q: downloading involves network latency. Decrypting is pretty instantaneous for me (am I missing something here?).

Here's a screenie-gif of the progress so far:

download_spinner

The behaviour in the screenie (as of time of writing) is only what is implemented in the PR (along with a test update).

Please confirm:

Test Plan

Added additions to relevant unit test.

Checklist

If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable:

  • I have tested these changes in the appropriate Qubes environment
  • I do not have an appropriate Qubes OS workstation set up (the reviewer will need to test these changes)
  • These changes should not need testing in Qubes

If these changes add or remove files other than client code, packaging logic (e.g., the AppArmor profile) may need to be updated. Please check as applicable:

  • I have submitted a separate PR to the packaging repo
  • No update to the packaging logic (e.g., AppArmor profile) is required for these changes
  • I don't know and would appreciate guidance

@eloquence
Copy link
Member

Decrypting is pretty instantaneous for me (am I missing something here?).

Upload size limit for individual files is 500 MB. How long does it take at or near that limit in Qubes?

@ntoll
Copy link
Contributor Author

ntoll commented Jan 15, 2020

@eloquence aha... good point. I've just been uploading goofy images and the like. In any case, the worst that will happen is the indicator will be simply displayed for longer.

@ninavizz
Copy link
Member

  • I would like to add a minimum-time of 3-seconds to the "Active" state
  • The states all appear to note line-up quite right... namely, from "Active" to "Downloaded" seems to hop.
  • Is there any kind of a hover state?

@ntoll
Copy link
Contributor Author

ntoll commented Jan 15, 2020

@ninavizz OK lets chat about this on Slack. :-)

Copy link
Contributor

@sssoleileraaa sssoleileraaa left a comment

Choose a reason for hiding this comment

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

It looks like the download links are out of alignment and with extra space after clicking on them, see before:

Screenshot from 2020-01-15 22-55-29

and after:

Screenshot from 2020-01-15 22-55-59

Also, the active refresh button would look better if it were the same light blue as the highlighted "DOWNLOAD" link and if it didn't flash by so quickly. Maybe adding a 1-2 second delay would improve the UX here. We can discuss in tomorrow's UX discussion.

@ntoll
Copy link
Contributor Author

ntoll commented Jan 16, 2020

@creviera by out of alignment do you mean they don't have the same width from before and afterwards..?

@ntoll
Copy link
Contributor Author

ntoll commented Jan 16, 2020

FWIW, I've not actually changed the spacing / layout of anything yet. I've just updated the icon on the button to the "spinner". So this out-of-alignment problem is probably already there before this PR was submitted. Happy to fix though, just need to know what you want. :-)

@ntoll
Copy link
Contributor Author

ntoll commented Jan 16, 2020

Also, (sorry for the number of message) the colour changes are what were defined in the Zepelin page I linked to. I agree, the colour looks "nicer" if it remains the same as the "DOWNLOAD" link. But I'm a programmer, what do I know..? ;-)

@ntoll
Copy link
Contributor Author

ntoll commented Jan 16, 2020

OK... so I've been looking into the layout of the file state as flagged by @creviera. It appears it was broken before this PR (current master displays this behaviour for me). I'm in the process of trying to clean this up.

@ntoll
Copy link
Contributor Author

ntoll commented Jan 16, 2020

Whoop-de-do. This took a while to figure out... ;-)

(Note hoe everything is all aligned now...)

fixed

Happy to change anything. Just let me know if this looks OK to you.

However I strongly disagree with including a delay of a few seconds so people see the spinner. To my mind that's making the UI appear inaccurate to the user. If something's finished, I humbly suggest the UI should reflect that. 🙂 After all, we want folks to trust this application right..? 😱

@ntoll
Copy link
Contributor Author

ntoll commented Jan 16, 2020

OK... borked some tests so fixing before I push.

@ntoll
Copy link
Contributor Author

ntoll commented Jan 16, 2020

Please check again. Thanks!

@ntoll ntoll mentioned this pull request Jan 16, 2020
Copy link
Contributor

@sssoleileraaa sssoleileraaa left a comment

Choose a reason for hiding this comment

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

The animation looks nice. Just two requests (the last request you can ignore if you think it's outside the scope of this PR/ if you'd like to continue discussion and followup in a separate issue):

(1) See how clicking on a download link while not logged in causes an error message to appear that says you must be logged in to perform this action? We don't want to show the download as in progress when we're not downloading the file, so this needs to be updated.
Screenshot from 2020-01-16 13-47-17

(2) If the download finishes very quickly (like less than a couple-few seconds), then I don't think we should show the animation because it looks broken when you can't tell what is flashing before your eyes. I think this is what @ninavizz mentioned here: #702 (comment). Basically you could add a timer that begins when a click occurs and then waits a couple seconds before animation begins.

@sssoleileraaa
Copy link
Contributor

However I strongly disagree with including a delay of a few seconds so people see the spinner.

Whoops I meant we maybe shouldn't show the spinner if it's only spinning for a second because it's so fast it's confusing what happened.

@ntoll
Copy link
Contributor Author

ntoll commented Jan 20, 2020

@creviera many apologies -- my misunderstanding.

I see what you mean. I don't know about you, but I sometimes find it infuriating if the computer flashes some important looking message up only for it to disappear immediately after you've registered there's an important looking message there and "dammit, what on earth did it say..?". ;-)

This is very much a case of helping the UI signal things in "human time" rather than the speedier "computer time". I guess we could schedule the change in UI state after something like 0.3 seconds, i.e. if the network latency is low and the file size is small, then it'll complete before then... BUT if there's a longer delay that's perceivable to the user, then the UI will look as if it's doing something.

I'll play around to see what I can do in this respect, but would welcome thoughts from more UX focussed folks with more experience and expertise than a numpty like me. :-) @ninavizz ???

@ntoll
Copy link
Contributor Author

ntoll commented Jan 20, 2020

OK... I've just used a QTimer.singleShot to make this work + added tests. Feedback please. :-)

@ninavizz
Copy link
Member

Hi @ntoll I'd like to get you permanent art for this spinner, but still do not know what format the art would need to be in. Could you let me know—and I'll create something straight away, for this PR and other animation needs? :D The timing concerns y'all discuss up above, I feel could be helpful to chat about synchronously before speaking to, here. Want to make sure I'm clearly understanding everyone/everything, first.

@sssoleileraaa
Copy link
Contributor

This is looking so good! I see you addressed issue 2 from my review, but it looks like issue 1 still exists:

one-issue

@ntoll
Copy link
Contributor Author

ntoll commented Jan 23, 2020

@creviera fixed in latest push.

Copy link
Contributor

@sssoleileraaa sssoleileraaa left a comment

Choose a reason for hiding this comment

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

lgtm, both issues addressed and confirmed to be fixed!

@sssoleileraaa sssoleileraaa merged commit 33856fb into freedomofpress:master Jan 23, 2020
@rmol rmol mentioned this pull request Jan 28, 2020
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement download/decryption activity indicator (first iteration)
4 participants