Fix for Mac Catalyst #247

0x0c · 2020-10-01T06:28:13Z

Hi, I fixed the issue that this repo could not build using Mac Catalyst.
To address this issue, I added #if targetEnvironment(macCatalyst) to ignore using [NSLayoutManager hyphenationFactor] that is deprecated in iOS13+.

ephread

I never used Catalyst, and never thought about using Instructions in Catalyst, so thanks a lot for this PR! I asked a couple of questions, because I'm curious about what it entails.

I'm requesting changes only because of the .DS_Store that creeped in the PR. It needs to be removed. I have them ignored in a global gitconfig, but you can add it to the project gitignore if you want.

Everything else looks good to me, thanks again!

ephread · 2020-10-03T10:08:40Z

Examples/Example/Snapshot Tests/BaseSnapshotTestCase.swift

@@ -1,6 +1,8 @@
 // Copyright (c) 2018-present Frédéric Maquin <fred@ephread.com> and contributors.
 // Licensed under the terms of the MIT License.

+#if targetEnvironment(macCatalyst)


Quick question about the snapshot tests and catalyst. Are they conditionally excluded because iOSSnapshotTestCase doesn't support catalyst? Did you run into issues?

I could not use iOSSnapshotTestCase on macOS as following error.

I tried carthage update --platform macOS but I got an error Dependency "ios-snapshot-test-case" has no shared framework schemes for any of the platforms: Mac.

I think iOSSnapshotTestCase does not support macOS.

ephread · 2020-10-03T10:10:11Z

Examples/Example/Sources/Core/Custom Views/CustomCoachMarkBodyView.swift

@@ -50,7 +50,10 @@ internal class CustomCoachMarkBodyView: UIView, CoachMarkBodyView {
        self.hintLabel.font = UIFont.systemFont(ofSize: 15.0)
        self.hintLabel.isScrollEnabled = false
        self.hintLabel.textAlignment = .justified
+        #if targetEnvironment(macCatalyst)
+        #else
        self.hintLabel.layoutManager.hyphenationFactor = 1.0


👍 I'll do a sweep after this PR to remove them, I didn't realised it was deprecated in iOS 14 and unavailable in Catalyst.

ephread · 2020-10-03T10:11:28Z

Examples/InstructionsExample.entitlements

+<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+<plist version="1.0">
+<dict>
+	<key>com.apple.security.app-sandbox</key>


Are these entitlements required by Catalyst?

To run an app on macOS Catalina, we have to notarize the app. This file is added by Xcode when I ran an example app on my mac. So I think, as you mentioned, the entitlements required by Catalyst.

0x0c · 2020-10-05T05:07:41Z

Thank you for your review!

I'm requesting changes only because of the .DS_Store that creeped in the PR. It needs to be removed. I have them ignored in a global gitconfig, but you can add it to the project gitignore if you want.

OK, I'll add new .gitignore which is generated from gitignore.io.
https://www.toptal.com/developers/gitignore/api/macos,windows,linux,swift,swiftpackagemanager,swiftpm,cocoapods,carthage,objective-c,ruby

Please check my changes again :)

0x0c · 2020-10-14T15:27:53Z

I can't understand why GitHub action fail :(
This commit (bba190c) passed the action and I didn't change any code.

Do you have any ideas? @ephread

ephread · 2020-10-14T16:30:59Z

@0x0c I'll have a look, sometimes it's just because XCUITest isn't great great and there's a race condition I've never been able to eliminate. It won't prevent the merge.

That said, 👍 for using a generated .gitignore, but in the process you removed some of the ignores I had put there previously (the most important one are **/Carthage/Checkouts and **/Carthage/Build). Can you add re-add these ignores in the right section? Thanks!

## Build generated
Tests

## Other
*.xccheckout
*.moved-aside
*.xcuserstate
*.xcscmblueprint

## Carthage
**/Carthage/Checkouts
**/Carthage/Build

Skassets
.swiftpm/

Hang in there, it's nearly merged. Thanks for your work!

0x0c · 2020-10-14T17:09:04Z

@ephread Thanks for your quick reply!
OK, I'll add some lines to .gitignore as you mentioned, but I have some questions and need your opinions before adding the lines you suggested.

First, there are some lines to ignore files that are generated by Carthage(See line 9 and line 11 in .gitignore). Isn't it enough? (Don't worry, I'll remove # from line 9.)
Second, what is purpose of adding xcuserstate? I think *.xcuserstate files may be ignored by xcuserdata/ at line 69. (See an attached image.)
Third, what is purpose of adding Tests and Skassets? I've never seen that and I'm interested in advantages for ignoring these files.

Thank you!

FYI: These lines are already added by gitignore.io :)

*.xccheckout
*.moved-aside
*.xcscmblueprint

See https://github.com/0x0c/Instructions/blob/main/.gitignore#L72 to https://github.com/0x0c/Instructions/blob/main/.gitignore#L78.

ephread · 2020-10-14T17:25:51Z

@0x0c

Thanks for your quick reply!

Cheers!

I have some questions and need your opinions before adding the lines you suggested.

Sure, happy to answer them. 🙂

First, there are some lines to ignore files that are generated by Carthage(See line 9 and line 11 in .gitignore). Isn't it enough? (Don't worry, I'll remove # from line 9.)

It might just be. The original reason for the double stars is that Carthage is nested somewhere inside Example/. I don't remember whether I encountered some issues, if this used to be a requirement in an older version of git or if it's actually the appropriate way to target nested directories.

Second, what is purpose of adding xcuserstate? I think *.xcuserstate files may be ignored by xcuserdata/ at line 69. (See an attached image.)

Yes, I think you're right. It's probably redundant. I simply didn't want to have stuff removed from the .gitgnore and accidentally commit files that were previously ignored. I'm being busy-lazy, since it's working, I don't want to break it.

Third, what is purpose of adding Tests and Skassets? I've never seen that and I'm interested about advantages for ignoring these files.

So these are tailored to Instructions. Tests is generated by Slather and SKAssets is just and old directory I have on my local setup with Sketch resources.

FYI: These lines are already added by gitignore.io :)

Sweet, I missed them, that's a good catch!

Bottom line, the less change, the better!

0x0c · 2020-10-14T17:33:10Z

Oh, I found an answer of the first question. The files that are generated by Carthage are not placed at the top level of this repository and it isn't enough to ignore the files. So, I suggest adding new .gitignore to inside of Example directory, is it ok? It may same file as current .gitignore.

0x0c · 2020-10-14T17:42:03Z

@ephread Oh no, we got a race condition lol. I posted my comment after you replied before I reload this page. Sorry.
I got it and I'll add the lines at the end of .gitignore.

How about my suggestion?

The files that are generated by Carthage are not placed at the top level of this repository and it isn't enough to ignore the files. So, I suggest adding new .gitignore to inside of Example directory, is it ok? It may same file as current .gitignore.

ephread · 2020-10-14T17:46:40Z

@0x0c Haha, no worries 😂 We're like XCUITest now!

How about my suggestion?

I think splitting the .gitignore will make things harder to maintain for me in the long run so I would prefer to keep everything in a top level file. I think **/Carthage would also match a top level Carthage directory, but feel free to have both patterns if it makes more sense.

Fix for some request to change. See ephread#247 (comment)

0x0c · 2020-10-14T17:52:10Z

@ephread I agree your opinion. I updated .gitignore just now. Please take a look :) 0x0c@99cb0d8

I thank for your polite reviews!

ephread · 2020-10-14T17:56:39Z

@0x0c almost there! The SwiftPM and Carthage files previous ignored are still committed in the PR, just need to remove them and we're good to go!

I thank for your polite reviews!

All good, thanks for working on this!

Fix for some request to change. See ephread#247 (comment)

0x0c · 2020-10-14T18:23:02Z

@ephread I removed Example/Carthage/ and .swiftpm directory. I made mistakes removing files and I noticed after push to main. So I checked out 99cb0d8 and make HEAD as main to remove some commits (e.g. 4065c10), and force-pushed to my repository.

ephread · 2020-10-14T19:03:14Z

Looks good to me. I'm going to merge, have a look at the tests later this week and hopefully release a new version this weekend! 🎉

Thanks again for your great work!

0x0c · 2020-10-14T19:04:04Z

Thanks a lot!

DisobedientMedia · 2020-11-03T11:31:03Z

Hi ephread. Thanks for looking into merging this. What's the current status? I'm hitting this problem myself today.

edit: Ah, sorry, I see you have already merged this into main. So I guess my question is actually - This change isnt coming up in my Swift Package Manager version. Do you need to make a release for this. Am I missing something - only just moved to SWP and still working it out.

Thanks

ephread · 2020-11-03T15:32:15Z

This change isnt coming up in my Swift Package Manager version. Do you need to make a release for this. Am I missing something - only just moved to SWP and still working it out.

@DisobedientMedia you probably need to point to the main branch in Xcode / Swift Package Manager as it's not released yet. I planned to create a new release a while ago, but it got postponed because I wanted to fix the race condition found in the tests first. I'll try to make a release this weekend regardless!

0x0c added 2 commits October 1, 2020 15:21

Fix for Mac Catalyst

3cbe9bc

Fix tests

bba190c

ephread reviewed Oct 3, 2020

View reviewed changes

0x0c added 2 commits October 5, 2020 14:14

update .gitignore

a36feb4

Add some files that are not ignored by .gitignore

19dbd49

Update .gitignore

e18915b

Fix for some request to change. See ephread#247 (comment)

Update .gitignore

99cb0d8

0x0c added a commit to 0x0c/Instructions that referenced this pull request Oct 14, 2020

Update .gitignore

4065c10

Fix for some request to change. See ephread#247 (comment)

0x0c force-pushed the main branch from 931c1ff to 99cb0d8 Compare October 14, 2020 18:15

Remove files that should be ignored.

1ed9e5b

ephread merged commit d1fb4bc into ephread:main Oct 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for Mac Catalyst #247

Fix for Mac Catalyst #247

0x0c commented Oct 1, 2020

ephread left a comment

ephread Oct 3, 2020

0x0c Oct 5, 2020

ephread Oct 3, 2020

0x0c Oct 5, 2020

ephread Oct 3, 2020

0x0c Oct 5, 2020

0x0c commented Oct 5, 2020 •

edited

Loading

0x0c commented Oct 14, 2020 •

edited

Loading

ephread commented Oct 14, 2020 •

edited

Loading

0x0c commented Oct 14, 2020 •

edited

Loading

ephread commented Oct 14, 2020

0x0c commented Oct 14, 2020 •

edited

Loading

0x0c commented Oct 14, 2020 •

edited

Loading

ephread commented Oct 14, 2020

0x0c commented Oct 14, 2020 •

edited

Loading

ephread commented Oct 14, 2020

0x0c commented Oct 14, 2020 •

edited

Loading

ephread commented Oct 14, 2020

0x0c commented Oct 14, 2020

DisobedientMedia commented Nov 3, 2020 •

edited

Loading

ephread commented Nov 3, 2020

Fix for Mac Catalyst #247

Fix for Mac Catalyst #247

Conversation

0x0c commented Oct 1, 2020

ephread left a comment

Choose a reason for hiding this comment

ephread Oct 3, 2020

Choose a reason for hiding this comment

0x0c Oct 5, 2020

Choose a reason for hiding this comment

ephread Oct 3, 2020

Choose a reason for hiding this comment

0x0c Oct 5, 2020

Choose a reason for hiding this comment

ephread Oct 3, 2020

Choose a reason for hiding this comment

0x0c Oct 5, 2020

Choose a reason for hiding this comment

0x0c commented Oct 5, 2020 • edited Loading

0x0c commented Oct 14, 2020 • edited Loading

ephread commented Oct 14, 2020 • edited Loading

0x0c commented Oct 14, 2020 • edited Loading

ephread commented Oct 14, 2020

0x0c commented Oct 14, 2020 • edited Loading

0x0c commented Oct 14, 2020 • edited Loading

ephread commented Oct 14, 2020

0x0c commented Oct 14, 2020 • edited Loading

ephread commented Oct 14, 2020

0x0c commented Oct 14, 2020 • edited Loading

ephread commented Oct 14, 2020

0x0c commented Oct 14, 2020

DisobedientMedia commented Nov 3, 2020 • edited Loading

ephread commented Nov 3, 2020

0x0c commented Oct 5, 2020 •

edited

Loading

0x0c commented Oct 14, 2020 •

edited

Loading

ephread commented Oct 14, 2020 •

edited

Loading

0x0c commented Oct 14, 2020 •

edited

Loading

0x0c commented Oct 14, 2020 •

edited

Loading

0x0c commented Oct 14, 2020 •

edited

Loading

0x0c commented Oct 14, 2020 •

edited

Loading

0x0c commented Oct 14, 2020 •

edited

Loading

DisobedientMedia commented Nov 3, 2020 •

edited

Loading