Skip to content
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.

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

Fix sync connect flow #1195

Merged
merged 5 commits into from
May 12, 2023
Merged

Fix sync connect flow #1195

merged 5 commits into from
May 12, 2023

Conversation

brindy
Copy link
Contributor

@brindy brindy commented May 11, 2023

Task/Issue URL: https://app.asana.com/0/0/1204398270301335/f
Tech Design URL:
CC:

Description:

Fixes the connect flow. No BSK changes.

Steps to test this PR:

  1. On iOS (or "another" device) turn off sync, then turn on sync, choose sync another device and have it show a code
  2. On macOS turn off sync, then turn on sync, choose sync another device and paste in the connect code from the other device
  3. The UI will close and the other device will connect. Eventually the new device will appear in the UI.
  4. Smoke test sync

Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

@brindy brindy requested a review from ayoy May 11, 2023 17:59
Copy link
Collaborator

@ayoy ayoy left a comment

Choose a reason for hiding this comment

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

Works great! 👏 Left a suggestion about Combine code, but I’m not strong on it.

@@ -269,7 +283,21 @@ extension SyncPreferences: ManagementDialogModelDelegate {
presentDialog(for: .deviceSynced)
} else if let connectKey = syncCode.connect {
// Unclear what the UX is supposed to be here given everything is happening on the other device
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just checking - has there been any progress on how the UX should be? Perhaps we can remove this comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remove this. Given this version reacts when a new device is added that might be good enough for now.

@MainActor
func presentShowOrEnterCodeDialog() {
Task { @MainActor in
let devicesAtStart = self.devices
showOrEnterCodeDevicesCancellable = self.$devices.sink { [weak self] value in
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose this should be equivalent:

showOrEnterCodeDevicesCancellable = self.$devices
    .removeDuplicates()
    .prefix(1)
    .sink { [weak self] _ in
        self?.managementDialogModel.endFlow()
    }

Using prefix(1) makes the subscription to complete after first emitted value, and as such it doesn’t have to be cancelled manually. Not sure if this version is more readable to people other than me, so if in doubt, keep your version :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not quite. The code checks that the array is different and then calls end flow if it is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

checking that the array is different is also there - handled by removeDuplicates

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh nice, i missed that! thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ended up having to drop the first event and then firing objectWillChange on the model, but it's much cleaner

@ayoy ayoy assigned brindy and unassigned ayoy May 12, 2023
@brindy brindy merged commit 6a631ff into develop May 12, 2023
@brindy brindy deleted the brindy/sync-fix-ios-connect branch May 12, 2023 16:06
samsymons added a commit that referenced this pull request May 23, 2023
* upstream_develop:
  sync device connected names (#1205)
  Autofill/subdomain matching (#1122)
  add-windows-browser-download-link-privacy-config (#1198)
  Update metadata
  Fix creating DMG from release workflow (#1200)
  Unit tests to cover sensitive areas of Burner Window (#1169)
  [Hackdays] Share via QR Code (#1177)
  Fix sync connect flow  (#1195)
  Update to BSK adding new remote message image (#1194)
  Set version to 1.39.0
  Update embedded files
  Update old Dax asset (#1193)
  WebKit find in page (#1188)
  modify manage bookmarks button (#1175)
  sync update device model / poll for devices (#1191)
Sign up for free to 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.

2 participants