Convert backupToInbox method from Promise to Future by ivan-ushakov · Pull Request #590 · FlowCrypt/flowcrypt-ios

ivan-ushakov · 2021-10-02T10:14:49Z

Tests(delete all except exactly one):

Does not need tests (refactor only, docs or internal changes)

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

ivan-ushakov · 2021-10-02T10:16:25Z

I found that it will be much easy to have service methods implemented without any internal async. So we could decide how we run that method when we use it in chain.

ivan-ushakov · 2021-10-02T13:20:51Z

I did mistake with queue dispatching and will fix it today.

tomholub · 2021-10-02T21:38:45Z

@Kharchevskyi does this look good to you? Please have a look.

…thod

tomholub · 2021-10-04T12:12:09Z

Thank you Anton. I'm unsure if your comments still apply after the PR is reworked after my comments - let's see after the rework.

…thod

tomholub

for me, this is exactly what I was looking for. Thank you!

@Kharchevskyi could you please give this a second look now?

…thod

tomholub

I suppose Anton is busy. To me it looks good. I'll try the app as well, and merge it if works well.

…thod

tomholub · 2021-10-06T14:27:26Z

I may be able to arrange dropping support for iOS 14. That means we'd transition to iOS 15 async/await syntax and stop it with these workarounds. I'll have more news soon.

ivan-ushakov · 2021-10-08T17:57:12Z

Could I continue now with this issue and try async/await approach?

tomholub · 2021-10-09T05:57:40Z

Could I continue now with this issue and try async/await approach?

Yes - please try that.

…thod

ivan-ushakov · 2021-10-12T15:53:47Z

 let encryptedPrv = try self.core.generateKey(passphrase: passPhrase, variant: .curve25519, userIds: [userId])

- try awaitPromise(self.backupService.backupToInbox(keys: [encryptedPrv.key], for: self.user))
+ let semaphore = DispatchSemaphore(value: 0)


This method uses a lot of other methods and too big for the first step. So I just wait for async task.

Ok. Can do in another PR.

tomholub

LGTM. The real refactoring will start once everything is async/await - it will highlight superfluous code more obviously. @sosnovsky please have a look for Swift-specific issues, see if it looks ok to you.

tomholub · 2021-10-12T17:34:32Z

 let encryptedPrv = try self.core.generateKey(passphrase: passPhrase, variant: .curve25519, userIds: [userId])

- try awaitPromise(self.backupService.backupToInbox(keys: [encryptedPrv.key], for: self.user))
+ let semaphore = DispatchSemaphore(value: 0)


Ok. Can do in another PR.

sosnovsky

Updated code looks good for me - async/await made it much more clear and easy to read, looking forward to next steps of async refactoring 🚀

tomholub · 2021-10-13T07:23:36Z

@ivan-ushakov anything left to do for this PR to be able to mark it as ready?

ivan-ushakov · 2021-10-13T17:57:26Z

@ivan-ushakov anything left to do for this PR to be able to mark it as ready?

I want to do some tests today.

ivan-ushakov · 2021-10-13T18:29:27Z

+ Task {
+ do {
+ try await composeMessageService.encryptAndSend(message: message, threadId: input.threadId)
+ handleSuccessfullySentMessage()


Since every view controller is marked with @MainActor we could simply call methods from task and they will be invoked on the main thread.

…ub.com:FlowCrypt/flowcrypt-ios into feature/issue-543-convert-backupToInbox-method

tomholub · 2021-10-14T21:36:56Z

@Kharchevskyi does this look good to you? If the tests pass I'll merge it, but if there is something to change, we let me know and we'll get it addressed in the next PR.

Convert backupToInbox method from Promise to Future
32de56d

ivan-ushakov requested a review from tomholub October 2, 2021 10:14

Convert backupToInbox method from Promise to Future
120e380

ivan-ushakov commented Oct 2, 2021
View reviewed changes

Comment threadFlowCrypt/Extensions/CombineExtensions.swift Outdated
Comment threadFlowCrypt/Controllers/Setup/SetupGenerateKeyViewController.swift Outdated

Convert backupToInbox method from Promise to Future
9e1edbc

tomholub requested a review from Kharchevskyi October 2, 2021 21:38

tomholub requested changes Oct 3, 2021
View reviewed changes

Comment threadFlowCrypt/Controllers/Setup/SetupGenerateKeyViewController.swift Outdated

tomholub marked this pull request as draft October 3, 2021 15:43

Merge branch 'master' into feature/issue-543-convert-backupToInbox-me…
491421f
…thod

Kharchevskyi reviewed Oct 4, 2021
View reviewed changes

Comment threadFlowCrypt/Controllers/Settings/Backup/Backups Option Scene/BackupOptionsViewController.swift Outdated
Comment thread...pt/Controllers/Settings/Backup/Backups Seleckt Key Scene/BackupSelectKeyViewController.swift Outdated

ivan-ushakov added 2 commits October 4, 2021 19:36

Merge branch 'master' into feature/issue-543-convert-backupToInbox-me…
d190276
…thod

Convert backupToInbox method from Promise to Future
8b44b42

tomholub approved these changes Oct 4, 2021
View reviewed changes

Merge branch 'master' into feature/issue-543-convert-backupToInbox-me…
dc4042d
…thod

tomholub previously approved these changes Oct 5, 2021
View reviewed changes

Merge branch 'master' into feature/issue-543-convert-backupToInbox-me…
73b0c49
…thod

tomholub marked this pull request as ready for review October 5, 2021 14:19

tomholub enabled auto-merge (squash) October 5, 2021 14:22

tomholub mentioned this pull request Oct 5, 2021
Replace Promise with async/await #404
Closed

Kharchevskyi suggested changes Oct 5, 2021
View reviewed changes

Comment threadFlowCrypt/Functionality/Services/Backup Services/BackupService.swift Outdated
Comment thread...pt/Controllers/Settings/Backup/Backups Seleckt Key Scene/BackupSelectKeyViewController.swift Outdated

tomholub requested a review from sosnovsky October 5, 2021 19:10

tomholub marked this pull request as draft October 5, 2021 20:07

auto-merge was automatically disabled October 5, 2021 20:07
Pull request was converted to draft

tomholub reviewed Oct 6, 2021
View reviewed changes

Comment threadFlowCrypt/Functionality/Services/Backup Services/BackupService.swift Outdated

tomholub mentioned this pull request Oct 7, 2021
set minimum required iOS 15 #626
Closed

ivan-ushakov added 2 commits October 11, 2021 20:10

Merge branch 'master' into feature/issue-543-convert-backupToInbox-me…
315a4a9
…thod

Convert backupToInbox method from Promise to Future
9dd2157

ivan-ushakov dismissed tomholub’s stale review via 9dd2157October 12, 2021 15:52

ivan-ushakov commented Oct 12, 2021
View reviewed changes

Comment threadFlowCrypt.xcodeproj/project.pbxproj

ivan-ushakov commented Oct 12, 2021
View reviewed changes

Comment threadFlowCryptAppTests/Core/FlowCryptCoreTests.swift

ivan-ushakov commented Oct 12, 2021
View reviewed changes

Comment threadFlowCrypt/Core/Core.swift

Convert backupToInbox method from Promise to Future
0c41f90

tomholub reviewed Oct 12, 2021
View reviewed changes

sosnovsky previously approved these changes Oct 12, 2021
View reviewed changes

ivan-ushakov marked this pull request as ready for review October 13, 2021 18:26

ivan-ushakov commented Oct 13, 2021
View reviewed changes

tomholub mentioned this pull request Oct 14, 2021
share secrets file with ivan #671
Closed

tomholub enabled auto-merge (squash) October 14, 2021 09:09

github-actionsBotand others added 3 commits October 14, 2021 11:11

Branch was auto-updated.
e989e66

Convert backupToInbox method from Promise to Future
06502d7

Merge branch 'feature/issue-543-convert-backupToInbox-method' of gith…
0cbc1c0
…ub.com:FlowCrypt/flowcrypt-ios into feature/issue-543-convert-backupToInbox-method

ivan-ushakov dismissed sosnovsky’s stale review via 0cbc1c0October 14, 2021 19:33

ivan-ushakovand others added 2 commits October 14, 2021 22:35

Convert backupToInbox method from Promise to Future
630e94e

Branch was auto-updated.
dd3466b

tomholub approved these changes Oct 14, 2021
View reviewed changes

tomholub mentioned this pull request Oct 14, 2021
further async/await refactoring #677
Closed

tomholub disabled auto-merge October 14, 2021 21:56

tomholub merged commit 16918ed into masterOct 14, 2021

tomholub deleted the feature/issue-543-convert-backupToInbox-method branch October 14, 2021 21:57

Uh oh!

Conversation

ivan-ushakov commented Oct 2, 2021• edited by tomholub Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

To be filled by reviewers

Uh oh!

ivan-ushakov commented Oct 2, 2021

Uh oh!

ivan-ushakov commented Oct 2, 2021

Uh oh!

Uh oh!

Uh oh!

tomholub commented Oct 2, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tomholub commented Oct 4, 2021

Uh oh!

tomholub left a comment

Choose a reason for hiding this comment

Uh oh!

tomholub left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tomholub commented Oct 6, 2021

Uh oh!

ivan-ushakov commented Oct 8, 2021

Uh oh!

tomholub commented Oct 9, 2021

Uh oh!

ivan-ushakovOct 12, 2021

Choose a reason for hiding this comment

Uh oh!

tomholubOct 12, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tomholub left a comment

Choose a reason for hiding this comment

Uh oh!

tomholubOct 12, 2021

Choose a reason for hiding this comment

Uh oh!

sosnovsky left a comment

Choose a reason for hiding this comment

Uh oh!

tomholub commented Oct 13, 2021

Uh oh!

ivan-ushakov commented Oct 13, 2021

Uh oh!

Uh oh!

Uh oh!

ivan-ushakovOct 13, 2021

Choose a reason for hiding this comment

Uh oh!

tomholub commented Oct 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ivan-ushakov commented Oct 2, 2021•
edited by tomholub
Loading