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

Reflect the default values in Settings.bundle in UserDefaults. #3

Closed

Conversation

griffin-stewie
Copy link
Contributor

Problems

In the current implementation, the value retrieved from UserDefaults is not True even if it appears True in Settings.app.

Solution

In this pull request, we give the Default Value of Settings.bundle as the Default Value of UserDefaults.

Testing

I tested this on my actual device iPad Pro.

@CLAassistant
Copy link

CLAassistant commented Jan 26, 2021

CLA assistant check
All committers have signed the CLA.

@niw
Copy link
Contributor

niw commented Jan 28, 2021

I see, this is nice finding! I haven’t noticed this behavior about default value in settings bundle.

However, this means we need to sync with two values in two places, and this is extra feature in the example.
I am wondering therefore, if it’s actually better to change Root.plist in Settings.bundle to have default value NO instead?

@griffin-stewie
Copy link
Contributor Author

I agree with you. Should I close this pull request and try to change the default value to NO in another pull request?

@niw
Copy link
Contributor

niw commented Jan 28, 2021

You can reuse this pull request (branch) to change Root.plist instead, or close this and create new one if you preferred.

@griffin-stewie
Copy link
Contributor Author

Alright, I created new pull request #11 so I close this. Thank you.

@griffin-stewie griffin-stewie deleted the fix/register_defaults branch January 29, 2021 14:05
niw pushed a commit that referenced this pull request Jan 31, 2021
**Problems**

In the current implementation, the value retrieved from UserDefaults is not True even if it appears True in Settings.app.

**Solution**

I suggested this #3 . We discussed and decided that it would be simpler to set the default value to `NO`, so I changed the default value in Settings.bundle to `NO`.

**Testing**

I tested this on my actual device iPad Pro.

GitHub Pull Request: #11
#11

JIRA Issues: IOS-94620

Differential Revision: https://phabricator.twitter.biz/D610563
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.

None yet

3 participants