ability to send encrypted attachments#505
Conversation
tomholub
changed the title
…ny file from Files app)t; Making Core.composeMail api call in background not to block UI for spinner; Refactored from Promises to Combine;
….com:FlowCrypt/flowcrypt-ios into feature/issue-196-ability-to-send-attachment
tomholub
left a comment
tomholub
left a comment
There was a problem hiding this comment.
Preliminarly this is is - In particular the encryption/sending looks correct. Adding some comments.
| case .text: return self.textNode(with: nodeHeight) | ||
| case .subjectDivider: return DividerCellNode() | ||
| } | ||
| case (.main, 2): |
There was a problem hiding this comment.
I wonder if the whole tableNode should be in some sort of decorator class? It looks very UI-heavy with not much business logic. What do you think @Kharchevskyi ?
There was a problem hiding this comment.
If so, then that would apply to the whole extension ComposeViewController: ASTableDelegate, ASTableDataSource { section
There was a problem hiding this comment.
I agree, we may create DataSource entity for this screen, @Kharchevskyi let's discuss that when it's convenient
FlowCrypt/Functionality/Services/Compose Message Service/ComposeMessageAttachment.swift
Outdated
Show resolved
Hide resolved
| let type: String | ||
| } | ||
|
|
||
| extension ComposeMessageAttachment { |
There was a problem hiding this comment.
Maybe I'm misunderstanding this whole class, but is it really necessary to handle it with different methods type by type? There may be many different file types. I know you mentioned something about using a "simpler method for file picking" so I suppose maybe this is the side effect of that.
In the future, will we will have to handle file types individually like this?
As an example, on browser extension, the browser picks up a file and it doesn't care what type of file it is. The name, data, and mime type string is all it cares.
There was a problem hiding this comment.
The problem is that ImagePickerDelegate returns different dictionaries with data. E.g. a photo taken from camera doesn't have a name, path and asset but photo that taken from library has all those fields.
FlowCrypt/Functionality/Services/Compose Message Service/ComposeMessageService.swift
Show resolved
Hide resolved
|
Converting to draft while this is being discussed & until adjustments done. Otherwise this is ready for me to start testing I suppose? |
|
yes, you may test it, I will answer on comments asap and apply fixes tomorrow. Thanks! |
tomholub
left a comment
tomholub
left a comment
There was a problem hiding this comment.
Well done as far as user test - I was able to send other types of files (video in my case). The sent file worked, I'm able to read it on the other end (browser extension).
There are a few things we'll later have to improve in the future, other than what I've mentioned above. All of these 4 are for followup issues for the future, since at this stage we're just focusing on the basics.
- if it's a bigger file, it takes a while: there should be a progress bar. We've done something similar on Android. The first 30% of the progress bar are taken up by encryption (which we have no way to track progress of) and the last 70% is upload of the message to Gmail servers, which you should be able to track progress of.
- on dark theme, the attachment names are barely readable (also when receiving)
- the
.pgpfilename should not be visible in that list. So you don't have to alter the filename, after all (the encryption method should be already transparently renaming them) - later I should have a way to remove attachment from compose screen if I changed my mind before sending
That's functionality-wise. I cannot comment too much on code, maybe @sosnovsky do you want to have a look?
….com:FlowCrypt/flowcrypt-ios into feature/issue-196-ability-to-send-attachment
|
|
||
| private func appendAttachmentIfAllowed(_ attachment: ComposeMessageAttachment) { | ||
| let totalSize = contextToSend.attachments.reduce(0, { $0 + $1.size }) | ||
| if totalSize > GeneralConstants.Global.attachmentSizeLimit { |
There was a problem hiding this comment.
Shouldn't this be totalSize + attachment.size > GeneralConstants.Global.attachmentSizeLimit ?
I checked code - looks great, it helped me to better understand app structure 👍 |
|
Since remaining improvements are filed as separate issues, I've merged it. |
4 participants
This PR adds the ability to send encrypted attachments
close #196
Tests (delete all except exactly one):
To be filled by reviewers
I have reviewed that this PR... (tick whichever items you personally focused on during this review):