Jetpack Focus: Enable iOS14+ Stats Widgets and Intents for Jetpack by staskus · Pull Request #19479 · wordpress-mobile/WordPress-iOS

staskus · 2022-10-18T11:29:55Z

Part of #19460

Description

This PR makes >=iOS 14 "Today", "This Week" and "This Year" Stats widgets available for the Jetpack app.

Solution

These are the main tasks accomplished to make Stats widgets work:

Create JetpackStatsWidgets target by duplicating WordPressStatsWidgets.

Change display name, bundle identifiers, schemes, and target dependencies

Split part of Constants.h into separate AppConstants.h for each app. It allows defining different UserDefault keys and different file names for saving widget information. Since Jetpack app and WordPress app share the same app group , we need to differentiate these parts.
Create JetpackIntents by duplicating WordPressIntents. Perform similar steps as with JetpackStatsWidgets. It allows supporting site selection in the Edit widget menu.
Add migration. If the user has Jetpack app already installed and logged in, we need to detect this case and initialize widgets after the app's launch.
Have different localization for empty widget states

Testing instructions

Case 1: Fresh install

Install Jetpack
Open Home Screen
Add Today, This Week, This Year widgets - they should be showing Stats for the default site
Click on the widgets - they should open Jetpack app and take to Stats screen
Long click on widgets and tap "Edit Widget" - a list of available sites should show and changing it refreshes the widget.
Come back to the app and logout - the widget should be showing "Please login to Jetpack..." message

Case 2: WordPress app with widgets exist

Install WordPress, log in and add widgets
Install Jetpack, log in to a different account and add widgets
Jetpack app should be showing different information to WordPress app

Case 3: Migration

Install an older version of Jetpack and log in
Install and launch a new version of Jetpack
Add widgets, they should work in all the scenarios as in Case 1

Regression Notes

Potential unintended areas of impact

Breaking WordPress stats widgets

What I did to test those areas of impact (or what existing automated tests I relied on)

Manual testing to ensure that WordPress widgets remain unaffected when interacting with Jetpack widgets

What automated tests I added (or what prevented me from doing so)

None

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.

TBD:

I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Images & Videos

Jetpack.-.schemes.MP4

Jetpack.-.add.widgets.mov

- Duplicating WordPressStatsWidget target - Changing Display Name, Bundle Identifier for all the configurations, Info.plist file, Code Signing Entitlements, WPCOM_SCHEME value

… for different apps - Jetpack and WordPress apps share the same AppGroup - We need to differentiate between the keys of UserDefaults and file names to save different Widget preferences for Jetpack and WordPress apps

…jetpack

Duplicate and change displayName, bundleIdentifiers, set target dependencies, change schemes

…s widgets After updating from an old version of Jetpack app that doesn't support Stats widgets to a newer version, required UserDefault flags and other information is not already saved and cannot be accessed by Jetpack Stats Widgets. Detect if these flags are not set and initialize widgets after the app is launched.

wpmobilebot · 2022-10-18T12:13:31Z

You can test the changes in WordPress from this Pull Request by:

Clicking here or scanning the QR code below to access App Center
Then installing the build number pr19479-7755426 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

staskus · 2022-10-18T15:15:40Z

📝 I noticed there're some issues with provisioning profiles in the buildkite builds. Working on it.

guarani

Thanks for the PR @staskus! I went through the three test cases and things worked well.

Product bundle identifier

From what I see, the new intent target for Jetpack is what lets the user pick their site while configuring the widget. The product bundle identifier uses com.automattic. for debug/release build configs, but com.jetpack. for alpha and internal. Was this intentional? I also see a potential typo of com.jetpac. for the alpha build config:

Duplicate target dependencies

Looking at the Jetpack target's dependencies, a couple of entries look duplicated (some might pre-date this PR, but I haven't checked). I tested without these and the app seems to work fine.

Potential duplication

It feels like adding the widgets to Jetpack duplicated things (user default keys, localized strings, etc). I understand though that these need to be unique and for some things (esp. localized strings), it might not be desirable or practical to make these dynamic.

Open questions

A couple of open questions which I haven't found time to answer today:

Have the changes been tested on iOS 13 (e.g. to ensure that the app handles gracefully the lack of widget support in iOS 13?)
Are the intents exposed to Siri and if so does Siri differentiate properly between the apps?
Should the widgets and their UI (e.g. for site selection) use a green accent/tint color instead of the current blue color?

I'll hold off approving since the product bundle identifier question looks to be a blocker, although the rest are not.

guarani · 2022-10-18T21:54:02Z

+				);
+				MTL_ENABLE_DEBUG_INFO = NO;
+				MTL_FAST_MATH = YES;
+				PRODUCT_BUNDLE_IDENTIFIER = com.jetpac.alpha.JetpackIntents;


This should be com.jetpack.alpha (missing "k")

staskus · 2022-10-19T07:05:27Z

Thanks for the review!

The product bundle identifier uses com.automattic. for debug/release build configs, but com.jetpack. for alpha and internal. Was this intentional?

It is intentional from my side. This is how code signing is set up for the main Jetpack target as well as new extensions we're creating.

Looking at the Jetpack target's dependencies, a couple of entries look duplicated

Good observation, thanks! I explicitly added them there and then they were automatically added again when I included extensions into Frameworks, Libraries, and Embedded Content. I will remove duplications.

Potential duplication. It feels like adding the widgets to Jetpack duplicated things (user default keys, localized strings, etc). I understand though that these need to be unique and for some things (esp. localized strings), it might not be desirable or practical to make these dynamic.

I would call it configuration instead of duplication. We need certain things to be different widget targets.

We could be explicit about it and have WidgetConfiguration.h instead of AppConstants.h to keep widget-related configuration constants. It could be accessed via AppConfiguration.widgets that we already use, keeping everything in one place.
We could use xcconfig files, to be explicit about all the differences and similarities between targets - including bundle identifiers AND constants. Maybe it could be a future consideration.
Of course, as you mention, we could also make configuration dynamic. For example., use something like \(AppConfiguration.userDefaultsPrefix)UserDefaultsKey) keys so they would be determined when the extension/app is running and not hard coded. Use %@ in localized strings, where %@ would be the value of the app name. However, I don't think that's desirable, since such differences would be scattered among different code files and would be hard to identify.

In my opinion, we should try defining the differences between Jetpack and WordPress (and their widgets) as explicitly as possible, without scattering them across the codebase and avoiding if/switch statements as much as possible.

These are 2 commits I make these changes in

Let me know what you think about such changes. I can easily revert them if they look a bit overhead.

Have the changes been tested on iOS 13 (e.g. to ensure that the app handles gracefully the lack of widget support in iOS 13?)

Yes. With these changes, Jetpack still doesn't have iOS 13 widgets and WordPress has older Today widgets. My intention is to add older iOS13 widgets in a separate PR.

Are the intents exposed to Siri and if so does Siri differentiate properly between the apps?

The SiriKit intents here are only used to have configurable widgets using "Edit Widget" menu. I don't see Siri voice commands being used in any way.

wpmobilebot · 2022-10-19T08:39:31Z

You can test the changes in Jetpack from this Pull Request by:

Clicking here or scanning the QR code below to access App Center
Then installing the build number pr19479-7755426 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

…atements in code

staskus · 2022-10-19T11:36:40Z

What was done since the review:

Fixed bundle identifier typo
Removed duplications in target dependencies
Made differences between Jetpack and WordPress widgets more explicit by moving them under AppConfiguration namespace
Started using accent color so Stats widgets would appear in blue on WordPress and in green on Jetpack.

Simulator.Screen.Recording.-.iPhone.14.-.2022-10-19.at.14.01.13.mp4

staskus · 2022-10-20T09:53:12Z

I couldn't spot any issues in the code. Can you reproduce it?

Yes, I could. Checking if there's some mismatch between the capabilities in release/alpha/internal profiles.

mokagio · 2022-10-21T05:55:35Z

I requested a review for myself because I wanted to look at the changes but don't have the time to do it right now.

This reverts commit ee66293.

…Widgets

…ase builds

peril-wordpress-mobile · 2022-10-21T07:11:13Z

	Warnings
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

…jetpack

staskus · 2022-10-21T07:18:54Z

I found out that for Release-Alpha builds we still use INTERNAL_BUILD preprocessor macro, which in Constants file chooses groups for Internal build and not for Alpha builds.

#ifdef INTERNAL_BUILD
NSString *const WPAppGroupName                                      = @"group.org.wordpress.internal";
NSString *const WPAppKeychainAccessGroup                    = @"99KV9Z6BKV.org.wordpress.internal";

Maybe it's the reason Widgets cannot load data in AppCenter (alpha) builds. It's hard to tell if such behavior is intentional. I set ALPHA_BUILD preprocessor macro for Release-Alpha and set appropriate group names in Constants.h.

The other possible solution would be to use same groups for both release-alpha or release-internal but that would require changes in profiles.

I can confirm that now Widget data loads on AppCenter version

mokagio · 2022-10-24T00:54:03Z

@staskus

I found out that for Release-Alpha builds we still use INTERNAL_BUILD preprocessor macro, which in Constants file chooses groups for Internal build and not for Alpha builds.

...

It's hard to tell if such behavior is intentional.

I haven't looked at the Git history, but that sounds like an oversight to me. Something that was not noticed at the time of adding the alpha build.

The fix you applied seems appropriate and something we should keep going forward. 👍

mokagio

None of my comments are blockers, but I'd like to know if you have any comments. Even just ideas on how to follow up later on.

The implementation at the project level is solid, as far as I can see.

I haven't tested the widgets locally, but consider this an approval for me once the other reviewers have a chance to look at the latest changes.

mokagio · 2022-10-24T01:45:39Z

+        @objc static let homeWidgetAllTimeFilename = "HomeWidgetAllTimeData.plist"
+        @objc static let homeWidgetThisWeekFilename = "HomeWidgetThisWeekData.plist"
+    }
+}


It's neat to have dedicated settings for WordPress vs Jetpack. Have you thought about how this approach of duplicating a class definition will scale?

In particular, here's something that I'm concerned might happen:

Developer working in Jetpack adds a new value to the AppConfigurationWidget version from Jetpack

They use it in a file shared between Jetpack and WordPress

Everything works fine for them because they are only building Jetpack

The build fails in CI because the value is not available in WordPress

This behavior can be seen in #19502.

Thank you for your comments.

Have you thought about how this approach of duplicating a class definition will scale?

Yes, I thought about it and this is not ideal. Your example is excellent. I had experience in other larger projects where we had to support multiple targets with different configurations. The use case was a bit different than we have with WordPress and Jetpack but technically the problem looks similar.

The worst approach was having multiple control statements scattered in the codebase. This made understanding the differences between different configurations hard and any changes needed to be made directly in particular classes.

We did try duplicating class definitions but we soon hit the problem you were describing. It's also not 100% transparent as well, which files are configuration files and which ones are included in which targets.

It's possible to move the configuration to xcconfig configuration files. It would allow us to have all the configuration, including build settings, in one place. Although that wouldn't guarantee we set all the required fields for all configurations. It could be used in tandem with some approaches in the code.

Our configurations could conform to one protocol and the correct configuration be resolved at runtime. This way both configurations could be included in both targets, allowing us to show build error when one of the configurations doesn't conform to a protocol. We ended up using a flavor of this approach in our previous project.

All in all, I think it deserves a wider discussion, how we could set up a configuration in a way that would support our needs going forward. Otherwise, we could end up with a messy solution.

All in all, I think it deserves a wider discussion, how we could set up a configuration in a way that would support our needs going forward. Otherwise, we could end up with a messy solution.

Definitely. I'd be great to address the problem before it happens, but the kind of changes we are discussing are too far-reaching and it would make this PR go stale.

I'm glad to hear about the protocol idea, because that's something I'd been thinking about as well. We'd then need something that at runtime allocates the correct one but that seems simpler to achieve than keeping duplicated classes in sync.

Alternatively, and depending on what the plan long term is for these widgets, it might be beneficial to extract the bulk of the logic in a framework, then have each widget target be a thin wrapper to it and pass a struct with configurations. That would centralize things big time, but does come with an extra layer of complexity in isolating stuff.

We'll see...

mokagio · 2022-10-24T01:51:17Z

+        static let unconfiguredViewThisWeekTitle = LocalizableStrings.unconfiguredViewJetpackThisWeekTitle
+        static let unconfiguredViewAllTimeTitle = LocalizableStrings.unconfiguredViewJetpackAllTimeTitle
+    }
+}


The same consideration about duplicating implementation mentioned for WidgetConfiguration.swift applies here.

mokagio · 2022-10-24T01:54:11Z

+        @objc static let statsTodayWidgetKeychainTokenKey = "OAuth2Token"
+        @objc static let statsTodayWidgetKeychainServiceName = "TodayWidget"
+        @objc static let statsTodayWidgetUserDefaultsSiteIdKey = "WordPressTodayWidgetSiteId"
+        @objc static let statsHomeWidgetsUserDefaultsSiteIdKey = "WordPressHomeWidgetsSiteId"


Have you considered increasing the granularity of these configurations?

@objc extension AppConfiguration { class Widget: NSObject { @objc(AppConfigurationWidgetStatsToday) class StatsToday: NSObject { @objc static let keychainTokenKey = "OAuth2Token" @objc static let keychainServiceName = "TodayWidget" @objc static let userDefaultsSiteIdKey = "WordPressTodayWidgetSiteId" } @objc(AppConfigurationWidgetStatsHome) class StatsHome: NSObject { @objc static let userDefaultsSiteIdKey = "WordPressHomeWidgetsSiteId" ...

I haven't but it would make sense since these keys are meant for different widgets. Although some of the keys are used for both iOS13 and iOS14+ widgets. I'll take a look what makes most sense.

staskus · 2022-10-25T13:20:12Z

+            return
+        }
+
+        userDefaults.setValue(AccountHelper.isLoggedIn, forKey: AppConfiguration.Widget.Stats.userDefaultsLoggedInKey)


Note: Check if we could check existence of access token with a static key

Unfortunately, the way the widgets are created we still rely on having defaultSiteId and preloaded widgetData when we initialize widgets for the first time. Only later StatsWidgetsService can be used inside of a widget to update the data.

Also, any refactoring we would do, we would still be unable to avoid user launching Jetpack at least once before adding widgets. After the launch correct token is set for AppConfiguration.Widget.Stats.keychainServiceName which is different between WordPress and Jetpack apps. Until then, the Widget wouldn't be able to display or fetch any information.

we would still be unable to avoid user launching Jetpack at least once before adding widgets

nitpick: I thought the risk was that the user adds widgets before launching Jetpack (opposite of what's described here). Either way, it sounds like this idea was a dead-end.

I thought the risk was that the user adds widgets before launching Jetpack

I don't see how can we avoid this risk, even if we make changes on what Widgets depends on. For those users who are updating the app from an older version that didn't support widgets, we would still need to set access token in the shared keychain with the expected key. Even though the key would be static, they would still differ from WordPress and would need to be set at least once.

guarani

Widgets couldn't be added now when building from Xcode, but work when installing the App Center builds.

guarani · 2022-10-25T18:11:15Z

+            return
+        }
+
+        userDefaults.setValue(AccountHelper.isLoggedIn, forKey: AppConfiguration.Widget.Stats.userDefaultsLoggedInKey)


we would still be unable to avoid user launching Jetpack at least once before adding widgets

nitpick: I thought the risk was that the user adds widgets before launching Jetpack (opposite of what's described here). Either way, it sounds like this idea was a dead-end.

guarani · 2022-10-26T13:52:40Z

Widgets couldn't be added now when building from Xcode, but work when installing the App Center builds.

Update: After debugging with @staskus the widgets worked again after restarting the device, so I'm re-reviewing now.

guarani

I ran through the test cases and things are working now. It looks like my earlier difficulties adding widgets in #19479 (review), were due to a device quirk because they went away after re-starting the device as you suggested.

Case 1: Fresh install

This works as expected. I noticed a bug that isn't related to this PR and is reproducible on WPiOS 21.0 (TestFlight): On one account with a site with few stats, I saw the "Unable to load site stats" message. The app was showing data in Stats for the site (Most Popular Time and All Time had data, while Today had no data), and the widgets displayed the error message even after visiting Stats. I'm creating an issue for this now (update: see #19519).

Case 2: WordPress app with widgets exist

Works as expected.

Case 3: Migration

Works as expected.

Thanks for your awesome work getting these widgets into the Jetpack app, @staskus! 💯

hassaanelgarem · 2022-11-14T23:50:02Z

+    class Widget: NSObject {
+        @objc(AppConfigurationWidgetStats)
+        class Stats: NSObject {
+            @objc static let keychainTokenKey = "OAuth2Token"


@staskus I'm working on doing something similar to the constants used by Extensions. Is there a reason for opting to use the same token key for both apps?

I didn't find it necessary, since it was used in tandem with keychainServiceName which was different for both apps. Did you encounter any problems with that?

staskus added 9 commits October 17, 2022 16:21

Create JetpackStatsWidgets target

7884b04

- Duplicating WordPressStatsWidget target - Changing Display Name, Bundle Identifier for all the configurations, Info.plist file, Code Signing Entitlements, WPCOM_SCHEME value

Include AppConstants into Bridging Headers

ee1aa6c

Use app-specific file name to store widget data

9a3b92b

Merge branch 'trunk' into fix/19460-jetpack-focus-enable-widgets-for-…

584a2a2

…jetpack

Created JetpackIntents target by duplicating WordPressIntents

22220e7

Duplicate and change displayName, bundleIdentifiers, set target dependencies, change schemes

Show different localizations for empty views in Jetpack Stats Widgets

81f6b14

Delete WordPressIntents copy-Info.plist

7e068d6

staskus added Stats Stats Widgets labels Oct 18, 2022

staskus added this to the 21.1 milestone Oct 18, 2022

staskus requested a review from guarani October 18, 2022 11:29

staskus requested review from guarani and removed request for guarani October 18, 2022 15:14

guarani reviewed Oct 19, 2022

View reviewed changes

Fix typo in JetpackStatsWidget release-alpha bundle identifier

205ee45

staskus closed this Oct 19, 2022

staskus reopened this Oct 19, 2022

Remove duplications

32c3d46

staskus added 4 commits October 19, 2022 13:06

Move different constants between widgets to AppConfiguration

3d12199

Add JetpackIntents and JetpackStatsWidgets folders to Xcode project

e24cafd

Move StatsWidget localizations into configuration to avoid control st…

fc09ccc

…atements in code

Use WordPress or Jetpack app accent color for Stats Widgets

27663c1

staskus mentioned this pull request Oct 20, 2022

[Draft] Jetpack Focus: Enable iOS13 Widgets for Jetpack #19490

Closed

4 tasks

staskus changed the title ~~Jetpack Focus: Enable Stats widgets and intents for Jetpack~~ Jetpack Focus: Enable iOS14+ Stats Widgets and Intents for Jetpack Oct 20, 2022

mokagio self-requested a review October 21, 2022 05:54

staskus added 3 commits October 21, 2022 09:34

Revert "Remove AppIdentifierPrefix for alpha and internal builds"

d512701

This reverts commit ee66293.

Set build scheme explicitly as Jetpack for JetpackIntents and Jetpack…

bff24f3

…Widgets

Set ALPHA_BUILD instead of INTERNAL_BUILD preprocessor macro for rele…

64c80b5

…ase builds

staskus added 2 commits October 21, 2022 10:18

Merge branch 'trunk' into fix/19460-jetpack-focus-enable-widgets-for-…

9dd6c22

…jetpack

Added release note

cf28e16

staskus requested a review from guarani October 21, 2022 08:25

mokagio reviewed Oct 24, 2022

View reviewed changes

staskus added 3 commits October 24, 2022 13:20

Remove Foundation imports

1305747

Split WidgetConfiguration into Stats and iOS13 StatsToday

6a505de

Use primary color for Stats widgets

f28171c

guarani mentioned this pull request Oct 24, 2022

Bump iOS deployment target from 13.0 to 14.0 #19496

Closed

4 tasks

staskus commented Oct 25, 2022

View reviewed changes

guarani reviewed Oct 25, 2022

View reviewed changes

Comment thread Podfile Outdated

guarani reviewed Oct 25, 2022

View reviewed changes

Update StatsWidgets pod targets to only depend on Gridicons

7755426

guarani self-requested a review October 26, 2022 13:52

guarani approved these changes Oct 26, 2022

View reviewed changes

staskus merged commit 3189c40 into trunk Oct 26, 2022

staskus deleted the fix/19460-jetpack-focus-enable-widgets-for-jetpack branch October 26, 2022 14:57

hassaanelgarem reviewed Nov 14, 2022

View reviewed changes

dvdchr mentioned this pull request Nov 25, 2022

Jetpack Extensions: Use different keys to avoid conflicts between WP and JP extensions #19665

Merged

4 tasks

Uh oh!

Conversation

staskus commented Oct 18, 2022

Description

Solution

Testing instructions

Case 1: Fresh install

Case 2: WordPress app with widgets exist

Case 3: Migration

Regression Notes

Images & Videos

Uh oh!

wpmobilebot commented Oct 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

staskus commented Oct 18, 2022

Uh oh!

guarani left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Product bundle identifier

Duplicate target dependencies

Potential duplication

Open questions

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staskus commented Oct 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Oct 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

staskus commented Oct 19, 2022

Uh oh!

staskus commented Oct 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mokagio commented Oct 21, 2022

Uh oh!

peril-wordpress-mobile Bot commented Oct 21, 2022

Uh oh!

staskus commented Oct 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mokagio commented Oct 24, 2022

Uh oh!

mokagio 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staskus Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staskus Oct 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guarani Oct 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staskus Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wpmobilebot commented Oct 18, 2022 •

edited

Loading

guarani left a comment •

edited

Loading

staskus commented Oct 19, 2022 •

edited

Loading

wpmobilebot commented Oct 19, 2022 •

edited

Loading

staskus commented Oct 20, 2022 •

edited

Loading

staskus commented Oct 21, 2022 •

edited

Loading

staskus Oct 24, 2022 •

edited

Loading

staskus Oct 25, 2022 •

edited

Loading

guarani Oct 25, 2022 •

edited

Loading

staskus Oct 26, 2022 •

edited

Loading

guarani Oct 25, 2022 •

edited

Loading

guarani left a comment •

edited

Loading