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

Remove Settings bundle #1379

Merged
merged 2 commits into from
Feb 10, 2016
Merged

Remove Settings bundle #1379

merged 2 commits into from
Feb 10, 2016

Conversation

kchitalia
Copy link
Contributor

No description provided.

@@ -15,11 +14,11 @@
4FD6C49F1B755242002F9F90 /* RootViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4FD6C49C1B755242002F9F90 /* RootViewController.swift */; };
4FD6C4A01B755242002F9F90 /* InitialViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4FD6C49D1B755242002F9F90 /* InitialViewController.swift */; };
C77FD628010EF64B9B2C4363 /* libPods-__NativeSwiftTemplateAppName__.a in Frameworks */ = {isa = PBXBuildFile; fileRef = 9737303E047BCEC3E2EC6AE5 /* libPods-__NativeSwiftTemplateAppName__.a */; };
E1CE60881C655FE6007D6DCB /* SalesforceSDKAssets.xcassets in Resources */ = {isa = PBXBuildFile; fileRef = E1CE60871C655FE6007D6DCB /* SalesforceSDKAssets.xcassets */; };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding SDK Assets as part of the template app.

@wmathurin
Copy link
Contributor

Settings.bundle and Images.xcassets are in shared/resources.
And the template apps simply have pointers to them (and instead of having copies), we should do the same for SalesforceSDKAssets.xcassets.

@wmathurin
Copy link
Contributor

Also is the settings.bundle removed from all the sample apps?

@kchitalia
Copy link
Contributor Author

@wmathurin I will modify the way I have copied the SalesforceSDKAssets.xcassets.

I removed the settings bundle from RestAPIExplorer, SmartSyncExplorer, SmartSyncExplorerHybrid, AccountEditor, NoteSync. Am I missing any sample app?

Also, I was able to create a sample app using forceios, all 3 flavors - native, native_swift, and native_react and see my changes for the login view controller.

@wmathurin
Copy link
Contributor

Those are all the sample apps we have in the iOS repo.

@@ -0,0 +1,23 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

As @wmathurin mentioned, we probably want these to be symlinks to a common shared folder like Settings.bundle is/was.

@bhariharan
Copy link
Contributor

I believe we have some special logic in SalesforceSDKManager for host changing from Settings.bundle, which can be removed, right @khawkins?

@khawkins
Copy link
Contributor

khawkins commented Feb 8, 2016

Two places I see where the Settings bundle is still referenced:

  1. build/build.xml, via the sdkAppSettingsBundleName property. It's not being used, though, so you should just be able to remove that property.
  2. In the configuration of the SalesforceHybridSDKTestApp target of the hybrid SDK.

@khawkins
Copy link
Contributor

khawkins commented Feb 8, 2016

So as @bhariharan mentions, there are some other areas of the SDK that we should clean up, in correlation with the removal of support for the Settings bundle login settings.

SFUserAccountManager

There are sections of support around the login host, for querying that value from the Settings app. In particular, [SFUserAccountManager appSettingsLoginHost] is called by [SFUserAccountManager loginHost] and [SFUserAccountManager updateLoginHost], to leverage any changes through that configuration.

SalesforceSDKManager

[SFUserAccountManager updateLoginHost] ripples up to SalesforceSDKManager, which checks for login host changes and the logout switch being set, in processLogoutAndUserSwitchSettings. This whole method can go away, since those settings will not otherwise be useful. This in turn impacts the handleAppForeground: and launch methods, but hopefully that refactor should be straightforward.

@kchitalia
Copy link
Contributor Author

@khawkins Thanks for the pointers. I will refactor the code in those two places.

@khawkins
Copy link
Contributor

khawkins commented Feb 8, 2016

@kchitalia Sure thing, let me know if you run into any questions with that, as I know my description is a bit open-ended. I forgot one related method: [SFUserAccountManager updateAppSettingsLoginHost:] can also go away. Basically, anything that uses the constants:

  • kAppSettingsLoginHost
  • kAppSettingsLoginHostCustomValue
  • kAppSettingsLoginHostIsCustom

Up to and including those constants, can go away. And to be clear, [SFUserAccountManager updateLoginHost] can also go away. As can the SFLoginHostUpdateResult object definition, and any consumers. I'll let you know if I come across more. :) But the basic exercise is to just verify whether or not any of that related stuff is being used anywhere that seems like it still needs to be, and otherwise remove it all.

@khawkins khawkins mentioned this pull request Feb 10, 2016
@khawkins khawkins merged commit 1d58b16 into forcedotcom:unstable Feb 10, 2016
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.

4 participants