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.

Sign up for GitHub

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

Display download progress for file downloads #2327

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Display download progress for file downloads #2327

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions client/securedrop_client/api_jobs/downloads.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
from securedrop_client.api_jobs.base import SingleObjectApiJob
from securedrop_client.crypto import CryptoError, GpgHelper
from securedrop_client.db import DownloadError, DownloadErrorCodes, File, Message, Reply
from securedrop_client.gui.base.progress import ProgressProxy
from securedrop_client.sdk import API, BaseError
from securedrop_client.sdk import Reply as SdkReply
from securedrop_client.sdk import Submission as SdkSubmission
Expand Down Expand Up @@ -56,6 +57,7 @@ class DownloadJob(SingleObjectApiJob):
def __init__(self, data_dir: str, uuid: str) -> None:
super().__init__(uuid)
self.data_dir = data_dir
self.progress: ProgressProxy | None = None

def _get_realistic_timeout(self, size_in_bytes: int) -> int:
"""
Expand Down Expand Up @@ -177,6 +179,8 @@ def _decrypt(self, filepath: str, db_object: File | Message | Reply, session: Se
"""
Decrypt the file located at the given filepath and mark it as decrypted.
"""
if self.progress:
self.progress.set_decrypting()
try:
original_filename = self.call_decrypt(filepath, session)
db_object.download_error = None
Expand Down Expand Up @@ -260,7 +264,7 @@ def call_download_api(self, api: API, db_object: Reply) -> tuple[str, str]:
# will want to pass the default request timeout to download_reply instead of setting it on
# the api object directly.
api.default_request_timeout = 20
return api.download_reply(sdk_object)
return api.download_reply(sdk_object, progress=self.progress)

def call_decrypt(self, filepath: str, session: Session | None = None) -> str:
"""
Expand Down Expand Up @@ -316,7 +320,7 @@ def call_download_api(self, api: API, db_object: Message) -> tuple[str, str]:
sdk_object.source_uuid = db_object.source.uuid
sdk_object.filename = db_object.filename
return api.download_submission(
sdk_object, timeout=self._get_realistic_timeout(db_object.size)
sdk_object, timeout=self._get_realistic_timeout(db_object.size), progress=self.progress
)

def call_decrypt(self, filepath: str, session: Session | None = None) -> str:
Expand Down Expand Up @@ -375,7 +379,7 @@ def call_download_api(self, api: API, db_object: File) -> tuple[str, str]:
sdk_object.source_uuid = db_object.source.uuid
sdk_object.filename = db_object.filename
return api.download_submission(
sdk_object, timeout=self._get_realistic_timeout(db_object.size)
sdk_object, timeout=self._get_realistic_timeout(db_object.size), progress=self.progress
)

def call_decrypt(self, filepath: str, session: Session | None = None) -> str:
Expand Down
2 changes: 2 additions & 0 deletions client/securedrop_client/gui/base/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,10 @@
SvgPushButton,
SvgToggleButton,
)
from securedrop_client.gui.base.progress import FileDownloadProgressBar

__all__ = [
"FileDownloadProgressBar",
"ModalDialog",
"PasswordEdit",
"SDPushButton",
Expand Down
114 changes: 114 additions & 0 deletions client/securedrop_client/gui/base/progress.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
import math
import time

from PyQt5.QtCore import QTimer, pyqtBoundSignal, pyqtSignal
from PyQt5.QtWidgets import QProgressBar

from securedrop_client.utils import humanize_speed

SMOOTHING_FACTOR = 0.3


class FileDownloadProgressBar(QProgressBar):
"""
A progress bar for file downloads.

It receives progress updates from the SDK and updates the total % downloaded,
as well as calculating the current speed.

We use an exponential moving average to smooth out the speed as suggested by
https://stackoverflow.com/a/3841706; the reported speed is 30% of the current
speed and 70% of the previous speed. It is updated every 100ms.
"""

size_signal = pyqtSignal(int)
decrypting_signal = pyqtSignal(bool)

def __init__(self, file_size: int) -> None:
super().__init__()
self.setObjectName("FileDownloadProgressBar")
self.setMaximum(file_size)
# n.b. we only update the bar's value and let the text get updated by
# the timer in update_speed
legoktm marked this conversation as resolved.
Show resolved Hide resolved
self.size_signal.connect(self.setValue)
self.decrypting_signal.connect(self.handle_decrypting)
self.timer = QTimer(self)
self.timer.setInterval(100)
self.timer.timeout.connect(self.update_speed)
self.timer.start()
# The most recently calculated speed
self.speed = 0.0
# The last time we updated the speed
self.last_total_time = 0.0
# The number of bytes downloaded the last time we updated the speed
self.last_total_bytes = 0

def handle_decrypting(self, decrypting: bool) -> None:
if decrypting:
# Stop the speed timer and then switch to an indeterminate progress bar
self.timer.stop()
self.setMaximum(0)
self.setValue(0)

def update_display(self) -> None:
"""Update the text displayed in the progress bar."""
# Use math.floor so we don't show 100% until we're actually done
maximum = self.maximum()
if maximum == 0:
# Race condition: we've likely switched to the indeterminate progress bar
# which has a maximum of 0. Treat it like 100% even though it won't show up
# just to avoid the DivisionByZero error.
percentage = 100
else:
percentage = math.floor((self.value() / maximum) * 100)
formatted_speed = humanize_speed(self.speed)
# TODO: localize this?
if percentage in (0, 100):
# If haven't started or have finished, don't display a 0B/s speed
self.setFormat(f"{percentage}%")
else:
self.setFormat(f"{percentage}% | {formatted_speed}")

def update_speed(self) -> None:
"""Calculate the new speed and trigger updating the display."""
now = time.monotonic()
value = self.value()

# If this is the first update we report the speed as 0
if self.last_total_time == 0:
self.last_total_time = now
self.last_total_bytes = value
self.speed = 0
return

time_diff = now - self.last_total_time
bytes_diff = value - self.last_total_bytes
if time_diff > 0:
self.speed = (
1 - SMOOTHING_FACTOR
) * self.speed + SMOOTHING_FACTOR * bytes_diff / time_diff

self.last_total_time = now
self.last_total_bytes = value
self.update_display()

def proxy(self) -> "ProgressProxy":
"""Get a proxy that updates this widget."""
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

return ProgressProxy(self.size_signal, self.decrypting_signal)


class ProgressProxy:
"""
Relay the current download progress over to the UI; see
the FileDownloadProgressBar widget.
"""

def __init__(self, size_signal: pyqtBoundSignal, decrypting_signal: pyqtBoundSignal) -> None:
self.size_signal = size_signal
self.decrypting_signal = decrypting_signal

def set_value(self, value: int) -> None:
self.size_signal.emit(value)

def set_decrypting(self) -> None:
self.decrypting_signal.emit(True)
33 changes: 20 additions & 13 deletions client/securedrop_client/gui/widgets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,13 @@
ExportConversationTranscriptAction,
PrintConversationAction,
)
from securedrop_client.gui.base import SecureQLabel, SvgLabel, SvgPushButton, SvgToggleButton
from securedrop_client.gui.base import (
FileDownloadProgressBar,
SecureQLabel,
SvgLabel,
SvgPushButton,
SvgToggleButton,
)
from securedrop_client.gui.conversation import DeleteConversationDialog
from securedrop_client.gui.datetime_helpers import format_datetime_local
from securedrop_client.gui.shortcuts import Shortcuts
Expand Down Expand Up @@ -2579,7 +2585,8 @@ def __init__(
self.download_button.setIcon(load_icon("download_file.svg"))
self.download_button.setFont(self.file_buttons_font)
self.download_button.setCursor(QCursor(Qt.PointingHandCursor))
self.download_animation = load_movie("download_file.gif")
self.download_progress = FileDownloadProgressBar(self.file.size)
self.download_progress.hide()
self.export_button = QPushButton(_("EXPORT"))
self.export_button.setObjectName("FileWidget_export_print")
self.export_button.setFont(self.file_buttons_font)
Expand All @@ -2590,6 +2597,9 @@ def __init__(
self.print_button.setFont(self.file_buttons_font)
self.print_button.setCursor(QCursor(Qt.PointingHandCursor))
file_options_layout.addWidget(self.download_button)
file_options_layout.addWidget(self.download_progress)
# Add a bit of padding after the progress bar
file_options_layout.addSpacing(5)
file_options_layout.addWidget(self.export_button)
file_options_layout.addWidget(self.middot)
file_options_layout.addWidget(self.print_button)
Expand Down Expand Up @@ -2675,6 +2685,7 @@ def _set_file_state(self) -> None:
logger.debug(f"Changing file {self.uuid} state to decrypted/downloaded")
self._set_file_name()
self.download_button.hide()
self.download_progress.hide()
self.no_file_name.hide()
self.export_button.show()
self.middot.show()
Expand All @@ -2693,6 +2704,7 @@ def _set_file_state(self) -> None:

self.download_button.setFont(self.file_buttons_font)
self.download_button.show()
self.download_progress.hide()

# Reset stylesheet
self.download_button.setStyleSheet("")
Expand Down Expand Up @@ -2793,34 +2805,29 @@ def _on_left_click(self) -> None:
if self.controller.api:
self.start_button_animation()
# Download the file.
self.controller.on_submission_download(File, self.uuid)
self.controller.on_submission_download(
File, self.uuid, self.download_progress.proxy()
)

def start_button_animation(self) -> None:
"""
Update the download button to the animated "downloading" state.
"""
self.downloading = True
self.download_animation.frameChanged.connect(self.set_button_animation_frame)
self.download_animation.start()
self.download_progress.setValue(0)
self.download_progress.show()
self.download_button.setText(_(" DOWNLOADING "))

# Reset widget stylesheet
self.download_button.setStyleSheet("")
self.download_button.setObjectName("FileWidget_download_button_animating")
self.download_button.setStyleSheet(self.DOWNLOAD_BUTTON_CSS)

def set_button_animation_frame(self, frame_number: int) -> None:
"""
Sets the download button's icon to the current frame of the spinner
animation.
"""
self.download_button.setIcon(QIcon(self.download_animation.currentPixmap()))

def stop_button_animation(self) -> None:
"""
Stops the download animation and restores the button to its default state.
"""
self.download_animation.stop()
self.download_progress.hide()
file = self.controller.get_file(self.file.uuid)
if file is None:
self.deleteLater()
Expand Down
21 changes: 17 additions & 4 deletions client/securedrop_client/logic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,13 @@
SendReplyJobTimeoutError,
)
from securedrop_client.crypto import GpgHelper
from securedrop_client.gui.base.progress import ProgressProxy
from securedrop_client.queue import ApiJobQueue
from securedrop_client.sdk import AuthError, RequestTimeoutError, ServerConnectionError
from securedrop_client.sdk import (
AuthError,
RequestTimeoutError,
ServerConnectionError,
)
from securedrop_client.sync import ApiSync
from securedrop_client.utils import check_dir_permissions

Expand Down Expand Up @@ -825,7 +830,11 @@ def set_status(self, message: str, duration: int = 5000) -> None:

@login_required
def _submit_download_job(
self, object_type: type[db.Reply] | type[db.Message] | type[db.File], uuid: str
self,
object_type: type[db.Reply] | type[db.Message] | type[db.File],
uuid: str,
# progress is only reported for file downloads
progress: ProgressProxy | None = None,
legoktm marked this conversation as resolved.
Show resolved Hide resolved
) -> None:
if object_type == db.Reply:
job: ReplyDownloadJob | MessageDownloadJob | FileDownloadJob = ReplyDownloadJob(
Expand All @@ -842,6 +851,7 @@ def _submit_download_job(
job.success_signal.connect(self.on_file_download_success)
job.failure_signal.connect(self.on_file_download_failure)

job.progress = progress
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

self.add_job.emit(job)

def download_new_messages(self) -> None:
Expand Down Expand Up @@ -974,12 +984,15 @@ def on_file_open(self, file: db.File) -> None:

@login_required
def on_submission_download(
self, submission_type: type[db.File] | type[db.Message], submission_uuid: str
self,
submission_type: type[db.File] | type[db.Message],
submission_uuid: str,
progress: ProgressProxy | None = None,
) -> None:
"""
Download the file associated with the Submission (which may be a File or Message).
"""
self._submit_download_job(submission_type, submission_uuid)
self._submit_download_job(submission_type, submission_uuid, progress)

def on_file_download_success(self, uuid: str) -> None:
"""
Expand Down
Loading
Loading