Implement missing parts of UserDefaults and fix consistency issues #1408

millenomi · 2018-01-25T17:54:20Z

This supersedes the patch at #1403.

While implementing persistent domains as a follow-up to volatile domains (and the arguments domain), I've found several issues in UserDefaults that required a whole bit of rethinking of the way this class works. It has been substantially changed.

This patch:

Implements volatile domains
Implements the arguments domain as a volatile domain that is searched at the appropriate time
Implements persistent domains
Changes UserDefaults so that all methods that return Any values return value types instead of reference types. On Linux, where we don't (yet?) have Foundation reference <-> value type bridging, we have to pick one of the two, and I feel it is more likely and desirable for an end user to want the value type rather than the reference type.
Changes the internals of the class so that any reference types are converted to value types ASAP where possible, and adopt _SwiftValue instead of trying to do ad-hoc conversions.

- Volatile domains. - UserDefaults.argumentDomain, a particular volatile domain, including arguments parsing. WIP: Requires testing.

- Add persistent domains - Guarantee that all values returned are value types.

millenomi · 2018-01-25T17:56:28Z

@swift-ci Please test.

Catfish-Man · 2018-01-25T19:18:17Z

Foundation/UserDefaults.swift

-            NotificationCenter.default.post(name: UserDefaults.didChangeNotification, object: self)
+            _ = defaults.synchronize()
+
+            if removedAny {


This notification is posted unconditionally in the ObjC implementation

Will change.

Catfish-Man · 2018-01-25T19:19:31Z

Foundation/UserDefaults.swift

        }
    }

    open func synchronize() -> Bool {
-        return CFPreferencesAppSynchronize(kCFPreferencesCurrentApplication)
+        return CFPreferencesAppSynchronize(suite?._cfObject ?? kCFPreferencesCurrentApplication)


Something I did in the ObjC implementation is add CF SPI for “synchronize everything” and call it here, since a ubiquitous pattern of misuse is synchronizing standard user defaults when you set something else.

… ah. hm. I'm inclined to have a follow-up on this one.

Catfish-Man · 2018-01-25T19:21:54Z

Foundation/UserDefaults_Arguments.swift

+        while index < count - 1 { // We're looking for pairs, so stop at the second-to-last argument.
+            let current = arguments[index]
+            let next = arguments[index + 1]
+            if current.hasPrefix("-") && !next.hasPrefix("-") {


I bet we have subtle unicode behavior handling differences due to Swift string vs NSString, but I don’t think that’s a problem we need to solve here, at least not right now

As far as I can see here, this allows a subset of all defaults we allow on Darwin — namely, we skip all defaults that start with a '-' code point but where that code point forms a grapheme, e.g. '-⃣'. It still allows basically all defaults keys of actual interest, though. But yep.

… all defaults arguments.

… only fetch values that are objects — it makes a new warning happen on Darwin that I can't silence, but it will work on both platforms and without warnings on Linux.

millenomi · 2018-01-25T20:19:16Z

@swift-ci please test

…ing this bit of code up.

… the test.

- Move `_parseArguments` into UserDefaults.swift.

millenomi · 2018-01-25T23:34:06Z

OK, this is ready for review and test (once CI comes back up.) @Catfish-Man @parkera

millenomi · 2018-01-26T01:23:46Z

@swift-ci please test

alblue · 2018-01-26T11:33:51Z

@swift-ci please test

Catfish-Man

A few review comments, but overall this looks fine

Catfish-Man · 2018-02-03T00:31:10Z

Foundation/UserDefaults.swift

        }
        return 0
    }
+
    open func bool(forKey defaultName: String) -> Bool {


One minor incompatibility between this and what darwin does, which we probably don’t want/need to fix: NSUserDefaults historically coerced values to int/bool/float using different logic from CF. I changed it so that subclasses maintain the old behavior (for compatibility reasons), but un-subclassed NSUserDefaults always uses the CF logic.

I haven’t checked whether this implements the NSUserDefaults version or the CFPrefs version, but I think we should use the CFPrefs behavior.

I'll double-check. If they're different, there will be a follow-up.

Catfish-Man · 2018-02-03T00:35:15Z

Foundation/UserDefaults.swift

+        let value = bridgeFromNSCFTypeIfNeeded(nonbridgedValue)
+
+        if let value = value as? [Any] {
+            for innerValue in value {


The swift tinkerer in me wants to say .lazy.filter { _isValueAllowed($0) }.count > 0, but tbh I’m not sure it’s worth it :)

I will reply with my standard reply, which is 'if it turns out to be a hot spot'.

Catfish-Man · 2018-02-03T00:40:51Z

Foundation/UserDefaults.swift

+        return _dictionaryRepresentation(searchingOutsideOfSuite: true)
+    }
+
+    private func _dictionaryRepresentation(searchingOutsideOfSuite: Bool) -> [String: Any] {


searchingOutsideOfSuite seems like a slightly odd name for this. “Suite” is a very fuzzy term in NSUD, but to the extent that it means anything I tend to think of it as referring to a search list, which would generally include registered prefs, etc...

I’m guessing the sense you mean here is “outside of the set of domains matching this identifier”, which is a subset of the search list for that identifier?

Yup. What about _dictionaryRepresentation(includingOnlySuiteNameDomains:)?

We chatted privately and agreed on _dictionaryRepresentation(includingVolatileDomains:).

…on Linux; fix them. Fix naming per review.

millenomi · 2018-02-03T03:47:13Z

@swift-ci Please test

millenomi · 2018-02-03T03:47:48Z

@swift-ci Please test and merge

millenomi added 7 commits January 25, 2018 09:53

Implement two missing features from UserDefaults:

9d47821

- Volatile domains. - UserDefaults.argumentDomain, a particular volatile domain, including arguments parsing. WIP: Requires testing.

More tests.

ca58521

Test using a heterogeneous dictionary.

c18cb07

Cut the tests that don’t work on CI; there’ll be a follow-up for them.

18fb5f5

Add persistent domains code.

dd3a9fa

Substantially rewrite UserDefaults to:

1aff228

- Add persistent domains - Guarantee that all values returned are value types.

Move the check for NSNumber just above the common types.

0995aa9

millenomi changed the title ~~Ud persistent domains~~ Implement missing parts of UserDefaults and fix consistency issues Jan 25, 2018

millenomi mentioned this pull request Jan 25, 2018

UserDefaults: Implement volatile domains, argument domain #1403

Closed

Catfish-Man reviewed Jan 25, 2018

View reviewed changes

This was mistakenly relying on a Darwin-style bridged cast. So…:

ac6a3f6

… only fetch values that are objects — it makes a new warning happen on Darwin that I can't silence, but it will work on both platforms and without warnings on Linux.

millenomi added 3 commits January 25, 2018 13:02

Fix the _other_ place where 'as AnyObject' was being used by refactor…

fae057c

…ing this bit of code up.

Fire the notification unconditionally, and check that it is posted in…

2134642

… the test.

Ninja does not quite support this extension arrangement.

c60c771

- Move `_parseArguments` into UserDefaults.swift.

Catfish-Man approved these changes Feb 3, 2018

View reviewed changes

These two casts were implicitly relying on bridging. They don’t work …

1bac2b3

…on Linux; fix them. Fix naming per review.

swift-ci merged commit a17cdd0 into swiftlang:master Feb 3, 2018

Implement missing parts of UserDefaults and fix consistency issues #1408

Implement missing parts of UserDefaults and fix consistency issues #1408

Uh oh!

Conversation

millenomi commented Jan 25, 2018

Uh oh!

millenomi commented Jan 25, 2018

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!

millenomi commented Jan 25, 2018

Uh oh!

millenomi commented Jan 25, 2018

Uh oh!

millenomi commented Jan 26, 2018

Uh oh!

alblue commented Jan 26, 2018

Uh oh!

Catfish-Man 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!

millenomi commented Feb 3, 2018

Uh oh!

millenomi commented Feb 3, 2018

Uh oh!

Uh oh!