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.

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] Check for registered LSP service methods before calling them #8788

Merged
merged 2 commits into from
Jan 17, 2025

Conversation

elliette
Copy link
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.

@@ -59,6 +61,18 @@ class EditorClient extends DisposableController
_supportsOpenDevToolsPage = isRegistered;
_supportsOpenDevToolsForceExternal =
capabilities?[Field.supportsForceExternal] == true;
} else if (method == LspMethod.editArgument.methodName) {
Copy link
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
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
24 of 25 checks passed
@DanTup
Copy link
Contributor

DanTup commented Jan 20, 2025

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
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