Skip to content

Bump iOS deployment target from 13.0 to 14.0 - #19496

Closed
mokagio wants to merge 7 commits into
trunkfrom
drop-ios-13
Closed

Bump iOS deployment target from 13.0 to 14.0#19496
mokagio wants to merge 7 commits into
trunkfrom
drop-ios-13

Conversation

@mokagio

@mokagio mokagio commented Oct 21, 2022

Copy link
Copy Markdown
Contributor

Bump iOS deployment target from 13.0 to 14.0.

To test

The app should build locally. Ideally, there should be no deprecation warnings or errors.

Currently, we have a bunch of errors due to "Today View extensions have been deprecated". The reason we have an error and not a warning is because those targets have "Treat Warnings as Errors" set to YES. Update: #19509 addressed those.

See Today View errors

Untitled

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 changed the base branch from mokagio/extract-deployment-target to trunk October 23, 2022 11:29
@staskus

staskus commented Oct 24, 2022

Copy link
Copy Markdown
Contributor

All iOS13 widget targets WordPressAllTimeWidget, WordPressThisWeekWidget, WordPressTodayWidget can be removed since we have iOS14+ WordPressStatsWidgets as a replacement.

@guarani

guarani commented Oct 24, 2022

Copy link
Copy Markdown
Contributor

If there are no objections to dropping iOS 13 support and it's possible to merge this PR by Friday, we could:

  1. Target v21.1 with this PR
  2. Remove Home Today View widgets in this PR to remove the errors
  3. Rebase Jetpack Focus: Enable iOS14+ Stats Widgets and Intents for Jetpack #19479 (which also targets 21.1) onto this branch
  4. Close [Draft] Jetpack Focus: Enable iOS13 Widgets for Jetpack #19490

From your side @mokagio, does this seem like a viable approach?
If it's not possible to merge this by Friday this week (e.g. if there are more errors to fix), we may have to support iOS 13 widgets on Jetpack until this PR is merged.
Wdyt @staskus? Are there better alternatives?

@mokagio

mokagio commented Oct 25, 2022

Copy link
Copy Markdown
Contributor Author

@guarani yes sounds good. I started working on this (mokagio/remove-ios-13-widgets) and was aiming to get it done today, but I need to drop off unexpectedly. Stay tuned.

@staskus

staskus commented Oct 25, 2022

Copy link
Copy Markdown
Contributor

Rebase #19479 (which also targets 21.1) onto this branch

I think maybe we should merge iOS14+ to trunk and then adjust this PR branch accordingly? Widgets changes are a must for 21.1, unlike a version bump.

If it's not possible to merge this by Friday this week (e.g. if there are more errors to fix), we may have to support iOS 13 widgets on Jetpack until this PR is merged.
Wdyt @staskus? Are there better alternatives?

@guarani I don't think it makes sense to support iOS13 widgets, even if we won't manage to bump the deployment target this release. The usability of iOS13 is too low and we would be removing the changes next release either way.

@mokagio

mokagio commented Oct 25, 2022

Copy link
Copy Markdown
Contributor Author

@guarani @staskus

Rebase #19479 (which also targets 21.1) onto this branch

I think maybe we should merge iOS14+ to trunk and then adjust this PR branch accordingly? Widgets changes are a must for 21.1, unlike a version bump.

The iOS 13 widget removals is ready for review, so there should be ample time for the Jetpack widgets to go on top of this.

Widgets changes are a must for 21.1

Are they? Is there an external commitment that must be met by that version? Just curious.

@guarani

guarani commented Oct 25, 2022

Copy link
Copy Markdown
Contributor

I think maybe we should merge iOS14+ to trunk and then adjust this PR branch accordingly? Widgets changes are a must for 21.1, unlike a version bump.

This is a good point, I think we can afford to ship iOS 14 Home Screen widgets on Jetpack, without trying to support iOS 13 Today View widgets on Jetpack as well. The only users who will benefit from iOS Today View widgets are:

  • At most 0.05% of current JP app users (this is the percentage of users on iOS 13, but the number is lower because not all will install widgets).
  • At most 0.5% of current WP users (this is the percentage of users on iOS 13 and assumes all switch to Jetpack)

Are they? Is there an external commitment that must be met by that version? Just curious.

Not an external commitment, but an internal goal for the Jetpack app to offer the same features the WordPress app offers by Nov 17. Shipping widgets on Jetpack in 21.1 contributes to this goal.

@mokagio

mokagio commented Oct 26, 2022

Copy link
Copy Markdown
Contributor Author

@guarani @staskus prioritizing the Jetpack widgets sounds good to me. There are only two days left for that in my time zone by now and we'd be cutting it close any other way.

I'll wait for that to land on trunk then update everything accordingly. Let me know if there's something I can do on the Jetpack widgets end.

@staskus

staskus commented Oct 26, 2022

Copy link
Copy Markdown
Contributor

@mokagio Jetpack Widgets merged ✅

@wpmobilebot

wpmobilebot commented Oct 27, 2022

Copy link
Copy Markdown
Contributor
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 pr19496-c13ed67 on your iPhone

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

This way, all the targets will use the same iOS deployment, all coming
from the project-level setting.
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));
```
@mokagio
mokagio marked this pull request as ready for review October 28, 2022 02:40
@mokagio mokagio added this to the 21.1 milestone Oct 28, 2022
@mokagio mokagio self-assigned this Oct 28, 2022
@mokagio
mokagio requested a review from crazytonyli October 28, 2022 03:17
Comment on lines -5 to -6
@available(iOS, deprecated: 14.0, message: "Use SchedulingDatePickerViewController, based on UIDatePicker.inline")
class SchedulingCalendarViewController: UIViewController, CalendarSheet, DateCoordinatorHandler, SchedulingViewControllerProtocol {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The deprecation annotation recommends using SchedulingDatePickerViewController instead of SchedulingCalendarViewController in iOS 14 and above.

Now that we support only iOS 14 and above, this whole file can be deleted. Notice that in this file we also had CalendarSheet and TimePickerViewController.

Comment on lines +62 to 67
// FIXME: This protocol is redundant as of dropping iOS 13.
//
// It was used as a facade in between `SchedulingCalendarViewController` (iOS 13) and
// `SchedulingDatePickerViewController` (iOS 14+). `SchedulingCalendarViewController` has been
// deleted so we can remove this as well.
protocol SchedulingViewControllerProtocol: UIViewController, UIViewControllerTransitioningDelegate, UIAdaptivePresentationControllerDelegate {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll follow this up later on. I wanted to keep the scope limited.


class TimePickerViewControllerTests: XCTestCase {

var timePickerVC: TimePickerViewController?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

TimePickerViewController was removed together with `

@mokagio

mokagio commented Oct 28, 2022

Copy link
Copy Markdown
Contributor Author

@crazytonyli I asked for your review on this PR with failing CI because I already have #19509 which addresses the issue and has been approved. I didn't want to merge it in, otherwise the diff would have become bigger.

Let me know what you think.

@crazytonyli

Copy link
Copy Markdown
Contributor

@mokagio Am I correct in saying that this PR is not longer needed? Can we change the target branch of #19509 from drop-ios-13 to trunk and merge it?

@mokagio

mokagio commented Oct 31, 2022

Copy link
Copy Markdown
Contributor Author

Can we change the target branch of #19509 from drop-ios-13 to trunk and merge it?

Oh yes, that would do it too. I guess what I should have done was create the widgets PR from trunk. So that, once they were merged, rebasing/merging trunk into this one would have showed a green CI.

But, given that PR was based off this one, your suggestion will save us a merge. Thanks!

@mokagio

mokagio commented Oct 31, 2022

Copy link
Copy Markdown
Contributor Author

Closing in favor of merging #19509 directly into trunk

@mokagio mokagio closed this Oct 31, 2022
@mokagio
mokagio deleted the drop-ios-13 branch December 2, 2022 06:06
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