Skip to content

Feature/issue 783 unlimited attachment size - #893

Merged
tomholub merged 6 commits into
masterfrom
feature/issue-783-unlimited-attachment-size
Nov 3, 2021
Merged

Feature/issue 783 unlimited attachment size#893
tomholub merged 6 commits into
masterfrom
feature/issue-783-unlimited-attachment-size

Conversation

@sosnovsky

@sosnovskysosnovsky commented Nov 2, 2021

Copy link
Copy Markdown
Collaborator

This PR allows loading of attachments over 5mb and adds progress spinner for download process.

close#783


Tests(delete all except exactly one):

  • Difficult to test (explain why) - it's mostly UI change

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

  • addresses the issue it closes (if any)
  • code is readable and understandable
  • is accompanied with tests, or tests are not needed
  • is free of vulnerabilities
  • is documented clearly and usefully, or doesn't need documentation

@tomholubtomholub left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

code looks good so far

@sosnovsky
sosnovsky marked this pull request as ready for review November 2, 2021 10:48
@sosnovsky

Copy link
Copy Markdown
CollaboratorAuthor

It'll be nice to have decrypt progress percentage in the future too, as it takes almost the same time as download for big files.

@sosnovsky

Copy link
Copy Markdown
CollaboratorAuthor

@tomholub currently I have only #812 task assigned to me, can you please check on what tasks should I work after this one?

@tomholub

tomholub commented Nov 2, 2021

Copy link
Copy Markdown
Collaborator

It'll be nice to have decrypt progress percentage in the future too, as it takes almost the same time as download for big files.

Download is network bound and decryption is cpu bound. You could do both at the same time, and it should take half the time overall, if you do streaming decryption. #230 But it's not simple to do across the Swift/JS boundary. But it's possible.

@tomholub

Copy link
Copy Markdown
Collaborator

@tomholub currently I have only #812 task assigned to me, can you please check on what tasks should I work after this one?

will do

@sosnovsky

Copy link
Copy Markdown
CollaboratorAuthor

It'll be nice to have decrypt progress percentage in the future too, as it takes almost the same time as download for big files.

Download is network bound and decryption is cpu bound. You could do both at the same time, and it should take half the time overall, if you do streaming decryption. #230 But it's not simple to do across the Swift/JS boundary. But it's possible.

Got it, it'll be a great addition for app's functionality and will make large emails loading much faster.

@tomholub

Copy link
Copy Markdown
Collaborator

I'd like you to debug/finalize #854 . How about merging sequence? I suppose it may be easier to merge the big one first and then rebase this onto that, but we could do it the other way around if you think it's easier. Up to you.

@sosnovsky

Copy link
Copy Markdown
CollaboratorAuthor

I agree - it should be easier to merge #854 to master first, and then rebase it into this PR, as there is less changes here. I'll check #854 now

@tomholub

Copy link
Copy Markdown
Collaborator

Tested, looks great

@tomholub
tomholub enabled auto-merge (squash) November 3, 2021 07:26
@tomholub

Copy link
Copy Markdown
Collaborator

@sosnovsky good to merge once conflicts addressed 👍

# Conflicts:
#	FlowCrypt/Controllers/Compose/ComposeViewController.swift
#	FlowCrypt/Controllers/Msg/MessageViewController.swift
#	FlowCrypt/Functionality/Mail Provider/Message Provider/MessageService.swift
@sosnovsky

Copy link
Copy Markdown
CollaboratorAuthor

@tomholub I merged master changes to this branch - the only issue was with fetchMsg function, it didn't execute async code for fetching mail size. I fixed it by using async instead of Promise.
This fix also affected Imap class (to make it conform to updated MessageProvider protocol) - I updated fetchMsg function there too 763ac4b#diff-8012f2f1a5073946e65bdfc88fd17e2b9ddad872c955b7cf6adfd7a1621bf345R12

@tomholubtomholub left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍

Comment on lines 157 to 161

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A detail - I'd prefer formatting like:

processedMessage =tryawait serviceActor.fetchDecryptAndRenderMsg(
message: input.objMessage,
path: input.path,
progressHandler:{[weak self] state inself?.handleFetchProgress(state: state)})

Else all of the inputs have no space due to large indentation.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Okay, I'll use such formatting in the next commits

Comment on lines -570 to -572

private func validateMessage(rawMimeData: Data, with passPhrase: String) {
handleFetchProgress(state: .decrypt)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I like explicit returns for the clarity :-) but don't insist.

@tomholub
tomholub merged commit 03e6f35 into masterNov 3, 2021
@tomholub
tomholub deleted the feature/issue-783-unlimited-attachment-size branch November 3, 2021 15:39
Sign up for freeto 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.

allow opening messages of unlimited size + add download progress

2 participants

@sosnovsky@tomholub