-
Notifications
You must be signed in to change notification settings - Fork 338
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
Changes from 5 commits
93737c2
f58c474
c1394bf
57089dd
ff72e62
8bb823a
bf86128
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -442,6 +442,66 @@ 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we have a test that covers this? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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( | ||
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({ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
|
||
|
@@ -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; | ||
|
@@ -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 | ||
|
@@ -154,6 +176,7 @@ class _TextInputState extends State<_TextInput> { | |
), | ||
style: theme.fixedFontStyle, | ||
onChanged: (newValue) { | ||
clearServerError(); | ||
setState(() { | ||
currentValue = newValue; | ||
}); | ||
|
@@ -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. | ||
|
@@ -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( | ||
|
@@ -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.'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! |
||
}); | ||
} | ||
} |
There was a problem hiding this comment.
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