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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
93737c2
Property editor error handling
elliette Jan 28, 2025
f58c474
Add a test case
elliette Jan 29, 2025
c1394bf
Respond to PR comments
elliette Jan 29, 2025
57089dd
Fix error codes and update test
elliette Jan 29, 2025
ff72e62
Increase bundle size in test
elliette Jan 29, 2025
8bb823a
Add TODOs
elliette Jan 29, 2025
bf86128
Add property name to error
elliette Jan 30, 2025
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
50 changes: 50 additions & 0 deletions packages/devtools_app/lib/src/shared/editor/api_classes.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,56 @@ 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 those listed in:
/// pkg/analysis_server/lib/src/lsp/constants.dart
Copy link
Member

Choose a reason for hiding this comment

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

can we have a test that covers this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm I was trying to think how we could test this. We can't pull in analysis_server as a dependency (otherwise I would just use the error definitions there instead) so I'm not sure how we could have a test to check they are in sync. Any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

can we load the raw content of this file from GitHub and parse it? A bit hacky but still could be worth it to have a way to catch changes

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed #8824 and added TODOs

enum EditArgumentError {
/// A request was made that requires use of workspace/applyEdit but the
/// current editor does not support it.
editsUnsupportedByEditor(-32016),

/// An editArgument request tried to modify an invocation at a position where
/// there was no invocation.
editArgumentInvalidPosition(-32017),

/// An editArgument request tried to modify a parameter that does not exist or
/// is not editable.
editArgumentInvalidParameter(-32018),

/// An editArgument request tried to set an argument value that is not valid.
editArgumentInvalidValue(-32019);

const EditArgumentError(this.code);

final int code;

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
View file
Open in desktop
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
9 changes: 6 additions & 3 deletions ...ools_app/lib/src/standalone_ui/ide_shared/property_editor/property_editor_controller.dart
View file
Open in desktop
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
101 changes: 84 additions & 17 deletions ...devtools_app/lib/src/standalone_ui/ide_shared/property_editor/property_editor_inputs.dart
View file
Open in desktop
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, property: property);
}

InputDecoration decoration(
Expand Down Expand Up @@ -231,4 +258,44 @@ 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, {
required EditableProperty property,
}) {
if (errorResponse == null || errorResponse.success) return;
setState(() {
_serverError = _errorMessage(errorResponse.errorType, property: property);
});
}

String _errorMessage(
EditArgumentError? errorType, {
required EditableProperty property,
}) {
final propertyName = property.name;
switch (errorType) {
case EditArgumentError.editArgumentInvalidParameter:
return 'Invalid parameter: $propertyName.';
Copy link
Member

Choose a reason for hiding this comment

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

what do you think about including the description in the EditArgumentError definitions so that the messages are defined where the enum values are? Something like:

enum EditArgumentError {
  editArgumentInvalidParameter(-32017, 'Invalid parameter')
  ...

  final String description;

General question. Seems like we use Argument in some places and Property in others. Should we pick a single term for consistency?

Copy link
Member Author

Choose a reason for hiding this comment

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

what do you think about including the description in the EditArgumentError definitions so that the messages are defined where the enum values are?

Yes, that's better! Made the change.

General question. Seems like we use Argument in some places and Property in others. Should we pick a single term for consistency?

I've been trying to use argument for the layer that calls the analysis server APIs (since the API methods are called editArgument and getEditableArguments) and property for the UI layer. The "conversion" from argument to property happens here:

devtools/packages/devtools_app/lib/src/standalone_ui/ide_shared/property_editor/property_editor_view.dart

Lines 70 to 78 in 80bbb54

...args
.map((arg) => argToProperty(arg))
.nonNulls
.map(
(property) => _EditablePropertyItem(
property: property,
controller: controller,
),
),

But I don't have a strong preference for renaming everywhere vs keeping as is!

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}.';
}
}
}
1 change: 1 addition & 0 deletions packages/devtools_app/pubspec.yaml
View file
Open in desktop
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.0
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 need to bring this dependency in? Is this for the RpcException type, and is this the type that DTD throws?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is for the RpcException type thrown by DTD. This used to be a dev dependency.

logging: ^1.1.1
meta: ^1.9.1
mime: ^2.0.0
Expand Down
Loading
Loading