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

[Update] Add Mac Catalyst support #93

Merged
merged 11 commits into from
Dec 7, 2022
Merged

[Update] Add Mac Catalyst support #93

merged 11 commits into from
Dec 7, 2022

Conversation

yo1995
Copy link
Collaborator

@yo1995 yo1995 commented Dec 5, 2022

Description

This PR adds Mac Catalyst as a deployment destination to the app. It also includes the following changes

  • Remove "Mac Designed for iPad" as Mac Catalyst is a better experience, plus also makes it possible to run on Intel Macs.
  • Remove toolkit from "Embed Frameworks" because it is a (local) source Swift package, and will produce error if embedded.
  • Moved the README files to "UNLOCALIZED_RESOURCES_FOLDER_PATH/READMEs", aka "Samples.app/Contents/Resources/READMEs". Semantically speaking, it should have been here already, as all the bundle resources should go under the Resources subpath, instead of under the root of the bundle.
  • Namespace collision for ArcGIS.Polygon is resolved.
  • Minimum deployment targets are added. Not sure what they are for, but I saw toolkit also added them.
    targets

Linked Issue(s)

  • swift/issues/3153

How To Test

  • Run on iPhone, iPad Simulator
  • Run on Mac Catalyst
  • Everything should work as before, except that on-demand resource is not working on Mac Catalyst (as expected)

Screenshots

Mac-Catalyst

To Discuss

  • (Slack) Team ID should be removed?

Copy link
Contributor

@dfeinzimer dfeinzimer left a comment

Choose a reason for hiding this comment

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

Everything should work as before, except that on-demand resource is not working on Mac Catalyst (as expected)

Would that be referring to this error?
Screenshot 2022-12-05 at 14 29 02

@yo1995
Copy link
Collaborator Author

yo1995 commented Dec 5, 2022

Would that be referring to this error?

Correct. See the discussions below.

This class ignores calls from Mac apps built with Mac Catalyst.

Some other iOS functionalities aren't supported either, see a similar discussion

@dfeinzimer
Copy link
Contributor

Ah okay, should we then conditionally exclude the affected samples from Catalyst builds until that support is added or the samples are reworked? I wonder if there's a good way to do so.

@yo1995
Copy link
Collaborator Author

yo1995 commented Dec 5, 2022

IMO there is still value leaving these samples in even if they cannot be opened on Mac. Users can still see the README and source code 😝 …

@mhdostal
Copy link
Member

mhdostal commented Dec 5, 2022

IMO there is still value leaving these samples in even if they cannot be opened on Mac. Users can still see the README and source code 😝 …

Will the Samples app go to the App Store at the final release and would it be listed as supporting Mac Catalyst? Is the loading of on-demand resources coming to Mac Catalyst for final? Not sure if Apple would allow it if there's functionality that doesn't work on Mac Catalyst.

@yo1995
Copy link
Collaborator Author

yo1995 commented Dec 5, 2022

Will the Samples app go to the App Store at the final release and would it be listed as supporting Mac Catalyst?

Samples app will go live on iOS App Store with 200.1 release. I believe Mac Catalyst support needs to be checked at that point.

Is the loading of on-demand resources coming to Mac Catalyst for final? Not sure if Apple would allow it if there's functionality that doesn't work on Mac Catalyst.

I don't think ODR for Mac will come anytime soon - it is an ancient API. I need to look up some articles to decide the best way to publish an iOS app with Mac Catalyst support that requires many binary files. I have a card for adding a button to download all ODR and maybe Mac Catalyst compiler directives can be added there. 🤔

For now, I think it is OK to leave it as-is, since we are not publishing it to the App Store yet. Also I can see if App Store gives any automated feedback when I upload the dummy app.

@yo1995 yo1995 self-assigned this Dec 5, 2022
@philium
Copy link
Contributor

philium commented Dec 6, 2022

For now, I think it is OK to leave it as-is, since we are not publishing it to the App Store yet.

It may be OK, but it isn't great. I would be easy enough to filter out samples with resource dependencies on Mac Catalyst, so I think we should do that. Alternatively, we could have a build script that copies the resources into the Mac Catalyst framework and then change the logic here slightly:

var usesOnDemandResources: Bool {
#if targetEnvironment(macCatalyst)
    return false
#else
    return sample.hasDependencies
#endif
}

var body: some View {
    Group {
        if usesOnDemandResources {
            ...
        } else {
            ...
        }
    }
    ...
}

Either one of those would be better than including samples that are broken/unusable. Yes, the user could view the README and source code, but they can do that from inside the project (which they'll be using "since we are not publishing it to the App Store yet" or on GitHub.

Samples (iOS).entitlements Outdated Show resolved Hide resolved
@yo1995
Copy link
Collaborator Author

yo1995 commented Dec 6, 2022

I would be easy enough to filter out samples with resource dependencies on Mac Catalyst, so I think we should do that.

I'll go with this approach in this PR as I want to submit the dummy app soon.

Alternatively, we could have a build script that copies the resources into the Mac Catalyst framework and then change the logic slightly

Copying all the resources to the bundle for Mac Catalyst sounds like a good plan. Do you recommend using a Swift script to do this, so I can use #if targetEnvironment(macCatalyst)? Otherwise I'll need to research how to only run a build phase on a certain platform.

@philium
Copy link
Contributor

philium commented Dec 6, 2022

You can't use #if targetEnvironment(macCatalyst) in the build script because the script is always being run on macOS. There should be an environment variable (not sure which one off the top of my head) that can be checked to determine the platform for which the build is being run.

On-demand resources aren't available on Mac Catalyst, thus those sample will display as "Couldn’t communicate with a helper application."
Later these binary resources will be copied to Mac Catalyst bundle, and additional logics will be added to handle that.
See: #93 (comment)
@yo1995
Copy link
Collaborator Author

yo1995 commented Dec 6, 2022

Ready for review.

@philium
Copy link
Contributor

philium commented Dec 6, 2022

I'm sorry, I think I misunderstood. It looks like the multiplatform app template places the entitlements file in the directory alongside the project:

image

That's where we should put it, unless there's a compelling reason to put it elsewhere.

Shared/SamplesApp.swift Outdated Show resolved Hide resolved
@yo1995
Copy link
Collaborator Author

yo1995 commented Dec 6, 2022

I'm sorry, I think I misunderstood. It looks like the multiplatform app template places the entitlements file in the directory alongside the project. That's where we should put it, unless there's a compelling reason to put it elsewhere.

Do you suggest putting it in the Shared folder? This project was originally created by Xcode 13 with a multiplatform template, and at that time it came with Shared and iOS folder. If we stick to this folder structure, I tend to create a macOS folder for this entitlements. The default behavior when I first added Mac Catalyst as a destination, Xcode put the entitlements file under the same level of the project file.

Instead, if we follow the new folder structure, it seems Xcode 14 put everything under the {project_name} folder. That would require renaming >80% files in the project, or at least we need to rename Shared to Samples

image

(Xcode 13 multiplatform folder structure)

Use `targetEnvironment` in a different way.

Co-authored-by: Philip Ridgeway <philip.ridgeway@gmail.com>
@yo1995 yo1995 requested a review from philium December 6, 2022 23:03
@philium
Copy link
Contributor

philium commented Dec 6, 2022

Instead, if we follow the new folder structure, it seems Xcode 14 put everything under the {project_name} folder. That would require renaming >80% files in the project, or at least we need to rename Shared to Samples

I understand if you don't want to do that right now—perhaps in a follow-up PR where the only change is relocating files. I do think it would be good to follow the Xcode 14 template as closely as possible since that's now the minimum version of Xcode. In other words, if we were creating the project today, that's what we'd end up with. Does that make sense? I'll let you decide when and how you want to rename/reorganize things towards that goal.

@yo1995
Copy link
Collaborator Author

yo1995 commented Dec 6, 2022

Thanks. As the project structure change is not very related to Mac Catalyst, I'll address them in a separate #95.

@yo1995 yo1995 merged commit 7ab2654 into v.next Dec 7, 2022
@yo1995 yo1995 deleted the Ting/Add-MacCatalyst branch December 7, 2022 00:07
@yo1995
Copy link
Collaborator Author

yo1995 commented Dec 7, 2022

Thanks for the review!

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.

None yet

4 participants