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

Show Views in the Class' Structure tool window #113

Merged
merged 14 commits into from
Oct 27, 2022

Conversation

jgreffe
Copy link
Contributor

@jgreffe jgreffe commented Oct 7, 2022

Fixes #102

  • jelly/jellytag files now have their own proper type / association / icon
  • when displaying the structure view on a java class, all the related jelly/jellytag files having corresponding tree are displayed
  • when displaying the structure view on a jelly/jellytag file, the related java class having corresponding tree is displayed

from java file src/main/java/this/is/a/test.java, we look for all jelly files contained in src/main/resources/this/is/a/test directory
from jelly file src/main/resources/another/test/*.jelly, we look for java class src/main/java/another/test.java

Manual tests with Intellij 2022.1.

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@duemir duemir self-requested a review October 9, 2022 04:50
@duemir duemir added this to the Hacktoberfest2022 milestone Oct 9, 2022
@duemir
Copy link
Member

duemir commented Oct 9, 2022

Whoa. This is interesting. Not what I imagined this feature to be. It pretty much adds the whole tree of the stapled class/view to the structured view of the other side. 🤔

Screen Shot 2022-10-09 at 16 14 09

The way I understand Stapler is that views are kinda like methods of the Class (e.g., doIndex() vs index.jelly). Hence my vision was to add views to the list of members of the class in a structured view. I didn't think about the other way around at all.

@duemir duemir requested a review from timja October 9, 2022 06:15
Comment on lines +66 to +72
<!-- jelly file definition parser -->
<lang.parserDefinition language="Jelly"
implementationClass="org.kohsuke.stapler.idea.language.JellyParserDefinition"/>
<!-- Additional link to jelly file from a java file -->
<lang.structureViewExtension implementation="org.kohsuke.stapler.idea.extension.JavaStructureViewExtension"/>
<!-- Additional link to java file from a jelly file -->
<lang.structureViewExtension implementation="org.kohsuke.stapler.idea.extension.JellyStructureViewExtension"/>
Copy link
Member

Choose a reason for hiding this comment

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

I've been on the fence about this for a while now. I am just unsure what kind of XML support features are lost versus what is gained by this.

Given structure view extension is "attached" to a PSI class, you kinda needed this. That said, you could have probably just used the XmlDocument.class and did the JellyFileTypeSchema.isJelly(parent.getContainingFile()) in the getChildren method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually the JellyLanguage extends the XMLLanguage, therefore all XML features are included in Jelly language.
With this, checking the file instance against JellyFile is enough.

More, this method JellyFileTypeSchema.isJelly(parent.getContainingFile()) simply checks the extension and we redeclare jelly and jellytag, but we shouldn't anymore as it's already linked/declared here.
i.e : if we add a new extension for jelly, like jellyanothertag, we would need to update plugin.xml, and also JellyFileTypeSchema, which sounds like a duplicate/overhead to me (?)

Copy link
Member

Choose a reason for hiding this comment

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

I see what you mean. All the File Type, Language and PSI classes extend corresponding XML ones, so theoretically all the extensions that look for those types should still work with these derived types. I suspected that this type of change is necessary to get proper JEXL integration, but I am not sure, and JEXL support is just too much work.

I got burned by doing incremental changes to the file types before. You can see it if you read through https://github.com/jenkinsci/idea-stapler-plugin/releases

There is also a question of what happens if the end-user made a manual association between jelly extension and XML file type. From my previous experience, manual assosciation takes precedence, so the introduction of a new file type might break some old setups.

I'll need to ponder and test some more.

which sounds like a duplicate/overhead to me (?)

It was duplication even before your change. It looked unavoidable. I used JavaFX plugin for inspiration when I implemented the file type checking in that way. With the introduction of the Jelly File type, those public static predicates should no longer be needed.

@jgreffe jgreffe requested review from duemir and removed request for timja October 10, 2022 08:20
@jgreffe jgreffe changed the title Feature 102: Show Views in the Class' Structure tool window Show Views in the Class' Structure tool window Oct 10, 2022
@rsandell
Copy link
Member

It would be nice to treat groovy views the same.

- additional LinkJellyFileImpl to avoid infinite loops when displaying structure view
@duemir
Copy link
Member

duemir commented Oct 12, 2022

@jglick, not really. GotoViewAction is analogous but less feature reach. I wrote down Issue #12 to note the improvements that should get it closer to NetBeans analogue.

@duemir
Copy link
Member

duemir commented Oct 13, 2022

jelly
I cobbled this svg together based on built-in JSP and YAML icons. It will look more native in the IDE compared to the current stapler icon. Let us use it?
for reference https://plugins.jetbrains.com/docs/intellij/work-with-icons-and-images.html#platform-vs-custom-icons

@duemir duemir added the bump-minor Tell release drafter to increase Minor version label Oct 13, 2022
@jgreffe
Copy link
Contributor Author

jgreffe commented Oct 13, 2022

It would be nice to treat groovy views the same.

Could be part of another dev?

@jgreffe
Copy link
Contributor Author

jgreffe commented Oct 13, 2022

@duemir: new jelly icon added

@jgreffe
Copy link
Contributor Author

jgreffe commented Oct 13, 2022

Build failed: 14:08:22 Timeout has been exceeded

@jgreffe
Copy link
Contributor Author

jgreffe commented Oct 13, 2022

Ok now

@duemir
Copy link
Member

duemir commented Oct 14, 2022

I'll build a snapshot and run it locally for a few days to see if I notice any regressions or anything.

@jgreffe
Copy link
Contributor Author

jgreffe commented Oct 14, 2022

@duemir : spotted an issue when creating unit tests : feature was not working correctly with nested inner classes.
It's fixed and also added unit tests for tricky behavior.

@duemir
Copy link
Member

duemir commented Oct 17, 2022

@jgreffe, the plugin is lacking in terms of tests, so any new ones are welcome. I'll pull and re-build and try to review.

@jgreffe
Copy link
Contributor Author

jgreffe commented Oct 17, 2022

@duemir : pushed unchecked warning fix

@jgreffe jgreffe requested a review from duemir October 17, 2022 07:02
…java

Co-authored-by: Denys Digtiar <duemir@gmail.com>
@jgreffe jgreffe requested a review from duemir October 19, 2022 02:16
@jgreffe jgreffe requested a review from duemir October 19, 2022 20:48
@duemir
Copy link
Member

duemir commented Oct 24, 2022

That last commit wasn't necessary. It just adds more noise to an already large review.

@jgreffe
Copy link
Contributor Author

jgreffe commented Oct 25, 2022

@duemir: reverted last commit

@duemir duemir merged commit 1a8c234 into jenkinsci:master Oct 27, 2022
@jgreffe
Copy link
Contributor Author

jgreffe commented Oct 28, 2022

🎊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump-minor Tell release drafter to increase Minor version major-enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show Views in the Class' Structure tool window
4 participants