From 0dffb5650e6a2c71eda15f5ac8256a50d7862c36 Mon Sep 17 00:00:00 2001 From: Kenzie Schmoll Date: Fri, 7 Jun 2024 09:08:39 -0700 Subject: [PATCH 1/2] Fix a regression with accessing the Flutter store file. --- .../lib/src/shared/server/_analytics_api.dart | 24 ++--- packages/devtools_shared/CHANGELOG.md | 1 + .../devtools_shared/lib/devtools_server.dart | 2 +- .../{usage.dart => devtools_store.dart} | 88 +----------------- .../lib/src/server/file_system.dart | 90 ++++++++++++++++++- .../lib/src/server/flutter_store.dart | 21 +++++ .../lib/src/server/server_api.dart | 65 ++++++++------ 7 files changed, 160 insertions(+), 131 deletions(-) rename packages/devtools_shared/lib/src/server/{usage.dart => devtools_store.dart} (70%) create mode 100644 packages/devtools_shared/lib/src/server/flutter_store.dart diff --git a/packages/devtools_app/lib/src/shared/server/_analytics_api.dart b/packages/devtools_app/lib/src/shared/server/_analytics_api.dart index 3902e9853b2..8bf77c854f3 100644 --- a/packages/devtools_app/lib/src/shared/server/_analytics_api.dart +++ b/packages/devtools_app/lib/src/shared/server/_analytics_api.dart @@ -57,22 +57,17 @@ Future setAnalyticsEnabled([bool value = true]) async { // '/api/devToolsEnabled' returns the value (identical VM service) and // '/api/devToolsEnabled?value=true' sets the value. -/// Request Flutter tool stored property value enabled (GA enabled) stored in -/// the file '~\.flutter'. +/// Whether GA is enabled in the Flutter store file ~\.flutter. /// -/// Return bool. -/// Return value of false implies either GA is disabled or the Flutter Tool has -/// never been run (null returned from the server). +/// A return value of false implies either GA is disabled or the Flutter Tool +/// has never been run. Future _isFlutterGAEnabled() async { bool enabled = false; if (isDevToolsServerAvailable) { final resp = await request(apiGetFlutterGAEnabled); if (resp?.statusOk ?? false) { - // A return value of 'null' implies Flutter tool has never been run so - // return false for Flutter GA enabled. - final responseValue = json.decode(resp!.body); - enabled = responseValue ?? false; + enabled = json.decode(resp!.body) as bool; } else { logWarning(resp, apiGetFlutterGAEnabled); } @@ -81,12 +76,12 @@ Future _isFlutterGAEnabled() async { return enabled; } -/// Request Flutter tool stored property value clientID (GA enabled) stored in -/// the file '~\.flutter'. +/// Requests the Flutter client id from the Flutter store file ~\.flutter. /// -/// Return as a String, empty string implies Flutter Tool has never been run. +/// If an empty String is returned, this means that Flutter Tool has never been +/// run. Future flutterGAClientID() async { - // Default empty string, Flutter tool never run. + // Default empty string, Flutter tool never ran. String clientId = ''; if (isDevToolsServerAvailable) { @@ -97,9 +92,6 @@ Future flutterGAClientID() async { if (resp?.statusOk ?? false) { clientId = json.decode(resp!.body); if (clientId.isEmpty) { - // Requested value of 'null' (Flutter tool never ran). Server request - // apiGetFlutterGAClientId should not happen because the - // isFlutterGAEnabled test should have been false. _log.warning('$apiGetFlutterGAClientId is empty'); } } else { diff --git a/packages/devtools_shared/CHANGELOG.md b/packages/devtools_shared/CHANGELOG.md index 2264c11a442..abd6c17decf 100644 --- a/packages/devtools_shared/CHANGELOG.md +++ b/packages/devtools_shared/CHANGELOG.md @@ -2,6 +2,7 @@ * Added helper `deserialize` and `deserializeNullable` * Extended serialization for `HeapSample` and `ExtensionEvents` * Added mixin `Serializable` +* Fix a regression with accessing the Flutter store file. # 10.0.0-dev.2 * Support detecting package roots for nested Dart projects in the diff --git a/packages/devtools_shared/lib/devtools_server.dart b/packages/devtools_shared/lib/devtools_server.dart index 2e6709c914b..e69a38f9a94 100644 --- a/packages/devtools_shared/lib/devtools_server.dart +++ b/packages/devtools_shared/lib/devtools_server.dart @@ -2,6 +2,6 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +export 'src/server/devtools_store.dart'; export 'src/server/file_system.dart'; export 'src/server/server_api.dart'; -export 'src/server/usage.dart'; diff --git a/packages/devtools_shared/lib/src/server/usage.dart b/packages/devtools_shared/lib/src/server/devtools_store.dart similarity index 70% rename from packages/devtools_shared/lib/src/server/usage.dart rename to packages/devtools_shared/lib/src/server/devtools_store.dart index 6e292b657d2..40df531e3e0 100644 --- a/packages/devtools_shared/lib/src/server/usage.dart +++ b/packages/devtools_shared/lib/src/server/devtools_store.dart @@ -2,14 +2,9 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -import 'dart:convert'; -import 'dart:io'; - -import 'package:path/path.dart' as path; - import 'file_system.dart'; -// Access the DevTools on disk store (~/.flutter-devtools/.devtools). +/// Provides access to the local DevTools store (~/.flutter-devtools/.devtools). class DevToolsUsage { DevToolsUsage() { LocalFileSystem.maybeMoveLegacyDevToolsStore(); @@ -158,84 +153,3 @@ extension type _ActiveSurveyJson(Map json) { bool get surveyActionTaken => json[DevToolsUsage._surveyActionTaken] as bool; int? get surveyShownCount => json[DevToolsUsage._surveyShownCount] as int?; } - -abstract class PersistentProperties { - PersistentProperties(this.name); - - final String name; - - // ignore: avoid-dynamic, dynamic by design. - dynamic operator [](String key); - - // ignore: avoid-dynamic, dynamic by design. - void operator []=(String key, dynamic value); - - /// Re-read settings from the backing store. - /// - /// May be a no-op on some platforms. - void syncSettings(); -} - -const JsonEncoder _jsonEncoder = JsonEncoder.withIndent(' '); - -class IOPersistentProperties extends PersistentProperties { - IOPersistentProperties( - String name, { - String? documentDirPath, - }) : super(name) { - final String fileName = name.replaceAll(' ', '_'); - documentDirPath ??= LocalFileSystem.devToolsDir(); - _file = File(path.join(documentDirPath, fileName)); - if (!_file.existsSync()) { - _file.createSync(recursive: true); - } - syncSettings(); - } - - IOPersistentProperties.fromFile(File file) : super(path.basename(file.path)) { - _file = file; - if (!_file.existsSync()) { - _file.createSync(recursive: true); - } - syncSettings(); - } - - late File _file; - - late Map _map; - - @override - // ignore: avoid-dynamic, necessary here. - dynamic operator [](String key) => _map[key]; - - @override - void operator []=(String key, Object? value) { - if (value == null && !_map.containsKey(key)) return; - if (_map[key] == value) return; - - if (value == null) { - _map.remove(key); - } else { - _map[key] = value; - } - - try { - _file.writeAsStringSync('${_jsonEncoder.convert(_map)}\n'); - } catch (_) {} - } - - @override - void syncSettings() { - try { - String contents = _file.readAsStringSync(); - if (contents.isEmpty) contents = '{}'; - _map = (jsonDecode(contents) as Map).cast(); - } catch (_) { - _map = {}; - } - } - - void remove(String propertyName) { - _map.remove(propertyName); - } -} diff --git a/packages/devtools_shared/lib/src/server/file_system.dart b/packages/devtools_shared/lib/src/server/file_system.dart index eb89707bab2..8bc401668c5 100644 --- a/packages/devtools_shared/lib/src/server/file_system.dart +++ b/packages/devtools_shared/lib/src/server/file_system.dart @@ -7,7 +7,7 @@ import 'dart:io'; import 'package:path/path.dart' as path; -import 'usage.dart'; +import 'devtools_store.dart'; // ignore: avoid_classes_with_only_static_members, requires refactor. class LocalFileSystem { @@ -76,9 +76,95 @@ class LocalFileSystem { return jsonEncode(json); } + /// The location of the Flutter store file, ~/.flutter. + static String flutterStoreLocation() { + return path.join(_userHomeDir(), '.flutter'); + } + /// Whether the flutter store file exists. static bool flutterStoreExists() { - final flutterStore = File(path.join(_userHomeDir(), '.flutter')); + final flutterStore = File(flutterStoreLocation()); return flutterStore.existsSync(); } } + +class IOPersistentProperties extends PersistentProperties { + IOPersistentProperties( + String name, { + String? documentDirPath, + }) : super(name) { + final String fileName = name.replaceAll(' ', '_'); + documentDirPath ??= LocalFileSystem._userHomeDir(); + _file = File(path.join(documentDirPath, fileName)); + if (!_file.existsSync()) { + _file.createSync(recursive: true); + } + syncSettings(); + } + + IOPersistentProperties.fromFile(File file) : super(path.basename(file.path)) { + _file = file; + if (!_file.existsSync()) { + _file.createSync(recursive: true); + } + syncSettings(); + } + + late File _file; + + late Map _map; + + @override + // ignore: avoid-dynamic, necessary here. + dynamic operator [](String key) => _map[key]; + + @override + void operator []=(String key, Object? value) { + if (value == null && !_map.containsKey(key)) return; + if (_map[key] == value) return; + + if (value == null) { + _map.remove(key); + } else { + _map[key] = value; + } + + try { + _file.writeAsStringSync('${_jsonEncoder.convert(_map)}\n'); + } catch (_) {} + } + + @override + void syncSettings() { + try { + String contents = _file.readAsStringSync(); + if (contents.isEmpty) contents = '{}'; + _map = (jsonDecode(contents) as Map).cast(); + } catch (_) { + _map = {}; + } + } + + void remove(String propertyName) { + _map.remove(propertyName); + } +} + +abstract class PersistentProperties { + PersistentProperties(this.name); + + final String name; + + // ignore: avoid-dynamic, dynamic by design. + dynamic operator [](String key); + + // ignore: avoid-dynamic, dynamic by design. + void operator []=(String key, dynamic value); + + /// Re-read settings from the backing store. + /// + /// May be a no-op on some platforms. + void syncSettings(); +} + +const JsonEncoder _jsonEncoder = JsonEncoder.withIndent(' '); diff --git a/packages/devtools_shared/lib/src/server/flutter_store.dart b/packages/devtools_shared/lib/src/server/flutter_store.dart new file mode 100644 index 00000000000..fc7cec9c9c1 --- /dev/null +++ b/packages/devtools_shared/lib/src/server/flutter_store.dart @@ -0,0 +1,21 @@ +// Copyright 2024 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'file_system.dart'; + +/// Provides access to the local Flutter store (~/.flutter). +class FlutterStore { + static const storeName = '.flutter'; + static const firstRunKey = 'firstRun'; + static const gaEnabledKey = 'enabled'; + static const flutterClientIdKey = 'clientId'; + + final properties = IOPersistentProperties(storeName); + + bool get isFirstRun => properties[firstRunKey] == true; + + bool get gaEnabled => properties[gaEnabledKey] == true; + + String get flutterClientId => properties[flutterClientIdKey] as String; +} diff --git a/packages/devtools_shared/lib/src/server/server_api.dart b/packages/devtools_shared/lib/src/server/server_api.dart index 74d9dd8a627..944132f90c7 100644 --- a/packages/devtools_shared/lib/src/server/server_api.dart +++ b/packages/devtools_shared/lib/src/server/server_api.dart @@ -22,8 +22,9 @@ import '../extensions/extension_manager.dart'; import '../service/service.dart'; import '../service_utils.dart'; import '../utils/file_utils.dart'; +import 'devtools_store.dart'; import 'file_system.dart'; -import 'usage.dart'; +import 'flutter_store.dart'; // TODO(kenz): consider using Dart augmentation libraries instead of part files // if there is a clear benefit. @@ -66,27 +67,38 @@ class ServerApi { queryParams, dtd, ); + // ----- Flutter Tool GA store. ----- case apiGetFlutterGAEnabled: // Is Analytics collection enabled? - return _encodeResponse('', api: api); + return _encodeResponse( + LocalFileSystem.flutterStoreExists() + ? _flutterStore.gaEnabled + : false, + api: api, + ); case apiGetFlutterGAClientId: // Flutter Tool GA clientId - ONLY get Flutter's clientId if enabled is // true. - return _encodeResponse('', api: api); + return _encodeResponse( + LocalFileSystem.flutterStoreExists() + ? _flutterStore.flutterClientId + : '', + api: api, + ); // ----- DevTools GA store. ----- case apiResetDevTools: - _devToolsUsage.reset(); + _devToolsStore.reset(); return _encodeResponse(true, api: api); case apiGetDevToolsFirstRun: // Has DevTools been run first time? To bring up analytics dialog. - final isFirstRun = _devToolsUsage.isFirstRun; + final isFirstRun = _devToolsStore.isFirstRun; return _encodeResponse(isFirstRun, api: api); case apiGetDevToolsEnabled: // Is DevTools Analytics collection enabled? - final isEnabled = _devToolsUsage.analyticsEnabled; + final isEnabled = _devToolsStore.analyticsEnabled; return _encodeResponse(isEnabled, api: api); case apiSetDevToolsEnabled: // Enable or disable DevTools analytics collection. @@ -94,9 +106,9 @@ class ServerApi { final analyticsEnabled = json.decode(queryParams[devToolsEnabledPropertyName]!); - _devToolsUsage.analyticsEnabled = analyticsEnabled; + _devToolsStore.analyticsEnabled = analyticsEnabled; } - return _encodeResponse(_devToolsUsage.analyticsEnabled, api: api); + return _encodeResponse(_devToolsStore.analyticsEnabled, api: api); // ----- DevTools survey store. ----- @@ -112,26 +124,26 @@ class ServerApi { final String theSurveyName = queryParams[activeSurveyName]!; // Set the current activeSurvey. - _devToolsUsage.activeSurvey = theSurveyName; + _devToolsStore.activeSurvey = theSurveyName; result = true; } return _encodeResponse(result, api: api); case apiGetSurveyActionTaken: // Request setActiveSurvey has not been requested. - if (_devToolsUsage.activeSurvey == null) { + if (_devToolsStore.activeSurvey == null) { return api.badRequest( '$errorNoActiveSurvey ' '- $apiGetSurveyActionTaken', ); } // SurveyActionTaken has the survey been acted upon (taken or dismissed) - return _encodeResponse(_devToolsUsage.surveyActionTaken, api: api); + return _encodeResponse(_devToolsStore.surveyActionTaken, api: api); // TODO(terry): remove the query param logic for this request. // setSurveyActionTaken should only be called with the value of true, so // we can remove the extra complexity. case apiSetSurveyActionTaken: // Request setActiveSurvey has not been requested. - if (_devToolsUsage.activeSurvey == null) { + if (_devToolsStore.activeSurvey == null) { return api.badRequest( '$errorNoActiveSurvey ' '- $apiSetSurveyActionTaken', @@ -140,46 +152,46 @@ class ServerApi { // Set the SurveyActionTaken. // Has the survey been taken or dismissed.. if (queryParams.containsKey(surveyActionTakenPropertyName)) { - _devToolsUsage.surveyActionTaken = + _devToolsStore.surveyActionTaken = json.decode(queryParams[surveyActionTakenPropertyName]!); } - return _encodeResponse(_devToolsUsage.surveyActionTaken, api: api); + return _encodeResponse(_devToolsStore.surveyActionTaken, api: api); case apiGetSurveyShownCount: // Request setActiveSurvey has not been requested. - if (_devToolsUsage.activeSurvey == null) { + if (_devToolsStore.activeSurvey == null) { return api.badRequest( '$errorNoActiveSurvey ' '- $apiGetSurveyShownCount', ); } // SurveyShownCount how many times have we asked to take survey. - return _encodeResponse(_devToolsUsage.surveyShownCount, api: api); + return _encodeResponse(_devToolsStore.surveyShownCount, api: api); case apiIncrementSurveyShownCount: // Request setActiveSurvey has not been requested. - if (_devToolsUsage.activeSurvey == null) { + if (_devToolsStore.activeSurvey == null) { return api.badRequest( '$errorNoActiveSurvey ' '- $apiIncrementSurveyShownCount', ); } // Increment the SurveyShownCount, we've asked about the survey. - _devToolsUsage.incrementSurveyShownCount(); - return _encodeResponse(_devToolsUsage.surveyShownCount, api: api); + _devToolsStore.incrementSurveyShownCount(); + return _encodeResponse(_devToolsStore.surveyShownCount, api: api); // ----- Release notes api. ----- case apiGetLastReleaseNotesVersion: return _encodeResponse( - _devToolsUsage.lastReleaseNotesVersion, + _devToolsStore.lastReleaseNotesVersion, api: api, ); case apiSetLastReleaseNotesVersion: if (queryParams.containsKey(lastReleaseNotesVersionPropertyName)) { - _devToolsUsage.lastReleaseNotesVersion = + _devToolsStore.lastReleaseNotesVersion = queryParams[lastReleaseNotesVersionPropertyName]!; } return _encodeResponse( - _devToolsUsage.lastReleaseNotesVersion, + _devToolsStore.lastReleaseNotesVersion, api: api, ); @@ -298,10 +310,13 @@ class ServerApi { : null; } - // Accessing DevTools usage file e.g., ~/.flutter-devtools/.devtools - static final _devToolsUsage = DevToolsUsage(); + /// Accessing DevTools store file e.g., ~/.flutter-devtools/.devtools + static final _devToolsStore = DevToolsUsage(); + + /// Accessing Flutter store file e.g., ~/.flutter + static final _flutterStore = FlutterStore(); - static DevToolsUsage get devToolsPreferences => _devToolsUsage; + static DevToolsUsage get devToolsPreferences => _devToolsStore; /// Provides read and write access to DevTools options files /// (e.g. path/to/app/root/devtools_options.yaml). From 8ea54be6523a18e2cfb25ba3997510a48a9ad13f Mon Sep 17 00:00:00 2001 From: Kenzie Schmoll Date: Fri, 7 Jun 2024 10:44:48 -0700 Subject: [PATCH 2/2] review comments --- .../lib/src/server/file_system.dart | 43 +++++-------------- 1 file changed, 11 insertions(+), 32 deletions(-) diff --git a/packages/devtools_shared/lib/src/server/file_system.dart b/packages/devtools_shared/lib/src/server/file_system.dart index 8bc401668c5..30eed1dc103 100644 --- a/packages/devtools_shared/lib/src/server/file_system.dart +++ b/packages/devtools_shared/lib/src/server/file_system.dart @@ -76,23 +76,18 @@ class LocalFileSystem { return jsonEncode(json); } - /// The location of the Flutter store file, ~/.flutter. - static String flutterStoreLocation() { - return path.join(_userHomeDir(), '.flutter'); - } - /// Whether the flutter store file exists. static bool flutterStoreExists() { - final flutterStore = File(flutterStoreLocation()); + final flutterStore = File(path.join(_userHomeDir(), '.flutter')); return flutterStore.existsSync(); } } -class IOPersistentProperties extends PersistentProperties { +class IOPersistentProperties { IOPersistentProperties( - String name, { + this.name, { String? documentDirPath, - }) : super(name) { + }) { final String fileName = name.replaceAll(' ', '_'); documentDirPath ??= LocalFileSystem._userHomeDir(); _file = File(path.join(documentDirPath, fileName)); @@ -102,7 +97,7 @@ class IOPersistentProperties extends PersistentProperties { syncSettings(); } - IOPersistentProperties.fromFile(File file) : super(path.basename(file.path)) { + IOPersistentProperties.fromFile(File file) : name = path.basename(file.path) { _file = file; if (!_file.existsSync()) { _file.createSync(recursive: true); @@ -110,15 +105,14 @@ class IOPersistentProperties extends PersistentProperties { syncSettings(); } + final String name; + late File _file; late Map _map; - @override - // ignore: avoid-dynamic, necessary here. - dynamic operator [](String key) => _map[key]; + Object? operator [](String key) => _map[key]; - @override void operator []=(String key, Object? value) { if (value == null && !_map.containsKey(key)) return; if (_map[key] == value) return; @@ -134,7 +128,9 @@ class IOPersistentProperties extends PersistentProperties { } catch (_) {} } - @override + /// Re-read settings from the backing store. + /// + /// May be a no-op on some platforms. void syncSettings() { try { String contents = _file.readAsStringSync(); @@ -150,21 +146,4 @@ class IOPersistentProperties extends PersistentProperties { } } -abstract class PersistentProperties { - PersistentProperties(this.name); - - final String name; - - // ignore: avoid-dynamic, dynamic by design. - dynamic operator [](String key); - - // ignore: avoid-dynamic, dynamic by design. - void operator []=(String key, dynamic value); - - /// Re-read settings from the backing store. - /// - /// May be a no-op on some platforms. - void syncSettings(); -} - const JsonEncoder _jsonEncoder = JsonEncoder.withIndent(' ');