Fix: fix android studio 4.1 plugin path for macOS by Mokusesei · Pull Request #69364 · flutter/flutter

Mokusesei · 2020-10-30T04:35:21Z

Description

Plugin path of android studio4.1 in MacOS has been changed(now is at HOME_DIR_PATH/Library/Application Support/Google/AndroidStudiox.x), which makes cmd 'flutter doctor' print wrong logs!

Related Issues

None yet!

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

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.

flutter-dashboard · 2020-10-30T04:35:23Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

jonahwilliams

This will need a test before we can submit it. The existing test file is https://github.com/flutter/flutter/blob/master/packages/flutter_tools/test/general.shard/android/android_studio_validator_test.dart. Though unfortunately we don't have any coverage of this code path yet :(

Ideally there would be two tests, one that looks up the < 4.1 path and one that looks up the > 4.1 path. This could be done by setting the platform the macOS using a FakePlatform(operatingSystem: 'macos') and then checking that the plugin path is correct for a given major/minor version

Mokusesei · 2020-11-02T06:25:15Z

This will need a test before we can submit it. The existing test file is https://github.com/flutter/flutter/blob/master/packages/flutter_tools/test/general.shard/android/android_studio_validator_test.dart. Though unfortunately we don't have any coverage of this code path yet :(

Ideally there would be two tests, one that looks up the < 4.1 path and one that looks up the > 4.1 path. This could be done by setting the platform the macOS using a FakePlatform(operatingSystem: 'macos') and then checking that the plugin path is correct for a given major/minor version

Hi @jonahwilliams, thanks for your kind reply.
I have added two tests for my modified code in my latest commit. Actually, I just learn Flutter&&Dart for less than two months. it is my first time to write a dart test, and i'm not sure if it meets your requirements.please check carefully for me, thanks for your time.🙂

I also test my code in my own dev env(macOS):
on the stable channel, the flutter doctor shows:

on my modified channel, the flutter doctor works well again:

…dio >= 4.1 on MacOS

jonahwilliams · 2020-11-02T16:00:12Z

packages/flutter_tools/lib/src/android/android_studio.dart

-        'AndroidStudio$major.$minor',
-      );
+      /// plugin path of Android Studio has been changed after version 4.1.
+      if(double.parse('$major.$minor') >= 4.1) {


Instead of concatenation these numbers parsing the string, check for the major and minor version separately:

(if major >= 4 && minor >= 1) {

jonahwilliams · 2020-11-02T16:02:12Z

packages/flutter_tools/lib/src/android/android_studio.dart

-        'Application Support',
-        pathsSelectorValue,
-      );
+      : double.parse('$major.$minor') >= 4.1 


The nested ternary expression is a big hard to read. I would break this up into an if (and use the separate major/minor checks like below):

String presetPluginsPath;
if (pathsSelectorValue != null) {
if (major >= 4 && minor >= 1) {
...
} else {
...
}
}

jonahwilliams · 2020-11-02T16:03:09Z

This is looking great, just a few small comments!

jonahwilliams · 2020-11-02T16:04:39Z

packages/flutter_tools/test/general.shard/android/android_studio_test.dart

      );
    });

+    testUsingContext('Can discover Android Studio >=4.1 location on Mac', () {


This is kind of a nit, but the spacing in these tests makes them a bit hard to read. I would generally split up tests into 3 sections:

creating objects/mocks and setting up state

performing the action to be tested

making expectations about the results

This is kind of a nit, but the spacing in these tests makes them a bit hard to read. I would generally split up tests into 3 sections:

creating objects/mocks and setting up state

performing the action to be tested

making expectations about the results

Hi @jonahwilliams ,

The devil is in the detail. I really sorry for having overlooked the code style.
Thank you very much for your suggestion.It's this kind of suggestion that I strive for to help me keep improving.

Best wishes!😁

jonahwilliams · 2020-11-03T16:05:08Z

You have a few minor analysis warnings that need to be cleaned up, otherwise it LGTM!

/b/s/w/ir/k/flutter/packages/flutter_tools/test/general.shard/android/android_studio_test.dart:120: trailing U+0020 space character
/b/s/w/ir/k/flutter/packages/flutter_tools/test/general.shard/android/android_studio_test.dart:126: trailing U+0020 space character
/b/s/w/ir/k/flutter/packages/flutter_tools/test/general.shard/android/android_studio_test.dart:144: trailing U+0020 space character
/b/s/w/ir/k/flutter/packages/flutter_tools/test/general.shard/android/android_studio_test.dart:159: trailing U+0020 space character

jonahwilliams · 2020-11-03T16:23:21Z

packages/flutter_tools/test/general.shard/android/android_studio_test.dart

+      FileSystem: () => fileSystem,
+      FileSystemUtils: () => fsUtils,
+      ProcessManager: () => FakeProcessManager.any(),
+      // Custom home paths are not supported on macOS nor Windows yet,


This comment actually seems wrong - this is testing mac right?

This comment actually seems wrong - this is testing mac right?

in fact, I don't have a wins pc to run this test. so i just copy this comment from other test case that in the same 'group' func (line 202).

Mokusesei · 2020-11-03T16:36:17Z

You have a few minor analysis warnings that need to be cleaned up, otherwise it LGTM!

/b/s/w/ir/k/flutter/packages/flutter_tools/test/general.shard/android/android_studio_test.dart:120: trailing U+0020 space character
/b/s/w/ir/k/flutter/packages/flutter_tools/test/general.shard/android/android_studio_test.dart:126: trailing U+0020 space character
/b/s/w/ir/k/flutter/packages/flutter_tools/test/general.shard/android/android_studio_test.dart:144: trailing U+0020 space character
/b/s/w/ir/k/flutter/packages/flutter_tools/test/general.shard/android/android_studio_test.dart:159: trailing U+0020 space character

solved.

jonahwilliams

LGTM

jonahwilliams · 2020-11-05T01:26:20Z

Thank you for the bug fix @Mokusesei , its much appreciated!

This reverts commit 422a97d.

packages/flutter_tools/lib/src/android/android_studio.dart

mraleph · 2020-11-05T10:14:46Z

I am reverting due to crashes it causes: #69863

mit-mit · 2020-11-05T15:41:56Z

cc @Mokusesei

Mokusesei · 2020-11-06T13:11:02Z

cc @Mokusesei

#69954 fixed

i'm so sorry for that.☕️

mit-mit · 2020-11-06T13:34:00Z

No problem @Mokusesei. Null errors are tricky to spot. This is a great real-world example of why we're adding null safety to Dart!

Mokusesei · 2020-11-06T14:39:49Z

No problem @Mokusesei. Null errors are tricky to spot. This is a great real-world example of why we're adding null safety to Dart!

Yeah, I think I will get a huge buzz from that feature!

Fix: fix android studio 4.1 plugin path for macOS

372dc25

flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Oct 30, 2020

google-cla bot added the cla: yes label Oct 30, 2020

Fix: fix android studio 4.1 plugin path for macOS --> update annotation

93a4b18

Mokusesei mentioned this pull request Oct 30, 2020

Fix android studio 4.1 plugin path for mac #69311

Closed

9 tasks

jonahwilliams self-requested a review October 30, 2020 06:12

jonahwilliams reviewed Oct 30, 2020

View reviewed changes

Mokusesei requested a review from jonahwilliams November 2, 2020 06:48

Mokusesei added 2 commits November 2, 2020 16:49

fix:modify the preset Plugins Path for Android Studio >= 4.1

2a9c324

test:add two tests for case of the changed plugin path of Android Stu…

0d6bf4d

…dio >= 4.1 on MacOS

Mokusesei force-pushed the fix-android-studio-4.1-plugin-path-for-mac branch from 0b2b39a to 0d6bf4d Compare November 2, 2020 08:51

jonahwilliams reviewed Nov 2, 2020

View reviewed changes

Mokusesei added 2 commits November 3, 2020 11:47

opt:optimize the version judge logic

d5e857f

style:format the test code style

b244bf7

Mokusesei requested a review from jonahwilliams November 3, 2020 04:18

format:clean up the analysis warnings

b447b09

jonahwilliams reviewed Nov 3, 2020

View reviewed changes

Mokusesei requested a review from jonahwilliams November 3, 2020 16:36

jonahwilliams approved these changes Nov 5, 2020

View reviewed changes

jonahwilliams added the waiting for tree to go green label Nov 5, 2020

jonahwilliams merged commit 422a97d into flutter:master Nov 5, 2020

mraleph added a commit that referenced this pull request Nov 5, 2020

Revert "Fix: fix android studio 4.1 plugin path for macOS (#69364)"

934cedf

This reverts commit 422a97d.

mraleph reviewed Nov 5, 2020

View reviewed changes

packages/flutter_tools/lib/src/android/android_studio.dart Show resolved Hide resolved

mraleph mentioned this pull request Nov 5, 2020

Revert "Fix: fix android studio 4.1 plugin path for macOS" #69870

Merged

Mokusesei mentioned this pull request Nov 6, 2020

Fix android studio 4.1 plugin path for mac #69954

Merged

Conversation

Mokusesei commented Oct 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Checklist

Breaking Change

Uh oh!

flutter-dashboard bot commented Oct 30, 2020

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Mokusesei commented Nov 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Nov 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Nov 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mokusesei commented Nov 3, 2020

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Nov 5, 2020

Uh oh!

Uh oh!

mraleph commented Nov 5, 2020

Uh oh!

mit-mit commented Nov 5, 2020

Uh oh!

Mokusesei commented Nov 6, 2020

Uh oh!

mit-mit commented Nov 6, 2020

Uh oh!

Mokusesei commented Nov 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Mokusesei commented Oct 30, 2020 •

edited

Loading