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

Upgrade to support Xcode 14.0 #7062

Merged
merged 16 commits into from
Sep 15, 2022
Merged

Upgrade to support Xcode 14.0 #7062

merged 16 commits into from
Sep 15, 2022

Conversation

mokagio
Copy link
Contributor

@mokagio mokagio commented Jun 9, 2022

Note: While I am listed as the PR author, @crazytonyli did all the heavy lifting to get the app building and up-to-date through all the betas! @mokagio


Description

This is going to be a long running branch to (try to) stay on top of the Xcode 14 betas PRs.

I expected many of you will want to checkout this branch locally to play with it. As such, I think we should keep it alive by merging trunk into it. But, I think once we'll officially switch to Xcode 14 stable, it'll make sense to do a big rebase so the history stays tidy.

I called the branch xcode-14. This might give us the option to keep the beta branch stable (🙃 🙂) and work on a new beta support without breaking the "stable" branch. That is, we might have an xcode-14-beta-2 branch in the future.

Ideally, we should implement as many fixes for errors and warnings found here as possible on trunk. See:

Testing instructions

Download Xcode 14 beta 1 and run this branch 😅 .

If you do, you might notice a bunch of:

Cannot access property 'cancellableSiteID' with a non-sendable type 'AnyCancellable?' from non-isolated deinit; this is an error in Swift 6

I haven't looked into it yet, other than finding this thread about it in the Swift forums.

Screenshots

Screen Shot 2022-06-09 at 7 37 25 pm Screen Shot 2022-06-09 at 7 44 16 pm

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

This also touched all the schemes to update the `LastUpgradeVersion` to
track they were checked in Xcode 14.0.
@mokagio mokagio added status: do not merge Dependent on another PR, ready for review but not ready for merge. category: tooling Anything that involves building & maintaining the project, including scripts, `Fastfile`, etc. labels Jun 9, 2022
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jun 9, 2022

You can test the changes from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr7062-21de10c on your iPhone

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

@crazytonyli
Copy link
Contributor

Hi @mokagio , do you think this PR is ready for review now?

@mokagio mokagio marked this pull request as ready for review September 15, 2022 03:56
@mokagio
Copy link
Contributor Author

mokagio commented Sep 15, 2022

Hi @mokagio , do you think this PR is ready for review now?

Yes it is! Pushing the button and reaching out to the devs.

@mokagio mokagio changed the title WIP – Xcode 14.0 Upgrade to support Xcode 14.0 Sep 15, 2022
@peril-woocommerce
Copy link

Warnings
⚠️ PR is not assigned to a milestone.

Generated by 🚫 dangerJS

Copy link
Contributor

@itsmeichigo itsmeichigo left a comment

Choose a reason for hiding this comment

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

This works great for me both on the simulator and physical devices.
Thanks for the updates!

mokagio and others added 3 commits September 15, 2022 15:16
This seems to address the "Update project to recommended settings"
warning for the Pods project.
…ings

Upgrade project to Xcode 14.0 stable recommended settings
Copy link
Contributor

@jaclync jaclync left a comment

Choose a reason for hiding this comment

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

Working as before on an iOS 15 device and iOS 16 simulator! Just some minor non-blocking questions

Comment on lines +47 to +51
super.viewWillDisappear(animated)

if #available(iOS 16.0, *) {
self.navigationController?.setNavigationBarHidden(false, animated: animated)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: does it make sense to reverse the order of the super call since this is a "disappear" event?

Suggested change
super.viewWillDisappear(animated)
if #available(iOS 16.0, *) {
self.navigationController?.setNavigationBarHidden(false, animated: animated)
}
if #available(iOS 16.0, *) {
self.navigationController?.setNavigationBarHidden(false, animated: animated)
}
super.viewWillDisappear(animated)

Copy link
Contributor

Choose a reason for hiding this comment

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

The API doc says

If you override this method, you must call super at some point in your implementation.

I don't think the order of these two statements would make any difference, the pop animation should begin and end at the same time.

// We want to hide navigation bar *only* on HubMenu screen. But on iOS 16, the `navigationBarHidden(true)`
// modifier on `HubMenu` view hides the navigation bar for the whole navigation stack.
// Here we manually hide or show navigation bar when entering or leaving the HubMenu screen.
if #available(iOS 16.0, *) {
Copy link
Contributor

Choose a reason for hiding this comment

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

do you still see the navigation bar issue #7091 with the latest Xcode 14? somehow I don't see this issue anymore on trunk using Xcode 14 in an iOS 16.0 simulator. I don't have an iOS 16 device though, might be worth testing on one to confirm if this workaround is still needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just tried to comment out viewWillAppear and viewWillDisappear - reproduced the issue on the device:

Payments Reviews
IMG_2289 IMG_2290

Copy link
Contributor

@crazytonyli crazytonyli Sep 15, 2022

Choose a reason for hiding this comment

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

@jaclync Hmm, that's strange. I just run the trunk branch on an iPhone 14 simulator using Xcode 14, and verified this is still an issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like something odd on my end then, thanks for checking both!

@@ -52,6 +52,9 @@ struct ShippingLineDetails: View {
symbol: nil,
keyboardType: .default)
.accessibilityIdentifier("add-shipping-name-field")
.onTapGesture {
focusAmountInput = false
Copy link
Contributor

Choose a reason for hiding this comment

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

do you have the testing steps for this change?

Copy link
Contributor

Choose a reason for hiding this comment

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

This code was added so that the test_creat_new_order UI test can pass on CI. It's a strange issue though, since the test failure only occurs on CI, not on my mac.

BTW, the issue was mentioned in my original PR.

Here is a very strange issue only happens in UI test on CI. I had to explicitly make a text field loose keyboard focus by changing the binding variable, for the other "add-shipping-name-field" text field to gain keyboard focus.

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, you can find my many attempts of fixing this issue in this buildkite branch builds.

From the test result bundle (you can download one from the artifacts of this step), it appears even thought "add-shipping-name-field" is tapped, it's still "add-shipping-amount-field" that has the keyboard focus, which caused the test to fail.
Screen Shot 2022-09-16 at 9 17 57 AM

Comment on lines +344 to +348
if #available(iOS 16.0, *) {
XCTAssertEqual(timeRangeBarViewModels.map { $0.timeRangeText }, ["Monday, Jan 3", "Monday, Jan 3 at 1:00 AM"])
} else {
XCTAssertEqual(timeRangeBarViewModels.map { $0.timeRangeText }, ["Monday, Jan 3", "Monday, Jan 3, 1:00 AM"])
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nice catch!

Copy link
Contributor

@ealeksandrov ealeksandrov left a comment

Choose a reason for hiding this comment

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

✅ Xcode 13.4.1 + iOS 14.5, 15.5 simulators
✅ Xcode 14 + iOS 16 simulator + iOS 16 device

There are couple of new warnings in Xcode 14:

warning build: Run script build phase 'SwiftLint' will be run during every build because it does not specify any outputs. To address this warning, either add output dependencies to the script phase, or configure it to run in every build by unchecking "Based on dependency analysis" in the script phase.


warning build: Run script build phase 'Check for nested frameworks' will be run during every build because it does not specify any outputs. To address this warning, either add output dependencies to the script phase, or configure it to run in every build by unchecking "Based on dependency analysis" in the script phase.

@crazytonyli
Copy link
Contributor

Hi @jaclync , I'll merge this PR for now. I'm happy to follow up if you have any further comments. Thanks! 😄

@crazytonyli crazytonyli merged commit c457ff0 into trunk Sep 15, 2022
@crazytonyli crazytonyli deleted the xcode-14 branch September 15, 2022 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: tooling Anything that involves building & maintaining the project, including scripts, `Fastfile`, etc. status: do not merge Dependent on another PR, ready for review but not ready for merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants