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

ICompletionParticipant#onXMLContent is not called for CDATA #1694

Closed
laeubi opened this issue Oct 27, 2024 · 12 comments
Closed

ICompletionParticipant#onXMLContent is not called for CDATA #1694

laeubi opened this issue Oct 27, 2024 · 12 comments
Assignees
Labels
completion This issue or enhancement is related to completion support enhancement New feature or request
Milestone

Comments

@laeubi
Copy link
Contributor

laeubi commented Oct 27, 2024

Assume the following XML fragment:

<project xmlns="http://maven.apache.org/POM/4.0.0"
	xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
	xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd">
	<modelVersion>4.0.0</modelVersion>
	<groupId>test</groupId>
	<artifactId>bnd-integration-test</artifactId>
	<version>0.0.1-SNAPSHOT</version>
	<name>BND-M2E Integration Test Project</name>
	<description>This project test the BND-m2e integration</description>
	<build>
		<plugins>
			<plugin>
				<groupId>biz.aQute.bnd</groupId>
				<artifactId>bnd-maven-plugin</artifactId>
				<version>7.0.0</version>
				<executions>
					<execution>
						<id>bnd-process</id>
						<goals>
							<goal>bnd-process</goal>
						</goals>
						<configuration>
						<bnd>
						
						<![CDATA[ 
						  
						]]>
						</bnd>
						</configuration>
					</execution>
				</executions>
			</plugin>
		</plugins>
	</build>
	<dependencies>
		<dependency>
			<groupId>org.osgi</groupId>
			<artifactId>osgi.core</artifactId>
			<version>8.0.0</version>
		</dependency>
	</dependencies>
</project>

if I place the cursor right after <bnd> (before the CDATA section) then ICompletionParticipant#onXMLContent is called with currentTag=bnd

Now place the cursor inside the

<![CDATA[ 					  

]]>

now onXMLContent is not called. As CDATA only marks some parts of the document to contain any chardata it is still XML Content of the element, so I would have expected that the method is still called (maybe with currentTag=CDATA and parentTag=bnd).

@angelozerr
Copy link
Contributor

As we have no usecases with cdata completion , lemminx doent support that.

If yiu need to support cdata completion, any contribution are welcome!

@laeubi
Copy link
Contributor Author

laeubi commented Oct 27, 2024

As we have no usecases with cdata completion , lemminx doent support that.

As CDATA can appear anywhere in a document / element content I'm not sure I understand why "non cdata" sections should be any different usecases?

e.g.

<dependencies>
	<dependency>
		<groupId>org.osgi</groupId>
		<artifactId>osgi.core</artifactId>
		<version><![CDATA[8.0.0]]></version>
	</dependency>
</dependencies

is valid XML (even though unusual) where lemminx-maven would usually provide a completion for the version number (wich won't work also).

I also added some debug information and the XML document gives me this:

INFORMATION: node={start: 763, end: 824, name: bnd, closed: true, 
	children:[
		{start: 782, end: 811, name: #text, closed: true}
	]

so the CDATA section seem to be recognized correctly as a text node child, so maybe the autocompletion should simply accept text nodes as content?

@angelozerr
Copy link
Contributor

You are the first person who requires this cdata support.

It would be nice to have that, but me I have no time to implement it but I will be happy to review your contribution.

@angelozerr
Copy link
Contributor

the CDATA section seem to be recognized correctly as a text node child.

Indeed the xml parser convers that. You just need to use those information to support onCDATA with participant

@laeubi
Copy link
Contributor Author

laeubi commented Oct 27, 2024

the CDATA section seem to be recognized correctly as a text node child.

Indeed the xml parser convers that. You just need to use those information to support onCDATA with participant

I'll try to take a look but as I'm not familiar with lemminx internals the "just" might become a more complicated (for me) ;-)

@angelozerr
Copy link
Contributor

I.will do my best to help you.

@laeubi
Copy link
Contributor Author

laeubi commented Oct 27, 2024

I debugged a little bit and found that the tokentype is CDATATagOpen at this point and org.eclipse.lemminx.services.XMLCompletions.doComplete(DOMDocument, Position, SharedSettings, CancelChecker) just fall into the default case then, then next token is of type CDATAContent ... so from code point of view (was not able to test yet) the case

case Content:

should actually be

case Content:
case CDATAContent:

could probably already fix the issue...

@angelozerr
Copy link
Contributor

angelozerr commented Oct 27, 2024

We have a lot of tests, so dont be shy to do a PR and breaks anything. We will see if it breaks existing tests.

laeubi added a commit to laeubi/lemminx that referenced this issue Nov 7, 2024
See eclipse-lemminx#1694

Signed-off-by: Christoph Läubrich <laeubi@laeubi-soft.de>
@laeubi
Copy link
Contributor Author

laeubi commented Nov 7, 2024

I now started with a first step by creating a testcase, it seems there is no "basic" test for the XML extension ("only" HTML variant) so I created a dedicated new test:

laeubi added a commit to laeubi/lemminx that referenced this issue Nov 7, 2024
See eclipse-lemminx#1694

Signed-off-by: Christoph Läubrich <laeubi@laeubi-soft.de>
laeubi added a commit to laeubi/lemminx that referenced this issue Nov 7, 2024
Fix eclipse-lemminx#1694

Signed-off-by: Christoph Läubrich <laeubi@laeubi-soft.de>
@angelozerr angelozerr added enhancement New feature or request completion This issue or enhancement is related to completion support labels Nov 7, 2024
@angelozerr angelozerr added this to the 0.29.0 milestone Nov 7, 2024
@angelozerr
Copy link
Contributor

Fixed with #1699

@laeubi
Copy link
Contributor Author

laeubi commented Nov 7, 2024

Thanks! After some struggling around I was able to finally test this:

Peek.2024-11-07.15-58.mp4

so it works like expected!

Sadly it seem to mean that:

  1. We need a new lemminx release
  2. then need to update wildwebdevelopers
  3. then need a new wildwebdevelopers release

so a quite long path from now on until its fully available. May I ask if there is any plans for new lemminx release already?

@angelozerr
Copy link
Contributor

angelozerr commented Nov 8, 2024

I think we can create a release of LemMinx but I don't know when. We need to find time to do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
completion This issue or enhancement is related to completion support enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants