Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[flutter_tools] Remove Version.unknown #124771

Merged
merged 16 commits into from
Apr 15, 2023
Merged

[flutter_tools] Remove Version.unknown #124771

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
c9df6f1
remove Version.unknown
andrewkolos Apr 12, 2023
8f667bd
document meaning of null in AndroidStudio
andrewkolos Apr 13, 2023
0e5cb91
fix xcode, make major version required
andrewkolos Apr 13, 2023
3bf8b19
oops
andrewkolos Apr 13, 2023
8fec718
nits
andrewkolos Apr 13, 2023
63fcb2c
nit
andrewkolos Apr 13, 2023
df60ac5
Update packages/flutter_tools/lib/src/android/android_studio.dart
andrewkolos Apr 13, 2023
80d6da4
nits
andrewkolos Apr 13, 2023
f231e4b
refactor --android-studio-dir stuff
andrewkolos Apr 13, 2023
968dc24
Merge remote-tracking branch 'upstream/master' into remove-version-un…
andrewkolos Apr 14, 2023
91694eb
add some tests for findLatest ordering
andrewkolos Apr 14, 2023
f06b47e
reorder sorting if statements
andrewkolos Apr 14, 2023
99aed0c
add equivalent version test
andrewkolos Apr 14, 2023
f288818
throw ToolExit when we configured Android Studio install does not exist
andrewkolos Apr 14, 2023
30007eb
fix test
andrewkolos Apr 14, 2023
f25b771
Merge branch 'master' into remove-version-unknown-null-object
andrewkolos Apr 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
58 changes: 36 additions & 22 deletions packages/flutter_tools/lib/src/android/android_studio.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,16 @@ final RegExp _dotHomeStudioVersionMatcher =
// See https://github.com/flutter/flutter/issues/124252.
String? get javaPath => globals.androidStudio?.javaPath;

class AndroidStudio implements Comparable<AndroidStudio> {
class AndroidStudio {
Copy link
Member

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?

Copy link
Contributor Author

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 Comparable unless 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

Copy link
Member

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.

/// A [version] value of null represents an unknown version.
AndroidStudio(
this.directory, {
Version? version,
this.version,
this.configured,
this.studioAppName = 'AndroidStudio',
this.presetPluginsPath,
}) : version = version ?? Version.unknown {
_init(version: version);
}) {
_init();
}

static AndroidStudio? fromMacOSBundle(String bundlePath) {
Expand Down Expand Up @@ -147,7 +148,12 @@ class AndroidStudio implements Comparable<AndroidStudio> {

final String directory;
final String studioAppName;
final Version version;

/// The version of Android Studio.
///
/// A null value represents an unknown version.
final Version? version;

final String? configured;
final String? presetPluginsPath;

Expand All @@ -163,8 +169,12 @@ class AndroidStudio implements Comparable<AndroidStudio> {
if (presetPluginsPath != null) {
return presetPluginsPath!;
}
final int major = version.major;
final int minor = version.minor;

// TODO(andrewkolos): This is a bug. We shouldn't treat an unknown
// version as the same 0.0.
andrewkolos marked this conversation as resolved.
Show resolved Hide resolved
// 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;
Expand Down Expand Up @@ -217,15 +227,6 @@ class AndroidStudio implements Comparable<AndroidStudio> {

List<String> get validationMessages => _validationMessages;

@override
int compareTo(AndroidStudio other) {
final int result = version.compareTo(other.version);
if (result == 0) {
return directory.compareTo(other.directory);
}
return result;
}

/// Locates the newest, valid version of Android Studio.
static AndroidStudio? latestValid() {
final String? configuredStudio = globals.config.getValue('android-studio-dir') as String?;
Expand All @@ -245,7 +246,17 @@ class AndroidStudio implements Comparable<AndroidStudio> {
}
AndroidStudio? newest;
for (final AndroidStudio studio in studios.where((AndroidStudio s) => s.isValid)) {
if (newest == null || studio.compareTo(newest) > 0) {
newest ??= studio;
andrewkolos marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this functionality unit tested?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some tests. I ran in my local master branch to (hopefully) verify that no behavior was changed.


if (studio.version != null && newest.version == null) {
newest = studio;
}
else if (studio.version == null && newest.version == null &&
andrewkolos marked this conversation as resolved.
Show resolved Hide resolved
studio.directory.compareTo(newest.directory) > 0 ) {
christopherfujino marked this conversation as resolved.
Show resolved Hide resolved
newest = studio;
}
else if (studio.version != null && newest.version != null &&
studio.version!.compareTo(newest.version!) > 0){
newest = studio;
}
}
Expand Down Expand Up @@ -337,7 +348,10 @@ class AndroidStudio implements Comparable<AndroidStudio> {
return false;
}
if (newerThan != null) {
return studio.version.compareTo(newerThan) >= 0;
if (studio.version == null) {
return false;
}
return studio.version!.compareTo(newerThan) >= 0;
}
return true;
});
Expand Down Expand Up @@ -438,7 +452,7 @@ class AndroidStudio implements Comparable<AndroidStudio> {
return keyMatcher.stringMatch(plistValue)?.split('=').last.trim().replaceAll('"', '');
}

void _init({Version? version}) {
void _init() {
christopherfujino marked this conversation as resolved.
Show resolved Hide resolved
_isValid = false;
_validationMessages.clear();

Expand All @@ -453,15 +467,15 @@ class AndroidStudio implements Comparable<AndroidStudio> {

final String javaPath;
if (globals.platform.isMacOS) {
if (version != null && version.major < 2020) {
if (version != null && version!.major < 2020) {
javaPath = globals.fs.path.join(directory, 'jre', 'jdk', 'Contents', 'Home');
} else if (version != null && version.major == 2022) {
} 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) {
if (version != null && version!.major == 2022) {
javaPath = globals.fs.path.join(directory, 'jbr');
} else {
javaPath = globals.fs.path.join(directory, 'jre');
Expand Down
3 changes: 1 addition & 2 deletions packages/flutter_tools/lib/src/android/android_studio_validator.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import '../base/config.dart';
import '../base/file_system.dart';
import '../base/platform.dart';
import '../base/user_messages.dart';
import '../base/version.dart';
import '../doctor_validator.dart';
import '../intellij/intellij.dart';
import 'android_studio.dart';
Expand Down Expand Up @@ -46,7 +45,7 @@ class AndroidStudioValidator extends DoctorValidator {
final List<ValidationMessage> messages = <ValidationMessage>[];
ValidationType type = ValidationType.missing;

final String? studioVersionText = _studio.version == Version.unknown
final String? studioVersionText = _studio.version == null
? null
: userMessages.androidStudioVersion(_studio.version.toString());
messages.add(ValidationMessage(
Expand Down
18 changes: 11 additions & 7 deletions packages/flutter_tools/lib/src/base/version.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,20 @@

import 'package:meta/meta.dart';

// TODO(reidbaker): Investigate using pub_semver instead of this class.
/// Represents the version of some piece of software.
///
/// While a [Version] object has fields resembling semver, it does not
/// necessarily represent a semver version.
@immutable
class Version implements Comparable<Version> {
/// Creates a new [Version] object.
factory Version(int? major, int? minor, int? patch, {String? text}) {
///
/// A null [minor] or [patch] version is logically equivalent to 0. Using null
/// for these parameters only affects the generation of [text], if no value
/// for it is provided.
factory Version(int major, int? minor, int? patch, {String? text}) {
if (text == null) {
text = major == null ? '0' : '$major';
text = '$major';
if (minor != null) {
text = '$text.$minor';
}
Expand All @@ -19,7 +26,7 @@ class Version implements Comparable<Version> {
}
}

return Version._(major ?? 0, minor ?? 0, patch ?? 0, text);
return Version._(major, minor ?? 0, patch ?? 0, text);
}

/// Public constant constructor when all fields are non-null, without default value fallbacks.
Expand Down Expand Up @@ -67,9 +74,6 @@ class Version implements Comparable<Version> {
return primary;
}


static Version get unknown => Version(0, 0, 0, text: 'unknown');

/// The major version number: "1" in "1.2.3".
final int major;

Expand Down
5 changes: 0 additions & 5 deletions packages/flutter_tools/lib/src/intellij/intellij_validator.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,11 +125,6 @@ abstract class IntelliJValidator extends DoctorValidator {
}

void _validateIntelliJVersion(List<ValidationMessage> messages, Version minVersion) {
// Ignore unknown versions.
if (minVersion == Version.unknown) {
return;
}

final Version? installedVersion = Version.parse(version);
if (installedVersion == null) {
return;
Expand Down
7 changes: 3 additions & 4 deletions packages/flutter_tools/lib/src/vscode/vscode.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,7 @@ const String extensionMarketplaceUrl =
'https://marketplace.visualstudio.com/items?itemName=$extensionIdentifier';

class VsCode {
VsCode._(this.directory, this.extensionDirectory, { Version? version, this.edition, required FileSystem fileSystem})
: version = version ?? Version.unknown {
VsCode._(this.directory, this.extensionDirectory, { this.version, this.edition, required FileSystem fileSystem}) {

if (!fileSystem.isDirectorySync(directory)) {
_validationMessages.add(ValidationMessage.error('VS Code not found at $directory'));
Expand Down Expand Up @@ -76,7 +75,7 @@ class VsCode {

final String directory;
final String extensionDirectory;
final Version version;
final Version? version;
final String? edition;

Version? _extensionVersion;
Expand Down Expand Up @@ -284,7 +283,7 @@ class VsCode {

@override
String toString() =>
'VS Code ($version)${_extensionVersion != Version.unknown ? ', Flutter ($_extensionVersion)' : ''}';
'VS Code ($version)${_extensionVersion != null ? ', Flutter ($_extensionVersion)' : ''}';

static String? _getVersionFromPackageJson(String packageJsonPath, FileSystem fileSystem) {
if (!fileSystem.isFileSync(packageJsonPath)) {
Expand Down
3 changes: 1 addition & 2 deletions packages/flutter_tools/lib/src/vscode/vscode_validator.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import 'package:process/process.dart';
import '../base/file_system.dart';
import '../base/platform.dart';
import '../base/user_messages.dart';
import '../base/version.dart';
import '../doctor_validator.dart';
import 'vscode.dart';

Expand All @@ -24,7 +23,7 @@ class VsCodeValidator extends DoctorValidator {

@override
Future<ValidationResult> validate() async {
final String? vsCodeVersionText = _vsCode.version == Version.unknown
final String? vsCodeVersionText = _vsCode.version == null
? null
: userMessages.vsCodeVersion(_vsCode.version.toString());

Expand Down
2 changes: 1 addition & 1 deletion packages/flutter_tools/test/general.shard/macos/xcode_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1064,7 +1064,7 @@ void main() {

class FakeXcodeProjectInterpreter extends Fake implements XcodeProjectInterpreter {
@override
Version version = Version.unknown;
Version version = Version(0, 0, 0);

@override
bool isInstalled = false;
Expand Down
5 changes: 2 additions & 3 deletions packages/flutter_tools/test/general.shard/utils_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,15 @@ baz=qux

group('Version', () {
testWithoutContext('can parse and compare', () {
expect(Version.unknown.toString(), equals('unknown'));
expect(Version(null, null, null).toString(), equals('0'));
expect(Version(0, null, null).toString(), equals('0'));
expect(const Version.withText(1, 2, 3, 'versionText').toString(), 'versionText');

final Version v1 = Version.parse('1')!;
expect(v1.major, equals(1));
expect(v1.minor, equals(0));
expect(v1.patch, equals(0));

expect(v1, greaterThan(Version.unknown));
expect(v1, greaterThan(Version(0, 0, 0)));

final Version v2 = Version.parse('1.2')!;
expect(v2.major, equals(1));
Expand Down
3 changes: 1 addition & 2 deletions packages/flutter_tools/test/general.shard/vscode/vscode_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
import 'package:file/memory.dart';
import 'package:flutter_tools/src/base/file_system.dart';
import 'package:flutter_tools/src/base/platform.dart';
import 'package:flutter_tools/src/base/version.dart';
import 'package:flutter_tools/src/vscode/vscode.dart';

import '../../src/common.dart';
Expand Down Expand Up @@ -38,7 +37,7 @@ void main() {

final VsCode vsCode = VsCode.fromDirectory('', '', fileSystem: fileSystem);

expect(vsCode.version, Version.unknown);
expect(vsCode.version, null);
});

testWithoutContext('can locate VS Code installed via Snap', () {
Expand Down