Skip to content

Support mixin targets reference and completion - #2633

Closed
Enaium wants to merge 1 commit into
minecraft-dev:devfrom
Enaium:dev
Closed

Support mixin targets reference and completion#2633
Enaium wants to merge 1 commit into
minecraft-dev:devfrom
Enaium:dev

Conversation

@Enaium

@Enaium Enaium commented Jul 25, 2026

Copy link
Copy Markdown
Contributor
202607206181421

@DenWav DenWav left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So the biggest problem with this PR is it implements a lot of stuff from scratch where it isn't necessary. I threw a few comments in with some of the stuff I noticed, but then I realized that the reference contributor could just extend JavaClassReferenceProvider.

Then I kind of rabbit-holed looking at this and implemented a couple of other things to make it work nicer. One important bit is raising the priority of MixinCompletionContributor, with it on last priority it wasn't actually running.

I didn't want to steal your thunder or anything, so I pushed my stuff up to a separate branch: https://github.com/minecraft-dev/MinecraftDev/tree/mixin-target-ref

Take a look if you like.

// When input is empty, also show all class names (limited)
val cache = PsiShortNamesCache.getInstance(project)
if (packageName.isEmpty() && text.firstOrNull()?.isUpperCase() == true) {
for (className in cache.allClassNames) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure this is a good idea. I'm concerned this could be too slow on slower machines. I think the easiest option would be to use AllClassesGetter.processJavaClasses, which would effectively do the same thing, but with process cancellation checks handled for you.

After testing the PR I do think this is way too slow to be useable. AllClassesGetter.processJavaClasses performs much better.

}

// Check if completing inside @Mixin targets attribute
if (isInsideMixinTargets(position)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Unless I'm missing something this call (and the whole delegating method) can be use the MixinTargetsReference.ELEMENT_PATTERN. You just need to get the PsiLiteral that position points to: val literal = PsiTreeUtil.getParentOfType(position, PsiLiteral::class.java).

Then just MixinTargetsReference.ELEMENT_PATTERN.accepts(literal).

@Enaium Enaium closed this Aug 5, 2026
@Enaium

Enaium commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

No need to argue; if my project's pr needs any changes, I will assist in making those changes.

@Enaium

Enaium commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Could you add me as a co-author of the commit 23c36a5?

@DenWav

DenWav commented Aug 5, 2026

Copy link
Copy Markdown
Member

Yes, I will. Thank you for your contribution. :)

@DenWav

DenWav commented Aug 5, 2026

Copy link
Copy Markdown
Member

Merged in c0908cb, with co-author credit.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants