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 4 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
32 changes: 25 additions & 7 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 @@ -444,27 +447,42 @@ class EditableArgumentsResult with Serializable {

/// Errors that the Analysis Server returns for failed argument edits.
///
/// These should be kept in sync with those listed in:
/// pkg/analysis_server/lib/src/lsp/constants.dart
/// 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(-32016),
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(-32017),
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(-32018),
editArgumentInvalidParameter(code: -32018, message: 'Invalid parameter.'),

/// An editArgument request tried to set an argument value that is not valid.
editArgumentInvalidValue(-32019);
editArgumentInvalidValue(
code: -32019,
message: 'Invalid value for parameter.',
);

const EditArgumentError(this.code);
const EditArgumentError({required this.code, required this.message});

final int code;
final String message;

static final _codeToErrorMap = EditArgumentError.values.fold(
<int, EditArgumentError>{},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ mixin _PropertyInputMixin<T extends StatefulWidget, U> on State<T> {

final value = property.convertFromInputString(valueAsString) as U?;
final response = await controller.editArgument(name: argName, value: value);
_maybeHandleServerError(response, property: property);
_maybeHandleServerError(response);
}

InputDecoration decoration(
Expand Down Expand Up @@ -270,32 +270,11 @@ mixin _PropertyInputMixin<T extends StatefulWidget, U> on State<T> {
});
}

void _maybeHandleServerError(
EditArgumentResponse? errorResponse, {
required EditableProperty property,
}) {
void _maybeHandleServerError(EditArgumentResponse? errorResponse) {
if (errorResponse == null || errorResponse.success) return;
setState(() {
_serverError = _errorMessage(errorResponse.errorType, property: property);
_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!

});
}

String _errorMessage(
EditArgumentError? errorType, {
required EditableProperty property,
}) {
final propertyName = property.name;
switch (errorType) {
case EditArgumentError.editArgumentInvalidParameter:
return 'Invalid parameter: $propertyName.';
case EditArgumentError.editArgumentInvalidPosition:
return 'Invalid position for parameter: $propertyName.';
case EditArgumentError.editArgumentInvalidValue:
return 'Invalid value for parameter: $propertyName.';
case EditArgumentError.editsUnsupportedByEditor:
return 'IDE does not support property edits.';
default:
return 'Encountered unknown error editing ${property.name}.';
}
}
}
3 changes: 1 addition & 2 deletions packages/devtools_app/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ dependencies:
http: ^1.1.0
image: ^4.1.3
intl: ^0.19.0
json_rpc_2: ^3.0.0
json_rpc_2: ^3.0.2
logging: ^1.1.1
meta: ^1.9.1
mime: ^2.0.0
Expand Down Expand Up @@ -70,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
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ void main() {

await tester.pumpAndSettle();
expect(
find.textContaining('Invalid value for parameter: title.'),
find.textContaining('Invalid value for parameter.'),
findsOneWidget,
);
});
Expand Down
Loading