Update device filtering and introduce isConnected and connectionInterface #121359
Conversation
…face, update device filtering to use a class, update some function names
flutter-dashboard
bot
added
a: text input
| this.mustBeSupportedByFlutter = false, | ||
| this.mustBeSupportedForProject = false, | ||
| this.mustBeSupportedForAll = false, | ||
| }); |
There was a problem hiding this comment.
In previous PR, @jmagman asked:
How about includeDevicesSupportedByFlutter, includeDevicesSupportedByProject, includeUnsupportedDevices or similar.
To which I responded:
Yeah I had a hard time picking variable names for this. So by default (if you don't set anything) it will include all devices and none will be filtered out. So I thought that using the word include was a bit confusing. For example, if everything is already included, what does includeDevicesSupportedByFlutter really mean? It really means exclude everything that's not supported by flutter. What do you think of excludeDevicesNotSupportedByFlutter, excludeDevicesNotSupportedByProject, excludeDevicesNotSupportedByAll?
Do the exclude names make more sense / more inline with our naming conventions?
There was a problem hiding this comment.
I'll defer to @jmagman on this one, I'm horrible at naming :)
| class DeviceDiscoverySupportFilter { | ||
| DeviceDiscoverySupportFilter({ | ||
| required this.flutterProject, | ||
| this.mustBeSupportedByFlutter = false, |
There was a problem hiding this comment.
Is this ever false? It looks like it's always true in #121262.
There was a problem hiding this comment.
You're right, it's unnecessary, removed it.
| /// If [mustBeSupportedForAll] is true, only devices supported by Flutter, | ||
| /// supported for the provided [flutterProject], and supported for | ||
| /// `--device all` will be included. | ||
| class DeviceDiscoverySupportFilter { |
There was a problem hiding this comment.
These filter classes seem like they push a lot up to the call sites. It seems confusing that you could do excludeDevicesNotSupportedByProject: false, excludeDevicesNotSupportedByFlutter: true, what would you get? It's also not enforcing that flutterProject is nonnull if excludeDevicesNotSupportedByProject is true. I think we can be more defensive about not letting the caller shoot themselves in the foot?
christopherfujino
left a comment
christopherfujino
left a comment
There was a problem hiding this comment.
This LGTM, though I'll leave final approval to Jenn
vashworth
added
the
autosubmit
3 participants
This change introduces to devices the concept of
isConnectedandconnectionInterface(DeviceConnectionInterface). As it stands in this PR, all devices will haveisConnectedbe true andconnectionInterfacebeDeviceConnectionInterface.attached.This PR also changed the way target devices are filtered so it's more flexible to use and encapsulated. To accommodate these changes, I also updated some function names and made a getter into a function.
Lastly, this PR also fixes a potential race situation related to populating the device cache.
No user experience is changed in this PR.
Part 1 in breakdown of #121262.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.