Display download progress for file downloads

[legoktm]

Status

Work in progress

TODO:

• Visual alignment issue; need padding on the right side
• update apparmor stuff?
• tests?

Description

Display a vanilla progress bar for file downloads that simply shows how much of the file has been downloaded so far.

A new FileDownloadProgressBar widget replaces the existing animated spinner, and we inject a signal through to the SDK to pass the current progress back to the widget.

Fixes #1104.

Test Plan

• Spin up dev server; then inside the container run NUM_SOURCES=10 --random-file-size 100000 to generate 10 sources with 100MB attachments
• Download them, including starting multiple parallel downloads and switching to other sources
• Progress bar continues to update as expected, etc.

Checklist

• These changes should not need testing in Qubes
  • everything should be testable with the dev client, since no proxy changes are needed
• I have updated the AppArmor profile
• No database schema changes are needed

[legoktm]

So right now I've implemented an exponential moving average based on https://stackoverflow.com/questions/2779600/how-to-estimate-download-time-remaining-accurately; however I missed the comment which says, "EMA will only work if the time-sampling rate is about the same. If, for example the download is updated every 1 MB rather than every 1 second and the speed fluctuates the output will most likely be nonsense". Unfortunately that's exactly what we're doing, we update on chunks rather than on a regular time interval. Will require some more 🤔

[legoktm]

I got a smoothed version of the download speed to work (by adjusting the SMOOTHING_FACTOR to be relative to the time period), but there's a more practical problem: we only update the speed after we receive a chunk, so if it stalls, we don't update the speed and it'll get stuck at e.g. 4MB/s.

So I want to rework this a bit, I'll move the calculation stuff into the widget and we can use a QTimer to update the widget's speed on an actual time interval.

[rocodes]

Thanks so much for working on this @legoktm . I haven't run the code yet, just went through a fast visual review. If you're still looking for input on tests I can come back to that tomorrow :)

[rocodes]

This stuck out to me a bit, because it means that the widget, which is otherwise "naive"/encapsulated and needs no outside knowledge of the sdk or any components, now has an internal dependency on the ProgressProxy. What would you think about the widget either taking a ProgressProxy object in its constructor (clearer dependency graph +/ easier to test) or being completely agnostic to how it's receiving updates as long as it exposes a @pyqtSlot that takes an int parameter and updates itself accordingly?

# in the parent widget  
progressbar = FileDownloadProgressBar(self.file.size)
proxy = ProgressProxy(progressbar.handle)
self.controller.on_submission_download(File, self.file.uuid, proxy)

[legoktm]

I wanted to encapsulate the construction of ProgressProxy in a single place so that if we e.g. wanted to add another signal, it only needs to be changed in one file. This isn't strictly true because we do have ProgressProxy(None) but it's close.

Also, my original design was that the SDK would have a Progress class that would be independent of PyQt but that didn't really work and I ended up needing the signal. What do you think if I moved the ProgressProxy class into the same file as the widget, since it's effectively tied together with that?

[legoktm]

What do you think if I moved the ProgressProxy class into the same file as the widget, since it's effectively tied together with that?

I've done this now so you can take a look at what it looks like.

[rocodes]

I was trying to figure out if there was any way of logically gating this (eg if it's a db.File then it always should have a non-null progress or there's an error worth logging), but that may not be true.

[legoktm]

Right, I think we just treat it as ProgressProxy | None all the way through until we actually call methods on it.

[legoktm]

Thanks for all the feedback @rocodes!

If you're still looking for input on tests I can come back to that tomorrow :)

Yes please, whenever you have time.

[legoktm]

(Still not 100% ready, but moving out of draft state so we can get notifications, etc.)

[rocodes]

Apologies for the long delay. I've looked at your changes again and taken it for a spin and I'm excited to see this feature coming together :) I think I may have been confusing previously, so I'm going to have another go at the dependencies question(s).

My original comment about hidden dependencies was that if FileDownloadProgressBar uses a ProgressProxy object, I'd suggest passing it in the constructor to make that dependency explicit (also avoids things like testfiledownloadprogressbar.progress = mocker.MagicMock(spec=ProgressProxy) during unit testing).

The bigger question I guess is about who defines ProgressProxy. Since the sdk isn't really sdk and there won't be any other consumers of it, it's not the end of the world that the sdk imports a gui component, but it does make it a little bit confusing and/or tightly coupled. (Because it's one single commit, I can't remember if the sdk importing a gui class is in response to my initial feedback or not; if it is, sorry, it's not what I had intended to convey).

I have a couple thoughts: one is, the "sdk" could contain the ProgressProxy class as an interface/ABC, and enforce a contract that the consumer has to implement (by subclassing ProgressProxy and implementing set_value). To me it looks like the DownloadJob could be the consumer and could be giving out progress signals that a widget could connect to to update itself.

My second thought/question though is, is a separate ProgressProxy object really needed? The sdk optionally needs a Callable[int] aka set_value, but needs nothing else from the ProgressProxy. Then, the FileDownloadProgressBar needs to be able to receive values (and currently also needs to receive the set_decrypting signal). The controller already connects to the file_download_success signal, which makes me wonder if it's not a candidate to also receive and emit progress updates to any listeners (and for better or worse, the FileWidget is aware of the controller already). I'm not suggesting that you change what the signals do, but in general, whenever we have a success signal, and a failure signal, and are looking at adding another signal (in this case progress), I think it's probably a good future opportunity for a refactor to one single pyqtSignal, which is what we started to do on the export side of things, and that makes me wonder if all the signals should be handled in the same place as well as a precursor.

[rocodes]

I did also do a bit of testing, and need to come back to this next week to see if I have any more specific suggestions, and also probably need to figure out testing with toxiproxy again, or going back to real tor not dev-tor, to have some longer download times :D

Off the top of my head here are a few gui-related notes:

• I think that, re your comment about padding, the FileWidget layout may also need a QSizePolicy, because in the default application screen size, the progress bar as well as the text that says "Downloading" are cut off. (I haven't looked at if the widget boundaries are hardcoded as well, and if that will need adjusting).
• The height of the progressbar may need to match the height of the other elements (and/or the parent may need its size policy adjusted?) so that the filewidget area doesn't change heights when the progress bar is displayed

[legoktm]

I'm not suggesting that you change what the signals do, but in general, whenever we have a success signal, and a failure signal, and are looking at adding another signal (in this case progress), I think it's probably a good future opportunity for a refactor to one single pyqtSignal, which is what we started to do on the export side of things, and that makes me wonder if all the signals should be handled in the same place as well as a precursor.

This makes perfect sense. Let me look at what it would take to refactor this. And if we get rid of ProgressProxy then we don't even need to discuss how it gets injected :)

[legoktm]

I think part of the problem I/we have been running into is that the existing file_download_started, file_ready_signal, file_missing signals are effectively global, while the new progress one I introduced is per-widget.

The existing signals are created in the Controller and then shared with each widget. This means that each signal has to emit the relevant UUID so each widget knows whether the status change concerns it. Feels like that would be a performance issue too, especially once we throw in a progress update every second.

The logic currently is FileWidget._on_left_click() -> Controller.on_submission_download() -> Controller._submit_download_job() -> creates and queues FileDownloadJob -> emits success/failure signal bound to Controller -> Controller.on_file_download_success/failure -> emits file_ready/file_missing signal bound to all FileWidgets

If we switch to per-widget signals and consolidate down to one, file_status, I think we'd end up with something like:

FileWidget._on_left_click() -> Controller.on_submission_download() -> Controller._submit_download_job() -> creates and queues FileDownloadJob -> emits success/failure signal bound to FileWidget -> FileWidget.on_file_status() -> Controller.on_file_download_success/failure

If that makes sense. I might put together a WIP to demo what I mean because I confused myself while writing this.

[legoktm]

If that makes sense. I might put together a WIP to demo what I mean because I confused myself while writing this.

Not a complete thing, but hopefully enough to get the idea of what I was thinking: 93879a9

[rocodes]

Not a complete thing, but hopefully enough to get the idea of what I was thinking: 93879a9

Yes for sure - it is much better if it's a per widget signal, I see where you're going with this and I agree, thanks for laying it out. And you're right- the controller is a bit of a kitchen sink, and we had had past efforts at disentangling it (eg #1553) but it's been a bit of a struggle.

I know you're balancing a lot of different considerations, so thank you for going back and forth on it. I think ultimately, where I'm landing on this is components being separately testable and dependencies being explicitly injected (or of course not existing :P) is more important than strict isolation of imports, but it's still a nice to have if it works out.