Skip to content

[Property Editor] Check for registered LSP service methods before calling them #8788

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

Merged
elliette merged 2 commits into from
Jan 17, 2025
Merged

[Property Editor] Check for registered LSP service methods before calling them #8788

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
97f3c49
Check for registered LSP service methods before calling them
elliette Jan 17, 2025
a5f5923
Remove accidental change
elliette Jan 17, 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
8 changes: 4 additions & 4 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 @@ -28,14 +28,14 @@ enum EditorMethod {
///
/// [code link]: https://github.com/dart-lang/sdk/blob/ebfcd436da65802a2b20d415afe600b51e432305/pkg/analysis_server/lib/src/lsp/constants.dart#L136
enum LspMethod {
editableArguments(
methodName: 'experimental/dart/textDocument/editableArguments',
),
editArgument(methodName: 'experimental/dart/textDocument/editArgument');
editableArguments(methodName: 'dart/textDocument/editableArguments'),
editArgument(methodName: 'dart/textDocument/editArgument');

const LspMethod({required this.methodName});

final String methodName;

String get experimentalMethodName => 'experimental/$methodName';
}

/// Known kinds of events that may come from the editor.
Expand Down
36 changes: 31 additions & 5 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 @@ -6,6 +6,7 @@ import 'dart:async';

import 'package:devtools_app_shared/utils.dart';
import 'package:dtd/dtd.dart';
import 'package:flutter/foundation.dart';
import 'package:logging/logging.dart';

import '../analytics/constants.dart';
Expand Down Expand Up @@ -37,7 +38,8 @@ class EditorClient extends DisposableController
}

final service = data.data['service'] as String?;
if (service == null || service != editorServiceName) {
if (service == null ||
(service != editorServiceName && service != lspServiceName)) {
return;
}

Expand All @@ -59,6 +61,18 @@ class EditorClient extends DisposableController
_supportsOpenDevToolsPage = isRegistered;
_supportsOpenDevToolsForceExternal =
capabilities?[Field.supportsForceExternal] == true;
} else if (method == LspMethod.editArgument.methodName) {

@kenzieschmoll kenzieschmoll Jan 17, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: a switch statement may be easier to read than this long chain of if / elses

@elliette elliette Jan 17, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It would be but unfortunately these are all getters, triggering the error: "Extension methods can't be used in constant expressions." (I'm guessing that is why this was written as a bunch of if statements in the first place)

_editArgumentMethodName.value = LspMethod.editArgument.methodName;
} else if (method == LspMethod.editArgument.experimentalMethodName) {
_editArgumentMethodName.value =
LspMethod.editArgument.experimentalMethodName;
} else if (method == LspMethod.editableArguments.methodName) {
_editableArgumentsMethodName.value =
LspMethod.editableArguments.methodName;
} else if (method ==
LspMethod.editableArguments.experimentalMethodName) {
_editableArgumentsMethodName.value =
LspMethod.editableArguments.experimentalMethodName;
} else {
return;
}
Expand Down Expand Up @@ -151,6 +165,14 @@ class EditorClient extends DisposableController
_supportsOpenDevToolsForceExternal;
var _supportsOpenDevToolsForceExternal = false;

ValueListenable<String?> get editArgumentMethodName =>
_editArgumentMethodName;
final _editArgumentMethodName = ValueNotifier<String?>(null);

ValueListenable<String?> get editableArgumentsMethodName =>
_editableArgumentsMethodName;
final _editableArgumentsMethodName = ValueNotifier<String?>(null);

/// A stream of [ActiveLocationChangedEvent]s from the edtior.
Stream<ActiveLocationChangedEvent> get activeLocationChangedStream =>
_activeLocationChangedController.stream;
Expand Down Expand Up @@ -235,8 +257,10 @@ class EditorClient extends DisposableController
required TextDocument textDocument,
required CursorPosition position,
}) async {
final method = editableArgumentsMethodName.value;
if (method == null) return null;
final response = await _callLspApi(
LspMethod.editableArguments,
method,
params: {
'type': 'Object', // This is required by DTD.
'textDocument': textDocument.toJson(),
Expand All @@ -256,8 +280,10 @@ class EditorClient extends DisposableController
required String name,
required T value,
}) async {
final method = editArgumentMethodName.value;
if (method == null) return;
final response = await _callLspApi(
LspMethod.editArgument,
method,
params: {
'type': 'Object', // This is required by DTD.
'textDocument': textDocument.toJson(),
Expand All @@ -278,10 +304,10 @@ class EditorClient extends DisposableController
}

Future<DTDResponse> _callLspApi(
LspMethod method, {
String methodName, {
Map<String, Object?>? params,
}) {
return _dtd.call(lspServiceName, method.methodName, params: params);
return _dtd.call(lspServiceName, methodName, params: params);
}
}

Expand Down
27 changes: 22 additions & 5 deletions ...s/devtools_app/lib/src/standalone_ui/ide_shared/property_editor/property_editor_view.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import 'package:flutter/services.dart';

import '../../../shared/editor/api_classes.dart';
import '../../../shared/primitives/utils.dart';
import '../../../shared/ui/common_widgets.dart';
import 'property_editor_controller.dart';

class PropertyEditorView extends StatelessWidget {
Expand All @@ -18,12 +19,28 @@ class PropertyEditorView extends StatelessWidget {

@override
Widget build(BuildContext context) {
return Column(
crossAxisAlignment: CrossAxisAlignment.start,
children: [
// TODO(elliette): Include widget name and documentation.
_PropertiesList(controller: controller),
return MultiValueListenableBuilder(
listenables: [
controller.editorClient.editArgumentMethodName,
controller.editorClient.editableArgumentsMethodName,
],
builder: (_, values, _) {
final editArgumentMethodName = values.first as String?;
final editableArgumentsMethodName = values.second as String?;

if (editArgumentMethodName == null ||
editableArgumentsMethodName == null) {
return const CenteredCircularProgressIndicator();
}

return Column(
crossAxisAlignment: CrossAxisAlignment.start,
children: [
// TODO(elliette): Include widget name and documentation.
_PropertiesList(controller: controller),
],
);
},
);
}
}
Expand Down
6 changes: 6 additions & 0 deletions packages/devtools_app/test/standalone_ui/ide_shared/property_editor_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,12 @@ void main() {
setGlobal(IdeTheme, IdeTheme());

mockEditorClient = MockEditorClient();
when(
mockEditorClient.editArgumentMethodName,
).thenReturn(ValueNotifier(LspMethod.editArgument.methodName));
when(
mockEditorClient.editableArgumentsMethodName,
).thenReturn(ValueNotifier(LspMethod.editableArguments.methodName));
when(
mockEditorClient.activeLocationChangedStream,
).thenAnswer((_) => eventStream);
Expand Down