is there any documentation to this magic string we can link to from a comment?

I discovered it by trying it out, but I just found this: https://android.googlesource.com/platform/packages/modules/adb/+/refs/heads/master/adb_mdns.h#30
so maybe I should take out the leading _. Should I add a comment linked to the android source code?

Is this PR introducing wireless support for android devices?

So Android wireless devices already worked with Flutter. You just had to pair the device through Android Studio or the command line and then it worked. When you did flutter devices, flutter run, etc the wireless device would just be grouped with all other devices found.

oh cool, TIL about this code.

i'm guessing this can be deleted

can this really be null?

Yes, when includeUnsupportedDevices is true, the flutterProject will be null. This is true in flutter logs and can be true in flutter drive.

Can we move this context to a dartdoc for this function?

nit. I think pulling some of these sub-expressions into named variables would make this easier to read:

nit don't re-assign an argument:

are device IDs not case sensitive? and is this true across platforms?

i suppose if this is the comparison we were making before then we should keep doing it. it's not obvious from the diff here if we were doing a similar comparison previously.

Yeah sorry about the diff. It is the same as before.

this code is susceptible to a very obscure gotcha I reported in dart-lang/sdk#51248

The seemingly silly anonymous closure we pass to .then() of (List<Device> devices) => devices ensures that the Future we call the error handling callback on is of type List<Device>, rather than that of a covariant type, such as List<IosDevice>.

FYI you were not expected to know this, I'm working on landing a lint to catch this.

actually, since you're awaiting this, I think you can just use a try catch:

    List<Device>? foundDevices;
    try {
      foundDevices = await devices(
        filter: filter,
      );
    } catch (error) {
      // Fail quietly so other discovers can find matches, even if this one fails.
      logger.printTrace('Ignored error discovering $deviceId: $error');
      foundDevices = const <Device>[];
    });

Nice!

@christopherfujino how do we decide what strings to put in user_messages? I think it's primarily the few strings that need to be overridden by g3?
(see google3_user_messages.dart in cs).

yes, that was the intent of the UserMessages class.

I see, perhaps I should move not-overridden ones into the file they are used and make them consts?

SGTM

How about includeDevicesSupportedByFlutter, includeDevicesSupportedByProject, includeUnsupportedDevices or similar.

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?

Or you could just return this conditional.

observedConnectionTypeByDevice?
You're tracking whether usb and wifi were both connected?

Yeah it tracks both so that if one disconnects, we can check if it is still connected a different way

Check out enhanced enums to hang strings and other enums off of specific enum values instead of this pattern: https://dart.dev/guides/language/language-tour#enumerated-types

Do these need to be marked unawaited?

is this behavior different from the flutter devices command?

Yes, in flutter devices we load the attached devices and display them immediately, show a message that we're checking for wireless devices. Then once the wireless devices are done loading, we print them.

In flutter doctor we just don't display any devices until they are all done loading.

spelling nit

spelling nit

So this is both known at the time we construct the class (that is, non-null) but also mutable (non-final)?

Yeah, so when we get the initial IOSDeviceConnectionInterface, it might be incorrect but we still get one. For example, xcdevice list will say a wireless device has an interface of usb (it will also have an error) if it didn't have long enough to check for the network connection.

Here's an example of what it will return for a wireless device it didn't have enough time to properly check:

  {
    "modelCode" : "iPad14,3",
    "simulator" : false,
    "modelName" : "iPad Pro (11-inch) (4th generation)\"",
    "error" : {
      "code" : -13,
      "failureReason" : "",
      "underlyingErrors" : [
        {
          "code" : 4,
          "failureReason" : "",
          "description" : "iPad is locked.",
          "recoverySuggestion" : "To use iPad with Xcode, unlock it.",
          "domain" : "DVTDeviceIneligibilityErrorDomain"
        }
      ],
      "description" : "iPad is not connected",
      "recoverySuggestion" : "Xcode will continue when iPad is connected and unlocked.",
      "domain" : "com.apple.platform.iphoneos"
    },
    "operatingSystemVersion" : "16.1.1 (20B101)",
    "identifier" : "xxx",
    "platform" : "com.apple.platform.iphoneos",
    "architecture" : "arm64e",
    "interface" : "usb",
    "available" : false,
    "name" : "iPad",
    "modelUTI" : "com.apple.ipad-pro-11-4th-2"
  },

Ahhh, that makes sense. Could you add a dartdoc to this field, noting its initial value from xcdevice list could be wrong?

Is the 30 here equivalent to DeviceManager.minimumWirelessTimeout or is that just coincidental?

oh wait, no, here the 30 is if we're wired, whereas if we're wireless it's 45. I'm a little worried we're spreading out the timeout values across these files.

@jmagman thoughts?

Yeah so DeviceManager.minimumWirelessTimeout is supposed to be the time we take to discover wireless devices for device selection. The 30/45 second timeout here is how long we wait to display error message when waiting for the dart vm service to be discovered. Maybe I should rename minimumWirelessTimeout to minimumWirelessDiscoveryForSelectionTimeout or minimumTargetDeviceWirelessDiscoveryTimeout?

what if, instead of always calling this method and requiring methods that don't need it to override it, we add a boolean field to FlutterCommand: final bool usesWirelessDevices = false;

Then commands that require it must explicitly opt in. This way someone adding a new command does not have to consciously opt out.

This line would then become:

    if (usesWirelessDevice) {
      startFindingWirelessDevices();
    }