Skip to content

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

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d209f93
Remove redundant `#available(iOS 13, *)` checks
mokagio Oct 25, 2022
da7bd21
Convert a few `static var` to `static let`
mokagio Oct 27, 2022
220efe0
Update all computed `static` colors to `static let` in `GutenGhostView`
mokagio Oct 27, 2022
6d49db7
Re-introduce accidentally removed check in `MainShareViewController`
mokagio Oct 28, 2022
cd07203
Restore accidentally removed `else` branch in `MainShareViewController`
mokagio Oct 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion WordPress/Classes/ViewRelated/Activity/Filter/FilterChipButton.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ class FilterChipButton: UIView {
override func traitCollectionDidChange(_ previousTraitCollection: UITraitCollection?) {
super.traitCollectionDidChange(previousTraitCollection)

if #available(iOS 13, *), traitCollection.hasDifferentColorAppearance(comparedTo: previousTraitCollection) {
if traitCollection.hasDifferentColorAppearance(comparedTo: previousTraitCollection) {
applyColors()
}
}
Expand Down
8 changes: 1 addition & 7 deletions WordPress/Classes/ViewRelated/Blog/Blog + Me/GravatarButtonView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,7 @@ extension GravatarButtonView {
extension GravatarButtonView {

private struct StandardBorder {
static var color: UIColor {
if #available(iOS 13, *) {
return .separator
}

return .gray(.shade20)
}
static var color: UIColor = .separator

static let width = CGFloat(0.5)
}
Expand Down
6 changes: 2 additions & 4 deletions .../Domain registration/RegisterDomainSuggestions/DomainSuggestionsTableViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,8 @@ class DomainSuggestionsTableViewController: UITableViewController {
override func traitCollectionDidChange(_ previousTraitCollection: UITraitCollection?) {
super.traitCollectionDidChange(previousTraitCollection)

if #available(iOS 13, *) {
if traitCollection.userInterfaceStyle != previousTraitCollection?.userInterfaceStyle {
tableView.reloadData()
}
if traitCollection.userInterfaceStyle != previousTraitCollection?.userInterfaceStyle {
tableView.reloadData()
}
}

Expand Down
4 changes: 1 addition & 3 deletions ...ess/Classes/ViewRelated/Gutenberg/GutenbergWeb/GutenbergWebNavigationViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,7 @@ class GutenbergWebNavigationController: UINavigationController {
super.present(viewControllerToPresent, animated: flag, completion: completion)
}

guard
#available(iOS 13, *),
let menuViewControllerClass = NSClassFromString("UIDocumentMenuViewController"), // Silence deprecation warning.
guard let menuViewControllerClass = NSClassFromString("UIDocumentMenuViewController"), // Silence deprecation warning.
viewControllerToPresent.isKind(of: menuViewControllerClass),
UIDevice.current.userInterfaceIdiom == .phone
else {
Expand Down
7 changes: 1 addition & 6 deletions WordPress/Classes/ViewRelated/Gutenberg/Views/GutenGhostView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,12 +93,7 @@ class GutenGhostView: UIView {
}

private extension UIColor {
static var ghostToolbarBackground: UIColor {
if #available(iOS 13, *) {
return UIColor(light: .clear, dark: UIColor.colorFromHex("2e2e2e"))
}
return .clear
}
static var ghostToolbarBackground: UIColor = UIColor(light: .clear, dark: UIColor.colorFromHex("2e2e2e"))
Copy link
Contributor

@AliSoftware AliSoftware Oct 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@mokagio mokagio Oct 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@mokagio mokagio Oct 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@mokagio mokagio Oct 27, 2022

Choose a reason for hiding this comment

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

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

image

Copy link
Contributor

@AliSoftware AliSoftware Oct 27, 2022

Choose a reason for hiding this comment

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

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?


static var ghostBlockBackground: UIColor {
return UIColor(light: .clear, dark: .systemGray5)
Expand Down
6 changes: 1 addition & 5 deletions WordPress/Classes/ViewRelated/Post/AuthorFilterViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,7 @@ class AuthorFilterViewController: UITableViewController {
tableView.separatorColor = .clear
tableView.isScrollEnabled = false
tableView.showsVerticalScrollIndicator = false
if #available(iOS 13, *) {
tableView.contentInset = .zero
} else {
tableView.contentInset = UIEdgeInsets(top: -Metrics.topinset, left: 0, bottom: 0, right: 0)
}
tableView.contentInset = .zero
}

required init?(coder aDecoder: NSCoder) {
Expand Down
4 changes: 1 addition & 3 deletions WordPress/Classes/ViewRelated/Post/PostTagPickerViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,9 +145,7 @@ class PostTagPickerViewController: UIViewController {

override func traitCollectionDidChange(_ previousTraitCollection: UITraitCollection?) {
super.traitCollectionDidChange(previousTraitCollection)
if #available(iOS 13, *) {
textViewContainer.layer.borderColor = UIColor.divider.cgColor
}
textViewContainer.layer.borderColor = UIColor.divider.cgColor
}

@objc func doneButtonPressed() {
Expand Down
20 changes: 8 additions & 12 deletions WordPress/Classes/ViewRelated/Reader/Detail/WebView/ReaderWebView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,19 +159,15 @@ class ReaderWebView: WKWebView {
/// Maps app colors to CSS colors to be applied in the webview
///
private func cssColors() -> String {
if #available(iOS 13, *) {
return """
@media (prefers-color-scheme: dark) {
\(mappedCSSColors(.dark))
}

@media (prefers-color-scheme: light) {
\(mappedCSSColors(.light))
}
"""
}
return """
@media (prefers-color-scheme: dark) {
\(mappedCSSColors(.dark))
}

return lightCSSColors()
@media (prefers-color-scheme: light) {
\(mappedCSSColors(.light))
}
"""
}

/// If iOS 13, returns light and dark colors
Expand Down
8 changes: 1 addition & 7 deletions WordPress/Classes/ViewRelated/Reader/Select Interests/ReaderInterestsStyleGuide.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,7 @@ class ReaderInterestsStyleGuide {
}

// MARK: - Next Button
public class var buttonContainerViewBackgroundColor: UIColor {
if #available(iOS 13, *) {
return .tertiarySystemBackground
}

return .white
}
public static var buttonContainerViewBackgroundColor: UIColor = .tertiarySystemBackground

public class func applyNextButtonStyle(button: FancyButton) {
let disabledBackgroundColor: UIColor
Expand Down
7 changes: 1 addition & 6 deletions WordPress/Classes/ViewRelated/Site Creation/Shared/TitleSubtitleTextfieldHeader.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,12 +90,7 @@ final class SearchTextField: UITextField {
}()

private lazy var activityIndicator: UIActivityIndicatorView = {
let activityIndicator: UIActivityIndicatorView
if #available(iOS 13, *) {
activityIndicator = UIActivityIndicatorView(style: .medium)
} else {
activityIndicator = UIActivityIndicatorView(style: .gray)
}
let activityIndicator = UIActivityIndicatorView(style: .medium)
activityIndicator.backgroundColor = UIColor.clear

return activityIndicator
Expand Down
7 changes: 1 addition & 6 deletions WordPress/Classes/ViewRelated/Stats/Extensions/WPStyleGuide+Stats.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,12 +262,7 @@ extension WPStyleGuide {
static let selectedDay = UIColor.accent
}

static var mapBackground: UIColor {
if #available(iOS 13, *) {
return .systemGray4
}
return .neutral(.shade10)
}
static var mapBackground: UIColor = .systemGray4

// MARK: - Posting Activity Collection View Styles

Expand Down
9 changes: 4 additions & 5 deletions WordPress/Classes/ViewRelated/Stats/Period Stats/Countries/Map/CountriesMapView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,11 @@ private extension CountriesMapView {
}

func mapColors() -> [UIColor] {
if #available(iOS 13, *) {
if traitCollection.userInterfaceStyle == .dark {
return [.primary(.shade90), .primary]
}
if traitCollection.userInterfaceStyle == .dark {
return [.primary(.shade90), .primary]
} else {
return [.primary(.shade5), .primary]
}
return [.primary(.shade5), .primary]
}

func setGradientColors() {
Expand Down
8 changes: 1 addition & 7 deletions WordPress/Classes/ViewRelated/Stats/Today Widgets/WidgetStyles.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,7 @@ class WidgetStyles: NSObject {
static let headlineFont = UIFont.systemFont(ofSize: UIFont.preferredFont(forTextStyle: .headline).pointSize)
static let footnoteNote = UIFont.systemFont(ofSize: UIFont.preferredFont(forTextStyle: .footnote).pointSize)

static var separatorColor: UIColor = {
if #available(iOS 13, *) {
return .separator
} else {
return .divider
}
}()
static var separatorColor: UIColor = .separator

static var separatorVibrancyEffect: UIVibrancyEffect = {
if #available(iOS 13, *) {
Expand Down
4 changes: 0 additions & 4 deletions WordPress/WordPressShareExtension/ExtensionPresentationController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,6 @@ class ExtensionPresentationController: UIPresentationController {
presentedView?.layer.cornerRadius = Appearance.cornerRadius
presentedView?.clipsToBounds = true
}
guard #available(iOS 13, *) else {
presentedView?.frame = frameOfPresentedViewInContainerView
return
}
presentedView?.frame = viewFrame
}

Expand Down
10 changes: 2 additions & 8 deletions WordPress/WordPressShareExtension/MainShareViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,14 +96,8 @@ private extension MainShareViewController {

let shareNavController = UINavigationController(rootViewController: editorController)

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
Copy link
Contributor

@AliSoftware AliSoftware Oct 28, 2022

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@mokagio mokagio Oct 31, 2022

Choose a reason for hiding this comment

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

See why I need tests?!?!

Copy link
Contributor

@AliSoftware AliSoftware Oct 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@mokagio mokagio Nov 2, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@mokagio mokagio Dec 1, 2022

Choose a reason for hiding this comment

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

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

Save as Draft

IMG_4733

IMG_4734

Share

IMG_4736

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.

Copy link
Contributor Author

@mokagio mokagio Dec 1, 2022

Choose a reason for hiding this comment

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

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

#19700

}
// We need to make sure we don't end up with stacked modal view controllers by using this:
shareNavController.modalPresentationStyle = .overFullScreen
Copy link
Contributor

@AliSoftware AliSoftware Oct 27, 2022

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@mokagio mokagio Oct 28, 2022

Choose a reason for hiding this comment

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

Great catch! Thanks!

Copy link
Contributor Author

@mokagio mokagio Oct 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

  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.
  3. 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.


present(shareNavController, animated: true)
}
Expand Down