Update device selection to wait for wireless devices to load by vashworth · Pull Request #122932 · flutter/flutter

vashworth · 2023-03-17T22:31:55Z

This PR updates the device selection flow to allow more time for wireless devices to load.

If the terminal has stdin and supports ANSI, a message is displayed while wireless devices are loading and other devices can be selected. After they are done loading, it will clear out the message and update with the device list

Connected devices:
target-device-1 (mobile) • xxx • ios • iOS 16
target-device-2 (mobile) • xxx • ios • iOS 16

Checking for wireless devices...

[1]: target-device-1 (xxx)
[2]: target-device-2 (xxx)

After done loading:

Multiple devices found:
target-device-1 (mobile) • xxx • ios • iOS 16
target-device-2 (mobile) • xxx • ios • iOS 16

Wirelessly connected devices:
target-device-5 (mobile) • xxx • ios • iOS 16

[1]: target-device-1 (xxx)
[2]: target-device-2 (xxx)
[3]: target-device-5 (xxx)

Fixes #118109.

Part 4 in breakdown of #121262.

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.

…iscovery_part_4

packages/flutter_tools/lib/src/commands/devices.dart

packages/flutter_tools/lib/src/runner/flutter_command.dart

jmagman · 2023-03-21T18:18:00Z

packages/flutter_tools/lib/src/runner/target_devices.dart

 }
+
+@visibleForTesting
+class MacOSTargetDevices extends TargetDevices {


This name seems like it's deploying a macOS desktop app?

I changed it to TargetDevicesWithExtendedWirelessDeviceDiscovery. I changed the one in the devices command too to DevicesCommandOutputWithExtendedWirelessDeviceDiscovery. Let me know if you think that's better

jmagman · 2023-03-21T18:24:07Z

packages/flutter_tools/lib/src/runner/target_devices.dart

+    final RegExp pattern = RegExp(r'\d+$|q', caseSensitive: false);
+    final String prompt = userMessages.flutterChooseOne;
+    String? choice;
+    globals.terminal.singleCharMode = true;


Can you use promptForCharInput instead?

👍

Looks like you'll have to check first if the user is in an interactive session (unless the callers of readUserInput already guard that).

Not without changing it extensively. promptForCharInput requires a list of acceptable characters and then only displays and allows input of those characters. With the new process, more devices might be loaded while promptForCharInput is still waiting for user input and it can't be updated to use a new list of acceptable characters.

this is fine

christopherfujino · 2023-03-21T18:35:20Z

packages/flutter_tools/test/commands.shard/hermetic/devices_test.dart

-    setUp(() {
-      cache = Cache.test(processManager: FakeProcessManager.any());
-      platform = FakePlatform();
+    testUsingContext('Ensure factory returns MacOSTargetDevices on MacOS', () async {


Does this and the next test actually rely on the context, or can they be invoked with testWithoutContext()? And if they do rely on the context, should we be overriding those values with fake objects?

It doesn't need the context, good catch. Fixed.

Follow up question, though. Sometimes when it does need the context to be overridden but it doesn't need anything more custom than the default overrides (see below), is it okay to use testUsingContext without extra overrides or is it better practice to always add the overrides it needs to the test?

flutter/packages/flutter_tools/test/src/context.dart

Lines 104 to 122 in b42e8db

AnsiTerminal: () => AnsiTerminal(platform: globals.platform, stdio: globals.stdio),

Config: () => buildConfig(globals.fs),

DeviceManager: () => FakeDeviceManager(),

Doctor: () => FakeDoctor(globals.logger),

FlutterVersion: () => FakeFlutterVersion(),

HttpClient: () => FakeHttpClient.any(),

IOSSimulatorUtils: () => const NoopIOSSimulatorUtils(),

OutputPreferences: () => OutputPreferences.test(),

Logger: () => BufferLogger.test(),

OperatingSystemUtils: () => FakeOperatingSystemUtils(),

PersistentToolState: () => buildPersistentToolState(globals.fs),

Usage: () => TestUsage(),

XcodeProjectInterpreter: () => FakeXcodeProjectInterpreter(),

FileSystem: () => LocalFileSystemBlockingSetCurrentDirectory(),

PlistParser: () => FakePlistParser(),

Signals: () => FakeSignals(),

Pub: () => ThrowingPub(), // prevent accidentally using pub.

CrashReporter: () => const NoopCrashReporter(),

TemplateRenderer: () => const MustacheTemplateRenderer(),

This is a good question. In answer to your question, it's fine to rely on the default (in contradiction of my earlier suggestion). In terms of improving explicitness and readability, the real fix is migrating completely to testWithoutContext.

christopherfujino · 2023-03-21T18:39:50Z

packages/flutter_tools/lib/src/base/logger.dart

-        _slowWarning = slowWarningCallback!();
+      final SlowWarningCallback? callback = slowWarningCallback;
+      if (_slowWarning.isEmpty && callback != null) {
+        final TerminalColor? color = warningColor;


I'm not familiar with how we handle colors in the tool. Is there a reason why we can't make this a global color? Is it because some warnings are red and some the normal text color? Or is it a difference of host platforms?

It's because some are normal text color. Before this change, all were normal text color. I added this so that the warning could be red

… extended discovery when discovery timeout is set

christopherfujino

LGTM

…iscovery_part_4

jmagman

LGTM!

jmagman · 2023-03-28T16:35:50Z

@XilaiZhang @CaseyHillers do you know why FRoB says "Do not test" for this PR? And why Google testing check is still pending then?

CaseyHillers · 2023-03-28T17:10:54Z

@XilaiZhang @CaseyHillers do you know why FRoB says "Do not test" for this PR? And why Google testing check is still pending then?

This PR has failures with Google Testing. For some unknown reason, the state changed from "Failed" to "Pending" after ~12 hours. Please do not skip the Google Testing check on this PR, as this is causing real failures.

…adb error

vashworth · 2023-03-28T21:32:11Z

@jmagman So I'm fairly confident the issue with g3 was that adb does not like being run multiple times at the same time, and would throw errors which would cause the tool to exit.

flutter/packages/flutter_tools/lib/src/android/android_device_discovery.dart

Lines 75 to 78 in e50e99d

    
           throwToolExit( 
        
             'Unable to run "adb", check your Android SDK installation and ' 
        
             '$kAndroidSdkRoot environment variable: ${exception.executable}', 
        
           );

So I updated the flow of things a bit so only iOS discovery runs simultaneously.

Can you review the newest commit?

…lutter/flutter#122932)

…#122932) Update device selection to wait for wireless devices to load

…lutter/flutter#122932)

vashworth added 3 commits March 17, 2023 16:12

Update device selection to wait for wireless devices to load

7ebfa28

move waiting for device to connect to TargetDevices and rename variable

b0b7ea9

remove unused class

9617deb

flutter-dashboard bot added a: text input Entering text in a text field or keyboard related problems platform-ios iOS applications specifically tool Affects the "flutter" command-line tool. See also t: labels. labels Mar 17, 2023

vashworth added 2 commits March 20, 2023 16:10

remove unused imports

1c6cc8a

Merge branch 'master' into wireless_device_discovery_part_4

5b63353

vashworth marked this pull request as ready for review March 20, 2023 22:56

Merge remote-tracking branch 'upstream/master' into wireless_device_d…

2579fc4

…iscovery_part_4

vashworth requested review from christopherfujino and jmagman March 20, 2023 23:49

jmagman reviewed Mar 21, 2023

View reviewed changes

christopherfujino reviewed Mar 21, 2023

View reviewed changes

vashworth added 2 commits March 22, 2023 13:21

renaming, add comments, replace usage of globals, add check to not do…

11ed0be

… extended discovery when discovery timeout is set

fix discovery timeout check

cad3700

christopherfujino approved these changes Mar 22, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/master' into wireless_device_d…

6a174b2

…iscovery_part_4

jmagman approved these changes Mar 23, 2023

View reviewed changes

Merge branch 'master' into wireless_device_discovery_part_4

74f2d0b

Avoid Android discovery from running twice simultaneously to prevent …

e50e99d

…adb error

jmagman approved these changes Mar 28, 2023

View reviewed changes

vashworth added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 29, 2023

auto-submit bot merged commit fa01649 into flutter:master Mar 29, 2023

engine-flutter-autoroll mentioned this pull request Mar 29, 2023

Roll Flutter from 12b36de8f7ac to fa01649a5970 (17 revisions) flutter/packages#3583

Closed

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

fa01649 Update device selection to wait for wireless devices to load (f…

d1f5d82

…lutter/flutter#122932)

engine-flutter-autoroll mentioned this pull request Mar 30, 2023

Roll Flutter from 12b36de8f7ac to 7b54a30e317c (43 revisions) flutter/packages#3591

Closed

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

fa01649 Update device selection to wait for wireless devices to load (f…

ebf90b8

…lutter/flutter#122932)

engine-flutter-autoroll mentioned this pull request Mar 30, 2023

Roll Flutter from 12b36de8f7ac to 8afd600e79b4 (44 revisions) flutter/packages#3592

Closed

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

fa01649 Update device selection to wait for wireless devices to load (f…

6aed63a

…lutter/flutter#122932)

engine-flutter-autoroll mentioned this pull request Mar 30, 2023

Roll Flutter from 12b36de8f7ac to 302c087738a4 (46 revisions) flutter/packages#3593

Closed

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

fa01649 Update device selection to wait for wireless devices to load (f…

c567aba

…lutter/flutter#122932)

engine-flutter-autoroll mentioned this pull request Mar 30, 2023

Roll Flutter from 12b36de8f7ac to 8c5d223bab70 (49 revisions) flutter/packages#3594

Closed

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

fa01649 Update device selection to wait for wireless devices to load (f…

a292174

…lutter/flutter#122932)

engine-flutter-autoroll mentioned this pull request Mar 30, 2023

Roll Flutter from 12b36de8f7ac to 5f7117948072 (50 revisions) flutter/packages#3595

Closed

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

fa01649 Update device selection to wait for wireless devices to load (f…

0d096fa

…lutter/flutter#122932)

engine-flutter-autoroll mentioned this pull request Mar 30, 2023

Roll Flutter from 12b36de8f7ac to a28aae918eae (51 revisions) flutter/packages#3597

Closed

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

fa01649 Update device selection to wait for wireless devices to load (f…

b87bff6

…lutter/flutter#122932)

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

fa01649 Update device selection to wait for wireless devices to load (f…

f8a6bf2

…lutter/flutter#122932)

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

fa01649 Update device selection to wait for wireless devices to load (f…

229c9d8

…lutter/flutter#122932)

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

fa01649 Update device selection to wait for wireless devices to load (f…

2256d74

…lutter/flutter#122932)

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

fa01649 Update device selection to wait for wireless devices to load (f…

21ebf42

…lutter/flutter#122932)

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

fa01649 Update device selection to wait for wireless devices to load (f…

6e687dc

…lutter/flutter#122932)

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

fa01649 Update device selection to wait for wireless devices to load (f…

37def68

…lutter/flutter#122932)

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

fa01649 Update device selection to wait for wireless devices to load (f…

0d63581

…lutter/flutter#122932)

reidbaker mentioned this pull request Apr 1, 2023

Add Java-Gradle-AGP validation to flutter analyze #123916

Merged

8 tasks

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

fa01649 Update device selection to wait for wireless devices to load (f…

772981b

…lutter/flutter#122932)

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

fa01649 Update device selection to wait for wireless devices to load (f…

85ed930

…lutter/flutter#122932)

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

fa01649 Update device selection to wait for wireless devices to load (f…

6b8a8dd

…lutter/flutter#122932)

reidbaker mentioned this pull request Apr 5, 2023

Update getGradleVersion to ignore commented out lines #124260

Merged

8 tasks

vashworth mentioned this pull request Apr 7, 2023

Adding wireless debugging information to the docs flutter/website#8456

Merged

6 tasks

exaby73 pushed a commit to NevercodeHQ/flutter that referenced this pull request Apr 17, 2023

Update device selection to wait for wireless devices to load (flutter…

da9e629

…#122932) Update device selection to wait for wireless devices to load

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

fa01649 Update device selection to wait for wireless devices to load (f…

989d282

…lutter/flutter#122932)

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

fa01649 Update device selection to wait for wireless devices to load (f…

6b3cd56

…lutter/flutter#122932)

vashworth deleted the wireless_device_discovery_part_4 branch September 27, 2023 15:36

jmagman mentioned this pull request Jan 30, 2024

iOS device discovery happening twice on flutter run #142558

Open

	AnsiTerminal: () => AnsiTerminal(platform: globals.platform, stdio: globals.stdio),
	Config: () => buildConfig(globals.fs),
	DeviceManager: () => FakeDeviceManager(),
	Doctor: () => FakeDoctor(globals.logger),
	FlutterVersion: () => FakeFlutterVersion(),
	HttpClient: () => FakeHttpClient.any(),
	IOSSimulatorUtils: () => const NoopIOSSimulatorUtils(),
	OutputPreferences: () => OutputPreferences.test(),
	Logger: () => BufferLogger.test(),
	OperatingSystemUtils: () => FakeOperatingSystemUtils(),
	PersistentToolState: () => buildPersistentToolState(globals.fs),
	Usage: () => TestUsage(),
	XcodeProjectInterpreter: () => FakeXcodeProjectInterpreter(),
	FileSystem: () => LocalFileSystemBlockingSetCurrentDirectory(),
	PlistParser: () => FakePlistParser(),
	Signals: () => FakeSignals(),
	Pub: () => ThrowingPub(), // prevent accidentally using pub.
	CrashReporter: () => const NoopCrashReporter(),
	TemplateRenderer: () => const MustacheTemplateRenderer(),

Conversation

vashworth commented Mar 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

jmagman commented Mar 28, 2023

Uh oh!

CaseyHillers commented Mar 28, 2023

Uh oh!

vashworth commented Mar 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vashworth commented Mar 17, 2023 •

edited

Loading