Skip to content

Conversation

@mokagio
Copy link
Contributor

@mokagio mokagio commented Oct 25, 2022

Removes the iOS 13 widgets, WordPressTodayWidget, WordPressThisWeekWidget, and WordPressAllTimeWidget, plush does as little work as possible to get the app to build with iOS 14 as the minimum deployment target (#19496).

See also commit messages for additional details on the rationale behind some of the changes.

While working on this, I addressed a few more deprecations, see #19512, but I decided to keep them separated to focus this PR on widgets.

Regression Notes

  1. Potential unintended areas of impact – N.A.
  2. What I did to test those areas of impact (or what existing automated tests I relied on) – N.A.
  3. What automated tests I added (or what prevented me from doing so) – N.A.

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

@mokagio mokagio self-assigned this Oct 25, 2022
@mokagio mokagio changed the base branch from trunk to drop-ios-13 October 25, 2022 05:54
Comment on lines -553 to -554
<key>UIApplicationShortcutWidget</key>
<string>org.wordpress.WordPressTodayWidget</string>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I could gather from the docs and the latest support page, this key was used at the time of the iOS 10 to 13 widgets but is no longer needed.

@@ -5593,13 +5461,10 @@
09C8BB8327DFF9CB00974175 /* ro */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = ro; path = ro.lproj/InfoPlist.strings; sourceTree = "<group>"; };
09C8BB8427DFF9CC00974175 /* sk */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = sk; path = sk.lproj/InfoPlist.strings; sourceTree = "<group>"; };
09F367D2BE684EDE2E4A40E3 /* Pods-WordPressDraftActionExtension.debug.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-WordPressDraftActionExtension.debug.xcconfig"; path = "../Pods/Target Support Files/Pods-WordPressDraftActionExtension/Pods-WordPressDraftActionExtension.debug.xcconfig"; sourceTree = "<group>"; };
0A27D496128FBCDFA3658994 /* Pods-WordPressHomeWidgetToday.release-internal.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-WordPressHomeWidgetToday.release-internal.xcconfig"; path = "../Pods/Target Support Files/Pods-WordPressHomeWidgetToday/Pods-WordPressHomeWidgetToday.release-internal.xcconfig"; sourceTree = "<group>"; };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fun fact. We've been carrying around these WordPressHomeWidgetToday dead references ever since renaming that target to WordPressTodayWidget 😳

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for noticing. The project will get a well-deserved clean-up then 👀

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Oct 25, 2022

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 pr19509-08f921a on your iPhone

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

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Oct 25, 2022

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 pr19509-08f921a on your iPhone

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

@mokagio mokagio force-pushed the mokagio/remove-ios-13-widgets branch 3 times, most recently from 089ffab to 46b53ec Compare October 25, 2022 12:04
@mokagio mokagio added this to the 21.1 milestone Oct 25, 2022
@mokagio mokagio requested a review from guarani October 25, 2022 12:24
@mokagio mokagio marked this pull request as ready for review October 25, 2022 12:38
@mokagio mokagio requested a review from staskus October 25, 2022 12:38
@mokagio
Copy link
Contributor Author

mokagio commented Oct 25, 2022

cc @crazytonyli, if you have the time to take a look.

Copy link
Contributor

@staskus staskus left a comment

Choose a reason for hiding this comment

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

Thank you for the changes, @mokagio! I reviewed commit by commit. Additional changes unrelated to Widgets make sense as well. 👍

⚠️ Additional files that can be removed (In Today Widgets / Shared Views):

  • WidgetDiffferenceCell
  • WidgetTwoCollumnCell
  • WidgetUnconfiguredCell
  • WidgetUrlCell
  • WidgetNoConnectionCell

Potentially, there could be minor details related to iOS13 Widgets that could be removed as well, for example, some Constants and details saved to UserDefaults. However, iOS13 Widget data-loading code is reused by iOS14+ Widgets, therefore, it's hard to tell if any small details could be removed without negatively affecting current widgets. The majority of the code is removed in this PR.

⚠️ I see there're multiple places that still have #available(iOS 14.0, *), including Stats-related StatsWidgetStore. However, they could be removed in the parent PR or later.

I tested the app:

  • iOS 14 widgets continue working
  • Existing shortcuts continue working

@mokagio mokagio force-pushed the mokagio/remove-ios-13-widgets branch from 46b53ec to fc4d3b8 Compare October 27, 2022 04:11
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Oct 27, 2022

Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 4 days Please, make sure to get it merged by then or assign it to a later expiring milestone
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

@mokagio mokagio requested a review from staskus October 27, 2022 04:17
@mokagio
Copy link
Contributor Author

mokagio commented Oct 27, 2022

Thank you for the review @staskus 🙌

I removed the Shared Views that this work made unused as you suggested.

I see there're multiple places that still have #available(iOS 14.0, *), including Stats-related StatsWidgetStore. However, they could be removed in the parent PR or later.

Agreed. I might leave them for a PR later on. As a matter of fact, I already removed them here but I think I'll submit them in isolation after #19496 lands.

Copy link
Contributor

@staskus staskus left a comment

Choose a reason for hiding this comment

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

@staskus staskus self-requested a review October 27, 2022 07:07
Copy link
Contributor

@staskus staskus left a comment

Choose a reason for hiding this comment

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

I don't want to block this PR, so after shared views are removed, we can move on. 👍

@mokagio
Copy link
Contributor Author

mokagio commented Oct 27, 2022

@mokagio, I don't see SharedViews being removed.

I didn't push them... 😓 But you can see them in the timeline now

image

Thank you for the approval to unblock. I'll merge as soon as CI is green.

@staskus
Copy link
Contributor

staskus commented Oct 28, 2022

I see the changes now, all good 🚀

Copy link
Contributor

@guarani guarani left a comment

Choose a reason for hiding this comment

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

Apologies for the late review, @mokagio. The code changes look right and I gave the Jetpack app and the WP app both a quick smoke test (adding Home Screen widgets, scheduling a post, etc.) – all good.

I love how this reduced the number of targets in the project ❤️ Thanks for wrangling this!

Are you planning to merge this in 21.1 or bump to the next version?

@mokagio mokagio changed the base branch from drop-ios-13 to trunk October 31, 2022 01:25
This way, all the targets will use the same iOS deployment, all coming
from the project-level setting.
mokagio and others added 17 commits October 31, 2022 12:28
The class property was deprecated in iOS 14. Xcode gave the following
warning (which we treat as an error):

>  Deprecated. Use the instance property `authorizationStatus` instead.
The deprecation warning in Xcode wasn't informative, but the deprecation
annotation suggested what to do:

```
/*
 *  locationManager:didChangeAuthorizationStatus:
 *
 *  Discussion:
 *    Invoked when the authorization status changes for this application.
 */
- (void)locationManager:(CLLocationManager *)manager didChangeAuthorizationStatus:(CLAuthorizationStatus)status API_DEPRECATED_WITH_REPLACEMENT("-locationManagerDidChangeAuthorization:", ios(4.2, 14.0), macos(10.7, 11.0), watchos(1.0, 7.0), tvos(9.0, 14.0));
```
This legacy widget extension has been superseded by
`WordPressStatsWidgets`.
This legacy widget extension has been superseded by
`WordPressStatsWidgets`.
This legacy widget extension has been superseded by
`WordPressStatsWidgets`.
These were there to be ready to use once the team was ready to add the
widgets. In the meantime, we decided to drop iOS 13, which removes the
need for them.
This would have been the name of the very first iOS 10 home widget we
added. Since then, it was renamed but we missed the dead references.
@mokagio mokagio force-pushed the mokagio/remove-ios-13-widgets branch from 0956955 to d6392db Compare October 31, 2022 01:44
@mokagio mokagio enabled auto-merge October 31, 2022 01:46
@mokagio mokagio merged commit 0405eb4 into trunk Oct 31, 2022
@mokagio mokagio deleted the mokagio/remove-ios-13-widgets branch October 31, 2022 02:34
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.

5 participants