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

Revamp Jelly completion suggestions #197

Open
wants to merge 32 commits into
base: master
Choose a base branch
from

Conversation

janfaracik
Copy link
Contributor

@janfaracik janfaracik commented Dec 3, 2024

This PR sets out to revamp the completion suggestions for .jelly files. It aims to fix several existing issues, as well as add new features.

Existing issues

  • Don't prepend namespace if it already exists in Jelly (Don't prepend namespace if it already exists in jelly #190)
  • You can get duplicates results (one from IntelliJ, one from this plugin)
  • Inconsistent behaviour between the duplicates (IntelliJs will add required fields by default, our implementation does not)

What's new

  • Offer suggestions for namespaces that aren't imported yet, based on other files in your project.
  • Automatically imports namespaces if they are not already present.
    • I'm often creating new .jelly files from scratch and having to manually add namespaces was a pain, now once you select a completion suggestion the namespace will get automatically imported.
  • Generates templates for components with required attributes, offering them to the user as completion suggestions.
Screen.Recording.2024-12-03.at.08.35.57.mov

Testing done

  • Tested with core and plugins (Design Library plugin)
  • Added some basic tests

Submitter checklist

  • 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

public class NamespaceUtil {

/** Collects all namespaces (and prefixes) from .jelly files in the project */
public static Map<String, String> collectProjectNamespaces(Project project) {
Copy link
Member

Choose a reason for hiding this comment

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

Need to check the performance of this and how often its called and if there's any caching

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aye, need to see if there's any performance regressions, and if so how we can avoid them with cache.

Copy link
Member

Choose a reason for hiding this comment

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

this seems to be called on every key press although I didn't notice any delay... might be worth optimising if it doesn't cause other issues

@timja
Copy link
Member

timja commented Dec 3, 2024

I've tested this out and it worked really nicely

classLoader.getResourceAsStream("org/kohsuke/stapler/idea/resources/schemas/" + file + ".xsd");

if (inputStream == null) {
throw new RuntimeException("Schema not found: " + file + ".xsd");
Copy link
Member

Choose a reason for hiding this comment

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

I'm able to trigger this in Jenkins core with this error message:

Error parsing XSD: Schema not found: hudson.util.jelly.MorphTagLibrary.xsd

Testing with f:textarea

* namespaces, and namespaces declared in the current file, in that order of precedence.
*/
private static Map<String, String> createMergedNamespaceMap(XmlTag tag) {
Map<String, String> projectNamespaces = collectProjectNamespaces(tag.getProject());
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if the value should be a set in some way.

image

In core everything gets repeated because some files for whatever reason use a different prefix for the namespace.

Alternatively could be just fixed by a PR to core to be consistent...

@duemir
Copy link
Member

duemir commented Dec 5, 2024

This is too much, IMO. The built-in XSD-based completion just works. JetBrains maintains it. And as you said, it works better than what we've got. It already knows about required attributes (and more) and adds them automatically. If there duplicates some time, I'd say figure out why and stop showing the duplicates. Don't reinvent the wheel.

@duemir
Copy link
Member

duemir commented Dec 5, 2024

You can get duplicates results (one from IntelliJ, one from this plugin)

They are all from this plugin.
IntelliJ only knows about Jelly tag library namespaces because this plugin registers XSDs. src/main/resources/META-INF/plugin.xml#L76-L115
For custom tag libraries that are just resources in a package marked with taglib file, there is src/main/java/org/kohsuke/stapler/idea/descriptor and this java/org/kohsuke/stapler/idea/JellyCompletionContributor.java.

If descriptors and JellyCompletionContributor clash and cause duplicate suggestions. I think it is better to reduce the amount of code and just remove JellyCompletionContributor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants