Skip to content

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

Merged
elliette merged 2 commits into
flutter:masterfrom
elliette:issue-8715
Jan 17, 2025
Merged

[Property Editor] Check for registered LSP service methods before calling them#8788
elliette merged 2 commits into
flutter:masterfrom
elliette:issue-8715

Conversation

@elliette

Copy link
Copy Markdown
Member

Fixes #8715
Work towards #1948

  • Waits for the editArgument and editableArguments service methods to be registered on the LSP service before loading the property editor.
  • Uses either the experimental or non-experimental method depending on which one is registered.

_supportsOpenDevToolsPage = isRegistered;
_supportsOpenDevToolsForceExternal =
capabilities?[Field.supportsForceExternal] == true;
} else if (method == LspMethod.editArgument.methodName) {

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

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)

@elliette
elliette merged commit 4f35735 into flutter:master Jan 17, 2025
@DanTup

DanTup commented Jan 20, 2025

Copy link
Copy Markdown
Contributor

Uses either the experimental or non-experimental method depending on which one is registered.

It's not a big deal, but I think it may have been better to hold off on this - the reason for the prefix was in case we needed to make any breaking changes, we could be sure there wasn't a version of PE/Server out there that try to communicate with different versions of the protocol (unless they were experimental versions).

(I think the API is fairly stable and changes will probably be compatible, although I'm not certain how we'll handle removing arguments yet)

@elliette

Copy link
Copy Markdown
Member Author

Uses either the experimental or non-experimental method depending on which one is registered.

It's not a big deal, but I think it may have been better to hold off on this - the reason for the prefix was in case we needed to make any breaking changes, we could be sure there wasn't a version of PE/Server out there that try to communicate with different versions of the protocol (unless they were experimental versions).

Ah got it! I think we can remove the non-experimental endpoints for now, and then add them back in when ready. I'll send out a change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Property Editor] Ensure DTD APIs are available before calling them

3 participants