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

Add iCloud syncing #229

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

StevenSorial
Copy link

@StevenSorial StevenSorial commented Feb 1, 2020

Adds #228

Usage:

// Prefs.swift
extension DefaultsKeys {
  var username: DefaultsKey<String?> { return .init("username") }
  var launchCount: DefaultsKey<Int> { return .init("launchCount", defaultValue: 0) }
}

// AppDelegate.swift

func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplication.LaunchOptionsKey: Any]?) -> Bool {
  Defaults.syncKeys([
    { $0.launchCount },
    { $0.username }
  ])
  return true
}

// ViewController.swift

@IBAction
func stopSyncingUsername(_ sender: UIButton) {
  Defaults.syncKeys([
    { $0.launchCount },
  ])
}

Obviously the worst part of this implementation is syncKeys syntax and that it takes an array of closures. I tried to make it accept an array of KeyPath or even PartialKeyPath but failed so this was the best alternative.
The other approach that I could have taken is for it to be without parameters and use Mirror internally:

func syncKeys() {
  let keys = Mirror(reflecting: keyStore).children.compactMap { $0 as? RawKeyRepresentable }.filter { $0.isSynced }.map { $0._key }
  DefaultsSyncer.shared.syncedKeys = Set(keys)
}

but i don't like using Mirror because i feel it's very hacky

The whole implementation is not perfect but it's acceptable given the current structure.

@sunshinejr I'm willing to take another run at it if it's not acceptable for you.

Edit: the Mirror implementation does not even work because it ignores computed properties but I'm sure we can runtime-hack it to get the keys list. still not preferred.

@StevenSorial StevenSorial force-pushed the icloud-support branch 2 times, most recently from 3554f6d to f142d29 Compare February 1, 2020 05:50
@StevenSorial StevenSorial marked this pull request as ready for review February 2, 2020 14:33
@StevenSorial
Copy link
Author

StevenSorial commented Feb 2, 2020

Changed it so each DefaultsAdapter has its own syncer.

Edit:
This change supports use cases where there are multiple DefaultsAdapters in the same app. This has one caveat where if there is a DefaultsKey with the same key in two or more DefaultsAdapters they would overwrite each other in iCloud. this is a very rare behavior and should be clarified in the Readme/Docs.

@StevenSorial
Copy link
Author

StevenSorial commented Feb 3, 2020

Got PartialKeyPath working 🎉🎉
It was a Swift type inference bug.
SR-5667 - forums

Usage:

// Prefs.swift
extension DefaultsKeys {
  var username: DefaultsKey<String?> { return .init("username") }
  var launchCount: DefaultsKey<Int> { return .init("launchCount", defaultValue: 0) }
}

// AppDelegate.swift

func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplication.LaunchOptionsKey: Any]?) -> Bool {
  Defaults.startSyncing(for: \DefaultsKeys.launchCount, \DefaultsKeys.username)
  return true
}

// ViewController.swift

@IBAction
func stopSyncingUsername(_ sender: UIButton) {
  Defaults.stopSyncing(for: \DefaultsKeys.username)
}

@IBAction
func stopSyncingCompletely(_ sender: UIButton) {
  Defaults.stopSyncingAll()
}

@sunshinejr I think I'm now satisfied with this implementation. 😄

@sunshinejr
Copy link
Owner

Hey @Stevenmagdy, the implementation (and the API!) looks really clean, thank you! Can you please:

  • add checks on watchOS (since the NSUbiquitousKeyValueStore is not available on watchOS)
  • add some tests for syncing?

I'll try to play with the sync as soon as possible as well!

@StevenSorial StevenSorial force-pushed the icloud-support branch 4 times, most recently from 63f6c29 to 801999e Compare February 4, 2020 16:01
@StevenSorial
Copy link
Author

StevenSorial commented Feb 4, 2020

@sunshinejr Thanks for the reply

  • add checks on watchOS (since the NSUbiquitousKeyValueStore is not available on watchOS)

Done.

  • add some tests for syncing?

I'm having a hard time imagining how to test syncing. I'm very inexperienced in testing, let alone sync testing. I would appreciate any help.

Unrelated question: what is UserDefaults doing on Linux? 🤔

@StevenSorial
Copy link
Author

hey @sunshinejr, I think I'm done with this PR. I tried different approaches for testing with no luck. I hope this feature will get implemented one day 🙏, either building on this PR or from scratch.

@sunshinejr
Copy link
Owner

hey @Stevenmagdy, sorry for the delay but I'm currently really busy and have a hard time to find few minutes to play with this - I might try early next week but cannot promise anything

don't worry, though, I really love the API and so we'll try to get this one in as soon as possible! and thank you for all the hard work! 🙇

@StevenSorial StevenSorial force-pushed the icloud-support branch 3 times, most recently from b9274db to f9f7714 Compare March 26, 2020 19:23
@rromanchuk
Copy link

Although i'm going to keep my NSUbiquitousKeyValueStore usage with https://github.com/ArtSabintsev/Zephyr this PR gave me some clues. All i'm trying to achieve is getting an array of all keys defined in DefaultsKeys, instead of manually duplicating and hardcoding string key names.

I'd like to just be able to pass Defaults.keys that returns [String]. I want to be a lazy developer and not have to specify specific keys, as i can't think of a context where i wouldn't want these to sync. It also defends against developer error when a key is added or removed, i don't want to babysit AppDelegate's sync list.

Zephyr.addKeysToBeMonitored(keys: ["i-dont-want-to-manage-these, "keys"] )
Zephyr.sync(keys: ["i-dont-want-to-manage-these, "keys"])`

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.

5 participants