-
Notifications
You must be signed in to change notification settings - Fork 331
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
Connect the Property Editor to DTD #8598
Conversation
@@ -256,6 +289,196 @@ class ThemeChangedEvent extends EditorEvent { | |||
Map<String, Object?> toJson() => {Field.theme: theme}; | |||
} | |||
|
|||
/// An event sent by an editor when the current cursor position/s change. |
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.
nit: "cursor position changes" ?
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.
I was trying to specify that this could be either a single or multiple cursor positions.
packages/devtools_app/lib/src/service/editor/editor_client.dart
Outdated
Show resolved
Hide resolved
...ages/devtools_app/lib/src/standalone_ui/ide_shared/property_editor/property_editor_view.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/test/standalone_ui/ide_shared/property_editor_test.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/test/standalone_ui/ide_shared/property_editor_test.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/test/standalone_ui/ide_shared/property_editor_test.dart
Outdated
Show resolved
Hide resolved
await tester.pumpWidget(wrap(propertyEditor)); | ||
eventController.add(activeLocationChangedEvent1); |
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.
why send this at the end of the test?
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.
It's not *really at the end of the test, this is what triggers the request for editable arguments, the existence of which we wait for in the waitForEditableArgs
invocation above.
packages/devtools_app/lib/src/service/editor/editor_client.dart
Outdated
Show resolved
Hide resolved
children: <Widget>[ | ||
...args.map((arg) => _EditablePropertyItem(argument: arg)), | ||
].joinWith(const PaddedDivider.noPadding()), |
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.
can we get rid of the outer list and just write:
children: args.map((arg) => _EditablePropertyItem(argument: arg)).toList().joinWith(const PaddedDivider.noPadding()),
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.
I think the outer list (or a cast) is necessary so that the list is of type Widget
instead of type _EditablePropertyItem
. Otherwise calling .joinWith
results in the error that PaddedDivider
is not type _EditablePropertyItem
.
packages/devtools_app/test/standalone_ui/ide_shared/property_editor_test.dart
Outdated
Show resolved
Hide resolved
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.
a few nits but overall lgtm
_currentDocument = textDocument; | ||
_currentCursorPosition = cursorPosition; | ||
// Get the editable arguments for the current position. | ||
final result = await editorClient.getEditableArguments( |
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.
Is there anything to ensure this service was registered before it's called? Currently the method is prefixed with experimental/
but that will change at some point in future. Since DevTools ships in the SDK, probably it'll be hard to get these out of sync, but it could certainly happen if running either the analysis server or DevTools from source.
There is an initialized
event sent over DTD's Lsp
stream to indicate when all LSP services have been registered, so if the registration has not happened before then, it isn't available.
Work towards: #8531, #1948
This PR:
This PR also: