[flutter_tools] Remove Version.unknown
#124771
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,15 +39,16 @@ final RegExp _dotHomeStudioVersionMatcher = | |
| // See https://github.com/flutter/flutter/issues/124252. | ||
| String? get javaPath => globals.androidStudio?.javaPath; | ||
|
|
||
| class AndroidStudio { | ||
| /// A [version] value of null represents an unknown version. | ||
| AndroidStudio( | ||
| this.directory, { | ||
| this.version, | ||
| this.configuredPath, | ||
| this.studioAppName = 'AndroidStudio', | ||
| this.presetPluginsPath, | ||
| }) { | ||
| _initAndValidate(); | ||
| } | ||
|
|
||
| static AndroidStudio? fromMacOSBundle(String bundlePath) { | ||
|
|
@@ -147,8 +148,13 @@ class AndroidStudio implements Comparable<AndroidStudio> { | |
|
|
||
| final String directory; | ||
| final String studioAppName; | ||
|
|
||
| /// The version of Android Studio. | ||
| /// | ||
| /// A null value represents an unknown version. | ||
| final Version? version; | ||
|
|
||
| final String? configuredPath; | ||
| final String? presetPluginsPath; | ||
|
|
||
| String? _javaPath; | ||
|
|
@@ -163,8 +169,12 @@ class AndroidStudio implements Comparable<AndroidStudio> { | |
| if (presetPluginsPath != null) { | ||
| return presetPluginsPath!; | ||
| } | ||
|
|
||
| // TODO(andrewkolos): This is a bug. We shouldn't treat an unknown | ||
| // version as equivalent to 0.0. | ||
| // See https://github.com/flutter/flutter/issues/121468. | ||
| final int major = version?.major ?? 0; | ||
| final int minor = version?.minor ?? 0; | ||
| final String? homeDirPath = globals.fsUtils.homeDirPath; | ||
| if (homeDirPath == null) { | ||
| return null; | ||
|
|
@@ -217,25 +227,20 @@ class AndroidStudio implements Comparable<AndroidStudio> { | |
|
|
||
| List<String> get validationMessages => _validationMessages; | ||
|
|
||
| /// Locates the newest, valid version of Android Studio. | ||
| /// | ||
| /// In the case that `--android-studio-dir` is configured, the version of | ||
| /// Android Studio found at that location is always returned, even if it is | ||
| /// invalid. | ||
| static AndroidStudio? latestValid() { | ||
| final String? configuredStudioPath = globals.config.getValue('android-studio-dir') as String?; | ||
| if (configuredStudioPath != null) { | ||
| String correctedConfiguredStudioPath = configuredStudioPath; | ||
| if (globals.platform.isMacOS && !correctedConfiguredStudioPath.endsWith('Contents')) { | ||
| correctedConfiguredStudioPath = globals.fs.path.join(correctedConfiguredStudioPath, 'Contents'); | ||
| } | ||
| return AndroidStudio(correctedConfiguredStudioPath, | ||
|
There was a problem hiding this comment. I know we used to do this, but should we not toolexit in the case where There was a problem hiding this comment. Added this and a test |
||
| configuredPath: configuredStudioPath); | ||
| } | ||
|
|
||
| // Find all available Studio installations. | ||
|
|
@@ -245,7 +250,18 @@ class AndroidStudio implements Comparable<AndroidStudio> { | |
| } | ||
| AndroidStudio? newest; | ||
| for (final AndroidStudio studio in studios.where((AndroidStudio s) => s.isValid)) { | ||
| if (newest == null) { | ||
| newest = studio; | ||
| continue; | ||
| } | ||
|
|
||
| if (studio.version != null && newest.version == null) { | ||
| newest = studio; | ||
| } else if (studio.version == null && newest.version == null && | ||
| studio.directory.compareTo(newest.directory) > 0) { | ||
This comment was marked as outdated.
Sorry, something went wrong. There was a problem hiding this comment. Actually, yeesh, so we're just alphabetically comparing these strings? There was a problem hiding this comment. Let me file a tracking issue about investigating whether we should really be doing this. There was a problem hiding this comment. Can you add a TODO with a link to #124880 There was a problem hiding this comment.
Only if 1) both installations have equivalent versions or 2) neither installation have known versions. I wonder what the original inspiration for this was...perhaps it was just a way keep ordering consistent even if the discovery logic changed. There was a problem hiding this comment. Actually, I might be slightly incorrect here. Let me re-examine the original code. There was a problem hiding this comment. Yeah, reading this more, I can see this is a last-resort fallback. There was a problem hiding this comment. I think I have the logic right. I added another test |
||
| newest = studio; | ||
| } else if (studio.version != null && newest.version != null && | ||
| studio.version! > newest.version!) { | ||
| newest = studio; | ||
| } | ||
| } | ||
|
|
@@ -331,13 +347,16 @@ class AndroidStudio implements Comparable<AndroidStudio> { | |
| static List<AndroidStudio> _allLinuxOrWindows() { | ||
| final List<AndroidStudio> studios = <AndroidStudio>[]; | ||
|
|
||
| bool alreadyFoundStudioAt(String path, { Version? newerThan }) { | ||
| return studios.any((AndroidStudio studio) { | ||
| if (studio.directory != path) { | ||
| return false; | ||
| } | ||
| if (newerThan != null) { | ||
| if (studio.version == null) { | ||
| return false; | ||
| } | ||
| return studio.version!.compareTo(newerThan) >= 0; | ||
| } | ||
| return true; | ||
| }); | ||
|
|
@@ -373,7 +392,7 @@ class AndroidStudio implements Comparable<AndroidStudio> { | |
|
|
||
| for (final Directory entity in entities) { | ||
| final AndroidStudio? studio = AndroidStudio.fromHomeDot(entity); | ||
| if (studio != null && !alreadyFoundStudioAt(studio.directory, newerThan: studio.version)) { | ||
| studios.removeWhere((AndroidStudio other) => other.directory == studio.directory); | ||
| studios.add(studio); | ||
| } | ||
|
|
@@ -404,7 +423,7 @@ class AndroidStudio implements Comparable<AndroidStudio> { | |
| version: Version.parse(version), | ||
| studioAppName: title, | ||
| ); | ||
| if (!alreadyFoundStudioAt(studio.directory, newerThan: studio.version)) { | ||
| studios.removeWhere((AndroidStudio other) => other.directory == studio.directory); | ||
| studios.add(studio); | ||
| } | ||
|
|
@@ -415,14 +434,14 @@ class AndroidStudio implements Comparable<AndroidStudio> { | |
| } | ||
|
|
||
| final String? configuredStudioDir = globals.config.getValue('android-studio-dir') as String?; | ||
| if (configuredStudioDir != null && !alreadyFoundStudioAt(configuredStudioDir)) { | ||
| studios.add(AndroidStudio(configuredStudioDir, | ||
| configuredPath: configuredStudioDir)); | ||
| } | ||
|
|
||
| if (globals.platform.isLinux) { | ||
| void checkWellKnownPath(String path) { | ||
| if (globals.fs.isDirectorySync(path) && !alreadyFoundStudioAt(path)) { | ||
| studios.add(AndroidStudio(path)); | ||
| } | ||
| } | ||
|
|
@@ -438,12 +457,12 @@ class AndroidStudio implements Comparable<AndroidStudio> { | |
| return keyMatcher.stringMatch(plistValue)?.split('=').last.trim().replaceAll('"', ''); | ||
| } | ||
|
|
||
| void _initAndValidate() { | ||
| _isValid = false; | ||
| _validationMessages.clear(); | ||
|
|
||
| if (configuredPath != null) { | ||
| _validationMessages.add('android-studio-dir = $configuredPath'); | ||
| } | ||
|
|
||
| if (!globals.fs.isDirectorySync(directory)) { | ||
|
|
@@ -453,15 +472,15 @@ class AndroidStudio implements Comparable<AndroidStudio> { | |
|
|
||
| final String javaPath; | ||
| if (globals.platform.isMacOS) { | ||
| if (version != null && version!.major < 2020) { | ||
| javaPath = globals.fs.path.join(directory, 'jre', 'jdk', 'Contents', 'Home'); | ||
| } else if (version != null && version!.major == 2022) { | ||
| javaPath = globals.fs.path.join(directory, 'jbr', 'Contents', 'Home'); | ||
| } else { | ||
| javaPath = globals.fs.path.join(directory, 'jre', 'Contents', 'Home'); | ||
| } | ||
| } else { | ||
| if (version != null && version!.major == 2022) { | ||
| javaPath = globals.fs.path.join(directory, 'jbr'); | ||
| } else { | ||
| javaPath = globals.fs.path.join(directory, 'jre'); | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Why did you move the comparison logic out of
AndroidStudio.compareTo?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.
My instinct is to never implement
Comparableunless the semantics of comparison are immediately obvious (i.e. do not require reading documentation or code). This makes sense for things like numbers and dates. It makes less sense for things like software installs where the version (or lack of it) and the directory name are involved.I would consider consider the implementation if we had a need to compare AndroidStudio installs in multiple places in code, but we are only doing it in one place now.
I don't mind either way
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.
Ahh ok, if we were only comparing this once, this makes sense.