Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Move Settings.bundle into included iosapp #12607

Merged
merged 1 commit into from
Sep 19, 2018

Conversation

captainbarbosa
Copy link
Contributor

@captainbarbosa captainbarbosa commented Aug 10, 2018

Removes the settings target and gets rid of the included example Settings.bundle from this project, which would be moved to the ios-sdk-examples project in mapbox/ios-sdk-examples#204. Closes #12554.

This will not require a changelog entry.

Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

@captainbarbosa captainbarbosa force-pushed the nb-move-settings-12554 branch from d625777 to 57847fa Compare August 27, 2018 19:40
@captainbarbosa captainbarbosa requested review from julianrex and friedbunny and removed request for friedbunny August 27, 2018 19:40
@captainbarbosa
Copy link
Contributor Author

@1ec5 I went ahead with your suggestion and moved the Settings.bundle into iosapp as well as removed the unused Settings target, please let me know if I've missed something.

Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

In addition to these changes, package.sh will need to be updated to extract Settings.bundle from the built iosapp bundle in the build products.

Additionally, it may be necessary to update the localization addition process to include manually adding a localization of Root.strings – not sure if Xcode automatically detects that strings file as being localizable when it lies within a Settings.bundle.

@1ec5 1ec5 changed the title [ios] Remove Settings.bundle from this project Convert Settings.bundle into copied resource Sep 1, 2018
@1ec5 1ec5 added the build label Sep 1, 2018
@captainbarbosa captainbarbosa changed the title Convert Settings.bundle into copied resource Move Settings.bundle into included iosapp Sep 6, 2018
@captainbarbosa
Copy link
Contributor Author

@julianrex @friedbunny and I spoke about the direction of this PR. Since package.sh currently doesn't build iosapp, having it do so just to copy the Settings.bundle seems inefficient. We agreed that the Settings.bundle should remain in the iosapp but should not be packaged with the framework for now.

@captainbarbosa captainbarbosa requested review from friedbunny and removed request for friedbunny September 6, 2018 21:52
@captainbarbosa captainbarbosa requested review from friedbunny and removed request for friedbunny September 6, 2018 22:03
@1ec5
Copy link
Contributor

1ec5 commented Sep 6, 2018

Change these two lines:

cp -rv ${PRODUCTS}/${BUILDTYPE}-iphoneos/Settings.bundle ${OUTPUT}
cp -rv ${PRODUCTS}/${BUILDTYPE}-iphonesimulator/Settings.bundle ${OUTPUT}

to:

cp -rv platform/ios/app/Settings.bundle ${OUTPUT}

and nothing changes about the final built SDK.

@friedbunny friedbunny added this to the ios-v4.5.0 milestone Sep 6, 2018
@friedbunny
Copy link
Contributor

@captainbarbosa Looks like Root.strings is still thought to exist in the SDK section of the Xcode project:

screen shot 2018-09-06 at 6 53 18 pm

@friedbunny
Copy link
Contributor

@captainbarbosa If we’re going to keep the localizations, please check that they work in their new home.

@captainbarbosa
Copy link
Contributor Author

captainbarbosa commented Sep 7, 2018

@friedbunny I tested on-device by changing the default language and checking the telemetry alert in iosapp:

@friedbunny
Copy link
Contributor

@captainbarbosa The strings in that dialog are handled by the maps SDK directly (and will appear regardless of what’s in an app’s Settings.bundle).

Settings bundles are where apps define what extra settings will appear in their panels in the system’s settings (“Settings.app”), alongside typical permissions (location, etc).

@captainbarbosa
Copy link
Contributor Author

@friedbunny Gotcha - I'm now noticing that the settings do localize the first time the language is changed, but then remain stuck on the first localization when the language is changed again. Not sure what the cause is but I'm seeing this on the device and with the simulator.

(After changing the default language from English to French and then to Japanese, the Settings remain localized to French.)

@friedbunny
Copy link
Contributor

friedbunny commented Sep 7, 2018

I'm now noticing that the settings do localize the first time the language is changed, but then remain stuck on the first localization when the language is changed again.

@captainbarbosa That also happens in master. It looks like Settings.app doesn’t use the Base localization in our Settings.bundle when it switches away from another localization and back to English. Explicitly adding an English localization for Settings.bundle (with the same strings as Base) fixes this issue. Disabling the Base localization (and moving it to English) also works.

@1ec5 Does this behavior seem like a bug to you, or are we doing something incorrectly?

References

@1ec5
Copy link
Contributor

1ec5 commented Sep 8, 2018

I'm now noticing that the settings do localize the first time the language is changed, but then remain stuck on the first localization when the language is changed again.

Did you force-quit Settings between changing languages? If I’m not mistaken, the Settings application does cache its contents until you close it. What happens in other applications that specify localized settings pages?

@captainbarbosa
Copy link
Contributor Author

@1ec5 force quitting the settings app doesn’t seem to work and other apps localize their custom settings as expected, so this only seems to be occurring with this project.

@captainbarbosa
Copy link
Contributor Author

@friedbunny I added the English localization and its back to working as expected.

@friedbunny
Copy link
Contributor

Did this ↓ end up being necessary, @captainbarbosa?

Additionally, it may be necessary to update the localization addition process to include manually adding a localization of Root.strings – not sure if Xcode automatically detects that strings file as being localizable when it lies within a Settings.bundle.

@captainbarbosa
Copy link
Contributor Author

@friedbunny No, it was not necessary - @1ec5 and went through it together and localizations seemed to take with no issues 👍

[ios] Remove copying of Settings.bundle from packaging script

[ios] Move Settings bundle under ios/app in file system

[ios] Copy bundle to iosapp in packaging script

[ios] Remove stranded Root.strings from SDK

[ios] Add English localization to example Settings.bundle
@captainbarbosa captainbarbosa merged commit a991eec into master Sep 19, 2018
@captainbarbosa captainbarbosa deleted the nb-move-settings-12554 branch September 19, 2018 21:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
build iOS Mapbox Maps SDK for iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants