Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[Property Editor] Handle errors from the Analysis Server #8818

Merged
merged 7 commits into from
Jan 30, 2025
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import 'package:test/test.dart';

// Benchmark size in kB.
const bundleSizeBenchmark = 5400;
const gzipBundleSizeBenchmark = 1550;
const gzipBundleSizeBenchmark = 1650;
Copy link
Member Author

Choose a reason for hiding this comment

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

Bundle size is now 1552kb, therefore bumping 1550 limit to 1650


void main() {
group('Web Compile', () {
Expand Down
68 changes: 68 additions & 0 deletions packages/devtools_app/lib/src/shared/editor/api_classes.dart
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ enum EditorMethod {
/// [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');
Expand Down Expand Up @@ -442,6 +445,71 @@ class EditableArgumentsResult with Serializable {
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({
Expand Down
49 changes: 35 additions & 14 deletions packages/devtools_app/lib/src/shared/editor/editor_client.dart
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import 'dart:async';
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';
Expand Down Expand Up @@ -274,26 +275,46 @@ class EditorClient extends DisposableController
}

/// 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',
elliette marked this conversation as resolved.
Show resolved Hide resolved
);
}
}

Future<DTDResponse> _call(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,14 @@ class PropertyEditorController extends DisposableController
);
}

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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import 'package:devtools_app_shared/ui.dart';
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';

Expand Down Expand Up @@ -89,48 +90,69 @@ class StringInput extends StatelessWidget {
}
}

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),
);
}).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;
Expand All @@ -142,9 +164,9 @@ class _TextInputState extends State<_TextInput> {
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
Expand All @@ -154,6 +176,7 @@ class _TextInputState extends State<_TextInput> {
),
style: theme.fixedFontStyle,
onChanged: (newValue) {
clearServerError();
setState(() {
currentValue = newValue;
});
Expand All @@ -166,20 +189,23 @@ class _TextInputState extends State<_TextInput> {
}

Future<void> _editProperty() async {
await widget.editProperty(
await editProperty(
widget.property,
valueAsString: currentValue,
controller: widget.controller,
);
}
}

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.
Expand All @@ -188,8 +214,9 @@ mixin _PropertyInputMixin<T> {
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);
}

InputDecoration decoration(
Expand Down Expand Up @@ -231,4 +258,23 @@ mixin _PropertyInputMixin<T> {
),
);
}

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) {
if (errorResponse == null || errorResponse.success) return;
setState(() {
_serverError =
errorResponse.errorType?.message ?? 'Encountered unknown error.';
Copy link
Member

Choose a reason for hiding this comment

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

do we still want to include the property in this error message? I think it would be helpful:
'${errorResponse.errorType?.message ?? 'Encountered unknown error.'} Property: $property.'

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

});
}
}
2 changes: 1 addition & 1 deletion packages/devtools_app/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ dependencies:
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
Expand Down Expand Up @@ -69,7 +70,6 @@ dev_dependencies:
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
Expand Down
Loading
Loading