Skip to content

Example of Combine usage #306

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

Closed
Kharchevskyi wants to merge 2 commits into from
Closed

Example of Combine usage #306

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
00d7dfd
Example of Combine usage
Kharchevskyi May 6, 2021
41e6a0c
Example of Combine
Kharchevskyi Jul 13, 2021
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
101 changes: 53 additions & 48 deletions FlowCrypt/Controllers/Compose/ComposeViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import AsyncDisplayKit
import FlowCryptCommon
import FlowCryptUI
import Promises
import Combine

final class ComposeViewController: TableNodeViewController {
struct Recipient {
Expand Down Expand Up @@ -44,6 +45,13 @@ final class ComposeViewController: TableNodeViewController {
case subject, subjectDivider, text
}

private enum SendMessageError: Error {
case internalError(String)
case missedSender
case noPubKeyForRecipient
}

private var cancellable = Set<AnyCancellable>()
private let messageSender: MessageGateway
private let notificationCenter: NotificationCenter
private let dataService: DataServiceType & KeyDataServiceType
Expand Down Expand Up @@ -111,6 +119,8 @@ final class ComposeViewController: TableNodeViewController {

// temporary disable search contacts - https://github.com/FlowCrypt/flowcrypt-ios/issues/217
// showScopeAlertIfNeeded()

cancellable.forEach { $0.cancel() }
Copy link
Collaborator

@tomholub tomholub Jul 14, 2021

Choose a reason for hiding this comment

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

What does this do?

}

deinit {
Expand Down Expand Up @@ -211,57 +221,52 @@ extension ComposeViewController {

showSpinner("sending_title".localized)

Promise<Bool> { [weak self] in
try await(self!.encryptAndSendMessage())
}.then(on: .main) { [weak self] sent in
if sent { // else it must have shown error to user
self?.handleSuccessfullySentMessage()
}
}.catch(on: .main) { [weak self] error in
self?.showAlert(error: error, message: "compose_error".localized)
}
encryptAndSendMessage()
}

private func encryptAndSendMessage() -> Promise<Bool> {
Promise<Bool> { [weak self] () -> Bool in
guard let self = self else { return false }

let recipients = self.contextToSend.recipients

guard recipients.isNotEmpty else {
assertionFailure("Recipients should not be empty. Fail in checking")
return false
}

guard let text = self.contextToSend.message else {
assertionFailure("Text and Email should not be nil at this point. Fail in checking")
return false
}

let subject = self.input.subjectReplyTitle
?? self.contextToSend.subject
?? "(no subject)"

guard let myPubKey = self.dataService.publicKey else {
self.showAlert(message: "compose_no_pub_sender".localized)
return false
}

guard let allRecipientPubs = self.getPubKeys(for: recipients) else {
return false
}

let encrypted = self.encryptMsg(
pubkeys: allRecipientPubs + [myPubKey],
subject: subject,
message: text,
to: recipients.map { $0.email }
)

try await(self.messageSender.sendMail(mime: encrypted.mimeEncoded))

return true
private func encryptAndSendMessage() {
let recipients = self.contextToSend.recipients

guard recipients.isNotEmpty else {
return
}
Comment on lines +230 to +232
Copy link
Collaborator

@tomholub tomholub Jul 14, 2021

Choose a reason for hiding this comment

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

Should add back error handling, not just return and ignore. If it's input validation error from user, then we render an alert and return. If it's a nilcheck that was already supposed to be checked earlier and we're just sanity-checking our code, than that's a coding error and should result in a crash, not be ignored.

Copy link
Contributor Author

@Kharchevskyi Kharchevskyi Jul 14, 2021

Choose a reason for hiding this comment

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

I would suggest to use implementation with Result<SendableMessage, SendMessageError> as it was in previous commit for all this checks instead of returning true/false, and use asynchronous Future only for send message operation.


guard let text = self.contextToSend.message else {
return
Comment on lines +234 to +235
Copy link
Collaborator

@tomholub tomholub Jul 14, 2021

Choose a reason for hiding this comment

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

Should add back error handling, not just return and ignore. If it's input validation error from user, then we render an alert and return. If it's a nilcheck that was already supposed to be checked earlier and we're just sanity-checking our code, than that's a coding error and should result in a crash, not be ignored.

Copy link
Contributor Author

@Kharchevskyi Kharchevskyi Jul 14, 2021

Choose a reason for hiding this comment

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

Skip this part for this example, will return back alert and proper handling

}

let subject = self.input.subjectReplyTitle
?? self.contextToSend.subject
?? "(no subject)"

guard let myPubKey = self.dataService.publicKey else {
return
}
Comment on lines +242 to 244
Copy link
Collaborator

@tomholub tomholub Jul 14, 2021

Choose a reason for hiding this comment

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

Also here bring back alert?


guard let allRecipientPubs = getPubKeys(for: recipients) else {
return
}
Comment on lines +246 to +248
Copy link
Collaborator

@tomholub tomholub Jul 14, 2021

Choose a reason for hiding this comment

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

Unsure how was this handled before, looks like we should improve it, but that would be for another PR.

Copy link
Contributor Author

@Kharchevskyi Kharchevskyi Jul 14, 2021

Choose a reason for hiding this comment

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

Will use same logic as it was before. This method will return true or false


let encrypted = self.encryptMsg(
pubkeys: allRecipientPubs + [myPubKey],
subject: subject,
message: text,
to: recipients.map { $0.email }
)

self.messageSender
.sendMail(mime: encrypted.mimeEncoded)
.sink(
receiveCompletion: { [weak self] result in
guard case .failure(let error) = result else {
return
}
self?.showAlert(error: error, message: "compose_error".localized)
},
receiveValue: { [weak self] in
self?.handleSuccessfullySentMessage()
})
.store(in: &cancellable)
Comment on lines +259 to +269
Copy link
Collaborator

@tomholub tomholub Jul 14, 2021

Choose a reason for hiding this comment

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

This PR looks good, and we could actually clean up and merge it.

What does the .sink represent?

I figured receiveCompletion triggers on both error and success, and you are filtering for error only? Isn't there a more convenient method like receiveFailure?

What does the .store(in: &cancellable) do?

Copy link
Contributor Author

@Kharchevskyi Kharchevskyi Jul 14, 2021

Choose a reason for hiding this comment

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

sink is used to "subscribe" to stream of events.
receive completion will be triggered in case of error and in case when stream is finished

 .sink(
   receiveCompletion: { completion in
      switch completion {
      case .failure(let error): print("Error \(error)")
      case .finished: print("Publisher is finished")
      }
   }
   ...

In this example with Future(promise), there will be only 1 value, which can be accessed in receiveValue: closure and 1 receiveCompletion.
But in cases with other publishers, receiveValue closure can be called multiple times but receiveCompletion - will be called only once when publisher finished or failed

There is sink(receiveValue:) function, but there is no just receiveError.

.store(in: &cancellable).
In case we started some long going request and left the screen cancellable will frees up any allocated resources and also will stop side effects.
For example user start fetching message with bad internet connection and was waiting for few seconds. Then taped back and left the screen, cancelable will terminate this execution and free up resources.

Copy link
Contributor Author

@Kharchevskyi Kharchevskyi Jul 14, 2021

Choose a reason for hiding this comment

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

All subscriptions should be stored in cancellable to prevent their execution if view controller/service is deallocated

}

private func getPubKeys(for recepients: [Recipient]) -> [String]? {
Expand Down
1 change: 0 additions & 1 deletion FlowCrypt/Controllers/SideMenu/Menu/MyMenuViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

import AsyncDisplayKit
import FlowCryptUI
import Promises

final class MyMenuViewController: ASDKViewController<ASDisplayNode> {
private enum Constants {
Expand Down
13 changes: 7 additions & 6 deletions FlowCrypt/Functionality/Mail Provider/Message Gateway/GmailService+send.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,16 @@
//

import Foundation
import Promises
import GoogleAPIClientForREST
import GTMSessionFetcher
import Combine

extension GmailService: MessageGateway {
func sendMail(mime: Data) -> Promise<Void> {
Promise { (resolve, reject) in
func sendMail(mime: Data) -> Future<Void, Error> {
Future { promise in
guard let raw = GTLREncodeBase64(mime) else {
return reject(GmailServiceError.messageEncode)
promise(.failure(GmailServiceError.messageEncode))
return
}

let gtlMessage = GTLRGmail_Message()
Expand All @@ -29,10 +30,10 @@ extension GmailService: MessageGateway {

self.gmailService.executeQuery(querySend) { (_, _, error) in
if let error = error {
reject(GmailServiceError.providerError(error))
promise(.failure(GmailServiceError.providerError(error)))
return
}
resolve(())
promise(.success(()))
}
}
}
Expand Down
17 changes: 12 additions & 5 deletions FlowCrypt/Functionality/Mail Provider/Message Gateway/Imap+send.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,22 @@
//

import Foundation
import Promises
import Combine

extension Imap: MessageGateway {
func sendMail(mime: Data) -> Promise<Void> {
Promise { [weak self] resolve, reject in
guard let self = self else { return reject(AppErr.nilSelf) }
func sendMail(mime: Data) -> Future<Void, Error> {
Future { [weak self] promise in
guard let self = self else { return promise(.failure(AppErr.nilSelf)) }

self.smtpSess?
.sendOperation(with: mime)
.start(self.finalizeVoid("send", resolve, reject, retry: { self.sendMail(mime: mime) }))
.start { error in
if let error = error {
promise(.failure(error))
} else {
promise(.success(()))
}
}
}
}
}
3 changes: 2 additions & 1 deletion FlowCrypt/Functionality/Mail Provider/Message Gateway/MessageGateway.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@

import Foundation
import Promises
import Combine

protocol MessageGateway {
func sendMail(mime: Data) -> Promise<Void>
func sendMail(mime: Data) -> Future<Void, Error>
}
2 changes: 1 addition & 1 deletion FlowCrypt/Functionality/Services/Backup Services/BackupService.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ extension BackupService: BackupServiceType {
atts: attachments
)
let backupEmail = try self.core.composeEmail(msg: message, fmt: .plain, pubKeys: nil)
try await(messageSender.sendMail(mime: backupEmail.mimeEncoded))
// try await(messageSender.sendMail(mime: backupEmail.mimeEncoded))
}
}

Expand Down