[Property Editor] Handle errors from the Analysis Server (#8818)
diff --git a/packages/devtools_app/benchmark/web_bundle_size_test.dart b/packages/devtools_app/benchmark/web_bundle_size_test.dart index 9490e53..3318c4c 100644 --- a/packages/devtools_app/benchmark/web_bundle_size_test.dart +++ b/packages/devtools_app/benchmark/web_bundle_size_test.dart
@@ -12,7 +12,7 @@ // Benchmark size in kB. const bundleSizeBenchmark = 5400; -const gzipBundleSizeBenchmark = 1550; +const gzipBundleSizeBenchmark = 1650; void main() { group('Web Compile', () {
diff --git a/packages/devtools_app/lib/src/shared/editor/api_classes.dart b/packages/devtools_app/lib/src/shared/editor/api_classes.dart index 6efde93..3b6c53d 100644 --- a/packages/devtools_app/lib/src/shared/editor/api_classes.dart +++ b/packages/devtools_app/lib/src/shared/editor/api_classes.dart
@@ -27,6 +27,9 @@ /// [pkg/analysis_server/lib/src/lsp/constants.dart][code link] /// /// [code link]: https://github.com/dart-lang/sdk/blob/ebfcd436da65802a2b20d415afe600b51e432305/pkg/analysis_server/lib/src/lsp/constants.dart#L136 +/// +/// TODO(https://github.com/flutter/devtools/issues/8824): Add tests that these +/// are in-sync with analysis_server. enum LspMethod { editableArguments(methodName: 'dart/textDocument/editableArguments'), editArgument(methodName: 'dart/textDocument/editArgument'); @@ -442,6 +445,71 @@ Map<String, Object?> toJson() => {Field.arguments: args}; } +/// Errors that the Analysis Server returns for failed argument edits. +/// +/// These should be kept in sync with the error coes defined at +/// [pkg/analysis_server/lib/src/lsp/constants.dart][code link] +/// +/// [code link]: https://github.com/dart-lang/sdk/blob/35a10987e1652b7d49991ab2dc2ee7f521fe8d8f/pkg/analysis_server/lib/src/lsp/constants.dart#L300 +/// +/// TODO(https://github.com/flutter/devtools/issues/8824): Add tests that these +/// are in-sync with analysis_server. +enum EditArgumentError { + /// A request was made that requires use of workspace/applyEdit but the + /// current editor does not support it. + editsUnsupportedByEditor( + code: -32016, + message: 'IDE does not support property edits.', + ), + + /// An editArgument request tried to modify an invocation at a position where + /// there was no invocation. + editArgumentInvalidPosition( + code: -32017, + message: 'Invalid position for argument.', + ), + + /// An editArgument request tried to modify a parameter that does not exist or + /// is not editable. + editArgumentInvalidParameter(code: -32018, message: 'Invalid parameter.'), + + /// An editArgument request tried to set an argument value that is not valid. + editArgumentInvalidValue( + code: -32019, + message: 'Invalid value for parameter.', + ); + + const EditArgumentError({required this.code, required this.message}); + + final int code; + final String message; + + static final _codeToErrorMap = EditArgumentError.values.fold( + <int, EditArgumentError>{}, + (map, error) { + map[error.code] = error; + return map; + }, + ); + + static EditArgumentError? fromCode(int? code) { + if (code == null) return null; + return _codeToErrorMap[code]; + } +} + +/// Response to an edit argument request. +class EditArgumentResponse { + EditArgumentResponse({required this.success, this.errorMessage, errorCode}) + : _errorCode = errorCode; + + final bool success; + final String? errorMessage; + final int? _errorCode; + + EditArgumentError? get errorType => EditArgumentError.fromCode(_errorCode); +} + /// Information about a single editable argument of a widget. class EditableArgument with Serializable { EditableArgument({
diff --git a/packages/devtools_app/lib/src/shared/editor/editor_client.dart b/packages/devtools_app/lib/src/shared/editor/editor_client.dart index 7214aec..78847d9 100644 --- a/packages/devtools_app/lib/src/shared/editor/editor_client.dart +++ b/packages/devtools_app/lib/src/shared/editor/editor_client.dart
@@ -7,6 +7,7 @@ import 'package:devtools_app_shared/utils.dart'; import 'package:dtd/dtd.dart'; import 'package:flutter/foundation.dart'; +import 'package:json_rpc_2/json_rpc_2.dart'; import 'package:logging/logging.dart'; import '../analytics/constants.dart'; @@ -271,26 +272,46 @@ } /// Requests that the Analysis Server makes a code edit for an argument. - Future<void> editArgument<T>({ + Future<EditArgumentResponse> editArgument<T>({ required TextDocument textDocument, required CursorPosition position, required String name, required T value, }) async { final method = editArgumentMethodName.value; - if (method == null) return; - final response = await _callLspApi( - method, - params: { - 'type': 'Object', // This is required by DTD. - 'textDocument': textDocument.toJson(), - 'position': position.toJson(), - 'edit': {'name': name, 'newValue': value}, - }, - ); - // TODO(elliette): Handle response, currently the response from the Analysis - // Server is null. - _log.info('editArgument response: ${response.result}'); + if (method == null) { + return EditArgumentResponse( + success: false, + errorMessage: 'API is unavailable.', + ); + } + try { + await _callLspApi( + method, + params: { + 'type': 'Object', // This is required by DTD. + 'textDocument': textDocument.toJson(), + 'position': position.toJson(), + 'edit': {'name': name, 'newValue': value}, + }, + ); + return EditArgumentResponse(success: true); + } on RpcException catch (e) { + final errorMessage = e.message; + _log.severe(errorMessage); + return EditArgumentResponse( + success: false, + errorCode: e.code, + errorMessage: errorMessage, + ); + } catch (e) { + final errorMessage = 'Unknown error: $e'; + _log.severe(errorMessage); + return EditArgumentResponse( + success: false, + errorMessage: 'Unknown error: $e', + ); + } } Future<DTDResponse> _call(
diff --git a/packages/devtools_app/lib/src/standalone_ui/ide_shared/property_editor/property_editor_controller.dart b/packages/devtools_app/lib/src/standalone_ui/ide_shared/property_editor/property_editor_controller.dart index 1c991ea..0925099 100644 --- a/packages/devtools_app/lib/src/standalone_ui/ide_shared/property_editor/property_editor_controller.dart +++ b/packages/devtools_app/lib/src/standalone_ui/ide_shared/property_editor/property_editor_controller.dart
@@ -49,11 +49,14 @@ ); } - Future<void> editArgument<T>({required String name, required T value}) async { + Future<EditArgumentResponse?> editArgument<T>({ + required String name, + required T value, + }) async { final document = _currentDocument; final position = _currentCursorPosition; - if (document == null || position == null) return; - await editorClient.editArgument( + if (document == null || position == null) return null; + return editorClient.editArgument( textDocument: document, position: position, name: name,
diff --git a/packages/devtools_app/lib/src/standalone_ui/ide_shared/property_editor/property_editor_inputs.dart b/packages/devtools_app/lib/src/standalone_ui/ide_shared/property_editor/property_editor_inputs.dart index 6a50b83..c04440b 100644 --- a/packages/devtools_app/lib/src/standalone_ui/ide_shared/property_editor/property_editor_inputs.dart +++ b/packages/devtools_app/lib/src/standalone_ui/ide_shared/property_editor/property_editor_inputs.dart
@@ -6,6 +6,7 @@ import 'package:flutter/material.dart'; import 'package:flutter/services.dart'; +import '../../../shared/editor/api_classes.dart'; import 'property_editor_controller.dart'; import 'property_editor_types.dart'; @@ -89,21 +90,37 @@ } } -class _DropdownInput<T> extends StatelessWidget with _PropertyInputMixin<T> { - _DropdownInput({super.key, required this.property, required this.controller}); +class _DropdownInput<T> extends StatefulWidget { + const _DropdownInput({ + super.key, + required this.property, + required this.controller, + }); final FiniteValuesProperty property; final PropertyEditorController controller; @override + State<_DropdownInput<T>> createState() => _DropdownInputState<T>(); +} + +class _DropdownInputState<T> extends State<_DropdownInput<T>> + with _PropertyInputMixin<_DropdownInput<T>, T> { + @override Widget build(BuildContext context) { final theme = Theme.of(context); return DropdownButtonFormField( - value: property.valueDisplay, - decoration: decoration(property, theme: theme, padding: denseSpacing), + value: widget.property.valueDisplay, + autovalidateMode: AutovalidateMode.onUserInteraction, + validator: (text) => inputValidator(text, property: widget.property), + decoration: decoration( + widget.property, + theme: theme, + padding: denseSpacing, + ), isExpanded: true, items: - property.propertyOptions.map((option) { + widget.property.propertyOptions.map((option) { return DropdownMenuItem( value: option, child: Text(option, style: theme.fixedFontStyle), @@ -111,26 +128,31 @@ }).toList(), onChanged: (newValue) async { await editProperty( - property, + widget.property, valueAsString: newValue, - controller: controller, + controller: widget.controller, ); }, ); } } -class _TextInput<T> extends StatefulWidget with _PropertyInputMixin<T> { - _TextInput({super.key, required this.property, required this.controller}); +class _TextInput<T> extends StatefulWidget { + const _TextInput({ + super.key, + required this.property, + required this.controller, + }); final EditableProperty property; final PropertyEditorController controller; @override - State<_TextInput> createState() => _TextInputState(); + State<_TextInput> createState() => _TextInputState<T>(); } -class _TextInputState extends State<_TextInput> { +class _TextInputState<T> extends State<_TextInput<T>> + with _PropertyInputMixin<_TextInput<T>, T> { String currentValue = ''; double paddingDiffComparedToDropdown = 1.0; @@ -142,9 +164,9 @@ initialValue: widget.property.valueDisplay, enabled: widget.property.isEditable, autovalidateMode: AutovalidateMode.onUserInteraction, - validator: widget.property.inputValidator, + validator: (text) => inputValidator(text, property: widget.property), inputFormatters: [FilteringTextInputFormatter.singleLineFormatter], - decoration: widget.decoration( + decoration: decoration( widget.property, theme: theme, // Note: The text input has an extra pixel compared to the dropdown @@ -154,6 +176,7 @@ ), style: theme.fixedFontStyle, onChanged: (newValue) { + clearServerError(); setState(() { currentValue = newValue; }); @@ -166,7 +189,7 @@ } Future<void> _editProperty() async { - await widget.editProperty( + await editProperty( widget.property, valueAsString: currentValue, controller: widget.controller, @@ -174,12 +197,15 @@ } } -mixin _PropertyInputMixin<T> { +mixin _PropertyInputMixin<T extends StatefulWidget, U> on State<T> { + String? _serverError; + Future<void> editProperty( EditableProperty property, { required PropertyEditorController controller, required String? valueAsString, }) async { + clearServerError(); final argName = property.name; // Can edit values to null. @@ -188,8 +214,9 @@ return; } - final value = property.convertFromInputString(valueAsString) as T?; - await controller.editArgument(name: argName, value: value); + final value = property.convertFromInputString(valueAsString) as U?; + final response = await controller.editArgument(name: argName, value: value); + _maybeHandleServerError(response, property: property); } InputDecoration decoration( @@ -232,4 +259,26 @@ ), ); } + + String? inputValidator(String? input, {required EditableProperty property}) { + if (_serverError != null) return _serverError; + return property.inputValidator(input); + } + + void clearServerError() { + setState(() { + _serverError = null; + }); + } + + void _maybeHandleServerError( + EditArgumentResponse? errorResponse, { + required EditableProperty property, + }) { + if (errorResponse == null || errorResponse.success) return; + setState(() { + _serverError = + '${errorResponse.errorType?.message ?? 'Encountered unknown error.'} (Property: ${property.name})'; + }); + } }
diff --git a/packages/devtools_app/pubspec.yaml b/packages/devtools_app/pubspec.yaml index f8d087d..a8e13e6 100644 --- a/packages/devtools_app/pubspec.yaml +++ b/packages/devtools_app/pubspec.yaml
@@ -37,6 +37,7 @@ http: ^1.1.0 image: ^4.1.3 intl: ^0.19.0 + json_rpc_2: ^3.0.2 logging: ^1.1.1 meta: ^1.9.1 mime: ^2.0.0 @@ -69,7 +70,6 @@ sdk: flutter integration_test: sdk: flutter - json_rpc_2: ^3.0.2 mockito: ^5.4.1 stager: ^1.0.1 stream_channel: ^2.1.1
diff --git a/packages/devtools_app/test/standalone_ui/ide_shared/property_editor_test.dart b/packages/devtools_app/test/standalone_ui/ide_shared/property_editor_test.dart index 3cdd703..9bc2941 100644 --- a/packages/devtools_app/test/standalone_ui/ide_shared/property_editor_test.dart +++ b/packages/devtools_app/test/standalone_ui/ide_shared/property_editor_test.dart
@@ -243,6 +243,9 @@ group('editing arguments', () { late Completer<String> nextEditCompleter; + // A fake argument that the server can't support. + const fakeBogusArgument = 'fakeBogusArgument'; + setUp(() { controller.initForTestsOnly( document: textDocument1, @@ -262,10 +265,17 @@ final calledWithArgs = realInvocation.namedArguments; final name = calledWithArgs[const Symbol('name')]; final value = calledWithArgs[const Symbol('value')]; + final success = value != fakeBogusArgument; nextEditCompleter.complete( - '$name: $value (TYPE: ${value?.runtimeType ?? 'null'})', + '$name: $value (TYPE: ${value?.runtimeType ?? 'null'}, SUCCESS: $success)', ); - return Future.value(); + + return Future.value( + EditArgumentResponse( + success: success, + errorCode: success ? null : -32019, + ), + ); }); }); @@ -282,7 +292,10 @@ // Verify the edit is expected. final nextEdit = await nextEditCompleter.future; - expect(nextEdit, equals('title: Brand New Title! (TYPE: String)')); + expect( + nextEdit, + equals('title: Brand New Title! (TYPE: String, SUCCESS: true)'), + ); }); }); @@ -298,7 +311,7 @@ // Verify the edit is expected. final nextEdit = await nextEditCompleter.future; - expect(nextEdit, equals('title: null (TYPE: null)')); + expect(nextEdit, equals('title: null (TYPE: null, SUCCESS: true)')); }); }); @@ -320,12 +333,42 @@ nextEdit, equals( 'title: ' - ' (TYPE: String)', + ' (TYPE: String, SUCCESS: true)', ), ); }); }); + testWidgets('editing a string input to an invalid parameter (title)', ( + tester, + ) async { + return await tester.runAsync(() async { + // Load the property editor. + controller.initForTestsOnly(editableArgs: result1.args); + await tester.pumpWidget(wrap(propertyEditor)); + + // Edit the title. + final titleInput = _findTextFormField('title'); + await _inputText(titleInput, text: fakeBogusArgument, tester: tester); + + // Verify the edit is expected. + final nextEdit = await nextEditCompleter.future; + expect( + nextEdit, + equals( + 'title: fakeBogusArgument' + ' (TYPE: String, SUCCESS: false)', + ), + ); + + await tester.pumpAndSettle(); + expect( + find.textContaining('Invalid value for parameter. (Property: title)'), + findsOneWidget, + ); + }); + }); + testWidgets('editing a numeric input (height)', (tester) async { return await tester.runAsync(() async { // Load the property editor. @@ -338,7 +381,7 @@ // Verify the edit is expected. final nextEdit = await nextEditCompleter.future; - expect(nextEdit, equals('height: 55.81 (TYPE: double)')); + expect(nextEdit, equals('height: 55.81 (TYPE: double, SUCCESS: true)')); }); }); @@ -354,7 +397,7 @@ // Verify the edit is expected. final nextEdit = await nextEditCompleter.future; - expect(nextEdit, equals('height: null (TYPE: null)')); + expect(nextEdit, equals('height: null (TYPE: null, SUCCESS: true)')); }); }); @@ -375,7 +418,10 @@ // Verify the edit is expected. final nextEdit = await nextEditCompleter.future; - expect(nextEdit, equals('align: Alignment.topLeft (TYPE: String)')); + expect( + nextEdit, + equals('align: Alignment.topLeft (TYPE: String, SUCCESS: true)'), + ); }); }); @@ -396,7 +442,7 @@ // Verify the edit is expected. final nextEdit = await nextEditCompleter.future; - expect(nextEdit, equals('align: null (TYPE: null)')); + expect(nextEdit, equals('align: null (TYPE: null, SUCCESS: true)')); }); }); @@ -417,7 +463,7 @@ // Verify the edit is expected. final nextEdit = await nextEditCompleter.future; - expect(nextEdit, equals('softWrap: false (TYPE: bool)')); + expect(nextEdit, equals('softWrap: false (TYPE: bool, SUCCESS: true)')); }); }); });