Example of Combine usage by Kharchevskyi · Pull Request #306 · FlowCrypt/flowcrypt-ios

Kharchevskyi · 2021-05-06T20:52:41Z

This is an example for #290 of how we can use Combine without huge changing of our codebase.
Please take a look on some improvements which can be added with using Combine

operation will be canceled if user close controller (just an example)
retry operation

tomholub · 2021-06-14T19:11:18Z

Thank you - I'll have a closer look next week

tomholub · 2021-07-09T15:43:22Z

Could you please produce an alternative PR that uses Combine promises? I think that could be the best compromise.

FlowCrypt/Functionality/Mail Provider/Message Gateway/GmailService+send.swift

tomholub

Thanks! more questions

tomholub · 2021-07-12T11:39:05Z

FlowCrypt/Controllers/Compose/ComposeViewController.swift

-    private func encryptAndSendMessage() -> Promise<Bool> {
-        Promise<Bool> { [weak self] () -> Bool in
-            guard let self = self else { return false }
+    private func prepareMessage() -> Result<Data, SendMessageError> {


Got it, the futures are similar to promises. Now I understand it it a bit better.

What is this particular method - it returns Result<Data, SendMessageError>. How is that different from Future<...>?

This is return synchronously.
In this case it's just a "helper function"

tomholub · 2021-07-12T11:42:26Z

FlowCrypt/Controllers/Compose/ComposeViewController.swift

+        case .success(let data):
+            self.messageSender.sendMail(mime: data)
+                .retry(3)
+                .sink{ [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)


For this demo, please replicate original behavior - without retry or other features not present in the original.

Is the switch prepareMessage() { the only way to handle a Result? Is there some other possible pattern like:

let preparedMessage = prepareMessage() if preparedMessage.isSuccess { } else { }

?

Please check latest commit. Implemented similar to what we have in original code.

tomholub · 2021-07-12T11:43:23Z

FlowCrypt/Controllers/Compose/ComposeViewController.swift

-        }.catch(on: .main) { [weak self] error in
-            self?.showAlert(error: error, message: "compose_error".localized)
+        case .success(let data):
+            self.messageSender.sendMail(mime: data)


Does self.messageSender.sendMail have to be in the result handling code? Could it be more similar to original code, where encryptAndSendMessage took care of sending as well?

tomholub

I can accept using Combine futures this way, it looks good. Thank you for the demo.

We can clean up this PR to settle on a pattern, then merge it.

When converging to Combine, please don't editorialize the code at the same time. Please try to match the code structure 1:1 else it's very difficult to see if original functionality was preserved. If you notice things to refactor, if possible or unless it's really tiny, please file issues with examples for later instead of doing it in the same step.

Please do self-review of the code after you commit it, you yourself will discover a lot of the things that I'm discovering and can fix it instead of having me raise the concern (like SendMessageError which is unused and errors dropped silently - a self review on GitHub would make this very obvious).

Going forward, whenever you'd like me to look at code, please go to Files changed first yourself and go through your changes as a reviewer would. See possible problems when comparing original and new code, and adjust them if needed. Then if you made changes, do another such review until you're satisfied with the changes as they show on GitHub. Then click Finish your review, choose the Comment type and say LGTM. I also do this when I submit my own PRs, it's a way to make sure to not unnecessarily burden the reviewer with pointing out small fixes that are obvious, and instead allow the reviewer to focus on the bigger picture. Thanks!

tomholub · 2021-07-14T07:15:04Z

FlowCrypt/Controllers/Compose/ComposeViewController.swift

+        guard recipients.isNotEmpty else {
+            return
+        }


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.

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.

tomholub · 2021-07-14T07:17:20Z

FlowCrypt/Controllers/Compose/ComposeViewController.swift

+        guard let text = self.contextToSend.message else {
+            return


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.

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

tomholub · 2021-07-14T07:17:38Z

FlowCrypt/Controllers/Compose/ComposeViewController.swift

+        guard let myPubKey = self.dataService.publicKey else {
+            return
        }


Also here bring back alert?

tomholub · 2021-07-14T07:18:24Z

FlowCrypt/Controllers/Compose/ComposeViewController.swift

+        guard let allRecipientPubs = getPubKeys(for: recipients) else {
+            return
+        }


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

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

tomholub · 2021-07-14T07:22:59Z

FlowCrypt/Controllers/Compose/ComposeViewController.swift

+            .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)


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?

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.

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

tomholub · 2021-07-14T07:30:57Z

FlowCrypt/Controllers/Compose/ComposeViewController.swift

        // temporary disable search contacts - https://github.com/FlowCrypt/flowcrypt-ios/issues/217
        // showScopeAlertIfNeeded()
+
+        cancellable.forEach { $0.cancel() }


What does this do?

Kharchevskyi · 2021-07-14T15:22:14Z

This PR, as title says is just an example of Combine usage. It doesn't meant to be reviewed at all.

tomholub · 2021-07-14T15:46:44Z

This PR, as title says is just an example of Combine usage. It doesn't meant to be reviewed at all.

I get it, it was only meant as a demonstration.

Demonstration succeeded - can we bring the PR from its current state to a mergeable one, so that it's not wasted effort? To begin work to get rid of google Promise library.

Please also help me by answering the questions above.

Kharchevskyi · 2021-07-14T16:05:40Z

Sure, thanks. On my way with answers
Are we going to use only Future from Combine? or we are free to use Combine itself?

tomholub · 2021-07-14T16:24:44Z

Initially, only Future for minimal changes in code structure. then when done, we will do a similar demo like this, for other Combine functionality where it makes most sense. Then if it looks good, start using that too.

…

On Wednesday, July 14, 2021, Anton Kharchevskyi ***@***.***> wrote: Sure, thanks. On my way with answers Are we going to use only Future from Combine? or we are free to use Combine itself? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#306 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABQDZEP3MP5DGQZB7LRIRHDTXWYV5ANCNFSM44H4WYIA> .

-- -- Tom James Holub <http://holub.me/>

Kharchevskyi · 2021-07-19T18:39:25Z

@tomholub can you please close this PR if favour of #400
(was hard to resolve all conflicts after pulling master)

tomholub · 2021-07-21T13:16:10Z

closing in favor of #400

Example of Combine usage

00d7dfd

Kharchevskyi requested a review from tomholub May 6, 2021 20:52

Kharchevskyi commented Jul 12, 2021

View reviewed changes

FlowCrypt/Functionality/Mail Provider/Message Gateway/GmailService+send.swift Show resolved Hide resolved

tomholub reviewed Jul 12, 2021

View reviewed changes

Example of Combine

41e6a0c

tomholub reviewed Jul 14, 2021

View reviewed changes

Kharchevskyi mentioned this pull request Jul 19, 2021

Improve send message checks #399

Closed

tomholub closed this Jul 21, 2021

tomholub deleted the feature/test-combine branch November 9, 2021 16:26

Conversation

Kharchevskyi commented May 6, 2021

Uh oh!

tomholub commented Jun 14, 2021

Uh oh!

tomholub commented Jul 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

tomholub left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomholub left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kharchevskyi commented Jul 14, 2021

Uh oh!

tomholub commented Jul 14, 2021

Uh oh!

Kharchevskyi commented Jul 14, 2021

Uh oh!

tomholub commented Jul 14, 2021 via email

Uh oh!

Kharchevskyi commented Jul 19, 2021

Uh oh!

tomholub commented Jul 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tomholub commented Jul 9, 2021 •

edited

Loading