-
Notifications
You must be signed in to change notification settings - Fork 42
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
Issue 389 sync #405
Issue 389 sync #405
Conversation
33ad9b3
to
48076ee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just one substantive comment - this is looking great !
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
class DownloadSubmissionJob(ApiJob): | ||
class MessageDownloadJob(ApiJob): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the plan still to inherit from a new common object to reduce code duplication with FileDownloadJob
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup! That's the plan: MessageDownloadJob
, FileDownloadJob
, and ReplyDownloadJob
will all inherit from a DownloadJob
class. I tried to keep the _decrypt_file
method generic enough so that we can move it into DownloadJob
. There should be a follow-up PR for this.
I just wanted to keep code changes small here since deleting the message_sync thread felt like a big change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filed #406 for this
class DownloadSubmissionJob(ApiJob): | ||
class MessageDownloadJob(ApiJob): | ||
|
||
CHUNK_SIZE = 4096 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(unused)
job.success_signal.connect(self.on_message_download_success, type=Qt.QueuedConnection) | ||
job.failure_signal.connect(self.on_message_download_failure, type=Qt.QueuedConnection) | ||
|
||
self.api_job_queue.enqueue(job) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
48076ee
to
9805dc8
Compare
9805dc8
to
adeb6dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
diff lgtm and works as advertised! 🥇
Description
Resolves #389
MessageDownloadJob
Test Plan