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

Merge upstream 3.2.0 #22

Merged
merged 199 commits into from
Aug 25, 2023
Merged

Merge upstream 3.2.0 #22

merged 199 commits into from
Aug 25, 2023

Conversation

manicmaniac
Copy link
Member

@manicmaniac manicmaniac commented Aug 24, 2023

Overview

  • Merged upstream/3.2.0 into master at 3fde158
  • Resolved a number of conflicts at d79d939 and ab772c4
  • Fixed unit test at c08c3dc
  • Resolved CI problems at 86bccf0 and the later commits

You don't need to see other commits.

Known issues

Example app cannnot be built as reported in MessageKit#1441

To workaround this, cherry-pick the PR.

git cherry-pick 94fd0709c1b901dd0e6586670d879c1517d2442f

Screenshot

image

@manicmaniac manicmaniac self-assigned this Aug 24, 2023
@manicmaniac manicmaniac force-pushed the merge-upstream-3.2.0 branch 5 times, most recently from 98c3dc1 to 23d215a Compare August 24, 2023 07:59
@github-actions
Copy link

github-actions bot commented Aug 24, 2023

4 Warnings
⚠️ Big Pull Request - Please consider splitting up your changes into smaller Pull Requests.
⚠️ Sources/Layout/MessageSizeCalculator.swift#L111 - Function body should span 40 lines or less excluding comments and whitespace: currently spans 43 lines
function_body_length MessageSizeCalculator.swift:111
⚠️ Sources/Layout/MessagesCollectionViewFlowLayout.swift#L181 - Function should have complexity 10 or less: currently complexity equals 12
cyclomatic_complexity MessagesCollectionViewFlowLayout.swift:181
⚠️ Sources/Views/Cells/MessageContentCell.swift#L348 - Limit vertical whitespace to a single empty line. Currently 2.
vertical_whitespace MessageContentCell.swift:348

Generated by 🚫 Danger

=======
let cells = ["Basic Example", "Advanced Example", "Autocomplete Example", "Embedded Example", "Subview Example", "SwiftUI Example", "Settings", "Source Code", "Contributors"]
>>>>>>> theirs
let cells = ["Announcement Example", "Basic Example", "Advanced Example", "Autocomplete Example", "Embedded Example", "Subview Example", "SwiftUI Example", "Settings", "Source Code", "Contributors"]
Copy link
Member Author

Choose a reason for hiding this comment

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

Announcement feature was added by @m-sugawara.

Comment on lines 29 to 32
case leftBubble
case rightBubble
case announcement
case warning(color: UIColor)
Copy link
Member Author

Choose a reason for hiding this comment

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

We heavily customize MessageStyle. Upstream changes are simply ignored.

@@ -50,7 +50,7 @@ class AvatarViewTests: XCTestCase {
func testWithImage() {
let avatar = Avatar(image: nil)
avatarView.set(avatar: avatar)
XCTAssertEqual(avatar.initials, "?")
XCTAssertEqual(avatar.initials, "")
Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -35,14 +35,14 @@ MODE="$1"

if [ "$MODE" = "tests" -o "$MODE" = "all" ]; then
echo "Running MessageKit tests."
carthage bootstrap --platform ios
carthage checkout && ./GitHubActions/install-dependencies.sh
Copy link
Member Author

Choose a reason for hiding this comment

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

Back then, Carthage did not support xcframework.
Nowadays we need to use xcframework to build on both Intel Mac and Apple Silicon like carthage bootstrap --platform ios --use-xcframeworks.

However, to use xcframeworks need to vastly change project settings, which leads conflicts in future update.

So I added ./GitHubActions/install-dependencies.sh script to build frameworks using xcodebuild directly.

Comment on lines +24 to +34
xcodebuild build \
-project "$project" \
-scheme "$scheme" \
-configuration Debug \
-derivedDataPath "$derived_data_dir" \
-sdk iphonesimulator \
-quiet \
ONLY_ACTIVE_ARCH=YES \
CODE_SIGNING_REQUIRED=NO \
CODE_SIGN_IDENTITY= \
CARTHAGE=YES
Copy link
Member Author

Choose a reason for hiding this comment

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

This is almost same as Carthage does but this script builds framework for active architecture only.

@@ -5,7 +5,7 @@ on: pull_request
jobs:
tests:
name: Build Example app
runs-on: macOS-latest
runs-on: macOS-11
Copy link
Member Author

@manicmaniac manicmaniac Aug 24, 2023

Choose a reason for hiding this comment

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

The current macOS-latest is equivalent to macOS-12.
At that time, macOS-11 should be the latest.

upstream/3.2.0 dependes on Ruby 2.7 which is not installed in macOS-12 but macOS-11.

@manicmaniac manicmaniac marked this pull request as ready for review August 24, 2023 11:50
@manicmaniac manicmaniac force-pushed the merge-upstream-3.2.0 branch 2 times, most recently from 12f4721 to d3ba066 Compare August 25, 2023 05:47
Example/Podfile Outdated
Comment on lines 20 to 33
# Workaround for https://github.com/pinterest/PINRemoteImage/issues/566
post_install do |installer|
installer.pods_project.targets.each do |target|
next unless %w[PINCache PINRemoteImage].include?(target.name)

target.build_configurations.each do |config|
config.build_settings['OTHER_CFLAGS'] = [
'$(inherited)',
'-Xclang',
'-fcompatibility-qualified-id-block-type-checking'
].join(' ')
end
end
end
Copy link
Member Author

Choose a reason for hiding this comment

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

One of the dependencies PINRemoteImage (and its subspec PINCache) is not compatible with Xcode 12.
The compiler complains that there's incompatible block pointers.

So I added workaround to disable this warning by passing extra flags.

Copy link

@k-kohey k-kohey left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@elliekwon elliekwon left a comment

Choose a reason for hiding this comment

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

Overviewに書かれてるcommitで疑問はありません!LGTM🚀

@manicmaniac manicmaniac merged commit 9772e58 into master Aug 25, 2023
4 checks passed
@manicmaniac manicmaniac changed the title Merge upstream/3.2.0 Merge upstream 3.2.0 Aug 25, 2023
@manicmaniac manicmaniac mentioned this pull request Aug 28, 2023
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.