Skip to content

Conversation

@jonahwilliams
Copy link
Contributor

Description

Adding desktop and web support will have the effect of increasing the number of available devices beyond those just plugged in. We should be smart about what we present to our users, so they do not get errors if attempting to run on a device which is lacking an editable host app.

The supported check is fairly simple, and shouldn't be applied if we're running from an application binary. We also shouldn't apply this filtering when running flutter devices

This should also be factored into the daemon somehow, so that IDE's can avoid suggesting devices which will always fail

Related Issues

Fixes #30724

Tests

I added the following tests: Lots

cc @devoncarew @DanTup

@DanTup
Copy link
Contributor

DanTup commented Apr 23, 2019

What's the preferred way to wire this up in the IDE? In VS Code we have a "current device" in the status bar, and it lets you switch between devices. The user could have one or more projects open, so I don't think filtering that list will guarantee the selected device is valid for the project they try to launch.

I wonder if we should leave that list to include all devices (eg. it's the "preferred device") and then when you try to launch a project, we verify if the device is valid for that project, and if not we'll prompt with a filtered list (and possibly set that as the selected device in the status bar)?

(I presume we can use ENABLE_FLUTTER_DESKTOP for testing - that'll make a desktop device show up, right?)

@devoncarew
Copy link
Contributor

What's the preferred way to wire this up in the IDE?
...
I wonder if we should leave that list to include all devices (eg. it's the "preferred device") and then when you try to launch a project, we verify if the device is valid for that project, and if not we'll prompt with a filtered list (and possibly set that as the selected device in the status bar)?

Another possibility is to show all devices in the IDE, and then on an app launch, fail if the device doesn't support the target platform (i.e., have a good error message come back from flutter_tools). the user could then select another device.

@jonahwilliams, without a better understanding of the workflow here, I don't anticipate IntelliJ at least using the daemon protocol's getDevices() projectDirectory method param.

@dnfield dnfield added the tool Affects the "flutter" command-line tool. See also t: labels. label Apr 24, 2019
@jonahwilliams
Copy link
Contributor Author

Another possibility is to show all devices in the IDE, and then on an app launch, fail if the device doesn't support the target platform (i.e., have a good error message come back from flutter_tools). the user could then select another device.

I think that sounds like the best approach

}
final List<FlutterDevice> flutterDevices = <FlutterDevice>[];
final FlutterProject flutterProject = await FlutterProject.current();
final String applicationBinaryPath = argResults['use-application-binary'];
Copy link
Member

Choose a reason for hiding this comment

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

Maybe un-hoist applicationBinaryPath unless there was a reason to hoist it.

fs.file('.packages').createSync();
final FlutterProject flutterProject = await FlutterProject.current();

expect(AndroidDevice('test').isSupportedForProject(flutterProject), true);
Copy link
Member

Choose a reason for hiding this comment

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

What about the contents of the pubspec.yaml file informs whether the project supports Android? I don't see anything mentioning Android in the pubspec.yaml file.

fs.file('.packages').createSync();
final FlutterProject flutterProject = await FlutterProject.current();

expect(IOSDevice('test').isSupportedForProject(flutterProject), true);
Copy link
Member

Choose a reason for hiding this comment

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

same question.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We support a "module" style application where flutter is hosted as part of an existing native application instead of containing an editable sub-app directory. This includes hidden .ios and .android folders which contain fully generated code necessary to link the dart + native projects.

The idea was that these folders would be fully managed and regenerated by the framework, so for the sake of backwards compatibility we cannot use the presence of the folders as a hint. Instead, the pubspec will contain a module: ...more_info field

fs.file('.packages').createSync();
final FlutterProject flutterProject = await FlutterProject.current();

expect(LinuxDevice().isSupportedForProject(flutterProject), false);
Copy link
Member

Choose a reason for hiding this comment

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

Is this still false when the pubspec.yaml is non-empty? That is, why does linux not have a test with non-empty pubspec.yaml like Android and iOS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

linux, macos, and windows don't support the module style package (and probably won't ever)

Jonah Williams added 2 commits April 24, 2019 22:28
@jonahwilliams jonahwilliams merged commit 6b19184 into flutter:master Apr 25, 2019
@jonahwilliams jonahwilliams deleted the devices_for_project branch April 25, 2019 19:25
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update device discovery to take current project into consideration.

6 participants