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

[Tech Debt] Change TARGET_OS_IPHONE macros in code #82

Open
mindgraffiti opened this issue Dec 6, 2018 · 9 comments
Open

[Tech Debt] Change TARGET_OS_IPHONE macros in code #82

mindgraffiti opened this issue Dec 6, 2018 · 9 comments
Labels

Comments

@mindgraffiti
Copy link
Contributor

Xcode 10 and Swift 4.2 no longer recognize the TARGET_OS_IPHONE macro, which was deprecated in Swift 3 and removed in 4.2. The side effect of this is that Xcode is now executing code that was not meant for simulators.

The code need reviewed and all TARGET_OS_IPHONE macros replaced. I don't have a specific recommendation because this project may also need updated to target iOS 9.2 instead of 9.0 for full compatibility with the new macros.

Ref. #78
Ref. woocommerce/woocommerce-ios#475

@jkmassel
Copy link
Contributor

Are we certain that TARGET_OS_IPHONE is deprecated?

Based on this: https://stackoverflow.com/a/37891230/496295, and /usr/include/TargetConditionals.h, it seems that TARGET_IPHONE_SIMULATOR is deprecated in favour of TARGET_OS_SIMULATOR, but I can't find a reference to the deprecation of TARGET_OS_IPHONE.

That said, if you're using Swift, the #if TARGET_OS_IPHONE syntax seems nicer, but if I'm not mistaken, isn't it still just referencing the same global constant?

@mindgraffiti – do you know what we should be migrating to instead? :)

@mindgraffiti
Copy link
Contributor Author

@jkmassel deprecated is the incorrect word, you are correct +1

The goal is to take the code that is isolated by TARGET_OS_IPHONE, and make it skip execution for simulators. So my suggestion would be to use TARGET_OS_IPHONE && !TARGET_OS_SIMULATOR in this project.

Thoughts?

@jkmassel
Copy link
Contributor

jkmassel commented Dec 18, 2018

Most of the use of TARGET_OS_IPHONE is actually for iOS vs macOS support. Because this project has partial support for macOS, I'm thinking we'd want to evaluate the use of these constants on a case-by-case basis.

Paging @astralbodies – is macOS support something we ought to polish up and get working, or would it be preferable to clean it up until we need it for something? I'm happy to add that to my list, if needed :)

@astralbodies
Copy link
Contributor

@jkmassel I believe we are using the Tracks library in Simplenote Mac. Right @jleandroperez?

@mindgraffiti
Copy link
Contributor Author

Wow. I didn't know this was used in the map app too. Good call @jkmassel 👍

@jleandroperez
Copy link
Contributor

Yup @jkmassel this is being used by Simplenote Mac (thanks for spotting that @astralbodies !!!).

@jkmassel
Copy link
Contributor

@mindgraffiti – this should be mostly resolved in e6e6a19.

The code is a bit simpler now – each method exists no matter what, and we modify the internal implementation depending on the target. A bit more repeating ourselves, but I find it's far easier to work with.

One thing that may cause issues is the fact that unless you have a watch paired to the simulator, this block might add a note to the log about not having a paired device:

-(BOOL)isAppleWatchConnected{
#if TARGET_OS_IPHONE
return [[WatchSessionManager shared] hasBeenPreviouslyPaired];
#else // Mac
return NO;
#endif
}

Given the fact that we don't do much watch development, I'm inclined to just return NO for simulators, but if we do, it'd then be incorrect down the road if we did start doing watch dev.

WDYT?

@mindgraffiti
Copy link
Contributor Author

mindgraffiti commented Jan 10, 2019

@jkmassel frankly, I don't know. @stevebaranski you have experience with watchOS dev right? What's your take on ☝️?

It's not really a decision I should be making because I wouldn't be the one to build a watch app (if one was created in the future).

@ghost
Copy link

ghost commented Jan 11, 2019

@mindgraffiti & @jkmassel thanks for reaching out.

Looking at the underlying method, it looks like it's just checking value in NSUserDefaults. I am not concerned about this behavior on-device versus a Simulator.

If you encounter spurious logging related to Apple Watch, I'd guess that it's occurring in the initializer. It should be easy enough to test, but I think we should be good with the reliance on isSupported() there.

There are a few other things that might be worth mentioning.

  1. The method named isAppleWatchConnected might be interpreted as "is a watch currently connected?" The method hasBeenPreviouslyPaired seems to answer the question: "has a watch ever been connected?". This is a small mismatch, but noteworthy.
  2. In researching the issue, I noticed that we appear to have two pairs of repeated lines here. That might be worth tidying up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants