From f73e1e6f474a2281a04a8c114b8d9990b0ed8a68 Mon Sep 17 00:00:00 2001 From: Kenzie Schmoll Date: Thu, 29 Aug 2024 13:21:08 -0700 Subject: [PATCH 01/18] Cleanup tech debt for some of the DevTools server APIs --- .../src/screens/app_size/app_size_screen.dart | 4 +- .../lib/src/shared/server/_app_size_api.dart | 8 +- .../src/shared/server/_release_notes_api.dart | 12 +-- packages/devtools_shared/CHANGELOG.md | 9 ++ .../devtools_shared/lib/src/devtools_api.dart | 94 +++++++++++++++---- .../lib/src/server/devtools_store.dart | 75 +++++++-------- .../lib/src/server/handlers/_app_size.dart | 49 ++++++++++ .../src/server/handlers/_release_notes.dart | 42 +++++++++ .../lib/src/server/handlers/_storage.dart | 32 +++++++ .../lib/src/server/server_api.dart | 64 +++++-------- packages/devtools_shared/pubspec.yaml | 2 +- 11 files changed, 285 insertions(+), 106 deletions(-) create mode 100644 packages/devtools_shared/lib/src/server/handlers/_app_size.dart create mode 100644 packages/devtools_shared/lib/src/server/handlers/_release_notes.dart create mode 100644 packages/devtools_shared/lib/src/server/handlers/_storage.dart diff --git a/packages/devtools_app/lib/src/screens/app_size/app_size_screen.dart b/packages/devtools_app/lib/src/screens/app_size/app_size_screen.dart index b1f5c792b54..2b38ee562e3 100644 --- a/packages/devtools_app/lib/src/screens/app_size/app_size_screen.dart +++ b/packages/devtools_app/lib/src/screens/app_size/app_size_screen.dart @@ -104,13 +104,13 @@ class _AppSizeBodyState extends State Future maybeLoadAppSizeFiles() async { final queryParams = loadQueryParams(); - final baseFilePath = queryParams[baseAppSizeFilePropertyName]; + final baseFilePath = queryParams[AppSizeApi.baseAppSizeFilePropertyName]; if (baseFilePath != null) { // TODO(kenz): does this have to be in a setState()? _preLoadingData = true; final baseAppSizeFile = await server.requestBaseAppSizeFile(baseFilePath); DevToolsJsonFile? testAppSizeFile; - final testFilePath = queryParams[testAppSizeFilePropertyName]; + final testFilePath = queryParams[AppSizeApi.testAppSizeFilePropertyName]; if (testFilePath != null) { testAppSizeFile = await server.requestTestAppSizeFile(testFilePath); } diff --git a/packages/devtools_app/lib/src/shared/server/_app_size_api.dart b/packages/devtools_app/lib/src/shared/server/_app_size_api.dart index 6ec55ac8761..889ddedb8b6 100644 --- a/packages/devtools_app/lib/src/shared/server/_app_size_api.dart +++ b/packages/devtools_app/lib/src/shared/server/_app_size_api.dart @@ -6,16 +6,16 @@ part of 'server.dart'; Future requestBaseAppSizeFile(String path) { return requestFile( - api: apiGetBaseAppSizeFile, - fileKey: baseAppSizeFilePropertyName, + api: AppSizeApi.getBaseAppSizeFile, + fileKey: AppSizeApi.baseAppSizeFilePropertyName, filePath: path, ); } Future requestTestAppSizeFile(String path) { return requestFile( - api: apiGetTestAppSizeFile, - fileKey: testAppSizeFilePropertyName, + api: AppSizeApi.getTestAppSizeFile, + fileKey: AppSizeApi.testAppSizeFilePropertyName, filePath: path, ); } diff --git a/packages/devtools_app/lib/src/shared/server/_release_notes_api.dart b/packages/devtools_app/lib/src/shared/server/_release_notes_api.dart index 9cc21416f11..913c7b8523d 100644 --- a/packages/devtools_app/lib/src/shared/server/_release_notes_api.dart +++ b/packages/devtools_app/lib/src/shared/server/_release_notes_api.dart @@ -10,11 +10,11 @@ part of 'server.dart'; Future getLastShownReleaseNotesVersion() async { String version = ''; if (isDevToolsServerAvailable) { - final resp = await request(apiGetLastReleaseNotesVersion); + final resp = await request(ReleaseNotesApi.getLastReleaseNotesVersion); if (resp?.statusOk ?? false) { version = json.decode(resp!.body); } else { - logWarning(resp, apiGetLastReleaseNotesVersion); + logWarning(resp, ReleaseNotesApi.getLastReleaseNotesVersion); } } return version; @@ -26,11 +26,11 @@ Future getLastShownReleaseNotesVersion() async { Future setLastShownReleaseNotesVersion(String version) async { if (isDevToolsServerAvailable) { final resp = await request( - '$apiSetLastReleaseNotesVersion' - '?$lastReleaseNotesVersionPropertyName=$version', + '${ReleaseNotesApi.setLastReleaseNotesVersion}' + '?$apiParameterValueKey=$version', ); - if (resp == null || !resp.statusOk || !(json.decode(resp.body) as bool)) { - logWarning(resp, apiSetLastReleaseNotesVersion); + if (resp == null || !resp.statusOk) { + logWarning(resp, ReleaseNotesApi.setLastReleaseNotesVersion); } } } diff --git a/packages/devtools_shared/CHANGELOG.md b/packages/devtools_shared/CHANGELOG.md index 7e4f748f68b..eba6c30a560 100644 --- a/packages/devtools_shared/CHANGELOG.md +++ b/packages/devtools_shared/CHANGELOG.md @@ -1,3 +1,12 @@ +# 10.1.0 +* Deprecate `apiGetLastReleaseNotesVersion` in favor of `ReleaseNotesApi.getLastReleaseNotesVersion`. +* Deprecate `apiSetLastReleaseNotesVersion` in favor of `ReleaseNotesApi.setLastReleaseNotesVersion`. +* Deprecate `lastReleaseNotesVersionPropertyName`. +* Deprecate `apiGetBaseAppSizeFile` in favor of `AppSizeApi.getBaseAppSizeFile`. +* Deprecate `apiGetTestAppSizeFile` in favor of `AppSizeApi.getTestAppSizeFile`. +* Deprecate `baseAppSizeFilePropertyName` in favor of `AppSizeApi.baseAppSizeFilePropertyName`. +* Deprecate `testAppSizeFilePropertyName` in favor of `AppSizeApi.testAppSizeFilePropertyName`. + # 10.0.2 * Update dependency `web_socket_channel: '>=2.4.0 <4.0.0'`. diff --git a/packages/devtools_shared/lib/src/devtools_api.dart b/packages/devtools_shared/lib/src/devtools_api.dart index 4bb67c379ab..b14a35d24c2 100644 --- a/packages/devtools_shared/lib/src/devtools_api.dart +++ b/packages/devtools_shared/lib/src/devtools_api.dart @@ -52,27 +52,87 @@ const apiGetSurveyShownCount = '${apiPrefix}getSurveyShownCount'; /// Increments the surveyShownCount of the of the activeSurvey (apiSetActiveSurvey). const apiIncrementSurveyShownCount = '${apiPrefix}incrementSurveyShownCount'; -const lastReleaseNotesVersionPropertyName = 'lastReleaseNotesVersion'; - -/// Returns the last DevTools version for which we have shown release notes. -const apiGetLastReleaseNotesVersion = '${apiPrefix}getLastReleaseNotesVersion'; - -/// Sets the last DevTools version for which we have shown release notes. -const apiSetLastReleaseNotesVersion = '${apiPrefix}setLastReleaseNotesVersion'; - -/// Returns the base app size file, if present. -const apiGetBaseAppSizeFile = '${apiPrefix}getBaseAppSizeFile'; - -/// Returns the test app size file used for comparing two files in a diff, if -/// present. -const apiGetTestAppSizeFile = '${apiPrefix}getTestAppSizeFile'; - -const baseAppSizeFilePropertyName = 'appSizeBase'; +@Deprecated( + 'Use apiParameterValueKey for the query parameter of the ' + 'ReleaseNotesApi.setLastReleaseNotesVersion request instead. ' + 'This field will be removed in devtools_shared >= 11.0.0.', +) +const lastReleaseNotesVersionPropertyName = apiParameterValueKey; + +@Deprecated( + 'Use ReleaseNotesApi.getLastReleaseNotesVersion instead. ' + 'This field will be removed in devtools_shared >= 11.0.0.', +) +const apiGetLastReleaseNotesVersion = + ReleaseNotesApi.getLastReleaseNotesVersion; + +@Deprecated( + 'Use ReleaseNotesApi.setLastReleaseNotesVersion instead. ' + 'This field will be removed in devtools_shared >= 11.0.0.', +) +const apiSetLastReleaseNotesVersion = + ReleaseNotesApi.setLastReleaseNotesVersion; + +abstract class ReleaseNotesApi { + /// Returns the last DevTools version for which we have shown release notes. + static const getLastReleaseNotesVersion = + '${apiPrefix}getLastReleaseNotesVersion'; + + /// Sets the last DevTools version for which we have shown release notes. + static const setLastReleaseNotesVersion = + '${apiPrefix}setLastReleaseNotesVersion'; +} -const testAppSizeFilePropertyName = 'appSizeTest'; +@Deprecated( + 'Use AppSizeApi.getBaseAppSizeFile instead. ' + 'This field will be removed in devtools_shared >= 11.0.0.', +) +const apiGetBaseAppSizeFile = AppSizeApi.getBaseAppSizeFile; + +@Deprecated( + 'Use AppSizeApi.getTestAppSizeFile instead. ' + 'This field will be removed in devtools_shared >= 11.0.0.', +) +const apiGetTestAppSizeFile = AppSizeApi.getTestAppSizeFile; + +@Deprecated( + 'Use AppSizeApi.baseAppSizeFilePropertyName instead. ' + 'This field will be removed in devtools_shared >= 11.0.0.', +) +const baseAppSizeFilePropertyName = AppSizeApi.baseAppSizeFilePropertyName; + +@Deprecated( + 'Use AppSizeApi.testAppSizeFilePropertyName instead. ' + 'This field will be removed in devtools_shared >= 11.0.0.', +) +const testAppSizeFilePropertyName = AppSizeApi.testAppSizeFilePropertyName; + +abstract class AppSizeApi { + /// Returns the base app size file, if present. + static const getBaseAppSizeFile = '${apiPrefix}getBaseAppSizeFile'; + + /// Returns the test app size file used for comparing two files in a diff, if + /// present. + static const getTestAppSizeFile = '${apiPrefix}getTestAppSizeFile'; + + /// The property name for the query parameter passed along with the + /// [getBaseAppSizeFile] request. + static const baseAppSizeFilePropertyName = 'appSizeBase'; + + /// The property name for the query parameter passed along with the + /// [getTestAppSizeFile] request. + static const testAppSizeFilePropertyName = 'appSizeTest'; +} abstract class DtdApi { + /// Gets the DTD URI from the DevTools server. + /// + /// DTD is either started from the user's IDE and passed to the DevTools + /// server, or it is started directly from the DevTools server. static const apiGetDtdUri = '${apiPrefix}getDtdUri'; + + /// The property name for the response that the server sends back upon + /// receiving an [apiGetDtdUri] request. static const uriPropertyName = 'dtdUri'; } diff --git a/packages/devtools_shared/lib/src/server/devtools_store.dart b/packages/devtools_shared/lib/src/server/devtools_store.dart index 40df531e3e0..daa6ff91e90 100644 --- a/packages/devtools_shared/lib/src/server/devtools_store.dart +++ b/packages/devtools_shared/lib/src/server/devtools_store.dart @@ -4,6 +4,28 @@ import 'file_system.dart'; +enum DevToolsStoreKeys { + /// The key holding the value for whether Google Analytics (legacy) for + /// DevTools have been enabled. + analyticsEnabled, + + /// The key holding the value for whether this is a user's first run of + /// DevTools. + isFirstRun, + + /// The key holding the value for the last DevTools version that the user + /// viewed release notes for. + lastReleaseNotesVersion, + + /// The key holding the value for whether the user has taken action on the + /// DevTools survey prompt. + surveyActionTaken, + + /// The key holding the value for number of times the user has seen the + /// DevTools survey prompt without taking action. + surveyShownCount, +} + /// Provides access to the local DevTools store (~/.flutter-devtools/.devtools). class DevToolsUsage { DevToolsUsage() { @@ -33,47 +55,23 @@ class DevToolsUsage { late IOPersistentProperties properties; - static const _surveyActionTaken = 'surveyActionTaken'; - static const _surveyShownCount = 'surveyShownCount'; - void reset() { - // TODO(kenz): remove this in Feb 2022. See - // https://github.com/flutter/devtools/issues/3264. The `firstRun` property - // has been replaced by `isFirstRun`. This is to force all users to answer - // the analytics dialog again. The 'enabled' property has been replaced by - // 'analyticsEnabled' for better naming. - properties.remove('firstRun'); - properties.remove('enabled'); - - properties.remove('firstDevToolsRun'); - properties['analyticsEnabled'] = false; + properties.remove(DevToolsStoreKeys.isFirstRun.name); + properties[DevToolsStoreKeys.analyticsEnabled.name] = false; } bool get isFirstRun { - // TODO(kenz): remove this in Feb 2022. See - // https://github.com/flutter/devtools/issues/3264.The `firstRun` property - // has been replaced by `isFirstRun`. This is to force all users to answer - // the analytics dialog again. - properties.remove('firstRun'); - - return properties['isFirstRun'] = properties['isFirstRun'] == null; + return properties[DevToolsStoreKeys.isFirstRun.name] = + properties[DevToolsStoreKeys.isFirstRun.name] == null; } bool get analyticsEnabled { - // TODO(kenz): remove this in Feb 2022. See - // https://github.com/flutter/devtools/issues/3264. The `enabled` property - // has been replaced by `analyticsEnabled` for better naming. - if (properties['enabled'] != null) { - properties['analyticsEnabled'] = properties['enabled']; - properties.remove('enabled'); - } - - return properties['analyticsEnabled'] = - properties['analyticsEnabled'] == true; + return properties[DevToolsStoreKeys.analyticsEnabled.name] = + properties[DevToolsStoreKeys.analyticsEnabled.name] == true; } set analyticsEnabled(bool value) { - properties['analyticsEnabled'] = value; + properties[DevToolsStoreKeys.analyticsEnabled.name] = value; } bool surveyNameExists(String surveyName) => properties[surveyName] != null; @@ -100,8 +98,8 @@ class DevToolsUsage { void rewriteActiveSurvey(bool actionTaken, int shownCount) { assert(activeSurvey != null); properties[activeSurvey!] = { - _surveyActionTaken: actionTaken, - _surveyShownCount: shownCount, + DevToolsStoreKeys.surveyActionTaken.name: actionTaken, + DevToolsStoreKeys.surveyShownCount.name: shownCount, }; } @@ -141,15 +139,18 @@ class DevToolsUsage { } String get lastReleaseNotesVersion { - return (properties['lastReleaseNotesVersion'] ??= '') as String; + return (properties[DevToolsStoreKeys.lastReleaseNotesVersion.name] ??= '') + as String; } set lastReleaseNotesVersion(String value) { - properties['lastReleaseNotesVersion'] = value; + properties[DevToolsStoreKeys.lastReleaseNotesVersion.name] = value; } } extension type _ActiveSurveyJson(Map json) { - bool get surveyActionTaken => json[DevToolsUsage._surveyActionTaken] as bool; - int? get surveyShownCount => json[DevToolsUsage._surveyShownCount] as int?; + bool get surveyActionTaken => + json[DevToolsStoreKeys.surveyActionTaken.name] as bool; + int? get surveyShownCount => + json[DevToolsStoreKeys.surveyShownCount.name] as int?; } diff --git a/packages/devtools_shared/lib/src/server/handlers/_app_size.dart b/packages/devtools_shared/lib/src/server/handlers/_app_size.dart new file mode 100644 index 00000000000..da5c41c2f11 --- /dev/null +++ b/packages/devtools_shared/lib/src/server/handlers/_app_size.dart @@ -0,0 +1,49 @@ +// 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. + +// ignore_for_file: avoid_classes_with_only_static_members + +part of '../server_api.dart'; + +abstract class _AppSizeHandler { + static shelf.Response getBaseAppSizeFile( + ServerApi api, + Map queryParams, + ) { + final missingRequiredParams = ServerApi._checkRequiredParameters( + [AppSizeApi.baseAppSizeFilePropertyName], + queryParams: queryParams, + api: api, + requestName: AppSizeApi.getBaseAppSizeFile, + ); + if (missingRequiredParams != null) return missingRequiredParams; + + final filePath = queryParams[AppSizeApi.baseAppSizeFilePropertyName]!; + final fileJson = LocalFileSystem.devToolsFileAsJson(filePath); + if (fileJson == null) { + return api.badRequest('No JSON file available at $filePath.'); + } + return api.success(fileJson); + } + + static shelf.Response getTestAppSizeFile( + ServerApi api, + Map queryParams, + ) { + final missingRequiredParams = ServerApi._checkRequiredParameters( + [AppSizeApi.testAppSizeFilePropertyName], + queryParams: queryParams, + api: api, + requestName: AppSizeApi.getTestAppSizeFile, + ); + if (missingRequiredParams != null) return missingRequiredParams; + + final filePath = queryParams[AppSizeApi.testAppSizeFilePropertyName]!; + final fileJson = LocalFileSystem.devToolsFileAsJson(filePath); + if (fileJson == null) { + return api.badRequest('No JSON file available at $filePath.'); + } + return api.success(fileJson); + } +} diff --git a/packages/devtools_shared/lib/src/server/handlers/_release_notes.dart b/packages/devtools_shared/lib/src/server/handlers/_release_notes.dart new file mode 100644 index 00000000000..8e0b6396772 --- /dev/null +++ b/packages/devtools_shared/lib/src/server/handlers/_release_notes.dart @@ -0,0 +1,42 @@ +// 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. + +// ignore_for_file: avoid_classes_with_only_static_members + +part of '../server_api.dart'; + +abstract class _ReleaseNotesHandler { + static shelf.Response getLastReleaseNotesVersion( + ServerApi api, + DevToolsUsage devToolsStore, + ) { + return _StorageHandler.handleGetStorageValue( + api, + devToolsStore, + key: DevToolsStoreKeys.lastReleaseNotesVersion.name, + defaultValue: '', + ); + } + + static shelf.Response setLastReleaseNotesVersion( + ServerApi api, + Map queryParams, + DevToolsUsage devToolsStore, + ) { + final missingRequiredParams = ServerApi._checkRequiredParameters( + [apiParameterValueKey], + queryParams: queryParams, + api: api, + requestName: ReleaseNotesApi.setLastReleaseNotesVersion, + ); + if (missingRequiredParams != null) return missingRequiredParams; + + return _StorageHandler.handleSetStorageValue( + api, + devToolsStore, + key: DevToolsStoreKeys.lastReleaseNotesVersion.name, + value: queryParams[apiParameterValueKey]!, + ); + } +} diff --git a/packages/devtools_shared/lib/src/server/handlers/_storage.dart b/packages/devtools_shared/lib/src/server/handlers/_storage.dart new file mode 100644 index 00000000000..fe80eaccd02 --- /dev/null +++ b/packages/devtools_shared/lib/src/server/handlers/_storage.dart @@ -0,0 +1,32 @@ +// 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. + +// ignore_for_file: avoid_classes_with_only_static_members + +part of '../server_api.dart'; + +abstract class _StorageHandler { + static shelf.Response handleGetStorageValue( + ServerApi api, + DevToolsUsage devToolsStore, { + required String key, + required T defaultValue, + }) { + final T value = (devToolsStore.properties[key] as T?) ?? defaultValue; + return ServerApi._encodeResponse( + value, + api: api, + ); + } + + static shelf.Response handleSetStorageValue( + ServerApi api, + DevToolsUsage devToolsStore, { + required String key, + required T value, + }) { + devToolsStore.properties[key] = value; + return ServerApi._encodeResponse(devToolsStore.properties[key], api: api); + } +} diff --git a/packages/devtools_shared/lib/src/server/server_api.dart b/packages/devtools_shared/lib/src/server/server_api.dart index ba9d1c67d02..99ca0d85d70 100644 --- a/packages/devtools_shared/lib/src/server/server_api.dart +++ b/packages/devtools_shared/lib/src/server/server_api.dart @@ -28,10 +28,13 @@ import 'flutter_store.dart'; // TODO(kenz): consider using Dart augmentation libraries instead of part files // if there is a clear benefit. +part 'handlers/_app_size.dart'; part 'handlers/_deeplink.dart'; part 'handlers/_devtools_extensions.dart'; part 'handlers/_dtd.dart'; part 'handlers/_general.dart'; +part 'handlers/_release_notes.dart'; +part 'handlers/_storage.dart'; /// The DevTools server API. /// @@ -180,49 +183,26 @@ class ServerApi { // ----- Release notes api. ----- - case apiGetLastReleaseNotesVersion: - return _encodeResponse( - _devToolsStore.lastReleaseNotesVersion, - api: api, + case ReleaseNotesApi.getLastReleaseNotesVersion: + return _ReleaseNotesHandler.getLastReleaseNotesVersion( + api, + _devToolsStore, + ); + + case ReleaseNotesApi.setLastReleaseNotesVersion: + return _ReleaseNotesHandler.setLastReleaseNotesVersion( + api, + queryParams, + _devToolsStore, ); - case apiSetLastReleaseNotesVersion: - if (queryParams.containsKey(lastReleaseNotesVersionPropertyName)) { - _devToolsStore.lastReleaseNotesVersion = - queryParams[lastReleaseNotesVersionPropertyName]!; - return _encodeResponse(true, api: api); - } - return _encodeResponse(false, api: api); // ----- App size api. ----- - case apiGetBaseAppSizeFile: - if (queryParams.containsKey(baseAppSizeFilePropertyName)) { - final filePath = queryParams[baseAppSizeFilePropertyName]!; - final fileJson = LocalFileSystem.devToolsFileAsJson(filePath); - if (fileJson == null) { - return api.badRequest('No JSON file available at $filePath.'); - } - return api.success(fileJson); - } - return api.badRequest( - 'Request for base app size file does not ' - 'contain a query parameter with the expected key: ' - '$baseAppSizeFilePropertyName', - ); - case apiGetTestAppSizeFile: - if (queryParams.containsKey(testAppSizeFilePropertyName)) { - final filePath = queryParams[testAppSizeFilePropertyName]!; - final fileJson = LocalFileSystem.devToolsFileAsJson(filePath); - if (fileJson == null) { - return api.badRequest('No JSON file available at $filePath.'); - } - return api.success(fileJson); - } - return api.badRequest( - 'Request for test app size file does not ' - 'contain a query parameter with the expected key: ' - '$testAppSizeFilePropertyName', - ); + case AppSizeApi.getBaseAppSizeFile: + return _AppSizeHandler.getBaseAppSizeFile(api, queryParams); + + case AppSizeApi.getTestAppSizeFile: + return _AppSizeHandler.getTestAppSizeFile(api, queryParams); // ----- Extensions api. ----- @@ -269,8 +249,14 @@ class ServerApi { queryParams, deeplinkManager, ); + + // ----- DTD api. ----- + case DtdApi.apiGetDtdUri: return _DtdApiHandler.handleGetDtdUri(api, dtd); + + // ----- Unimplemented. ----- + default: return api.notImplemented(); } diff --git a/packages/devtools_shared/pubspec.yaml b/packages/devtools_shared/pubspec.yaml index aed0fb7849d..3c9593686e1 100644 --- a/packages/devtools_shared/pubspec.yaml +++ b/packages/devtools_shared/pubspec.yaml @@ -1,7 +1,7 @@ name: devtools_shared description: Package of shared Dart structures between devtools_app, dds, and other tools. -version: 10.0.2 +version: 10.1.0 repository: https://github.com/flutter/devtools/tree/master/packages/devtools_shared From 2cf9b57e8f7759499f5b2174e9f5d32902e2dd7e Mon Sep 17 00:00:00 2001 From: Kenzie Schmoll Date: Thu, 29 Aug 2024 14:13:58 -0700 Subject: [PATCH 02/18] Cleanup tech debt for the survey-related DevTools server APIs --- .../lib/src/shared/server/_survey_api.dart | 24 +++--- .../devtools_app/lib/src/shared/survey.dart | 4 + packages/devtools_shared/CHANGELOG.md | 7 ++ .../devtools_shared/lib/src/devtools_api.dart | 79 +++++++++++++++---- .../lib/src/server/server_api.dart | 30 ++++--- 5 files changed, 102 insertions(+), 42 deletions(-) diff --git a/packages/devtools_app/lib/src/shared/server/_survey_api.dart b/packages/devtools_app/lib/src/shared/server/_survey_api.dart index 80cf1a2492d..92db3ce8af9 100644 --- a/packages/devtools_app/lib/src/shared/server/_survey_api.dart +++ b/packages/devtools_app/lib/src/shared/server/_survey_api.dart @@ -15,13 +15,13 @@ part of 'server.dart'; Future setActiveSurvey(String value) async { if (isDevToolsServerAvailable) { final resp = await request( - '$apiSetActiveSurvey' - '?$activeSurveyName=$value', + '${SurveyApi.setActiveSurvey}' + '?$apiParameterValueKey=$value', ); if ((resp?.statusOk ?? false) && json.decode(resp!.body)) { return true; } else { - logWarning(resp, apiSetActiveSurvey); + logWarning(resp, SurveyApi.setActiveSurvey); } } return false; @@ -36,11 +36,11 @@ Future surveyActionTaken() async { bool surveyActionTaken = false; if (isDevToolsServerAvailable) { - final resp = await request(apiGetSurveyActionTaken); + final resp = await request(SurveyApi.getSurveyActionTaken); if (resp?.statusOk ?? false) { surveyActionTaken = json.decode(resp!.body); } else { - logWarning(resp, apiGetSurveyActionTaken); + logWarning(resp, SurveyApi.getSurveyActionTaken); } } @@ -55,11 +55,11 @@ Future surveyActionTaken() async { Future setSurveyActionTaken() async { if (isDevToolsServerAvailable) { final resp = await request( - '$apiSetSurveyActionTaken' - '?$surveyActionTakenPropertyName=true', + '${SurveyApi.setSurveyActionTaken}' + '?$apiParameterValueKey=true', ); if (resp == null || !resp.statusOk || !(json.decode(resp.body) as bool)) { - logWarning(resp, apiSetSurveyActionTaken); + logWarning(resp, SurveyApi.setSurveyActionTaken); } } } @@ -73,11 +73,11 @@ Future surveyShownCount() async { int surveyShownCount = 0; if (isDevToolsServerAvailable) { - final resp = await request(apiGetSurveyShownCount); + final resp = await request(SurveyApi.getSurveyShownCount); if (resp?.statusOk ?? false) { surveyShownCount = json.decode(resp!.body); } else { - logWarning(resp, apiGetSurveyShownCount); + logWarning(resp, SurveyApi.getSurveyShownCount); } } @@ -94,11 +94,11 @@ Future incrementSurveyShownCount() async { int surveyShownCount = 0; if (isDevToolsServerAvailable) { - final resp = await request(apiIncrementSurveyShownCount); + final resp = await request(SurveyApi.incrementSurveyShownCount); if (resp?.statusOk ?? false) { surveyShownCount = json.decode(resp!.body); } else { - logWarning(resp, apiIncrementSurveyShownCount); + logWarning(resp, SurveyApi.incrementSurveyShownCount); } } return surveyShownCount; diff --git a/packages/devtools_app/lib/src/shared/survey.dart b/packages/devtools_app/lib/src/shared/survey.dart index 63f6996ac5d..3c3dde75f03 100644 --- a/packages/devtools_app/lib/src/shared/survey.dart +++ b/packages/devtools_app/lib/src/shared/survey.dart @@ -54,6 +54,10 @@ class SurveyService { _cachedSurvey ??= await fetchSurveyContent(); if (_cachedSurvey?.id != null) { + // TODO(kenz): consider setting this value on the [SurveyService] and then + // we can send the active survey as a parameter in each survey-related + // DevTools server request. This would simplify the server API for + // DevTools surveys. await server.setActiveSurvey(_cachedSurvey!.id!); } diff --git a/packages/devtools_shared/CHANGELOG.md b/packages/devtools_shared/CHANGELOG.md index eba6c30a560..edf655e0731 100644 --- a/packages/devtools_shared/CHANGELOG.md +++ b/packages/devtools_shared/CHANGELOG.md @@ -6,6 +6,13 @@ * Deprecate `apiGetTestAppSizeFile` in favor of `AppSizeApi.getTestAppSizeFile`. * Deprecate `baseAppSizeFilePropertyName` in favor of `AppSizeApi.baseAppSizeFilePropertyName`. * Deprecate `testAppSizeFilePropertyName` in favor of `AppSizeApi.testAppSizeFilePropertyName`. +* Deprecate `apiSetActiveSurvey` in favor of `SurveyApi.setActiveSurvey`. +* Deprecate `activeSurveyName`. +* Deprecate `apiGetSurveyActionTaken` in favor of `SurveyApi.getSurveyActionTaken`. +* Deprecate `apiSetSurveyActionTaken` in favor of `SurveyApi.setSurveyActionTaken`. +* Deprecate `surveyActionTakenPropertyName`. +* Deprecate `apiGetSurveyShownCount` in favor of `SurveyApi.getSurveyShownCount`. +* Deprecate `apiIncrementSurveyShownCount` in favor of `SurveyApi.incrementSurveyShownCount`. # 10.0.2 * Update dependency `web_socket_channel: '>=2.4.0 <4.0.0'`. diff --git a/packages/devtools_shared/lib/src/devtools_api.dart b/packages/devtools_shared/lib/src/devtools_api.dart index b14a35d24c2..ace6f9c745e 100644 --- a/packages/devtools_shared/lib/src/devtools_api.dart +++ b/packages/devtools_shared/lib/src/devtools_api.dart @@ -28,29 +28,74 @@ const apiSetDevToolsEnabled = '${apiPrefix}setDevToolsEnabled'; /// in queryParameter: const devToolsEnabledPropertyName = 'enabled'; -/// Survey properties APIs: -/// setActiveSurvey sets the survey property to fetch and save JSON values e.g., Q1-2020 -const apiSetActiveSurvey = '${apiPrefix}setActiveSurvey'; +@Deprecated( + 'Use SurveyApi.setActiveSurvey instead. ' + 'This field will be removed in devtools_shared >= 11.0.0.', +) +const apiSetActiveSurvey = SurveyApi.setActiveSurvey; -/// Survey name passed in apiSetActiveSurvey, the activeSurveyName is the property name -/// passed as a queryParameter and is the property in ~/.devtools too. -const activeSurveyName = 'activeSurveyName'; +@Deprecated( + 'Use apiParameterValueKey for the query parameter of the ' + 'SurveyApi.setActiveSurvey request instead. ' + 'This field will be removed in devtools_shared >= 11.0.0.', +) +const activeSurveyName = apiParameterValueKey; -/// Returns the surveyActionTaken of the activeSurvey (apiSetActiveSurvey). -const apiGetSurveyActionTaken = '${apiPrefix}getSurveyActionTaken'; +@Deprecated( + 'Use SurveyApi.getSurveyActionTaken instead. ' + 'This field will be removed in devtools_shared >= 11.0.0.', +) +const apiGetSurveyActionTaken = SurveyApi.getSurveyActionTaken; + +@Deprecated( + 'Use SurveyApi.setSurveyActionTaken instead. ' + 'This field will be removed in devtools_shared >= 11.0.0.', +) +const apiSetSurveyActionTaken = SurveyApi.setSurveyActionTaken; -/// Sets the surveyActionTaken of the of the activeSurvey (apiSetActiveSurvey). -const apiSetSurveyActionTaken = '${apiPrefix}setSurveyActionTaken'; +@Deprecated( + 'Use apiParameterValueKey for the query parameter of the ' + 'SurveyApi.setSurveyActionTaken request instead. ' + 'This field will be removed in devtools_shared >= 11.0.0.', +) +const surveyActionTakenPropertyName = apiParameterValueKey; -/// Property name to apiSetSurveyActionTaken the surveyActionTaken is the name -/// passed in queryParameter: -const surveyActionTakenPropertyName = 'surveyActionTaken'; +@Deprecated( + 'Use SurveyApi.getSurveyShownCount instead. ' + 'This field will be removed in devtools_shared >= 11.0.0.', +) +const apiGetSurveyShownCount = SurveyApi.getSurveyShownCount; -/// Returns the surveyShownCount of the of the activeSurvey (apiSetActiveSurvey). -const apiGetSurveyShownCount = '${apiPrefix}getSurveyShownCount'; +@Deprecated( + 'Use SurveyApi.incrementSurveyShownCount instead. ' + 'This field will be removed in devtools_shared >= 11.0.0.', +) +const apiIncrementSurveyShownCount = SurveyApi.incrementSurveyShownCount; -/// Increments the surveyShownCount of the of the activeSurvey (apiSetActiveSurvey). -const apiIncrementSurveyShownCount = '${apiPrefix}incrementSurveyShownCount'; +abstract class SurveyApi { + /// Sets the active survey value for the DevTools session. + /// + /// The active survey is used as a key to get and set values within the + /// DevTools store file. + static const setActiveSurvey = '${apiPrefix}setActiveSurvey'; + + /// Returns the 'surveyActionTaken' value for the active survey set by + /// [setActiveSurvey]. + static const getSurveyActionTaken = '${apiPrefix}getSurveyActionTaken'; + + /// Sets the 'surveyActionTaken' value for the active survey set by + /// [setActiveSurvey]. + static const setSurveyActionTaken = '${apiPrefix}setSurveyActionTaken'; + + /// Returns the 'surveyShownCount' value for the active survey set by + /// [setActiveSurvey]. + static const getSurveyShownCount = '${apiPrefix}getSurveyShownCount'; + + /// Increments the 'surveyShownCount' value for the active survey set by + /// [setActiveSurvey]. + static const incrementSurveyShownCount = + '${apiPrefix}incrementSurveyShownCount'; +} @Deprecated( 'Use apiParameterValueKey for the query parameter of the ' diff --git a/packages/devtools_shared/lib/src/server/server_api.dart b/packages/devtools_shared/lib/src/server/server_api.dart index 99ca0d85d70..7ff6970f8e1 100644 --- a/packages/devtools_shared/lib/src/server/server_api.dart +++ b/packages/devtools_shared/lib/src/server/server_api.dart @@ -113,9 +113,13 @@ class ServerApi { } return _encodeResponse(_devToolsStore.analyticsEnabled, api: api); + // ----- Preferences api. ----- + + // TODO(kenz): move all the handlers into a separate handler class as a + // follow up PR to preserve the diff. // ----- DevTools survey store. ----- - case apiSetActiveSurvey: + case SurveyApi.setActiveSurvey: // Assume failure. bool result = false; @@ -123,20 +127,20 @@ class ServerApi { // apiSetSurveyActionTaken, apiGetSurveyShownCount, and // apiIncrementSurveyShownCount calls. if (queryParams.keys.length == 1 && - queryParams.containsKey(activeSurveyName)) { - final surveyName = queryParams[activeSurveyName]!; + queryParams.containsKey(apiParameterValueKey)) { + final surveyName = queryParams[apiParameterValueKey]!; // Set the current activeSurvey. _devToolsStore.activeSurvey = surveyName; result = true; } return _encodeResponse(result, api: api); - case apiGetSurveyActionTaken: + case SurveyApi.getSurveyActionTaken: // Request setActiveSurvey has not been requested. if (_devToolsStore.activeSurvey == null) { return api.badRequest( '$errorNoActiveSurvey ' - '- $apiGetSurveyActionTaken', + '- ${SurveyApi.getSurveyActionTaken}', ); } // SurveyActionTaken has the survey been acted upon (taken or dismissed) @@ -144,37 +148,37 @@ class ServerApi { // 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: + case SurveyApi.setSurveyActionTaken: // Request setActiveSurvey has not been requested. if (_devToolsStore.activeSurvey == null) { return api.badRequest( '$errorNoActiveSurvey ' - '- $apiSetSurveyActionTaken', + '- ${SurveyApi.setSurveyActionTaken}', ); } // Set the SurveyActionTaken. // Has the survey been taken or dismissed.. - if (queryParams.containsKey(surveyActionTakenPropertyName)) { + if (queryParams.containsKey(apiParameterValueKey)) { _devToolsStore.surveyActionTaken = - json.decode(queryParams[surveyActionTakenPropertyName]!); + json.decode(queryParams[apiParameterValueKey]!); } return _encodeResponse(_devToolsStore.surveyActionTaken, api: api); - case apiGetSurveyShownCount: + case SurveyApi.getSurveyShownCount: // Request setActiveSurvey has not been requested. if (_devToolsStore.activeSurvey == null) { return api.badRequest( '$errorNoActiveSurvey ' - '- $apiGetSurveyShownCount', + '- ${SurveyApi.getSurveyShownCount}', ); } // SurveyShownCount how many times have we asked to take survey. return _encodeResponse(_devToolsStore.surveyShownCount, api: api); - case apiIncrementSurveyShownCount: + case SurveyApi.incrementSurveyShownCount: // Request setActiveSurvey has not been requested. if (_devToolsStore.activeSurvey == null) { return api.badRequest( '$errorNoActiveSurvey ' - '- $apiIncrementSurveyShownCount', + '- ${SurveyApi.incrementSurveyShownCount}', ); } // Increment the SurveyShownCount, we've asked about the survey. From 19ecbce17492f03ffcaf2936d8f56e875e8651d3 Mon Sep 17 00:00:00 2001 From: Kenzie Schmoll Date: Thu, 29 Aug 2024 14:38:28 -0700 Subject: [PATCH 03/18] remove lines --- packages/devtools_shared/lib/src/server/server_api.dart | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/devtools_shared/lib/src/server/server_api.dart b/packages/devtools_shared/lib/src/server/server_api.dart index 7ff6970f8e1..cdea32ab730 100644 --- a/packages/devtools_shared/lib/src/server/server_api.dart +++ b/packages/devtools_shared/lib/src/server/server_api.dart @@ -113,8 +113,6 @@ class ServerApi { } return _encodeResponse(_devToolsStore.analyticsEnabled, api: api); - // ----- Preferences api. ----- - // TODO(kenz): move all the handlers into a separate handler class as a // follow up PR to preserve the diff. // ----- DevTools survey store. ----- From 9d290d052e7939b042c61992cb6a6c9a2a6c83fe Mon Sep 17 00:00:00 2001 From: Kenzie Schmoll Date: Thu, 29 Aug 2024 14:45:36 -0700 Subject: [PATCH 04/18] Refactor the survey API handlers --- .../lib/src/shared/server/_survey_api.dart | 5 +- .../devtools_extensions/lib/src/api/api.dart | 2 +- .../devtools_shared/lib/src/devtools_api.dart | 6 +- .../lib/src/server/handlers/_survey.dart | 83 +++++++++++++++++++ .../lib/src/server/server_api.dart | 73 +++------------- 5 files changed, 98 insertions(+), 71 deletions(-) create mode 100644 packages/devtools_shared/lib/src/server/handlers/_survey.dart diff --git a/packages/devtools_app/lib/src/shared/server/_survey_api.dart b/packages/devtools_app/lib/src/shared/server/_survey_api.dart index 92db3ce8af9..b7e2fc31c5f 100644 --- a/packages/devtools_app/lib/src/shared/server/_survey_api.dart +++ b/packages/devtools_app/lib/src/shared/server/_survey_api.dart @@ -54,10 +54,7 @@ Future surveyActionTaken() async { /// Requires [setActiveSurvey] to have been called prior to calling this method. Future setSurveyActionTaken() async { if (isDevToolsServerAvailable) { - final resp = await request( - '${SurveyApi.setSurveyActionTaken}' - '?$apiParameterValueKey=true', - ); + final resp = await request(SurveyApi.setSurveyActionTaken); if (resp == null || !resp.statusOk || !(json.decode(resp.body) as bool)) { logWarning(resp, SurveyApi.setSurveyActionTaken); } diff --git a/packages/devtools_extensions/lib/src/api/api.dart b/packages/devtools_extensions/lib/src/api/api.dart index 9c0a7f23f79..5709de281f9 100644 --- a/packages/devtools_extensions/lib/src/api/api.dart +++ b/packages/devtools_extensions/lib/src/api/api.dart @@ -38,7 +38,7 @@ enum DevToolsExtensionEventType { /// An event that an extension can send to DevTools asking DevTools to copy /// some content to the user's clipboard. - /// + /// /// It is preferred that extensions send this event to DevTools to copy text /// instead of calling `Clipboard.setData` directly because DevTools contains /// additional logic for copying text from within an IDE-embedded web view. diff --git a/packages/devtools_shared/lib/src/devtools_api.dart b/packages/devtools_shared/lib/src/devtools_api.dart index ace6f9c745e..24890d0648f 100644 --- a/packages/devtools_shared/lib/src/devtools_api.dart +++ b/packages/devtools_shared/lib/src/devtools_api.dart @@ -54,11 +54,11 @@ const apiGetSurveyActionTaken = SurveyApi.getSurveyActionTaken; const apiSetSurveyActionTaken = SurveyApi.setSurveyActionTaken; @Deprecated( - 'Use apiParameterValueKey for the query parameter of the ' - 'SurveyApi.setSurveyActionTaken request instead. ' + 'This query parameter is no longer required for the ' + 'SurveyApi.setSurveyActionTaken request. ' 'This field will be removed in devtools_shared >= 11.0.0.', ) -const surveyActionTakenPropertyName = apiParameterValueKey; +const surveyActionTakenPropertyName = 'surveyActionTaken'; @Deprecated( 'Use SurveyApi.getSurveyShownCount instead. ' diff --git a/packages/devtools_shared/lib/src/server/handlers/_survey.dart b/packages/devtools_shared/lib/src/server/handlers/_survey.dart new file mode 100644 index 00000000000..fdb4fb201f3 --- /dev/null +++ b/packages/devtools_shared/lib/src/server/handlers/_survey.dart @@ -0,0 +1,83 @@ +// 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. + +// ignore_for_file: avoid_classes_with_only_static_members + +part of '../server_api.dart'; + +abstract class _SurveyHandler { + static shelf.Response setActiveSurvey( + ServerApi api, + Map queryParams, + DevToolsUsage devToolsStore, + ) { + final missingRequiredParams = ServerApi._checkRequiredParameters( + [apiParameterValueKey], + queryParams: queryParams, + api: api, + requestName: ReleaseNotesApi.setLastReleaseNotesVersion, + ); + if (missingRequiredParams != null) return missingRequiredParams; + + final surveyName = queryParams[apiParameterValueKey]!; + devToolsStore.activeSurvey = surveyName; + return ServerApi._encodeResponse(true, api: api); + } + + static shelf.Response getSurveyActionTaken( + ServerApi api, + DevToolsUsage devToolsStore, + ) { + final activeSurveySet = _checkActiveSurveySet(api, devToolsStore); + if (activeSurveySet != null) return activeSurveySet; + + return ServerApi._encodeResponse(devToolsStore.surveyActionTaken, api: api); + } + + static shelf.Response setSurveyActionTaken( + ServerApi api, + DevToolsUsage devToolsStore, + ) { + final activeSurveySet = _checkActiveSurveySet(api, devToolsStore); + if (activeSurveySet != null) return activeSurveySet; + + devToolsStore.surveyActionTaken = true; + return ServerApi._encodeResponse(devToolsStore.surveyActionTaken, api: api); + } + + static shelf.Response getSurveyShownCount( + ServerApi api, + DevToolsUsage devToolsStore, + ) { + final activeSurveySet = _checkActiveSurveySet(api, devToolsStore); + if (activeSurveySet != null) return activeSurveySet; + + return ServerApi._encodeResponse(devToolsStore.surveyShownCount, api: api); + } + + static shelf.Response incrementSurveyShownCount( + ServerApi api, + DevToolsUsage devToolsStore, + ) { + final activeSurveySet = _checkActiveSurveySet(api, devToolsStore); + if (activeSurveySet != null) return activeSurveySet; + + devToolsStore.incrementSurveyShownCount(); + return ServerApi._encodeResponse(devToolsStore.surveyShownCount, api: api); + } + + static const _errorNoActiveSurvey = 'ERROR: setActiveSurvey not called.'; + + static shelf.Response? _checkActiveSurveySet( + ServerApi api, + DevToolsUsage devToolsStore, + ) { + return devToolsStore.activeSurvey == null + ? api.badRequest( + '$_errorNoActiveSurvey ' + '- ${SurveyApi.getSurveyActionTaken}', + ) + : null; + } +} diff --git a/packages/devtools_shared/lib/src/server/server_api.dart b/packages/devtools_shared/lib/src/server/server_api.dart index cdea32ab730..944b1b68c81 100644 --- a/packages/devtools_shared/lib/src/server/server_api.dart +++ b/packages/devtools_shared/lib/src/server/server_api.dart @@ -35,6 +35,7 @@ part 'handlers/_dtd.dart'; part 'handlers/_general.dart'; part 'handlers/_release_notes.dart'; part 'handlers/_storage.dart'; +part 'handlers/_survey.dart'; /// The DevTools server API. /// @@ -42,7 +43,6 @@ part 'handlers/_storage.dart'; class ServerApi { static const logsKey = 'logs'; static const errorKey = 'error'; - static const errorNoActiveSurvey = 'ERROR: setActiveSurvey not called.'; /// Determines whether or not [request] is an API call. static bool canHandle(shelf.Request request) { @@ -113,75 +113,22 @@ class ServerApi { } return _encodeResponse(_devToolsStore.analyticsEnabled, api: api); - // TODO(kenz): move all the handlers into a separate handler class as a - // follow up PR to preserve the diff. // ----- DevTools survey store. ----- case SurveyApi.setActiveSurvey: - // Assume failure. - bool result = false; - - // Set the active survey used to store subsequent apiGetSurveyActionTaken, - // apiSetSurveyActionTaken, apiGetSurveyShownCount, and - // apiIncrementSurveyShownCount calls. - if (queryParams.keys.length == 1 && - queryParams.containsKey(apiParameterValueKey)) { - final surveyName = queryParams[apiParameterValueKey]!; - - // Set the current activeSurvey. - _devToolsStore.activeSurvey = surveyName; - result = true; - } - return _encodeResponse(result, api: api); + return _SurveyHandler.setActiveSurvey(api, queryParams, _devToolsStore); + case SurveyApi.getSurveyActionTaken: - // Request setActiveSurvey has not been requested. - if (_devToolsStore.activeSurvey == null) { - return api.badRequest( - '$errorNoActiveSurvey ' - '- ${SurveyApi.getSurveyActionTaken}', - ); - } - // SurveyActionTaken has the survey been acted upon (taken or dismissed) - 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. + return _SurveyHandler.getSurveyActionTaken(api, _devToolsStore); + case SurveyApi.setSurveyActionTaken: - // Request setActiveSurvey has not been requested. - if (_devToolsStore.activeSurvey == null) { - return api.badRequest( - '$errorNoActiveSurvey ' - '- ${SurveyApi.setSurveyActionTaken}', - ); - } - // Set the SurveyActionTaken. - // Has the survey been taken or dismissed.. - if (queryParams.containsKey(apiParameterValueKey)) { - _devToolsStore.surveyActionTaken = - json.decode(queryParams[apiParameterValueKey]!); - } - return _encodeResponse(_devToolsStore.surveyActionTaken, api: api); + return _SurveyHandler.setSurveyActionTaken(api, _devToolsStore); + case SurveyApi.getSurveyShownCount: - // Request setActiveSurvey has not been requested. - if (_devToolsStore.activeSurvey == null) { - return api.badRequest( - '$errorNoActiveSurvey ' - '- ${SurveyApi.getSurveyShownCount}', - ); - } - // SurveyShownCount how many times have we asked to take survey. - return _encodeResponse(_devToolsStore.surveyShownCount, api: api); + return _SurveyHandler.getSurveyShownCount(api, _devToolsStore); + case SurveyApi.incrementSurveyShownCount: - // Request setActiveSurvey has not been requested. - if (_devToolsStore.activeSurvey == null) { - return api.badRequest( - '$errorNoActiveSurvey ' - '- ${SurveyApi.incrementSurveyShownCount}', - ); - } - // Increment the SurveyShownCount, we've asked about the survey. - _devToolsStore.incrementSurveyShownCount(); - return _encodeResponse(_devToolsStore.surveyShownCount, api: api); + return _SurveyHandler.incrementSurveyShownCount(api, _devToolsStore); // ----- Release notes api. ----- From 3ca3ae1622cef90f88083af32f13ad6c3dac144f Mon Sep 17 00:00:00 2001 From: Kenzie Schmoll Date: Thu, 29 Aug 2024 16:08:32 -0700 Subject: [PATCH 05/18] fix comment --- packages/devtools_shared/lib/src/server/server_api.dart | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/devtools_shared/lib/src/server/server_api.dart b/packages/devtools_shared/lib/src/server/server_api.dart index 0a6a5e23806..7899cd77088 100644 --- a/packages/devtools_shared/lib/src/server/server_api.dart +++ b/packages/devtools_shared/lib/src/server/server_api.dart @@ -113,9 +113,7 @@ class ServerApi { } return _encodeResponse(_devToolsStore.analyticsEnabled, api: api); - // TODO(kenz): move all the handlers into a separate handler class as a - // follow up PR to preserve the diff. - // ----- DevTools survey store. ----- + // ----- DevTools survey api. ----- case SurveyApi.setActiveSurvey: return _SurveyHandler.setActiveSurvey(api, queryParams, _devToolsStore); From ef557d8f8c97035f2c200eb1e8c690b2b75f90eb Mon Sep 17 00:00:00 2001 From: Kenzie Schmoll Date: Thu, 29 Aug 2024 16:28:10 -0700 Subject: [PATCH 06/18] Migrate get & set preference APIs to use the standard server connection. --- .../_framework_initialize_web.dart | 11 ++-- .../src/shared/server/_preferences_api.dart | 45 +++++++++++++++++ .../lib/src/shared/server/server.dart | 1 + .../src/shared/server/server_api_client.dart | 17 ------- .../devtools_shared/lib/src/devtools_api.dart | 17 +++++++ .../lib/src/server/handlers/_preferences.dart | 50 +++++++++++++++++++ .../lib/src/server/handlers/_storage.dart | 4 +- .../lib/src/server/server_api.dart | 16 ++++++ 8 files changed, 135 insertions(+), 26 deletions(-) create mode 100644 packages/devtools_app/lib/src/shared/server/_preferences_api.dart create mode 100644 packages/devtools_shared/lib/src/server/handlers/_preferences.dart diff --git a/packages/devtools_app/lib/src/shared/config_specific/framework_initialize/_framework_initialize_web.dart b/packages/devtools_app/lib/src/shared/config_specific/framework_initialize/_framework_initialize_web.dart index 0b21a73ab8b..30795d963dd 100644 --- a/packages/devtools_app/lib/src/shared/config_specific/framework_initialize/_framework_initialize_web.dart +++ b/packages/devtools_app/lib/src/shared/config_specific/framework_initialize/_framework_initialize_web.dart @@ -11,6 +11,7 @@ import 'package:web/web.dart' hide Storage; import '../../../service/service_manager.dart'; import '../../globals.dart'; import '../../primitives/storage.dart'; +import '../../server/server.dart' as server; import '../../server/server_api_client.dart'; /// Return the url the application is launched from. @@ -29,7 +30,7 @@ Future initializePlatform() async { // establishing this connection is a best-effort. final connection = await DevToolsServerConnection.connect(); if (connection != null) { - setGlobal(Storage, ServerConnectionStorage(connection)); + setGlobal(Storage, ServerConnectionStorage()); } else { setGlobal(Storage, BrowserStorage()); } @@ -89,18 +90,14 @@ void _sendKeyPressToParent(KeyboardEvent event) { } class ServerConnectionStorage implements Storage { - ServerConnectionStorage(this.connection); - - final DevToolsServerConnection connection; - @override Future getValue(String key) { - return connection.getPreferenceValue(key); + return server.getPreferenceValue(key); } @override Future setValue(String key, String value) async { - await connection.setPreferenceValue(key, value); + await server.setPreferenceValue(key, value); } } diff --git a/packages/devtools_app/lib/src/shared/server/_preferences_api.dart b/packages/devtools_app/lib/src/shared/server/_preferences_api.dart new file mode 100644 index 00000000000..a2d3d67cb78 --- /dev/null +++ b/packages/devtools_app/lib/src/shared/server/_preferences_api.dart @@ -0,0 +1,45 @@ +// 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. + +part of 'server.dart'; + +/// Requests the DevTools preference for the [key]. +/// +/// This value is stored in the file '~/.flutter-devtools/.devtools'. +Future getPreferenceValue(String key) async { + if (isDevToolsServerAvailable) { + final uri = Uri( + path: PreferencesApi.getPreferenceValue, + queryParameters: { + PreferencesApi.preferenceKeyProperty: key, + }, + ); + final resp = await request(uri.toString()); + if (resp?.statusOk ?? false) { + return resp!.body; + } else { + logWarning(resp, PreferencesApi.getPreferenceValue); + } + } + return null; +} + +/// Sets the DevTools preference [value] for the [key]. +/// +/// This value is stored in the file '~/.flutter-devtools/.devtools'. +Future setPreferenceValue(String key, Object value) async { + if (isDevToolsServerAvailable) { + final uri = Uri( + path: PreferencesApi.setPreferenceValue, + queryParameters: { + PreferencesApi.preferenceKeyProperty: key, + apiParameterValueKey: value, + }, + ); + final resp = await request(uri.toString()); + if (resp == null || !resp.statusOk) { + logWarning(resp, PreferencesApi.setPreferenceValue); + } + } +} diff --git a/packages/devtools_app/lib/src/shared/server/server.dart b/packages/devtools_app/lib/src/shared/server/server.dart index e9f4698bff2..583996a9c38 100644 --- a/packages/devtools_app/lib/src/shared/server/server.dart +++ b/packages/devtools_app/lib/src/shared/server/server.dart @@ -19,6 +19,7 @@ part '_analytics_api.dart'; part '_app_size_api.dart'; part '_deep_links_api.dart'; part '_extensions_api.dart'; +part '_preferences_api.dart'; part '_release_notes_api.dart'; part '_survey_api.dart'; part '_dtd_api.dart'; diff --git a/packages/devtools_app/lib/src/shared/server/server_api_client.dart b/packages/devtools_app/lib/src/shared/server/server_api_client.dart index 6c6db4dee4a..bd637354315 100644 --- a/packages/devtools_app/lib/src/shared/server/server_api_client.dart +++ b/packages/devtools_app/lib/src/shared/server/server_api_client.dart @@ -204,23 +204,6 @@ class DevToolsServerConnection { unawaited(_callMethod('disconnected')); } - /// Retrieves a preference value from the DevTools configuration file at - /// ~/.flutter-devtools/.devtools. - Future getPreferenceValue(String key) { - return _callMethod('getPreferenceValue', { - 'key': key, - }); - } - - /// Sets a preference value in the DevTools configuration file at - /// ~/.flutter-devtools/.devtools. - Future setPreferenceValue(String key, String value) async { - await _callMethod('setPreferenceValue', { - 'key': key, - 'value': value, - }); - } - /// Allows the server to ping the client to see that it is definitely still /// active and doesn't just appear to be connected because of SSE timeouts. void ping() { diff --git a/packages/devtools_shared/lib/src/devtools_api.dart b/packages/devtools_shared/lib/src/devtools_api.dart index 24890d0648f..49d98573ebe 100644 --- a/packages/devtools_shared/lib/src/devtools_api.dart +++ b/packages/devtools_shared/lib/src/devtools_api.dart @@ -28,6 +28,23 @@ const apiSetDevToolsEnabled = '${apiPrefix}setDevToolsEnabled'; /// in queryParameter: const devToolsEnabledPropertyName = 'enabled'; +abstract class PreferencesApi { + /// Returns the preference value in the DevTools store file for the key + /// specified by the [preferenceKeyProperty] query parameter. + static const getPreferenceValue = 'getPreferenceValue'; + + /// Sets the preference value in the DevTools store file for the key + /// specified by the [preferenceKeyProperty] query parameter. + /// + /// The value must be specified by the [apiParameterValueKey] query parameter. + static const setPreferenceValue = 'setPreferenceValue'; + + /// The property name for the query parameter passed along with the + /// [getPreferenceValue] and [setPreferenceValue] requests that describes the + /// preference key in the DevTools store file. + static const preferenceKeyProperty = 'key'; +} + @Deprecated( 'Use SurveyApi.setActiveSurvey instead. ' 'This field will be removed in devtools_shared >= 11.0.0.', diff --git a/packages/devtools_shared/lib/src/server/handlers/_preferences.dart b/packages/devtools_shared/lib/src/server/handlers/_preferences.dart new file mode 100644 index 00000000000..7a4d2c97990 --- /dev/null +++ b/packages/devtools_shared/lib/src/server/handlers/_preferences.dart @@ -0,0 +1,50 @@ +// 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. + +// ignore_for_file: avoid_classes_with_only_static_members + +part of '../server_api.dart'; + +abstract class _PreferencesApiHandler { + static shelf.Response getPreferenceValue( + ServerApi api, + Map queryParams, + DevToolsUsage devToolsStore, + ) { + final missingRequiredParams = ServerApi._checkRequiredParameters( + [PreferencesApi.preferenceKeyProperty], + queryParams: queryParams, + api: api, + requestName: PreferencesApi.getPreferenceValue, + ); + if (missingRequiredParams != null) return missingRequiredParams; + + return _StorageHandler.handleGetStorageValue( + api, + devToolsStore, + key: queryParams[PreferencesApi.preferenceKeyProperty]!, + ); + } + + static shelf.Response setPreferenceValue( + ServerApi api, + Map queryParams, + DevToolsUsage devToolsStore, + ) { + final missingRequiredParams = ServerApi._checkRequiredParameters( + [PreferencesApi.preferenceKeyProperty, apiParameterValueKey], + queryParams: queryParams, + api: api, + requestName: PreferencesApi.setPreferenceValue, + ); + if (missingRequiredParams != null) return missingRequiredParams; + + return _StorageHandler.handleSetStorageValue( + api, + devToolsStore, + key: queryParams[PreferencesApi.preferenceKeyProperty]!, + value: queryParams[apiParameterValueKey]! as T, + ); + } +} diff --git a/packages/devtools_shared/lib/src/server/handlers/_storage.dart b/packages/devtools_shared/lib/src/server/handlers/_storage.dart index fe80eaccd02..070c0397566 100644 --- a/packages/devtools_shared/lib/src/server/handlers/_storage.dart +++ b/packages/devtools_shared/lib/src/server/handlers/_storage.dart @@ -11,9 +11,9 @@ abstract class _StorageHandler { ServerApi api, DevToolsUsage devToolsStore, { required String key, - required T defaultValue, + T? defaultValue, }) { - final T value = (devToolsStore.properties[key] as T?) ?? defaultValue; + final T? value = (devToolsStore.properties[key] as T?) ?? defaultValue; return ServerApi._encodeResponse( value, api: api, diff --git a/packages/devtools_shared/lib/src/server/server_api.dart b/packages/devtools_shared/lib/src/server/server_api.dart index 7899cd77088..46a38810272 100644 --- a/packages/devtools_shared/lib/src/server/server_api.dart +++ b/packages/devtools_shared/lib/src/server/server_api.dart @@ -33,6 +33,7 @@ part 'handlers/_deeplink.dart'; part 'handlers/_devtools_extensions.dart'; part 'handlers/_dtd.dart'; part 'handlers/_general.dart'; +part 'handlers/_preferences.dart'; part 'handlers/_release_notes.dart'; part 'handlers/_storage.dart'; part 'handlers/_survey.dart'; @@ -113,6 +114,21 @@ class ServerApi { } return _encodeResponse(_devToolsStore.analyticsEnabled, api: api); + // ----- Preferences api. ----- + case PreferencesApi.getPreferenceValue: + return _PreferencesApiHandler.getPreferenceValue( + api, + queryParams, + _devToolsStore, + ); + + case PreferencesApi.setPreferenceValue: + return _PreferencesApiHandler.setPreferenceValue( + api, + queryParams, + _devToolsStore, + ); + // ----- DevTools survey api. ----- case SurveyApi.setActiveSurvey: From 1832f65dbaab94f74eb918b19cba45daf8f3097c Mon Sep 17 00:00:00 2001 From: Kenzie Schmoll Date: Fri, 30 Aug 2024 09:22:01 -0700 Subject: [PATCH 07/18] add todo --- .../framework_initialize/_framework_initialize_web.dart | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/devtools_app/lib/src/shared/config_specific/framework_initialize/_framework_initialize_web.dart b/packages/devtools_app/lib/src/shared/config_specific/framework_initialize/_framework_initialize_web.dart index 30795d963dd..08fc6216ad4 100644 --- a/packages/devtools_app/lib/src/shared/config_specific/framework_initialize/_framework_initialize_web.dart +++ b/packages/devtools_app/lib/src/shared/config_specific/framework_initialize/_framework_initialize_web.dart @@ -25,6 +25,8 @@ Future initializePlatform() async { }.toJS, ); + // TODO(kenz): this server connection initialized listeners that are never + // disposed, so this is likely leaking resources. // Here, we try and initialize the connection between the DevTools web app and // its local server. DevTools can be launched without the server however, so // establishing this connection is a best-effort. From 77efe6f8f65baa1875f4e8d699f91e7a37ddfbb7 Mon Sep 17 00:00:00 2001 From: Kenzie Schmoll Date: Fri, 30 Aug 2024 09:23:24 -0700 Subject: [PATCH 08/18] review comments --- .../src/shared/server/_preferences_api.dart | 52 +++++++++---------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/packages/devtools_app/lib/src/shared/server/_preferences_api.dart b/packages/devtools_app/lib/src/shared/server/_preferences_api.dart index a2d3d67cb78..c8b37d06198 100644 --- a/packages/devtools_app/lib/src/shared/server/_preferences_api.dart +++ b/packages/devtools_app/lib/src/shared/server/_preferences_api.dart @@ -8,38 +8,38 @@ part of 'server.dart'; /// /// This value is stored in the file '~/.flutter-devtools/.devtools'. Future getPreferenceValue(String key) async { - if (isDevToolsServerAvailable) { - final uri = Uri( - path: PreferencesApi.getPreferenceValue, - queryParameters: { - PreferencesApi.preferenceKeyProperty: key, - }, - ); - final resp = await request(uri.toString()); - if (resp?.statusOk ?? false) { - return resp!.body; - } else { - logWarning(resp, PreferencesApi.getPreferenceValue); - } + if (!isDevToolsServerAvailable) return null; + + final uri = Uri( + path: PreferencesApi.getPreferenceValue, + queryParameters: { + PreferencesApi.preferenceKeyProperty: key, + }, + ); + final resp = await request(uri.toString()); + if (resp?.statusOk ?? false) { + return resp!.body; + } else { + logWarning(resp, PreferencesApi.getPreferenceValue); + return null; } - return null; } /// Sets the DevTools preference [value] for the [key]. /// /// This value is stored in the file '~/.flutter-devtools/.devtools'. Future setPreferenceValue(String key, Object value) async { - if (isDevToolsServerAvailable) { - final uri = Uri( - path: PreferencesApi.setPreferenceValue, - queryParameters: { - PreferencesApi.preferenceKeyProperty: key, - apiParameterValueKey: value, - }, - ); - final resp = await request(uri.toString()); - if (resp == null || !resp.statusOk) { - logWarning(resp, PreferencesApi.setPreferenceValue); - } + if (!isDevToolsServerAvailable) return; + + final uri = Uri( + path: PreferencesApi.setPreferenceValue, + queryParameters: { + PreferencesApi.preferenceKeyProperty: key, + apiParameterValueKey: value, + }, + ); + final resp = await request(uri.toString()); + if (resp == null || !resp.statusOk) { + logWarning(resp, PreferencesApi.setPreferenceValue); } } From 83d3841036376d1d84b865b8f2de9dacdc0269ce Mon Sep 17 00:00:00 2001 From: Kenzie Schmoll Date: Fri, 30 Aug 2024 10:48:42 -0700 Subject: [PATCH 09/18] add changelog --- packages/devtools_shared/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/devtools_shared/CHANGELOG.md b/packages/devtools_shared/CHANGELOG.md index edf655e0731..aba136d763f 100644 --- a/packages/devtools_shared/CHANGELOG.md +++ b/packages/devtools_shared/CHANGELOG.md @@ -13,6 +13,7 @@ * Deprecate `surveyActionTakenPropertyName`. * Deprecate `apiGetSurveyShownCount` in favor of `SurveyApi.getSurveyShownCount`. * Deprecate `apiIncrementSurveyShownCount` in favor of `SurveyApi.incrementSurveyShownCount`. +* Add `PreferencesApi` to get and set preference values. # 10.0.2 * Update dependency `web_socket_channel: '>=2.4.0 <4.0.0'`. From 8b57b5626ca59e9e7340ee824220b874de3166bc Mon Sep 17 00:00:00 2001 From: Kenzie Schmoll Date: Fri, 30 Aug 2024 12:27:23 -0700 Subject: [PATCH 10/18] Fix api strings --- packages/devtools_shared/lib/src/devtools_api.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/devtools_shared/lib/src/devtools_api.dart b/packages/devtools_shared/lib/src/devtools_api.dart index 49d98573ebe..23a771c5eea 100644 --- a/packages/devtools_shared/lib/src/devtools_api.dart +++ b/packages/devtools_shared/lib/src/devtools_api.dart @@ -31,13 +31,13 @@ const devToolsEnabledPropertyName = 'enabled'; abstract class PreferencesApi { /// Returns the preference value in the DevTools store file for the key /// specified by the [preferenceKeyProperty] query parameter. - static const getPreferenceValue = 'getPreferenceValue'; + static const getPreferenceValue = '${apiPrefix}getPreferenceValue'; /// Sets the preference value in the DevTools store file for the key /// specified by the [preferenceKeyProperty] query parameter. /// /// The value must be specified by the [apiParameterValueKey] query parameter. - static const setPreferenceValue = 'setPreferenceValue'; + static const setPreferenceValue = '${apiPrefix}setPreferenceValue'; /// The property name for the query parameter passed along with the /// [getPreferenceValue] and [setPreferenceValue] requests that describes the From e658b532146e547601c75d9c91987438f411dda8 Mon Sep 17 00:00:00 2001 From: Kenzie Schmoll Date: Fri, 30 Aug 2024 12:27:43 -0700 Subject: [PATCH 11/18] clean up --- .../devtools_shared/lib/src/server/handlers/_storage.dart | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/devtools_shared/lib/src/server/handlers/_storage.dart b/packages/devtools_shared/lib/src/server/handlers/_storage.dart index 070c0397566..fd762bfaf86 100644 --- a/packages/devtools_shared/lib/src/server/handlers/_storage.dart +++ b/packages/devtools_shared/lib/src/server/handlers/_storage.dart @@ -14,10 +14,7 @@ abstract class _StorageHandler { T? defaultValue, }) { final T? value = (devToolsStore.properties[key] as T?) ?? defaultValue; - return ServerApi._encodeResponse( - value, - api: api, - ); + return ServerApi._encodeResponse(value, api: api); } static shelf.Response handleSetStorageValue( From 9d209cf37855d1dd076f030037a3a2e1d09286ac Mon Sep 17 00:00:00 2001 From: Kenzie Schmoll Date: Fri, 30 Aug 2024 14:05:40 -0700 Subject: [PATCH 12/18] Increase integration test timeout --- .../devtools_shared/lib/src/test/integration_test_runner.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/devtools_shared/lib/src/test/integration_test_runner.dart b/packages/devtools_shared/lib/src/test/integration_test_runner.dart index 2e737a070ca..5eefc0a0630 100644 --- a/packages/devtools_shared/lib/src/test/integration_test_runner.dart +++ b/packages/devtools_shared/lib/src/test/integration_test_runner.dart @@ -103,7 +103,7 @@ class IntegrationTestRunner with IOMixin { ); bool testTimedOut = false; - final timeout = Future.delayed(const Duration(minutes: 8)).then((_) { + final timeout = Future.delayed(const Duration(minutes: 12)).then((_) { testTimedOut = true; }); From 7c480d9f2e837d50d1a77a4b6a05554964b65b03 Mon Sep 17 00:00:00 2001 From: Kenzie Schmoll Date: Fri, 30 Aug 2024 17:00:44 -0700 Subject: [PATCH 13/18] Fix type issue --- .../_framework_initialize_desktop.dart | 2 +- .../_framework_initialize_web.dart | 5 +- .../src/shared/preferences/preferences.dart | 67 +++++++++++++------ .../src/shared/server/_preferences_api.dart | 4 +- .../test/test_infra/flutter_test_storage.dart | 2 +- 5 files changed, 55 insertions(+), 25 deletions(-) diff --git a/packages/devtools_app/lib/src/shared/config_specific/framework_initialize/_framework_initialize_desktop.dart b/packages/devtools_app/lib/src/shared/config_specific/framework_initialize/_framework_initialize_desktop.dart index 581b59b5d7a..e8bb9365070 100644 --- a/packages/devtools_app/lib/src/shared/config_specific/framework_initialize/_framework_initialize_desktop.dart +++ b/packages/devtools_app/lib/src/shared/config_specific/framework_initialize/_framework_initialize_desktop.dart @@ -29,7 +29,7 @@ class FlutterDesktopStorage implements Storage { } @override - Future setValue(String key, String value) async { + Future setValue(String key, String value) async { _values[key] = value; const encoder = JsonEncoder.withIndent(' '); diff --git a/packages/devtools_app/lib/src/shared/config_specific/framework_initialize/_framework_initialize_web.dart b/packages/devtools_app/lib/src/shared/config_specific/framework_initialize/_framework_initialize_web.dart index 08fc6216ad4..d091c9ca44f 100644 --- a/packages/devtools_app/lib/src/shared/config_specific/framework_initialize/_framework_initialize_web.dart +++ b/packages/devtools_app/lib/src/shared/config_specific/framework_initialize/_framework_initialize_web.dart @@ -93,8 +93,9 @@ void _sendKeyPressToParent(KeyboardEvent event) { class ServerConnectionStorage implements Storage { @override - Future getValue(String key) { - return server.getPreferenceValue(key); + Future getValue(String key) async { + final value = await server.getPreferenceValue(key); + return value == null ? null : '$value'; } @override diff --git a/packages/devtools_app/lib/src/shared/preferences/preferences.dart b/packages/devtools_app/lib/src/shared/preferences/preferences.dart index 4e91a17f0e3..c5a92903bfd 100644 --- a/packages/devtools_app/lib/src/shared/preferences/preferences.dart +++ b/packages/devtools_app/lib/src/shared/preferences/preferences.dart @@ -28,6 +28,23 @@ part '_performance_preferences.dart'; const _thirdPartyPathSegment = 'third_party'; +/// DevTools preferences for UI-related settings. +enum _UiPreferences { + darkMode, + vmDeveloperMode; + + String get storageKey => '$storagePrefix.$name'; + + static const storagePrefix = 'ui'; +} + +/// DevTools preferences for general settings. +/// +/// These values are not stored in the DevTools storage file with a prefix. +enum _GeneralPreferences { + verboseLogging, +} + /// A controller for global application preferences. class PreferencesController extends DisposableController with AutoDisposeControllerMixin { @@ -45,7 +62,6 @@ class PreferencesController extends DisposableController final verboseLoggingEnabled = ValueNotifier(Logger.root.level == verboseLoggingLevel); - static const _verboseLoggingStorageId = 'verboseLogging'; // TODO(https://github.com/flutter/devtools/issues/7860): Clean-up after // Inspector V2 has been released. @@ -69,44 +85,57 @@ class PreferencesController extends DisposableController Future init() async { // Get the current values and listen for and write back changes. - final darkModeValue = await storage.getValue('ui.darkMode'); + await _initDarkMode(); + await _initVmDeveloperMode(); + await _initVerboseLogging(); + + await inspector.init(); + await memory.init(); + await logging.init(); + await performance.init(); + await devToolsExtensions.init(); + + setGlobal(PreferencesController, this); + } + + Future _initDarkMode() async { + final darkModeValue = + await storage.getValue(_UiPreferences.darkMode.storageKey); final useDarkMode = (darkModeValue == null && useDarkThemeAsDefault) || darkModeValue == 'true'; ga.impression(gac.devToolsMain, gac.startingTheme(darkMode: useDarkMode)); toggleDarkModeTheme(useDarkMode); addAutoDisposeListener(darkModeEnabled, () { - storage.setValue('ui.darkMode', '${darkModeEnabled.value}'); + storage.setValue( + _UiPreferences.darkMode.storageKey, + '${darkModeEnabled.value}', + ); }); + } + Future _initVmDeveloperMode() async { final vmDeveloperModeValue = await boolValueFromStorage( - 'ui.vmDeveloperMode', + _UiPreferences.vmDeveloperMode.storageKey, defaultsTo: false, ); toggleVmDeveloperMode(vmDeveloperModeValue); addAutoDisposeListener(vmDeveloperModeEnabled, () { - storage.setValue('ui.vmDeveloperMode', '${vmDeveloperModeEnabled.value}'); + storage.setValue( + _UiPreferences.vmDeveloperMode.storageKey, + '${vmDeveloperModeEnabled.value}', + ); }); - - await _initVerboseLogging(); - - await inspector.init(); - await memory.init(); - await logging.init(); - await performance.init(); - await devToolsExtensions.init(); - - setGlobal(PreferencesController, this); } Future _initVerboseLogging() async { final verboseLoggingEnabledValue = await boolValueFromStorage( - _verboseLoggingStorageId, + _GeneralPreferences.verboseLogging.name, defaultsTo: false, ); toggleVerboseLogging(verboseLoggingEnabledValue); addAutoDisposeListener(verboseLoggingEnabled, () { storage.setValue( - 'verboseLogging', + _GeneralPreferences.verboseLogging.name, verboseLoggingEnabled.value.toString(), ); }); @@ -122,14 +151,14 @@ class PreferencesController extends DisposableController super.dispose(); } - /// Change the value for the dark mode setting. + /// Change the value of the dark mode setting. void toggleDarkModeTheme(bool? useDarkMode) { if (useDarkMode != null) { darkModeEnabled.value = useDarkMode; } } - /// Change the value for the VM developer mode setting. + /// Change the value of the VM developer mode setting. void toggleVmDeveloperMode(bool? enableVmDeveloperMode) { if (enableVmDeveloperMode != null) { vmDeveloperModeEnabled.value = enableVmDeveloperMode; diff --git a/packages/devtools_app/lib/src/shared/server/_preferences_api.dart b/packages/devtools_app/lib/src/shared/server/_preferences_api.dart index c8b37d06198..a75381eb122 100644 --- a/packages/devtools_app/lib/src/shared/server/_preferences_api.dart +++ b/packages/devtools_app/lib/src/shared/server/_preferences_api.dart @@ -7,7 +7,7 @@ part of 'server.dart'; /// Requests the DevTools preference for the [key]. /// /// This value is stored in the file '~/.flutter-devtools/.devtools'. -Future getPreferenceValue(String key) async { +Future getPreferenceValue(String key) async { if (!isDevToolsServerAvailable) return null; final uri = Uri( @@ -18,7 +18,7 @@ Future getPreferenceValue(String key) async { ); final resp = await request(uri.toString()); if (resp?.statusOk ?? false) { - return resp!.body; + return jsonDecode(resp!.body); } else { logWarning(resp, PreferencesApi.getPreferenceValue); return null; diff --git a/packages/devtools_app/test/test_infra/flutter_test_storage.dart b/packages/devtools_app/test/test_infra/flutter_test_storage.dart index b884b4ba3a4..3bb2f2e2d9e 100644 --- a/packages/devtools_app/test/test_infra/flutter_test_storage.dart +++ b/packages/devtools_app/test/test_infra/flutter_test_storage.dart @@ -16,7 +16,7 @@ class FlutterTestStorage implements Storage { } @override - Future setValue(String key, String value) async { + Future setValue(String key, String value) async { values[key] = value; } } From 52065ce83d16cd6a58f2130253576b63a5bdbe55 Mon Sep 17 00:00:00 2001 From: Kenzie Schmoll Date: Wed, 4 Sep 2024 08:45:12 -0700 Subject: [PATCH 14/18] enable debug logging on integration tests --- .../devtools_app/integration_test/test_infra/run/_utils.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/devtools_app/integration_test/test_infra/run/_utils.dart b/packages/devtools_app/integration_test/test_infra/run/_utils.dart index 012ed47e72f..1ec03224827 100644 --- a/packages/devtools_app/integration_test/test_infra/run/_utils.dart +++ b/packages/devtools_app/integration_test/test_infra/run/_utils.dart @@ -4,7 +4,7 @@ // ignore_for_file: avoid_print -bool debugTestScript = false; +bool debugTestScript = true; void debugLog(String log) { if (debugTestScript) { From 03860cba7e76bd92aa4ce70323cafdbe25fd7e6e Mon Sep 17 00:00:00 2001 From: Kenzie Schmoll Date: Wed, 4 Sep 2024 11:13:52 -0700 Subject: [PATCH 15/18] Logs and update flutter version --- flutter-candidate.txt | 2 +- .../lib/src/test/integration_test_runner.dart | 6 +++++- packages/devtools_shared/lib/src/test/io_utils.dart | 2 +- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/flutter-candidate.txt b/flutter-candidate.txt index 9a66865d80c..c7fccacdc92 100644 --- a/flutter-candidate.txt +++ b/flutter-candidate.txt @@ -1 +1 @@ -3e4f1cdf494e09304587f648456f2560d3628fe9 +80e7e4e1d2dd11173ad589ebf7477999118d44fe diff --git a/packages/devtools_shared/lib/src/test/integration_test_runner.dart b/packages/devtools_shared/lib/src/test/integration_test_runner.dart index 5eefc0a0630..177002f446a 100644 --- a/packages/devtools_shared/lib/src/test/integration_test_runner.dart +++ b/packages/devtools_shared/lib/src/test/integration_test_runner.dart @@ -103,7 +103,7 @@ class IntegrationTestRunner with IOMixin { ); bool testTimedOut = false; - final timeout = Future.delayed(const Duration(minutes: 12)).then((_) { + final timeout = Future.delayed(const Duration(minutes: 8)).then((_) { testTimedOut = true; }); @@ -112,6 +112,10 @@ class IntegrationTestRunner with IOMixin { timeout, ]); + debugLog( + 'shutting down processes because ' + '${testTimedOut ? 'test timed out' : 'test finished'}', + ); debugLog('attempting to kill the flutter drive process'); process.kill(); debugLog('flutter drive process has exited'); diff --git a/packages/devtools_shared/lib/src/test/io_utils.dart b/packages/devtools_shared/lib/src/test/io_utils.dart index a5ce8d7b020..83c4117456a 100644 --- a/packages/devtools_shared/lib/src/test/io_utils.dart +++ b/packages/devtools_shared/lib/src/test/io_utils.dart @@ -82,7 +82,7 @@ mixin IOMixin { }) async { final processId = process.pid; if (debugLogging) { - print('Sending SIGTERM to $processId..'); + print('Sending SIGTERM to $processId.'); } await cancelAllStreamSubscriptions(); Process.killPid(processId); From 846418bde6eb35346d4dd3e594cd15a7798a7e3d Mon Sep 17 00:00:00 2001 From: Kenzie Schmoll Date: Wed, 4 Sep 2024 11:18:06 -0700 Subject: [PATCH 16/18] More logging --- .../lib/src/test/integration_test_runner.dart | 31 +++++++++---------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/packages/devtools_shared/lib/src/test/integration_test_runner.dart b/packages/devtools_shared/lib/src/test/integration_test_runner.dart index 177002f446a..53f1ec6de05 100644 --- a/packages/devtools_shared/lib/src/test/integration_test_runner.dart +++ b/packages/devtools_shared/lib/src/test/integration_test_runner.dart @@ -36,22 +36,21 @@ class IntegrationTestRunner with IOMixin { Future runTest({required int attemptNumber}) async { debugLog('starting the flutter drive process'); - final process = await Process.start( - 'flutter', - [ - 'drive', - // Debug outputs from the test will not show up in profile mode. Since - // we rely on debug outputs for detecting errors and exceptions from the - // test, we cannot run this these tests in profile mode until this issue - // is resolved. See https://github.com/flutter/flutter/issues/69070. - // '--profile', - '--driver=$testDriver', - '--target=$testTarget', - '-d', - headless ? 'web-server' : 'chrome', - for (final arg in dartDefineArgs) '--dart-define=$arg', - ], - ); + final flutterDriveArgs = [ + 'drive', + // Debug outputs from the test will not show up in profile mode. Since + // we rely on debug outputs for detecting errors and exceptions from the + // test, we cannot run this these tests in profile mode until this issue + // is resolved. See https://github.com/flutter/flutter/issues/69070. + // '--profile', + '--driver=$testDriver', + '--target=$testTarget', + '-d', + headless ? 'web-server' : 'chrome', + for (final arg in dartDefineArgs) '--dart-define=$arg', + ]; + debugLog('> flutter ${flutterDriveArgs.join(' ')}'); + final process = await Process.start('flutter', flutterDriveArgs); bool stdOutWriteInProgress = false; bool stdErrWriteInProgress = false; From 35fc81c5b321bfb4e9ebd25f4431e1b6f2f78c07 Mon Sep 17 00:00:00 2001 From: Kenzie Schmoll Date: Wed, 4 Sep 2024 13:42:39 -0700 Subject: [PATCH 17/18] ridiculous timeout --- .../devtools_shared/lib/src/test/integration_test_runner.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/devtools_shared/lib/src/test/integration_test_runner.dart b/packages/devtools_shared/lib/src/test/integration_test_runner.dart index 53f1ec6de05..edb4ee20fe7 100644 --- a/packages/devtools_shared/lib/src/test/integration_test_runner.dart +++ b/packages/devtools_shared/lib/src/test/integration_test_runner.dart @@ -102,7 +102,7 @@ class IntegrationTestRunner with IOMixin { ); bool testTimedOut = false; - final timeout = Future.delayed(const Duration(minutes: 8)).then((_) { + final timeout = Future.delayed(const Duration(minutes: 60)).then((_) { testTimedOut = true; }); From e8dfe08981088047f07603dd07660dde5a573f89 Mon Sep 17 00:00:00 2001 From: Kenzie Schmoll Date: Fri, 6 Sep 2024 08:46:13 -0700 Subject: [PATCH 18/18] Clean up --- flutter-candidate.txt | 2 +- .../test_infra/run/_utils.dart | 2 +- packages/devtools_shared/CHANGELOG.md | 1 + .../lib/src/test/integration_test_runner.dart | 18 +++++++++++++----- .../devtools_shared/lib/src/test/io_utils.dart | 2 +- 5 files changed, 17 insertions(+), 8 deletions(-) diff --git a/flutter-candidate.txt b/flutter-candidate.txt index c7fccacdc92..9a66865d80c 100644 --- a/flutter-candidate.txt +++ b/flutter-candidate.txt @@ -1 +1 @@ -80e7e4e1d2dd11173ad589ebf7477999118d44fe +3e4f1cdf494e09304587f648456f2560d3628fe9 diff --git a/packages/devtools_app/integration_test/test_infra/run/_utils.dart b/packages/devtools_app/integration_test/test_infra/run/_utils.dart index 1ec03224827..012ed47e72f 100644 --- a/packages/devtools_app/integration_test/test_infra/run/_utils.dart +++ b/packages/devtools_app/integration_test/test_infra/run/_utils.dart @@ -4,7 +4,7 @@ // ignore_for_file: avoid_print -bool debugTestScript = true; +bool debugTestScript = false; void debugLog(String log) { if (debugTestScript) { diff --git a/packages/devtools_shared/CHANGELOG.md b/packages/devtools_shared/CHANGELOG.md index aba136d763f..dbbcac39337 100644 --- a/packages/devtools_shared/CHANGELOG.md +++ b/packages/devtools_shared/CHANGELOG.md @@ -13,6 +13,7 @@ * Deprecate `surveyActionTakenPropertyName`. * Deprecate `apiGetSurveyShownCount` in favor of `SurveyApi.getSurveyShownCount`. * Deprecate `apiIncrementSurveyShownCount` in favor of `SurveyApi.incrementSurveyShownCount`. +* Support Chrome's new headless mode in the integration test runner. * Add `PreferencesApi` to get and set preference values. # 10.0.2 diff --git a/packages/devtools_shared/lib/src/test/integration_test_runner.dart b/packages/devtools_shared/lib/src/test/integration_test_runner.dart index edb4ee20fe7..dcaf525267d 100644 --- a/packages/devtools_shared/lib/src/test/integration_test_runner.dart +++ b/packages/devtools_shared/lib/src/test/integration_test_runner.dart @@ -35,7 +35,9 @@ class IntegrationTestRunner with IOMixin { } Future runTest({required int attemptNumber}) async { + debugLog('starting attempt #$attemptNumber for $testTarget'); debugLog('starting the flutter drive process'); + final flutterDriveArgs = [ 'drive', // Debug outputs from the test will not show up in profile mode. Since @@ -47,8 +49,18 @@ class IntegrationTestRunner with IOMixin { '--target=$testTarget', '-d', headless ? 'web-server' : 'chrome', + // --disable-gpu speeds up tests that use ChromeDriver when run on + // GitHub Actions. See https://github.com/flutter/devtools/issues/8301. + '--web-browser-flag=--disable-gpu', + if (headless) ...[ + // Flags to avoid breakage with chromedriver 128. See + // https://github.com/flutter/devtools/issues/8301. + '--web-browser-flag=--headless=old', + '--web-browser-flag=--disable-search-engine-choice-screen', + ], for (final arg in dartDefineArgs) '--dart-define=$arg', ]; + debugLog('> flutter ${flutterDriveArgs.join(' ')}'); final process = await Process.start('flutter', flutterDriveArgs); @@ -102,7 +114,7 @@ class IntegrationTestRunner with IOMixin { ); bool testTimedOut = false; - final timeout = Future.delayed(const Duration(minutes: 60)).then((_) { + final timeout = Future.delayed(const Duration(minutes: 8)).then((_) { testTimedOut = true; }); @@ -111,10 +123,6 @@ class IntegrationTestRunner with IOMixin { timeout, ]); - debugLog( - 'shutting down processes because ' - '${testTimedOut ? 'test timed out' : 'test finished'}', - ); debugLog('attempting to kill the flutter drive process'); process.kill(); debugLog('flutter drive process has exited'); diff --git a/packages/devtools_shared/lib/src/test/io_utils.dart b/packages/devtools_shared/lib/src/test/io_utils.dart index 83c4117456a..a5ce8d7b020 100644 --- a/packages/devtools_shared/lib/src/test/io_utils.dart +++ b/packages/devtools_shared/lib/src/test/io_utils.dart @@ -82,7 +82,7 @@ mixin IOMixin { }) async { final processId = process.pid; if (debugLogging) { - print('Sending SIGTERM to $processId.'); + print('Sending SIGTERM to $processId..'); } await cancelAllStreamSubscriptions(); Process.killPid(processId);