Adding vmservice to get iOS app settings by chunhtai · Pull Request #123156 · flutter/flutter

chunhtai · 2023-03-21T20:24:21Z

fixes #120405

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 Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
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.

jmagman

Is this a draft or ready for review?

Just in case you didn't know, you can mark PRs as drafts in GitHub itself instead of putting a plug in the title:

packages/flutter_tools/lib/src/ios/plist_parser.dart

packages/flutter_tools/lib/src/xcode_project.dart

jmagman · 2023-04-19T22:19:23Z

packages/flutter_tools/test/general.shard/project_test.dart

Can you add a test where this is nested in a directory, like Runner/DebugProfile.entitlements?

done, added a new test for nested path

jmagman · 2023-04-19T22:20:52Z

packages/flutter_tools/lib/src/vmservice.dart

I forget how this tool is designed, how will you know what params to pass into this service?

The caller will first need to call flutterGetIOSBuildOptions vm services to get all of the possible build options, and then they can pick a combination and call this service

Can a test be written for this in packages/flutter_tools/test/general.shard/vmservice_test.dart?

The flutterGetIOSBuildOptions and this pr are two complete different vm services, and we already have coverage over flutterGetIOSBuildOptions. If such test should exist, I think it should be written in the devtool when it will use these two services. WDYT?

Good point, that makes sense. I was just thinking it could be good to have a test connecting the two so that if someone in the future tweaks one, the test will fail and make them aware that flutterGetIOSDeeplinkSettings relies on flutterGetIOSBuildOptions. But I get your point that technically it doesn't rely on it until it's implemented in the devtool so the testing may be better added then.

chunhtai · 2023-04-19T23:04:11Z

This is ready for review, I forgot to update the title

packages/flutter_tools/lib/src/xcode_project.dart

vashworth

LGTM

christopherfujino

LGTM

jmagman

Sorry for the delay, LGTM

auto-submit · 2023-05-04T16:20:17Z

auto label is removed for flutter/flutter, pr: 123156, due to - The status or check suite ci.yaml validation has failed. Please fix the issues identified (or deflake) before re-applying this label.

auto-submit · 2023-05-04T16:28:12Z

auto label is removed for flutter/flutter, pr: 123156, due to - The status or check suite Mac_arm64 tool_host_cross_arch_tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

)

flutter-dashboard bot added platform-ios iOS applications specifically tool Affects the "flutter" command-line tool. See also t: labels. labels Mar 21, 2023

chunhtai force-pushed the issues/120405-settings branch from addad49 to 34e4f91 Compare March 27, 2023 22:49

chunhtai force-pushed the issues/120405-settings branch 3 times, most recently from 31de2fe to a9df81c Compare April 19, 2023 21:16

chunhtai marked this pull request as ready for review April 19, 2023 21:23

chunhtai requested a review from jmagman April 19, 2023 21:23

jmagman reviewed Apr 19, 2023

View reviewed changes

chunhtai changed the title ~~[Draft] Adding vmservice to get iOS app settings~~ Adding vmservice to get iOS app settings Apr 19, 2023

christopherfujino reviewed Apr 20, 2023

View reviewed changes

packages/flutter_tools/lib/src/xcode_project.dart Outdated Show resolved Hide resolved

christopherfujino reviewed Apr 20, 2023

View reviewed changes

packages/flutter_tools/lib/src/xcode_project.dart Outdated Show resolved Hide resolved

chunhtai force-pushed the issues/120405-settings branch from 104e667 to 05311a4 Compare April 21, 2023 22:22

chunhtai requested review from christopherfujino and jmagman April 21, 2023 22:24

jmagman requested a review from vashworth April 21, 2023 22:41

vashworth approved these changes Apr 27, 2023

View reviewed changes

christopherfujino approved these changes Apr 28, 2023

View reviewed changes

jmagman approved these changes Apr 28, 2023

View reviewed changes

chunhtai force-pushed the issues/120405-settings branch from 05311a4 to 9039e89 Compare April 28, 2023 18:50

chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 28, 2023

chunhtai force-pushed the issues/120405-settings branch 2 times, most recently from 84b5437 to 9ff54a7 Compare May 4, 2023 16:18

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label May 4, 2023

WIP

468d4a6

chunhtai force-pushed the issues/120405-settings branch from 9ff54a7 to 468d4a6 Compare May 4, 2023 16:21

chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label May 4, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label May 4, 2023

chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label May 4, 2023

auto-submit bot merged commit b00f1c4 into flutter:master May 4, 2023

engine-flutter-autoroll mentioned this pull request May 4, 2023

Manual roll Flutter from db6074ade4e4 to 0b657236cae4 (54 revisions) flutter/packages#3908

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 4, 2023

b00f1c4 Adding vmservice to get iOS app settings (flutter/flutter#123156

fa3e9d7

)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2023

b00f1c4 Adding vmservice to get iOS app settings (flutter/flutter#123156

e0e405e

)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

b00f1c4 Adding vmservice to get iOS app settings (flutter/flutter#123156

90b2606

)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

b00f1c4 Adding vmservice to get iOS app settings (flutter/flutter#123156

a5db5d2

)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

b00f1c4 Adding vmservice to get iOS app settings (flutter/flutter#123156

73f2a80

)

Conversation

chunhtai commented Mar 21, 2023

Pre-launch Checklist

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jmagman Apr 19, 2023

Choose a reason for hiding this comment

Uh oh!

chunhtai Apr 21, 2023

Choose a reason for hiding this comment

Uh oh!

jmagman Apr 19, 2023

Choose a reason for hiding this comment

Uh oh!

chunhtai Apr 19, 2023

Choose a reason for hiding this comment

Uh oh!

vashworth Apr 24, 2023

Choose a reason for hiding this comment

Uh oh!

chunhtai Apr 25, 2023

Choose a reason for hiding this comment

Uh oh!

vashworth Apr 27, 2023

Choose a reason for hiding this comment

Uh oh!

chunhtai commented Apr 19, 2023

Uh oh!

Uh oh!

Uh oh!

vashworth left a comment

Choose a reason for hiding this comment

Uh oh!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented May 4, 2023

Uh oh!

auto-submit bot commented May 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants