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.

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

Jump to bottom

[ios_platform_images] migrate objC to swift #4847

Merged
merged 39 commits into from
Oct 21, 2023
Merged

[ios_platform_images] migrate objC to swift #4847

merged 39 commits into from
Oct 21, 2023

Conversation

Mairramer
Copy link
Contributor

@Mairramer Mairramer commented Sep 5, 2023 •
edited by stuartmorgan
Loading

Migrate ios_platform_images to Swift.

Fixes flutter/flutter#119101

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@Mairramer Mairramer marked this pull request as ready for review September 8, 2023 13:25
stuartmorgan
stuartmorgan requested changes Sep 11, 2023
View reviewed changes
Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

It looks like we don't have any end-to-end tests for this plugin at the moment, so I added them in #4899. Once that lands this can be synced with main; that will give us coverage to ensure that the PR isn't breaking anything (as I believe the current version does, per my comments below.)

@stuartmorgan
Copy link
Contributor

Please merge in a version of main newer than #4899 so that we can ensure that the new integration tests still pass.

@stuartmorgan
Copy link
Contributor

Sorry, it turns out they aren't actually run in CI yet; #4920 will need to land and then be merged in here as well.

In the meantime though, you can run them locally using the repository tooling with the arguments: drive-examples --ios --packages=ios_platform_images and see that this breaks the integration tests (as expected since the argument parsing issue I flagged hasn't been fixed yet).

@stuartmorgan
Copy link
Contributor

Since no mocking is necessary in the unit tests, please create a new PR that extracts just the unit tests you've added here, so that we can land that separately in advance of this PR. There have been bugs in this PR that would have been caught that way, such as the incorrect argument parsing, so doing it in two parts will help you find issues in the porting, isolated to specific unit tests.

@stuartmorgan
Copy link
Contributor

Merging in main will now pick up all of the new tests for this plugin (including in CI).

hellohuanlin
hellohuanlin approved these changes Oct 17, 2023
View reviewed changes
Copy link
Contributor

@hellohuanlin hellohuanlin left a comment

Choose a reason for hiding this comment

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

Looks clean with some nits

stuartmorgan
stuartmorgan reviewed Oct 20, 2023
View reviewed changes
packages/ios_platform_images/ios/ios_platform_images.podspec
s.dependency 'Flutter'
s.platform = :ios, '11.0'

# Flutter.framework does not contain a i386 slice. Only x86_64 simulators are supported.
s.pod_target_xcconfig = { 'DEFINES_MODULE' => 'YES' }
Copy link
Contributor

@stuartmorgan stuartmorgan Oct 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This shouldn't be removed; I'll push a commit to re-add it (and remove the bridging header import in the unit test that was only necessary because of its removal).

stuartmorgan
stuartmorgan approved these changes Oct 20, 2023
View reviewed changes
Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

I made a few trivial edits for style; LGTM with those.

@stuartmorgan stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 21, 2023
@auto-submit auto-submit bot merged commit 0fbf368 into flutter:main Oct 21, 2023
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 23, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 23, 2023
@engine-flutter-autoroll
0fbf368 [ios_platform_images] migrate objC to swift (flutter/packages…
592640b 
…#4847)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Oct 23, 2023
@engine-flutter-autoroll
Roll Packages from be915be to 4bf5114 (12 revisions) (#137062)
6dc5d2f 
flutter/packages@be915be...4bf5114

2023-10-23 [email protected] [camera] CameraPlatform.createCameraWithSettings (flutter/packages#3615)
2023-10-23 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.22.3 to 2.22.4 (flutter/packages#5201)
2023-10-22 [email protected] Roll Flutter from 6f4850d to 823e083 (3 revisions) (flutter/packages#5198)
2023-10-21 49699333+dependabot[bot]@users.noreply.github.com Bump actions/checkout from 4.1.0 to 4.1.1 (flutter/packages#5167)
2023-10-21 [email protected] Roll Flutter from 0883cb2 to 6f4850d (24 revisions) (flutter/packages#5196)
2023-10-21 [email protected] [ios_platform_images]  migrate objC to swift (flutter/packages#4847)
2023-10-20 49699333+dependabot[bot]@users.noreply.github.com [in_app_pur]: Bump org.json:json from 20230618 to 20231013 in /packages/in_app_purchase/in_app_purchase_android/example/android/app (flutter/packages#5149)
2023-10-20 49699333+dependabot[bot]@users.noreply.github.com [in_app_pur]: Bump org.json:json from 20230618 to 20231013 in /packages/in_app_purchase/in_app_purchase_android/android (flutter/packages#5150)
2023-10-20 [email protected] [quick_actions] convert to pigeon (flutter/packages#5159)
2023-10-20 [email protected] [ci] Add build-only Windows Arm64 tests (flutter/packages#5142)
2023-10-20 [email protected] Add '--no-tree-shake-icons' option to `BenchmarkServer` (flutter/packages#5186)
2023-10-20 [email protected] Roll Flutter from c2bd2c1 to 0883cb2 (24 revisions) (flutter/packages#5192)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
sybrands-place pushed a commit to sybrands-place/packages that referenced this pull request Oct 24, 2023
Merge branch 'main' into webview_patchedv5
813131e 
* main: (139 commits)
  Change firebase device tests from Android api 30 to 33 (flutter#5185)
  [url_launcher] Add an `inAppBrowserView` mode (flutter#5205)
  [ci] Remove device_type property from Windows Arm64 properties (flutter#5193)
  [url_launcher_web] Disallows launching "javascript:" URLs. (flutter#5180)
  [local_auth]: Bump androidx.fragment:fragment from 1.6.0 to 1.6.1 in /packages/local_auth/local_auth_android/android (flutter#4600)
  Roll Flutter from 823e083 to 5e8b5f4 (13 revisions) (flutter#5208)
  [tool] Add optional swift-format support (flutter#5204)
  [camera] CameraPlatform.createCameraWithSettings (flutter#3615)
  Bump github/codeql-action from 2.22.3 to 2.22.4 (flutter#5201)
  Roll Flutter from 6f4850d to 823e083 (3 revisions) (flutter#5198)
  Bump actions/checkout from 4.1.0 to 4.1.1 (flutter#5167)
  Roll Flutter from 0883cb2 to 6f4850d (24 revisions) (flutter#5196)
  [ios_platform_images]  migrate objC to swift (flutter#4847)
  [in_app_pur]: Bump org.json:json from 20230618 to 20231013 in /packages/in_app_purchase/in_app_purchase_android/example/android/app (flutter#5149)
  [in_app_pur]: Bump org.json:json from 20230618 to 20231013 in /packages/in_app_purchase/in_app_purchase_android/android (flutter#5150)
  [quick_actions] convert to pigeon (flutter#5159)
  [ci] Add build-only Windows Arm64 tests (flutter#5142)
  Add '--no-tree-shake-icons' option to `BenchmarkServer` (flutter#5186)
  Roll Flutter from c2bd2c1 to 0883cb2 (24 revisions) (flutter#5192)
  [ci] Finalize migration to x64 specific Windows platform (flutter#5174)
  ...
HugoOlthof pushed a commit to moneybird/packages that referenced this pull request Dec 13, 2023
@Mairramer @HugoOlthof
[ios_platform_images] migrate objC to swift (flutter#4847)
795b4a5 
`ios_platform_images` part of flutter/flutter#119101

Migrate ios_platform_images to Swift.
@reidbaker reidbaker mentioned this pull request Jan 12, 2024
Closed
@reidbaker reidbaker mentioned this pull request Jan 19, 2024
Merged
12 tasks
@reidbaker reidbaker mentioned this pull request Feb 19, 2024
Merged
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hellohuanlin hellohuanlin hellohuanlin approved these changes

@stuartmorgan stuartmorgan stuartmorgan approved these changes

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: ios_platform_images platform-ios
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ios_platform_images] iOS Swift migration
3 participants
@Mairramer @stuartmorgan @hellohuanlin