Site Settings in SwiftUI: Initial

[kean]

HACK week: update Site Settings to use SwiftUI

To test:

• Add to AppDelegate:

        DispatchQueue.main.asyncAfter(deadline: .now() + .seconds(1)) {
            let settings = SiteSettingsView(blog: {
                let context = ContextManager.shared.mainContext
                return Blog.lastUsedOrFirst(in: context)!
            }())
            let host = UIHostingController(rootView: settings)
            let nav = UINavigationController(rootViewController: host)
            self.window?.rootViewController?.present(nav, animated: true)
        }

• Change the site title
• Test that the title is updated

Regression Notes

1. Potential unintended areas of impact: Site Settings screen
2. What I did to test those areas of impact (or what existing automated tests I relied on): manual tests
3. What automated tests I added (or what prevented me from doing so): n/a

PR submission checklist:

• I have completed the Regression Notes.
• I have considered adding unit tests for my changes.
• I have considered adding accessibility improvements for my changes.
• I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

UI Changes testing checklist:

• Portrait and landscape orientations.
• Light and dark modes.
• Fonts: Larger, smaller and bold text.
• High contrast.
• VoiceOver.
• Languages with large words or with letters/accents not frequently used in English.
• Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
• iPhone and iPad.
• Multi-tasking: Split view and Slide over. (iPad)

[kean]

I will remove it once iOS 14 support is dropped.

[wpmobilebot]

📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.

	App Name	WordPress Alpha
	Configuration	Release-Alpha
	Build Number	pr20838-71a7368
	Version	22.6
	Bundle ID	org.wordpress.alpha
	Commit	71a7368
	App Center Build	WPiOS - One-Offs #5976

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

[wpmobilebot]

📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.

	App Name	Jetpack Alpha
	Configuration	Release-Alpha
	Build Number	pr20838-71a7368
	Version	22.6
	Bundle ID	com.jetpack.alpha
	Commit	71a7368
	App Center Build	jetpack-installable-builds #5004

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

[mokagio]

Could you please use reverse DNS keys for localized strings? We lack a way to enforce them at the moment, but the rationale is that they help us disambiguate duplicated strings used in different contexts. More at #19028

[kean]

Yes, that is a good point. I was reluctant to do that in the same PR because these are the existing keys from settings, and changing them will require updating all the localizable strings files (presumable we want to keep the localizations?).

I'm not sure how to approach it. I'm not removing the SiteSettingsViewController just yet, and I'd rather avoid updating it to use the new keys. You also have to be careful in case other screens use the same keys since they are not namespaced.

I think it may be worth updating the keys at the end if I have time by the end of the week. I'm trying to reduce the scope because it's already a big task.

[mokagio]

these are the existing keys from settings, and changing them will require updating all the localizable strings files (presumable we want to keep the localizations?).

So I'm guessing these are duplicated here for usage convenience in SwiftUI? E.g. this is the "original" title localized string:

WordPress-iOS/WordPress/Classes/ViewRelated/Blog/Site Settings/SiteSettingsViewController.m

Line 124 in e4db21b

self.navigationItem.title = NSLocalizedString(@"Settings", @"Title for screen that allows configuration of your blog/site settings.");

For what is worth, localizations are cheap to change, and it wouldn't be a problem, IMHO, if this added more strings to localize.

For context, we generate new .strings via custom genstrings wrappers during code freeze. These are sent to GlotPress for translations, and every time we build a binary for deployment we download up-to-date strings.

You also have to be careful in case other screens use the same keys since they are not namespaced.

Exactly. Updating to use a reverse-DNS key will create a unique^* localized string, e.g.:

Suggested change

	static let title = NSLocalizedString("Settings", comment: "Title for screen that allows configuration of your blog/site settings.")
	static let title = NSLocalizedString("sitesettings.title", value: "Settings", comment: "Title for screen that allows configuration of your blog/site settings.")

Whereas currently the "Settings" key+value is shared by four instances:

WordPress-iOS/WordPress/Resources/en.lproj/Localizable.strings

Lines 7725 to 7730 in e4db21b

	/* Link to plugin's Settings
	Section title
	The menu item to select during a guided tour.
	Title for screen that allows configuration of your blog/site settings.
	Title for the Jetpack Security Settings Screen */
	"Settings" = "Settings";

---

* - genstrings is capable of detecting duplicated key usages, but doesn't fail when that happens. We have it in the roadmap to at some point address that.

[kean]

There aren't going to be too many new strings, so I'll add new ones, as you suggest. Updated: https://github.com/wordpress-mobile/WordPress-aiOS/pull/20838/commits/b16704dbc6fc41516b9bc71a2b39c3ef6983ad41.

[momo-ozawa]

Works as described!

Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-06-14.at.16.01.13.mp4

[momo-ozawa]

Wdyt of injecting the service?

Suggested change

	init(blog: Blog) {
	init(blog: Blog, service: BlogService = BlogService(coreDataStack: ContextManager.shared)) {

[kean]

Good point – gotta keep is testable. Updated.

I'm prioritizing updating the UI. The original implementation had no tests, but I'll try to add some (and make it more testable).