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.

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

Jump to bottom

[XML+HTML] Support for autoCloseTags #865

Merged
merged 3 commits into from
Oct 19, 2022
Merged

[XML+HTML] Support for autoCloseTags #865

merged 3 commits into from
Oct 19, 2022

Conversation

angelozerr
Copy link
Contributor

@angelozerr angelozerr commented Sep 8, 2022 •
edited
Loading

Fixes #143

This PR provides auto close tags for XML and HTML both with UI preferences to enable/disable it.

  • XML UI preference page

image

here a demo with XML:

AutoCloseTagDemo

  • HTML UI preference page

image

here a demo with HTML which supports auto close tags and auto insert quote (see in the demo with div/@id attribute, when = is typped, the quote are inserted):

HTMLAutoCloseTags

@angelozerr
Copy link
Contributor Author

I created quickly this PR but my code is very uggly but I would like to know some feedback if it is in the right direction.

Here a demo with this PR:

AutoCloseTagDemo

I need to sleep the Thread which consumes the custom LSP request closeTag to be sure that didOpen does the update of the document in language server side with the '>'.

Please note that HTML have too a custom LSP request which closes tag. Once this PR will be merged I could manage it in the same way.

I created NoThreadReconciler which avoid creating a new Thread.

mickaelistria
mickaelistria requested changes Sep 13, 2022
View reviewed changes
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.

Please add copyright headers.
If possible, please add a test.
Maybe consider dropping the NoThreadReciler and stick to IReconciler+IDocumentListener?

...wildwebdeveloper.xml/src/org/eclipse/wildwebdeveloper/xml/internal/XMLLanguageServerAPI.java
import org.eclipse.lsp4j.services.LanguageServer;
import org.eclipse.wildwebdeveloper.xml.internal.lsp.AutoCloseTagResponse;

public interface XMLLanguageServerAPI extends LanguageServer {
Copy link
Contributor

Choose a reason for hiding this comment

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

You may call it just LemminxServer

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 keep this name because I use the similar name for HTML. Is it OK for you?

...wildwebdeveloper.xml/src/org/eclipse/wildwebdeveloper/xml/internal/XMLLanguageServerAPI.java Show resolved Hide resolved
...webdeveloper.xml/src/org/eclipse/wildwebdeveloper/xml/internal/lsp/ClosingTagReconciler.java Outdated
import org.eclipse.jface.text.DocumentEvent;
import org.eclipse.jface.text.ITextViewer;

public class ClosingTagReconciler extends NoThreadReconciler {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not a IReconciler?

...per.xml/src/org/eclipse/wildwebdeveloper/xml/internal/lsp/ClosingTagReconcilingStrategy.java Outdated Show resolved Hide resolved
...per.xml/src/org/eclipse/wildwebdeveloper/xml/internal/lsp/ClosingTagReconcilingStrategy.java Outdated Show resolved Hide resolved
...per.xml/src/org/eclipse/wildwebdeveloper/xml/internal/lsp/ClosingTagReconcilingStrategy.java Outdated
// we receive a text like
// $0</foo>
// $0 should be used for set the cursor.
String text = r.snippet.replace("$0", "");
Copy link
Contributor

Choose a reason for hiding this comment

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

One days, when snippets are usable more widely in LSP, we'll extract the LSCompletionProposal stuff that supports snippets as an API so such extension will be able to support it.
Or maybe the snippets will be generalized in LSP first and we'll be able to get rid of this custom operation before we have need for an API in LSP4E!

angelozerr
angelozerr commented Sep 13, 2022
View reviewed changes
...per.xml/src/org/eclipse/wildwebdeveloper/xml/internal/lsp/ClosingTagReconcilingStrategy.java Outdated
CompletableFuture.supplyAsync(() -> {
try {
// Wait for textDocument/didChange
Thread.sleep(100);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mickaelistria do you know how I could fix this problem of wait for textDocument/didChange?

@angelozerr angelozerr force-pushed the auto-close-tag branch 7 times, most recently from 9ef3d59 to 0fbbd1d Compare October 18, 2022 15:46
@angelozerr angelozerr changed the title [XML] Support for autoCloseTags [XML+HTML] Support for autoCloseTags Oct 18, 2022
@angelozerr angelozerr marked this pull request as ready for review October 19, 2022 06:16
@mickaelistria
Copy link
Contributor

A lot of changes are caused by the renaming of the Messages class and hide that actual changes. Would you mind creating separate commits to isolate the refactoring so we can more easily focus review on what's changed?

@angelozerr
Copy link
Contributor Author

A lot of changes are caused by the renaming of the Messages class

I have not updated this class, perhaps you mean XMLPreferenceConstants -> XMLPreferenceServerConstants ? If it that I could rename to XMLPreferenceConstants again?

@mickaelistria
Copy link
Contributor

XMLPreferenceConstants -> XMLPreferenceServerConstants

Yes sorry.
I don't mind the renaming, it's probably a good thing; it's more that I'd rather see it in a separate commit to be able to review the actual change here; while most of what I see in the current diff is caused by this rename.

@angelozerr
[XML+HTML] Support for autoCloseTags
7e24d6b 
Fixes eclipse-wildwebdeveloper#143

Signed-off-by: azerr <azerr@redhat.com>
@angelozerr
Rename XMLPreferenceConstants -> XMLPreferenceServerConstants
f30f10f 
Signed-off-by: azerr <azerr@redhat.com>
@angelozerr
Copy link
Contributor Author

I don't mind the renaming, it's probably a good thing; it's more that I'd rather see it in a separate commit to be able to review the actual change here; while most of what I see in the current diff is caused by this rename.

done with a new commit.

@mickaelistria
Copy link
Contributor

Please bump versions and add note to RELEASE_NOTES about this addition.

@angelozerr
Copy link
Contributor Author

Please bump versions and add note to RELEASE_NOTES about this addition.

done

@mickaelistria
Copy link
Contributor

Including features that were not touched since release need to be bumped as well, and I suspect the version in .product as well.

@angelozerr
Bump versions + RELEASE NOTE
d073edf 
Signed-off-by: azerr <azerr@redhat.com>
@angelozerr
Copy link
Contributor Author

Including features that were not touched since release need to be bumped as well, and I suspect the version in .product as well.

It should be fixed.

@angelozerr angelozerr mentioned this pull request Oct 19, 2022
Merged
@mickaelistria mickaelistria merged commit ad41cbc into eclipse-wildwebdeveloper:master Oct 19, 2022
@mickaelistria
Copy link
Contributor

Thanks!

@angelozerr angelozerr mentioned this pull request Oct 24, 2022
Closed
@mickaelistria mickaelistria linked an issue Oct 24, 2022 that may be closed by this pull request
Auto-closing tags in HTML #208
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mickaelistria mickaelistria Awaiting requested review from mickaelistria

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Auto-closing tags in HTML [XML] Support for autoCloseTags
2 participants
@angelozerr @mickaelistria