Remove redundant `#available(iOS 13, *)` checks #19513

mokagio · 2022-10-25T12:13:52Z

They always return true since the deployment version is 13.0, and will soon become 14.0.

Only one has been left, in WidgetStyles.swift, because it will change again when bumping the deployment target to 14+.

I was looking at #available statements as part of #19496 and realized we still has some of these laying around.

Regression Notes

Potential unintended areas of impact – N.A.
What I did to test those areas of impact (or what existing automated tests I relied on) – N.A.
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.

They always return `true` since the deployment version is 13.0, and will soon become 14.0. Only one has been left, in `WidgetStyles.swift`, because it will change again when bumping the deployment target to 14+.

wpmobilebot · 2022-10-26T11:06:27Z

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 pr19513-cd07203 on your iPhone

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

wpmobilebot · 2022-10-26T11:09:07Z

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 pr19513-cd07203 on your iPhone

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

WordPress/Classes/ViewRelated/Blog/Blog + Me/GravatarButtonView.swift

AliSoftware · 2022-10-26T15:19:52Z

WordPress/Classes/ViewRelated/Gutenberg/Views/GutenGhostView.swift

-        }
-        return .clear
-    }
+    static var ghostToolbarBackground: UIColor = UIColor(light: .clear, dark: UIColor.colorFromHex("2e2e2e"))


Suggested change

static var ghostToolbarBackground: UIColor = UIColor(light: .clear, dark: UIColor.colorFromHex("2e2e2e"))

static let ghostToolbarBackground: UIColor = UIColor(light: .clear, dark: UIColor.colorFromHex("2e2e2e"))

PS: I'm surprised that you can do that in an extension as you can't add instance variable / storage in extension… but not sure about add static ones on the type, so maybe that's ok because of static? (I must be rusty in Swift already…)

If that's not allowed, I'd assume that restriction applies equally to static let and static var storages, and so neither code would compile, and the solution would be to go back to a computed variable static var ghostToolbarBackground: UIColor { .init(…) } instead. But CI seems to be compiling the code happily, so, that's probably ok after all?

Yes, it is allowed.

I wouldn't be surprised if it was a recent Swift change though, because I remember running into the constraint of not being able to add stored properties in extension in the past. Or, maybe I never tried to do it with static.

I wouldn't be surprised if it was a recent Swift change though, because I remember running into the constraint of not being able to add stored properties in extension in the past. Or, maybe I never tried to do it with static.

Might actually be a case of me never running into this with static or always assuming it worked the same as with instance. There is at least one StackOverflow answer using static left in an extension, dated 2014.

Oh, and by the way, stored properties are still not allowed:

Ah, yeah, that makes sense I guess.

I totally get why there is this limitation for var instance variables. But given that let are constants, and thus not taking storage space on the instance and thus not modifying the underlying memory layout of an instance of the type, I wonder why instance constants let are not allowed tbh (after all, they could be seen as synonyms to var potato: String { "potato" }, at least in behavior…).

Maybe this will be allowed in a future version of Swift though?

WordPress/Classes/ViewRelated/Reader/Select Interests/ReaderInterestsStyleGuide.swift

WordPress/Classes/ViewRelated/Stats/Extensions/WPStyleGuide+Stats.swift

WordPress/Classes/ViewRelated/Stats/Today Widgets/WidgetStyles.swift

Co-authored-by: Olivier Halligon <olivier.halligon@automattic.com>

Maybe due to a recent Swift improvement, we are now able to declare `static var` and `static let` within an `extension`. There's no need to compute the value at runtime anymore.

AliSoftware · 2022-10-27T10:13:01Z

WordPress/WordPressShareExtension/MainShareViewController.swift

-        if #available(iOS 13, *), editorController.originatingExtension == .saveToDraft {
-            // iOS 13 has proper animations and presentations for share and action sheets. So the `else` case should be removed when iOS 13 is minimum.
-            // We  need to make sure we don't end up with stacked modal view controllers by using this:
-            shareNavController.modalPresentationStyle = .overFullScreen
-        } else {
-            shareNavController.transitioningDelegate = extensionTransitioningManager
-            shareNavController.modalPresentationStyle = .custom
-        }
+        // We need to make sure we don't end up with stacked modal view controllers by using this:
+        shareNavController.modalPresentationStyle = .overFullScreen


This is not an equivalent conversion, as you lost the if editorController.originatingExtension == .saveToDraft condition.

Great catch! Thanks!

@AliSoftware I addressed this in 6d49db7 (#19513), thanks again.

I started looking at how to add a unit test to catch
this issue but I run into a number of problems.

The code is in the WordPressShareExtension target and we cannot test
that one directly; Xcode doesn't let you create test target against
app extensions

Even if we could access the code, the logic depends on
editorController, an instance property that sets the state based
whether Bundle.main.bundleIdentifier contains "DraftAction". That
is something we cannot modify at test run time.

Plus a few access level constraints, but those could be easily changed. I don't think we need to worry much about private vs internal in the context of an app extension.

In short, to test this trivial behavior, we'll need to put in place a
far-from trivial test harness. The idea I have at the moment would be to
extract as much of the logic shared between WordPressShareExtension,
WordPressDraftActionExtension, and their two Jetpack counter parts into
a framework, which we could then test in isolation and design in such a
way that we don't need to inspect the bundle identifier at runtime.

It's worth saying that folks were aware of the code limitations at the
time of implementing the behavior but must have been pressed for time to
ship a bug fix, as evidenced by the comment in the editorController
implementation.

This occurred while removing `#available(iOS 13, *)` checks a few commits before this. I was too eager in my deletions. See discussion at #19513 (comment) For the record, I started looking at how to add a unit test to catch this issue but I run into a number of problems. 1. The code is in the WordPressShareExtension target and we cannot test that one directly; Xcode doesn't let you create test target against app extensions 2. Even if we could access the code, the logic depends on `editorController`, an instance property that sets the state based whether `Bundle.main.bundleIdentifier` contains "DraftAction". That is something we cannot modify at test run time. In short, to test this trivial behavior, we'll need to put in place a far-from trivial test harness. The idea I have at the moment would be to extract as much of the logic shared between WordPressShareExtension, WordPressDraftActionExtension, and their two Jetpack counter parts into a framework, which we could then test in isolation and design in such a way that we don't need to inspect the bundle identifier at runtime. It's worth saying that folks were aware of the code limitations at the time of implementing the behavior but must have been pressed for time to ship a bug fix, as evidenced by the comment in the `editorController` implementation.

AliSoftware · 2022-10-28T09:00:58Z

WordPress/WordPressShareExtension/MainShareViewController.swift

-        } else {
-            shareNavController.transitioningDelegate = extensionTransitioningManager
-            shareNavController.modalPresentationStyle = .custom


@mokagio you still need to keep this else clause and code path for the case when the source is not the SaveToDraft extension

See why I need tests?!?!

Oh wait, and now I just read the // code comment that was there before your change, and that said:

// iOS 13 has proper animations and presentations for share and action sheets. So the `else` case should be removed when iOS 13 is minimum.

So maybe you were right to remove the else clause after all! 🤔 (so sorry for the back-and-forth 😅 I should be more mindful of code comments in diffs)

Though that comment still begs the question, if we remove that else clause, are we also supposed to remove the if editorController.originatingExtension == .saveToDraft as well, and use the same modalPresentationStyle = .custom for all cases? Or should we still keep the if condition and only use .custom for SaveToDraft but not to other cases?

@bjtitus, as the author of that code back in #13146 (even if this was 3 years ago and you moved to other products since…), maybe you remember?

The release notes in that PR list

Fixed bugs with the "Save as Draft" action extension's navigation bar colors and iPad sizing in iOS 13.

Skimming the PR description, I think the editorController.originatingExtension == .saveToDraft check is intentional and required.

I'll verify the behavior for the two extensions and in both versions of the code and report. Stay tuned.

With the code at cd07203 (#19513), I'm getting the following

Save as Draft

Share

Share doesn't look right. Might need more work. The first thing I'll try will be removing the else branch and add some break points. I'm also not sure why I'm getting that error, given the app was setup alright.

Share doesn't look right. Might need more work.

#19700

…roller` See discussion at #19513 (comment)

See discussion at #19513 (comment)

They always return `true` since the deployment version is 13.0, and will soon become 14.0. Only two have been left: - In `WidgetStyles.swift`, because it will change again when bumping the deployment target to 14+. - In `MainShareViewController.swift` because we might want to take the occasion to restructure that logic. See conversation at #19513 (comment)

mokagio · 2022-12-01T16:49:44Z

Closing in favor of #19699 and #19700.

mokagio added this to the 21.1 milestone Oct 25, 2022

mokagio added the Tooling Build, Release, and Validation Tools label Oct 26, 2022

Remove redundant #available(iOS 13, *) checks

d209f93

They always return `true` since the deployment version is 13.0, and will soon become 14.0. Only one has been left, in `WidgetStyles.swift`, because it will change again when bumping the deployment target to 14+.

mokagio force-pushed the mokagio/remove-ios-13-available branch from 90e82d9 to d209f93 Compare October 26, 2022 10:44

mokagio enabled auto-merge October 26, 2022 11:35

mokagio requested a review from a team October 26, 2022 11:36

AliSoftware reviewed Oct 26, 2022

View reviewed changes

mokagio and others added 2 commits October 27, 2022 11:16

Convert a few static var to static let

da7bd21

Co-authored-by: Olivier Halligon <olivier.halligon@automattic.com>

Update all computed static colors to static let in GutenGhostView

220efe0

Maybe due to a recent Swift improvement, we are now able to declare `static var` and `static let` within an `extension`. There's no need to compute the value at runtime anymore.

mokagio requested review from AliSoftware and crazytonyli October 27, 2022 00:24

AliSoftware requested changes Oct 27, 2022

View reviewed changes

AliSoftware requested changes Oct 28, 2022

View reviewed changes

mokagio modified the milestones: 21.1, 21.2 Oct 31, 2022

mokagio added a commit that referenced this pull request Oct 31, 2022

Re-introduce accidentally removed else brance in `MainShareViewCont…

5f04c7f

…roller` See discussion at #19513 (comment)

Restore accidentally removed else branch in MainShareViewController

cd07203

See discussion at #19513 (comment)

mokagio force-pushed the mokagio/remove-ios-13-available branch from 5f04c7f to cd07203 Compare October 31, 2022 03:38

mokagio modified the milestones: 21.2, 21.3 Nov 11, 2022

oguzkocer modified the milestones: 21.3, 21.4 Nov 28, 2022

This was referenced Dec 1, 2022

Remove redundant #available(iOS 13, *) and @available(iOS 13, *) checks #19699

Merged

Fix share extension navigation bar being transparent #19700

Merged

mokagio closed this Dec 1, 2022

auto-merge was automatically disabled December 1, 2022 16:49
Pull request was closed

mokagio deleted the mokagio/remove-ios-13-available branch December 1, 2022 16:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove redundant `#available(iOS 13, *)` checks #19513

Remove redundant `#available(iOS 13, *)` checks #19513

mokagio commented Oct 25, 2022 •

edited

Loading

wpmobilebot commented Oct 26, 2022 •

edited

Loading

wpmobilebot commented Oct 26, 2022 •

edited

Loading

AliSoftware Oct 26, 2022 •

edited

Loading

mokagio Oct 27, 2022 •

edited

Loading

mokagio Oct 27, 2022 •

edited

Loading

mokagio Oct 27, 2022

AliSoftware Oct 27, 2022

AliSoftware Oct 27, 2022

mokagio Oct 28, 2022

mokagio Oct 28, 2022 •

edited

Loading

AliSoftware Oct 28, 2022

mokagio Oct 31, 2022

AliSoftware Oct 31, 2022 •

edited

Loading

mokagio Nov 2, 2022

mokagio Dec 1, 2022

mokagio Dec 1, 2022

mokagio commented Dec 1, 2022

	static var ghostToolbarBackground: UIColor = UIColor(light: .clear, dark: UIColor.colorFromHex("2e2e2e"))
	static let ghostToolbarBackground: UIColor = UIColor(light: .clear, dark: UIColor.colorFromHex("2e2e2e"))

Remove redundant #available(iOS 13, *) checks #19513

Remove redundant #available(iOS 13, *) checks #19513

Conversation

mokagio commented Oct 25, 2022 • edited Loading

Regression Notes

wpmobilebot commented Oct 26, 2022 • edited Loading

wpmobilebot commented Oct 26, 2022 • edited Loading

AliSoftware Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

mokagio Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

mokagio Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mokagio Oct 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AliSoftware Oct 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mokagio commented Dec 1, 2022

Remove redundant `#available(iOS 13, *)` checks #19513

Remove redundant `#available(iOS 13, *)` checks #19513

mokagio commented Oct 25, 2022 •

edited

Loading

wpmobilebot commented Oct 26, 2022 •

edited

Loading

wpmobilebot commented Oct 26, 2022 •

edited

Loading

AliSoftware Oct 26, 2022 •

edited

Loading

mokagio Oct 27, 2022 •

edited

Loading

mokagio Oct 27, 2022 •

edited

Loading

mokagio Oct 28, 2022 •

edited

Loading

AliSoftware Oct 31, 2022 •

edited

Loading