Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
WIP: Display download progress for file downloads
Browse files Browse the repository at this point in the history
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.

TODO:
* Visual alignment issue; need padding on the right side
* tests?

Fixes #1104.
legoktm committed Dec 10, 2024
1 parent e5402cd commit ec75f0b
Showing 9 changed files with 151 additions and 56 deletions.
9 changes: 5 additions & 4 deletions client/securedrop_client/api_jobs/downloads.py
Original file line number Diff line number Diff line change
@@ -12,7 +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.sdk import API, BaseError
from securedrop_client.sdk import API, BaseError, ProgressProxy
from securedrop_client.sdk import Reply as SdkReply
from securedrop_client.sdk import Submission as SdkSubmission
from securedrop_client.storage import (
@@ -56,6 +56,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:
"""
@@ -260,7 +261,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:
"""
@@ -316,7 +317,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:
@@ -375,7 +376,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:
2 changes: 2 additions & 0 deletions client/securedrop_client/gui/base/__init__.py
Original file line number Diff line number Diff line change
@@ -26,8 +26,10 @@
SvgPushButton,
SvgToggleButton,
)
from securedrop_client.gui.base.progress import FileDownloadProgressBar

__all__ = [
"FileDownloadProgressBar",
"ModalDialog",
"PasswordEdit",
"SDPushButton",
29 changes: 29 additions & 0 deletions client/securedrop_client/gui/base/progress.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
from PyQt5.QtCore import pyqtSignal
from PyQt5.QtWidgets import QProgressBar

from securedrop_client.utils import humanize_filesize


class FileDownloadProgressBar(QProgressBar):
"""
A progress bar for file downloads.
Use the signal to increment the progress; see the SDK's ProgressProxy type.
"""

# first value is bytes fetched, second is smoothed speed in bytes/second
signal = pyqtSignal((int, int))

def __init__(self, file_size: int) -> None:
super().__init__()
self.setObjectName("FileDownloadProgressBar")
self.setMaximum(file_size)
self.signal.connect(self.update_progress)

def update_progress(self, fetched: int, speed: int) -> None:
self.setValue(fetched)
percentage = int((fetched / self.maximum()) * 100)
# TODO: does this remove too much precision?
formatted_speed = humanize_filesize(speed) + "/s"
# TODO: localize this?
self.setFormat(f"{percentage}% | {formatted_speed}")
31 changes: 19 additions & 12 deletions client/securedrop_client/gui/widgets.py
Original file line number Diff line number Diff line change
@@ -83,13 +83,20 @@
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
from securedrop_client.gui.source import DeleteSourceDialog
from securedrop_client.logic import Controller
from securedrop_client.resources import load_css, load_icon, load_image, load_movie
from securedrop_client.sdk import ProgressProxy
from securedrop_client.storage import source_exists
from securedrop_client.utils import humanize_filesize

@@ -2579,6 +2586,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_progress = FileDownloadProgressBar(self.file.size)
self.download_progress.hide()
self.download_animation = load_movie("download_file.gif")
self.export_button = QPushButton(_("EXPORT"))
self.export_button.setObjectName("FileWidget_export_print")
@@ -2590,6 +2599,7 @@ 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)
file_options_layout.addWidget(self.export_button)
file_options_layout.addWidget(self.middot)
file_options_layout.addWidget(self.print_button)
@@ -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()
@@ -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("")
@@ -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, ProgressProxy(self.download_progress.signal)
)

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()
20 changes: 16 additions & 4 deletions client/securedrop_client/logic.py
Original file line number Diff line number Diff line change
@@ -60,7 +60,12 @@
)
from securedrop_client.crypto import GpgHelper
from securedrop_client.queue import ApiJobQueue
from securedrop_client.sdk import AuthError, RequestTimeoutError, ServerConnectionError
from securedrop_client.sdk import (
AuthError,
ProgressProxy,
RequestTimeoutError,
ServerConnectionError,
)
from securedrop_client.sync import ApiSync
from securedrop_client.utils import check_dir_permissions

@@ -825,7 +830,10 @@ 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: ProgressProxy | None = None,
) -> None:
if object_type == db.Reply:
job: ReplyDownloadJob | MessageDownloadJob | FileDownloadJob = ReplyDownloadJob(
@@ -842,6 +850,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
self.add_job.emit(job)

def download_new_messages(self) -> None:
@@ -974,12 +983,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:
"""
26 changes: 22 additions & 4 deletions client/securedrop_client/sdk/__init__.py
Original file line number Diff line number Diff line change
@@ -11,6 +11,7 @@

from securedrop_client.config import Config

from .progress import ProgressProxy
from .sdlocalobjects import (
AuthError,
BaseError,
@@ -131,7 +132,10 @@ def _rpc_target(self) -> list:
return [f"{target_directory}/debug/securedrop-proxy"]

def _streaming_download(
self, data: dict[str, Any], env: dict
self,
data: dict[str, Any],
env: dict,
progress: ProgressProxy,
) -> StreamedResponse | JSONResponse:
fobj = tempfile.TemporaryFile("w+b")

@@ -183,6 +187,7 @@ def _streaming_download(

fobj.write(chunk)
bytes_written += len(chunk)
progress.set_value(bytes_written)
logger.debug(f"Retry {retry}, bytes written: {bytes_written:,}")

# Wait for the process to end
@@ -303,6 +308,7 @@ def _send_json_request(
body: str | None = None,
headers: dict[str, str] | None = None,
timeout: int | None = None,
progress: ProgressProxy | None = None,
) -> StreamedResponse | JSONResponse:
"""Build a JSON-serialized request to pass to the proxy.
Handle the JSON or streamed response back, plus translate HTTP error statuses
@@ -328,9 +334,12 @@ def _send_json_request(
if self.development_mode:
env["SD_PROXY_ORIGIN"] = self.server

if not progress:
progress = ProgressProxy(None)

# Streaming
if stream:
return self._streaming_download(data, env)
return self._streaming_download(data, env, progress)

# Not streaming
data_str = json.dumps(data).encode()
@@ -360,6 +369,7 @@ def _send_json_request(
logger.error("Internal proxy error (non-streaming)")
raise BaseError(f"Internal proxy error: {error_desc}")

progress.set_value(len(response.stdout))
return self._handle_json_response(response.stdout)

def authenticate(self, totp: str | None = None) -> bool:
@@ -683,7 +693,11 @@ def delete_submission(self, submission: Submission) -> bool:
return False

def download_submission(
self, submission: Submission, path: str | None = None, timeout: int | None = None
self,
submission: Submission,
path: str | None = None,
timeout: int | None = None,
progress: ProgressProxy | None = None,
) -> tuple[str, str]:
"""
Returns a tuple of etag (format is algorithm:checksum) and file path for
@@ -712,6 +726,7 @@ def download_submission(
stream=True,
headers=self.build_headers(),
timeout=timeout or self.default_download_timeout,
progress=progress,
)

if isinstance(response, JSONResponse):
@@ -909,7 +924,9 @@ def get_all_replies(self) -> list[Reply]:

return result

def download_reply(self, reply: Reply, path: str | None = None) -> tuple[str, str]:
def download_reply(
self, reply: Reply, path: str | None = None, progress: ProgressProxy | None = None
) -> tuple[str, str]:
"""
Returns a tuple of etag (format is algorithm:checksum) and file path for
a given Reply object. This method requires a directory path
@@ -936,6 +953,7 @@ def download_reply(self, reply: Reply, path: str | None = None) -> tuple[str, st
stream=True,
headers=self.build_headers(),
timeout=self.default_request_timeout,
progress=progress,
)

if isinstance(response, JSONResponse):
49 changes: 49 additions & 0 deletions client/securedrop_client/sdk/progress.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import time

from PyQt5.QtCore import pyqtBoundSignal

SMOOTHING_FACTOR = 0.3


class ProgressProxy:
"""
Relay the current download progress over to the UI
We smooth the speed over time to reduce noise by using
an exponential moving average (motivated by https://stackoverflow.com/a/3841706).
The reported speed is 30% of the current speed and 70% of the previous
smoothed speed.
"""

def __init__(self, inner: pyqtBoundSignal | None) -> None:
self.inner = inner
self.ema_speed = 0
self.last_total_time: float | None = None
self.last_total_bytes = 0

def update_speed(self, value: int) -> None:
now = time.time()

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

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

self.last_total_time = now
self.last_total_bytes = value

def set_value(self, value: int) -> None:
if not self.inner:
return

self.update_speed(value)
self.inner.emit(value, self.ema_speed)
16 changes: 9 additions & 7 deletions client/tests/api_jobs/test_downloads.py
Original file line number Diff line number Diff line change
@@ -12,7 +12,7 @@
ReplyDownloadJob,
)
from securedrop_client.crypto import CryptoError, GpgHelper
from securedrop_client.sdk import BaseError
from securedrop_client.sdk import BaseError, Progress
from securedrop_client.sdk import Submission as SdkSubmission
from tests import factory

@@ -351,7 +351,9 @@ def test_FileDownloadJob_happy_path_no_etag(mocker, homedir, session, session_ma
gpg = GpgHelper(homedir, session_maker, is_qubes=False)
mock_decrypt = patch_decrypt(mocker, homedir, gpg, file_.filename)

def fake_download(sdk_obj: SdkSubmission, timeout: int) -> tuple[str, str]:
def fake_download(
sdk_obj: SdkSubmission, timeout: int, progress: Progress | None
) -> tuple[str, str]:
"""
:return: (etag, path_to_dl)
"""
@@ -389,7 +391,7 @@ def test_FileDownloadJob_happy_path_sha256_etag(
gpg = GpgHelper(homedir, session_maker, is_qubes=False)
mock_decrypt = patch_decrypt(mocker, homedir, gpg, file_.filename)

def fake_download(sdk_obj: SdkSubmission, timeout: int) -> tuple[str, str]:
def fake_download(sdk_obj: SdkSubmission, timeout: int, progress: Progress) -> tuple[str, str]:
"""
:return: (etag, path_to_dl)
"""
@@ -426,7 +428,7 @@ def test_FileDownloadJob_bad_sha256_etag(

gpg = GpgHelper(homedir, session_maker, is_qubes=False)

def fake_download(sdk_obj: SdkSubmission, timeout: int) -> tuple[str, str]:
def fake_download(sdk_obj: SdkSubmission, timeout: int, progress: Progress) -> tuple[str, str]:
"""
:return: (etag, path_to_dl)
"""
@@ -455,7 +457,7 @@ def test_FileDownloadJob_happy_path_unknown_etag(mocker, homedir, session, sessi

gpg = GpgHelper(homedir, session_maker, is_qubes=False)

def fake_download(sdk_obj: SdkSubmission, timeout: int) -> tuple[str, str]:
def fake_download(sdk_obj: SdkSubmission, timeout: int, progress: Progress) -> tuple[str, str]:
"""
:return: (etag, path_to_dl)
"""
@@ -494,7 +496,7 @@ def test_FileDownloadJob_decryption_error(
gpg = GpgHelper(homedir, session_maker, is_qubes=False)
mock_decrypt = mocker.patch.object(gpg, "decrypt_submission_or_reply", side_effect=CryptoError)

def fake_download(sdk_obj: SdkSubmission, timeout: int) -> tuple[str, str]:
def fake_download(sdk_obj: SdkSubmission, timeout: int, progress: Progress) -> tuple[str, str]:
"""
:return: (etag, path_to_dl)
"""
@@ -535,7 +537,7 @@ def test_FileDownloadJob_raises_on_path_traversal_attack(
api_client = mocker.MagicMock()
download_fn = mocker.patch.object(api_client, "download_reply")

def fake_download(sdk_obj: SdkSubmission, timeout: int) -> tuple[str, str]:
def fake_download(sdk_obj: SdkSubmission, timeout: int, progress: Progress) -> tuple[str, str]:
"""
:return: (etag, path-to-download)
"""
25 changes: 0 additions & 25 deletions client/tests/gui/test_widgets.py
Original file line number Diff line number Diff line change
@@ -3416,31 +3416,6 @@ def test_FileWidget_on_left_click_open(mocker, session, source):
fw.controller.on_file_open.assert_called_once_with(file_)


def test_FileWidget_set_button_animation_frame(mocker, session, source):
"""
Left click on download when file is not downloaded should trigger
a download.
"""
file_ = factory.File(source=source["source"], is_downloaded=False, is_decrypted=None)
session.add(file_)
session.commit()

controller = mocker.MagicMock()

fw = FileWidget(
file_,
controller,
mocker.MagicMock(),
mocker.MagicMock(),
mocker.MagicMock(),
0,
123,
)
fw.download_button = mocker.MagicMock()
fw.set_button_animation_frame(1)
assert fw.download_button.setIcon.call_count == 1


def test_FileWidget_update(mocker, session, source):
"""
The update method should show/hide widgets if file is downloaded

0 comments on commit ec75f0b

Please sign in to comment.