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

CodeLens, References, Rename, Diagnostics support for XML references #1435

Merged
merged 1 commit into from
Jan 13, 2023

Conversation

angelozerr
Copy link
Contributor

@angelozerr angelozerr commented Jan 4, 2023

CodeLens, References, Rename, Diagnostics support for XML references

Signed-off-by: azerr azerr@redhat.com

@angelozerr angelozerr force-pushed the reference-codelens branch 22 times, most recently from 4d4a3dd to 0570f26 Compare January 11, 2023 09:08
@angelozerr
Copy link
Contributor Author

angelozerr commented Jan 11, 2023

@datho7561 the code should be good now (I need just to comments some piece of code) and tests are written.

I support too multiple attribites (when referenced is declared in an attribute with spaces like:

<link ana="jos-syn:Root" target="#ssj1.1.1 #ssj1.1.1.t24"/>

Here some suggestion how to test this PR.

Declare this settings:

"xml.references": [
    {
      "pattern": "**/*.xml",
      "expressions": [

        {
          "prefix": "#",
          "from": "@corresp",
          "to": "@xml:id"
        },
        {
          "prefix": "#",
          "from": "@resp",
          "to": "persName/@xml:id"
        },
        {
          "prefix": "#",
          "from": "link/@target",
          "to": "@xml:id",
          "multiple":true
        }
      ]
    }
]

Open this TEI file https://github.com/clarinsi/TEI-schema/blob/master/tei_clarin_example.xml

You will see that there are a lof references like

https://github.com/clarinsi/TEI-schema/blob/2eb380d763f1d026b525edcf633b5c51d0eb56d9/tei_clarin_example.xml#L459

Here a demo

XMLReferencesCodeLensDemo

@angelozerr angelozerr marked this pull request as ready for review January 11, 2023 09:18
@datho7561
Copy link
Contributor

datho7561 commented Jan 11, 2023

If I try to rename comingsoon-asdf to comingsoon-asdfhjkl in the following document (using the rename instead of linked editing):

<web-app xmlns="http://xmlns.jcp.org/xml/ns/javaee"
  xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
  xsi:schemaLocation="http://xmlns.jcp.org/xml/ns/javaee http://xmlns.jcp.org/xml/ns/javaee/web-app_3_1.xsd"
  version="3.1">
  <servlet>
    <servlet-name>comingsoon-asdf</servlet-name>
    <servlet-class>mysite.server.ComingSoonServlet</servlet-class>
  </servlet>
  <servlet-mapping>
    <servlet-name>comingsoon-asdf</servlet-name>
    <url-pattern>/*</url-pattern>
  </servlet-mapping>
</web-app>

Then it instead gets renamed to ccomingsoon-asdfhjklf.

I can also confirm that the hyphen isn't what's causing this issue.

@datho7561
Copy link
Contributor

I tried the following for multiple attributes:

{
  "from": "@ref",
  "to": "bbb/text()",
  "multiple": true
},

with the following XML:

<aaa ref="child1 child2 child3">
  <bbb>child1</bbb>
  <bbb>child2</bbb>
  <bbb>child3</bbb>
</aaa>

The CodeLens appear, but they are not clickable. Rename, go to definition, go to reference, highlights, and linked editing don't work for this case.

@datho7561
Copy link
Contributor

datho7561 commented Jan 11, 2023

If I have:

{
  "from": "bbb/text()",
  "to": "ccc/text()",
  "multiple": true
},

With the XML:

<aaa>
  <bbb>asdf</bbb>
  <bbb>hjkl</bbb>
  <ccc>hjkl</ccc>
  <ccc>asdf</ccc>
</aaa>

Then if I try to rename at the |:

<aaa>
  <bbb>as|df</bbb>
  <bbb>hjkl</bbb>
  <ccc>hjkl</ccc>
  <ccc>asdf</ccc>
</aaa>

It lets me, but nothing happens (neither from nor to are changed). Also, linked editing doesn't appear to work in this case.

@datho7561
Copy link
Contributor

datho7561 commented Jan 11, 2023

If I have the following document, with the same reference configuration as with the previous comment, and I try to rename between the two spaces at the |:

<aaa>
  <bbb>asdf | hjkl</bbb>
  <bbb>hjkl</bbb>
  <ccc>hjkl</ccc>
  <ccc>asdf</ccc>
</aaa>

It allows me to attempt to rename it.

Is this related to prepareRename?

@angelozerr angelozerr force-pushed the reference-codelens branch 2 times, most recently from fa8f51b to ad009c0 Compare January 11, 2023 18:16
@angelozerr angelozerr force-pushed the reference-codelens branch 2 times, most recently from 32ea1aa to af06503 Compare January 12, 2023 17:23
@datho7561
Copy link
Contributor

If I attempt to rename after the root element:

I cannot reproduce it, have you some references settings for this sample?

I think that another bug that you fixed also fixed this bug

Copy link
Contributor

@datho7561 datho7561 left a comment

Choose a reason for hiding this comment

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

Alright, so I made it through the code. The only things left to be fixed are:

  • the case where if you have just the prefix for a reference, you can open the rename popup
  • this license header is slightly different from the one we normally use

After that, I think the PR is ready to be merged.

@datho7561
Copy link
Contributor

If you have the following XML:

<aaa>
  <bbb>| </bbb>
  <ccc>asdf</ccc>
</aaa>

with the following references setting:

{
  "from": "bbb/text()",
  "to": "ccc/text()",
  "multiple": true,
  "prefix": "" // also happens with no "prefix" defined
},

Then, if you try to rename at the | it allows you to open the pop up.

@angelozerr
Copy link
Contributor Author

If I have just the prefix, it allows me to attempt to rename (applying rename does nothing):

It should be fixed (with test too)

@datho7561
Copy link
Contributor

There's a test failure, but it looks like you just typed in the expected output incorrectly.

@angelozerr
Copy link
Contributor Author

Then, if you try to rename at the | it allows you to open the pop up.
Could you retry it please, I cannot reproduce it.

@datho7561
Copy link
Contributor

Could you retry it please, I cannot reproduce it.

You fixed that issue in one of the changes you made. It should work now.

@datho7561 datho7561 self-requested a review January 12, 2023 21:08
@angelozerr angelozerr force-pushed the reference-codelens branch 4 times, most recently from fcb9eb3 to be9fef0 Compare January 13, 2023 13:13
Copy link
Contributor

@datho7561 datho7561 left a comment

Choose a reason for hiding this comment

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

Sorry, I missed something in my previous review: I think the prefix diagnostic wording could be improved

@angelozerr angelozerr force-pushed the reference-codelens branch 2 times, most recently from 544cee8 to fcbc782 Compare January 13, 2023 14:25
Copy link
Contributor

@datho7561 datho7561 left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for being patient with my review, Angelo.

@datho7561 datho7561 added the enhancement New feature or request label Jan 13, 2023
@datho7561 datho7561 added this to the 0.24.0 milestone Jan 13, 2023
@datho7561 datho7561 merged commit 8948c15 into eclipse-lemminx:main Jan 13, 2023
@angelozerr
Copy link
Contributor Author

Looks good! Thanks for being patient with my review, Angelo.

Thanks so much for having taken so many time to test corner usescases and report so many bugs :)

@angelozerr
Copy link
Contributor Author

@oscarlevin I think this issue could interest you.

If you write this settings

"xml.references": [
	{
		"pattern": "**/*.ptx",
		"expressions": [
			{
				"from": "xref/@ref",
				"to": "@xml:id"
			}
		]
	}
]

You should benefit with all XML references support described in https://github.com/redhat-developer/vscode-xml/blob/main/docs/Features/XMLReferencesFeatures.md

Here a demo with your ptx files:

ReferencesWithPretext

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

Successfully merging this pull request may close these issues.

2 participants