-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Allow filtering devices to only those supported by current project #31446
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
Allow filtering devices to only those supported by current project #31446
Conversation
|
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 |
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 |
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']; |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
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 devicesThis 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