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

Fix #1332 - Add autoinsert support #1333

Merged
merged 1 commit into from Sep 18, 2023

Conversation

zulus
Copy link
Contributor

@zulus zulus commented Sep 17, 2023

I started from AutoInsert reconciler, but as the result , there is a quiet big path:

  1. AutoInsert support
  2. Original VSCode preferences mapper (plus registered preferences in Eclipse) - I'm reading descriptions and types from vscode plugin package.json and map to IEclipsePreferences
  3. Vue server can consume preferences from CSS/HTML/JS/TS and probably others (see fallbackCollect)
  4. Special map for "typescript-server" settings to vscode settings aliases (vue require names as in vscode settings.json)
  5. typescript-server-vue plugin

In general, preference support is nighmare. If server can consume extra preferences, each section server have to be mapped manually. I think this should be somehow pluggable, probably on LSP4E level

CompletableFuture.supplyAsync(() -> {
try {
// Wait for textDocument/didChange
Thread.sleep(100);
Copy link
Contributor

Choose a reason for hiding this comment

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

This code that I de for XML is so bad and can give some blocking thread bugs.

@mickaelistria LSP4e should provide an API to execute somerhing after à didChange

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I off course agree, for now this is copy-paste from HTMLAutoInsertReconciler

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made some tests and prepared pull request for lsp4e: eclipse/lsp4e#812

Copy link
Contributor

@mickaelistria mickaelistria left a comment

Choose a reason for hiding this comment

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

I don't know much about vue; but it looks OK to me (I've just put 1 comment about code but it's not important).
So I think it's OK to merge it if build succeeds.

// we receive a text like
// $0</foo>
// $0 should be used for set the cursor.
String newText = response.map(l -> l, r -> r.getNewText());
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be

Suggested change
String newText = response.map(l -> l, r -> r.getNewText());
String newText = response.map(Function.identity(), r::getNewText);

Copy link
Contributor Author

@zulus zulus Sep 18, 2023

Choose a reason for hiding this comment

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

Done, as I wrote in description this patch is much bigger than AutoInsert. Take a look to VuePreferences, and how it work with vscode-vue package.json. It's completely different to HTMLPreferenceServerConstants and similar

Copy link
Contributor

Choose a reason for hiding this comment

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

Done, as I wrote in description this patch is much bigger than AutoInsert.

Do you think it's worth reworking this patch in a series of easier to understand yet functionally valuable patches to facilitate further maintenance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, one moment

Copy link
Contributor Author

@zulus zulus Sep 18, 2023

Choose a reason for hiding this comment

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

I published autoinsert-only part (works on default settings). For example auto close html tags <tag>|</tag>, and transform {{|}} to {{ | }} in templates. No new rows in package.json

@mickaelistria
Copy link
Contributor

OK, feel free to merge this one as soon as you're ready for it ;)

@zulus
Copy link
Contributor Author

zulus commented Sep 18, 2023

Sure, when I will gain privileges :D

@mickaelistria
Copy link
Contributor

Sure, when I will gain privileges :D

OK, I see the nomination process pretty advanced but not complete yet (requires EMO or PMC to give a final approval I guess). Do you prefer I merge it now, or you merge it later?

@zulus
Copy link
Contributor Author

zulus commented Sep 18, 2023

Please merge as-is. THX

@mickaelistria mickaelistria merged commit bd4d902 into eclipse-wildwebdeveloper:master Sep 18, 2023
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants