[flutter tools] Revert desktop device name changes and print the category instead #60395

jamesderlin · 2020-06-26T22:53:04Z

Revert #58812 and instead make flutter devices/flutter doctor
print the device category, which is what the IntelliJ IDE plugin
does. The format will be:

• deviceName (category) • deviceID • targetPlatformName • osName

This does mean that the device name again will be the same the OS
name for desktop devices.

@devoncarew also suggested suffixing -device for the device names.

Related Issues

#58812

Tests

None

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

I reverted tests back to the state prior to #58812.

@devoncarew

…gory instead Revert flutter#58812 and instead make `flutter devices`/`flutter doctor` print the device category, which is what the IntelliJ IDE plugin does. The format will be: • deviceName (category) • deviceID • targetPlatformName • osName This does mean that the device name again will be the same the OS name for desktop devices. @devoncarew also suggested suffixing `-device` for the device names.

devoncarew

Thanks for the changes!

One main comment - I think the device ID for the desktop devices should have a -desktop suffix (instead of a -device suffix) to help differentiate them from other devices.

devoncarew · 2020-06-27T15:00:44Z

packages/flutter_tools/lib/src/linux/linux_device.dart

@@ -17,7 +17,7 @@ import 'linux_workflow.dart';
 /// A device that represents a desktop Linux target.
 class LinuxDevice extends DesktopDevice {
  LinuxDevice() : super(
-      'linux',
+      'linux-device',


linux-desktop?

devoncarew · 2020-06-27T15:01:03Z

packages/flutter_tools/lib/src/macos/macos_device.dart

@@ -15,7 +15,7 @@ import 'macos_workflow.dart';
 /// A device that represents a desktop MacOS target.
 class MacOSDevice extends DesktopDevice {
  MacOSDevice() : super(
-      'macos',
+      'macos-device',


macos-desktop?

devoncarew · 2020-06-27T15:01:15Z

packages/flutter_tools/lib/src/windows/windows_device.dart

@@ -18,7 +18,7 @@ import 'windows_workflow.dart';
 /// A device that represents a desktop Windows target.
 class WindowsDevice extends DesktopDevice {
  WindowsDevice() : super(
-      'windows',
+      'windows-device',


windows-desktop?

stuartmorgan · 2020-06-27T17:24:06Z

to help differentiate them from other devices

Out of curiosity, what devices are windows, macos, and linux being confused with? I don't really mind suffixing them as long as the unsuffixed versions still work, but I'm not sure what problem is being solved here.

stuartmorgan · 2020-06-27T17:24:51Z

packages/flutter_tools/lib/src/device.dart

@@ -559,7 +559,7 @@ abstract class Device {
        supportIndicator += ' ($type)';
      }
      table.add(<String>[
-        device.name,


This will change everything, not just desktop; is that desired?

That was my intent, yes. Whether it's desirable it up to review. ;)

stuartmorgan · 2020-06-27T17:25:25Z

packages/flutter_tools/lib/src/device.dart

@@ -559,7 +559,7 @@ abstract class Device {
        supportIndicator += ' ($type)';
      }
      table.add(<String>[
-        device.name,


This change doesn't seem to have any associated test changes. If we want this behavior change, we should test it.

devoncarew · 2020-06-27T21:56:28Z

to help differentiate them from other devices

Out of curiosity, what devices are windows, macos, and linux being confused with? I don't really mind suffixing them as long as the unsuffixed versions still work, but I'm not sure what problem is being solved here.

I believe the main motivation is to avoid the Linux • Linux • linux-x64 • Linux devices description (from #58812).

stuartmorgan · 2020-06-27T22:18:50Z

I believe the main motivation is to avoid the Linux • Linux • linux-x64 • Linux devices description (from #58812).

That isn't the line now, so mission accomplished if that's the whole problem statement 🙂 But the statement there was "it has confused people", which doesn't really tell me what the specific problem is. What, exactly, is it being confused with, or confusing them about? Do we know enough to know whether "Linux (desktop) • linux • linux-x64 • Linux" will still be confusing, and how? In what way will adding the word "desktop" to that line a second time reduce confusion?

Without understanding exactly what problem we're trying to solve, it's hard to evaluate proposals.

For instance, if the problem is that it's just the word Linux a bunch of times and people don't know why it keeps saying the same thing, adding more details to the last component, the way we do on other platforms (e.g., Mac OS X 10.15.5 19F101), would address that in a way that's useful rather than just repeating a different word.

jonahwilliams · 2020-06-27T22:35:48Z

I agree with @stuartmorgan that foo-desktop is a bit redundant. The category option should be enough to break up the text, but we should be including more details in the OS name category anyway. And as always, it should be tested - even if it wasn't being tested before

Eventually, we'll also fix the device architecture to something like x64/arm64/arm32` which should reduce more of the repetition.

jonahwilliams · 2020-06-27T22:36:33Z

Actually it was tested 😄

02:11 +239: test/commands.shard/hermetic/devices_test.dart: devices available devices and diagnostics                                                                                                  
Expected: '2 connected devices:\n'
            '\n'
            'ephemeral • ephemeral • android-arm    • Test SDK (1.2.3) (emulator)\n'
            'webby     • webby     • web-javascript • Web SDK (1.2.4) (emulator)\n'
            '\n'
            '• Cannot connect to device ABC\n'
            ''
  Actual: '2 connected devices:\n'
            '\n'
            'ephemeral (mobile) • ephemeral • android-arm    • Test SDK (1.2.3) (emulator)\n'
            'webby (mobile)     • webby     • web-javascript • Web SDK (1.2.4) (emulator)\n'
            '\n'
            '• Cannot connect to device ABC\n'
            ''
   Which: is different.
          Expected: ... ephemeral • ephemera ...
            Actual: ... ephemeral (mobile) • ...
                                  ^
           Differ at offset 34

devoncarew · 2020-06-27T23:11:19Z

Without understanding exactly what problem we're trying to solve, it's hard to evaluate proposals.

Ah; the main overall issue is that the original change regressed the UX in IDEs. Whatever else we change, we should address that. You can see the issue in the linked PR. I care much less about reducing redundancy in the flutter devices output (or rather, it's much less of an issue).

stuartmorgan · 2020-06-28T01:24:17Z

Ah; the main overall issue is that the original change regressed the UX in IDEs. Whatever else we change, we should address that. You can see the issue in the linked PR.

I understand that part; I'm asking for more specific details about the motivation for the initial PR. The change from including "desktop" in the name to displaying the category in devices addressed the regression entirely unless I'm missing something.

I care much less about reducing redundancy in the flutter devices output (or rather, it's much less of an issue).

This PR does two seemingly unrelated things: fixes the regression for IDE display (I'm all for that), and then for reasons I don't understand adds redundancy to the devices display. It's the second part I'm trying to understand the motivation for.

jamesderlin · 2020-06-28T04:01:29Z

to help differentiate them from other devices

Out of curiosity, what devices are windows, macos, and linux being confused with? I don't really mind suffixing them as long as the unsuffixed versions still work, but I'm not sure what problem is being solved here.

macos: Nothing.

windows: In practice, nothing. Historically there have been other Windows devices other than desktops. Granted, those other Windows-based platforms are dead now, and Flutter doesn't support them.

linux: This one is the confusing one, and I changed the macos and windows devices for consistency. Android is Linux-based; does that count? What about other potential Linux-based platforms? It was particularly confusing if you ran flutter doctor without any explicitly attached ephemeral devices and you saw "Linux • Linux • linux-x64 • Linux" listed.

I'm not particularly tied to changing the device IDs (which is why I settled with just lowercasing them in #58812), so I don't mind leaving that alone, but I thought it would be nice to break up the device-name/device-id/os-name monotony slightly.

jamesderlin · 2020-06-30T17:55:37Z

I fixed the tests, and since the device ID changes seem a bit controversial, I've changed those back

jonahwilliams

LGTM

…gory instead (flutter#60395)

jamesderlin requested review from stuartmorgan, devoncarew and jonahwilliams June 26, 2020 22:53

fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jun 26, 2020

googlebot added the cla: yes label Jun 26, 2020

devoncarew reviewed Jun 27, 2020

View reviewed changes

stuartmorgan reviewed Jun 27, 2020

View reviewed changes

Fix tests, revert to the plain device IDs

3a2c448

jamesderlin force-pushed the jamesderlin/revert-desktop-device-names branch from bc09e34 to 3a2c448 Compare June 30, 2020 00:00

Merge commit '59aed97647' into jamesderlin/revert-desktop-device-names

c76a953

jonahwilliams approved these changes Jun 30, 2020

View reviewed changes

jamesderlin added the waiting for tree to go green label Jun 30, 2020

fluttergithubbot merged commit cf8fbc3 into flutter:master Jun 30, 2020

jamesderlin deleted the jamesderlin/revert-desktop-device-names branch July 1, 2020 04:34

mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020

[flutter tools] Revert desktop device name changes and print the cate…

4ef50fe

…gory instead (flutter#60395)

github-actions bot locked as resolved and limited conversation to collaborators Jul 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[flutter tools] Revert desktop device name changes and print the category instead #60395

[flutter tools] Revert desktop device name changes and print the category instead #60395

jamesderlin commented Jun 26, 2020

devoncarew left a comment

devoncarew Jun 27, 2020

devoncarew Jun 27, 2020

devoncarew Jun 27, 2020

stuartmorgan commented Jun 27, 2020

stuartmorgan Jun 27, 2020

jamesderlin Jun 28, 2020

stuartmorgan Jun 27, 2020

devoncarew commented Jun 27, 2020

stuartmorgan commented Jun 27, 2020 •

edited

jonahwilliams commented Jun 27, 2020

jonahwilliams commented Jun 27, 2020

devoncarew commented Jun 27, 2020

stuartmorgan commented Jun 28, 2020

jamesderlin commented Jun 28, 2020

jamesderlin commented Jun 30, 2020

jonahwilliams left a comment

[flutter tools] Revert desktop device name changes and print the category instead #60395

[flutter tools] Revert desktop device name changes and print the category instead #60395

Conversation

jamesderlin commented Jun 26, 2020

Related Issues

Tests

Checklist

Breaking Change

devoncarew left a comment

Choose a reason for hiding this comment

devoncarew Jun 27, 2020

Choose a reason for hiding this comment

devoncarew Jun 27, 2020

Choose a reason for hiding this comment

devoncarew Jun 27, 2020

Choose a reason for hiding this comment

stuartmorgan commented Jun 27, 2020

stuartmorgan Jun 27, 2020

Choose a reason for hiding this comment

jamesderlin Jun 28, 2020

Choose a reason for hiding this comment

stuartmorgan Jun 27, 2020

Choose a reason for hiding this comment

devoncarew commented Jun 27, 2020

stuartmorgan commented Jun 27, 2020 • edited

jonahwilliams commented Jun 27, 2020

jonahwilliams commented Jun 27, 2020

devoncarew commented Jun 27, 2020

stuartmorgan commented Jun 28, 2020

jamesderlin commented Jun 28, 2020

jamesderlin commented Jun 30, 2020

jonahwilliams left a comment

Choose a reason for hiding this comment

stuartmorgan commented Jun 27, 2020 •

edited