Skip to content

Fixing compilation errors after platform localization. Some refactoring - #313

Merged
VISTALL merged 2 commits into
consulo:masterfrom
unv-unv:compilation-fix
Oct 11, 2026
Merged

VISTALL merged 2 commits into
consulo:masterfrom
unv-unv:compilation-fix

Conversation

@unv-unv

@unv-unv unv-unv commented Oct 11, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@unv-unv

unv-unv commented Oct 11, 2026

Copy link
Copy Markdown
Contributor Author

@claude review pr with inline comments

@github-actions

github-actions Bot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @unv-unv's task in 2m 37s —— View job


PR review: 11 inline comments posted

  • Gather context
  • Review the diff (38 files)
  • Post inline comments with suggestion blocks
  • Final summary

Most of the PR is mechanical: String becomes LocalizeValue for getPresentableText(), getElementDescription() and RefactoringUIUtil.getDescription(); AllIcons / JavaIcons become PlatformIconGroup / JavaPsiImplIconGroup; bundles move to *Localize; plus reformatting and pattern-matching instanceof. It looks consistent overall. I couldn't compile it here because the platform artifacts aren't in this runner, so the API points below are based on reading the code.

Likely bugs

  • HighlightOverridingMethodsHandler.java:103 and RefactoringConflictsUtil.java:237-239: a LocalizeValue is passed into MessageFormat or string concatenation without .get(). The text then depends on LocalizeValue.toString(). A suggestion adds .get() in both places.
  • SuperTypeGroup.java:92: LocalizeValue.of(toString()) gets null when PsiClass.getName() is null. Suggested change: ofNullable.
  • JavaNavBarExtension.java:56: this used to return null ("not handled") when there was no description, and now returns "". A suggestion keeps the null fallback.

Behavior change to confirm

  • SuperTypeGroup.java:45: the INHERITS icon changed from General.InheritedMethod to hierarchySubtypes(), which is a different image. Is that intended?

Localization semantics

  • JavaLambdaTreeElement.java:50,56, JavaAnonymousClassTreeElement.java:49,56 and JavaVariableBaseTreeElement.java:30: generated names and code text (lambda$x$0, $1, name: Type) are wrapped in localizeTODO. They should use LocalizeValue.of(...), as PsiMethodTreeElement and PropertyGroup already do. AnonymousClassesSorter also parses the $N value, so it must never be translated.

Minor

  • SuperTypeGroup.java:102: equals() is missing @Override (hashCode() has it).
  • ExtractEnumProcessor.java:25: the import consulo.util.lang.StringUtil; line is now unused. I couldn't post this one inline because the line isn't in the diff.

Points to check (no inline comment)

  • The ElementDescriptionProvider implementations now return LocalizeValue.empty() instead of null. This only works if the platform's ElementDescriptionUtil skips empty values and moves on to the next provider (including the default). Please make sure it does, or the first Java provider's empty result will hide the others.
  • In InlineConstantFieldProcessor and InlineMethodProcessor, stringToSearch.get() can now be "" where it used to be null, for example a member with no qualified container in the NON_JAVA location. Adding a guard such as if (!stringToSearch.isEmpty()) before addUsagesInStringsAndComments / addTextOccurences would avoid searching for an empty string.

name = LocalizeValue.empty();
}
myHintText = LocalizeValue.localizeTODO(
CodeInsightBundle.message("no.methods.overriding.0.are.found", classes.size(), name)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

name is now a LocalizeValue, and it goes into CodeInsightBundle.message(...) (a MessageFormat with an Object... vararg), so the hint text depends on LocalizeValue.toString(). Calling .get() here makes the string explicit:

Suggested change
CodeInsightBundle.message("no.methods.overriding.0.are.found", classes.size(), name)
CodeInsightBundle.message("no.methods.overriding.0.are.found", classes.size(), name.get())

Comment on lines 237 to 239
RefactoringUIUtil.getDescription(fieldInSubClass, true).capitalize().get() +
" would hide " + RefactoringUIUtil.getDescription(refMember, true) +
" which is used by moved " + RefactoringUIUtil.getDescription(member, false)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

getDescription() now returns LocalizeValue, and two of the three calls in this concatenation have no .get(). They rely on LocalizeValue.toString() to produce the text:

Suggested change
RefactoringUIUtil.getDescription(fieldInSubClass, true).capitalize().get() +
" would hide " + RefactoringUIUtil.getDescription(refMember, true) +
" which is used by moved " + RefactoringUIUtil.getDescription(member, false)
RefactoringUIUtil.getDescription(fieldInSubClass, true).capitalize().get() +
" would hide " + RefactoringUIUtil.getDescription(refMember, true).get() +
" which is used by moved " + RefactoringUIUtil.getDescription(member, false).get()

@Override
@RequiredReadAction
public LocalizeValue getPresentableText() {
return LocalizeValue.of(toString());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

toString() returns superClass.getName(), which can be null (for example, an anonymous class). The other providers in this PR use ofNullable for PSI names:

Suggested change
return LocalizeValue.of(toString());
return LocalizeValue.ofNullable(toString());

public static enum OwnershipType {
IMPLEMENTS(PlatformIconGroup.gutterImplementingmethod()),
OVERRIDES(PlatformIconGroup.gutterOverridingmethod()),
INHERITS(PlatformIconGroup.hierarchySubtypes());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This changes the icon. INHERITS used AllIcons.General.InheritedMethod before and now uses hierarchySubtypes(), which is a different image. IMPLEMENTS and OVERRIDES map 1:1 to the gutter icons. Is the change intended, or does PlatformIconGroup have an inherited-method icon that should be used here?

return superClass != null ? superClass.getName() : IdeLocalize.nodeStructureviewInvalid().get();
}

@RequiredReadAction

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hashCode() has @Override but equals() doesn't:

Suggested change
@RequiredReadAction
@Override
@RequiredReadAction

@RequiredReadAction
public LocalizeValue getPresentableText() {
if (myName != null) {
return LocalizeValue.localizeTODO(myName);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The VM name (lambda$foo$0) is generated from code and will never be translated, so localizeTODO marks it wrongly. Only the "Lambda" fallback needs localizeTODO. The same applies at line 56.

Suggested change
return LocalizeValue.localizeTODO(myName);
return LocalizeValue.of(myName);

}
if (element != null) {
myName = PsiLambdaNameHelper.getVMName(element);
return LocalizeValue.localizeTODO(myName);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same as line 50: this is a VM name, not UI text.

Suggested change
return LocalizeValue.localizeTODO(myName);
return LocalizeValue.of(myName);

@RequiredReadAction
public LocalizeValue getPresentableText() {
if (myName != null) {
return LocalizeValue.localizeTODO(myName);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

myName is a generated name like $1, not text to translate. Also, AnonymousClassesSorter parses this value with Integer.parseInt(s.substring(1)), so it must never be localized. Only the "Anonymous" fallback should use localizeTODO. The same applies at line 56.

Suggested change
return LocalizeValue.localizeTODO(myName);
return LocalizeValue.of(myName);

if (element != null) {
myName = JavaAnonymousClassesHelper.getName((PsiAnonymousClass) element);
if (myName != null) {
return LocalizeValue.localizeTODO(myName);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same as line 49.

Suggested change
return LocalizeValue.localizeTODO(myName);
return LocalizeValue.of(myName);

), ":", ": ");
}
boolean dumb = DumbService.isDumb(field.getProject());
return LocalizeValue.localizeTODO(StringUtil.replace(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is formatted Java code (name: Type = init), not UI text. PsiMethodTreeElement and PropertyGroup use LocalizeValue.of(...) for the same kind of value:

Suggested change
return LocalizeValue.localizeTODO(StringUtil.replace(
return LocalizeValue.of(StringUtil.replace(

return name != null ? name : JavaBundle.message("dependencies.tree.node.default.package.abbreviation");
} else if (object instanceof PsiDirectory && JrtFileSystem.isRoot(((PsiDirectory) object).getVirtualFile())) {
return JavaBundle.message("jrt.node.short");
return ElementDescriptionUtil.getElementDescription(member, UsageViewShortNameLocation.INSTANCE).get();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Small behavior change. Before, a member without a description returned null, which means "not handled" to the nav bar model, so it fell back to the default presentation. Now .get() returns "", which can show an empty nav bar item. To keep the old behavior:

Suggested change
return ElementDescriptionUtil.getElementDescription(member, UsageViewShortNameLocation.INSTANCE).get();
LocalizeValue description = ElementDescriptionUtil.getElementDescription(member, UsageViewShortNameLocation.INSTANCE);
return description.isEmpty() ? null : description.get();

@unv-unv

unv-unv commented Oct 11, 2026

Copy link
Copy Markdown
Contributor Author

@claude review pr with inline comments

@github-actions

github-actions Bot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @unv-unv's task in 1m 27s —— View job


PR review (re-run on e903782)

  • Gather context: the branch was amended since the first review, and 8 of the 11 earlier inline comments are now outdated
  • Check which earlier findings were addressed
  • Review the current diff (38 files)
  • Post new inline comments with suggestion blocks
  • Final summary

I couldn't compile the code here because the platform artifacts aren't on this runner. Everything below comes from reading the code.

Fixed since the first review ✅

  • RefactoringConflictsUtil now calls .get() on all three getDescription(...) values.
  • SuperTypeGroup:
    • getPresentableText() uses ofNullable and returns IdeLocalize.nodeStructureviewInvalid() directly.
    • toString() now delegates to it.
    • equals() has @Override and uses Objects.equals.
  • JavaLambdaTreeElement and JavaAnonymousClassTreeElement now use LocalizeValue.of(myName). Only the "Lambda" and "Anonymous" fallbacks use localizeTODO, so AnonymousClassesSorter gets the raw $N it parses.
  • JavaVariableBaseTreeElement uses LocalizeValue.of(...).
  • JavaNavBarExtension uses .getNullIfEmpty(), so it returns null ("not handled") again.

Still open from the first review

  • HighlightOverridingMethodsHandler.java:103: name is still passed to CodeInsightBundle.message(...) as a LocalizeValue without .get(). The existing inline suggestion still applies.
  • SuperTypeGroup.java:45: INHERITS now uses hierarchySubtypes() where it used to use General.InheritedMethod. Please confirm that's intended.
  • ExtractEnumProcessor.java:25: import consulo.util.lang.StringUtil; is now unused. This line isn't in the diff, so I can't comment on it inline.

New inline comment (1)

  • JavaVariableBaseTreeElement.java:11: org.jspecify.annotations.Nullable is unused now that the @Nullable String return type is gone. The suggestion removes the import.

Rest of the diff

I found nothing else to flag:

  • PropertyGroup: equals and hashCode behave the same after the rewrite.
  • JavaHighlightUsagesDescriptionProvider: the LocalizeValue.join(...) calls build the same text as the old concatenation.
  • The other ElementDescriptionProvider implementations, the refactoring processors and SliceManager look like correct mechanical String → LocalizeValue conversions.

Two points from the first review still depend on platform behavior:

  • Empty provider results: ElementDescriptionUtil has to treat LocalizeValue.empty() as "try the next provider", the way it used to treat null.
  • Empty search strings: InlineConstantFieldProcessor and InlineMethodProcessor now pass stringToSearch.get(), which can be "" where it used to be null.

import consulo.localize.LocalizeValue;
import consulo.project.DumbService;
import consulo.util.lang.StringUtil;
import org.jspecify.annotations.Nullable;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Now that getPresentableText() returns LocalizeValue instead of @Nullable String, nothing in this file uses @Nullable, so this import is unused:

Suggested change
import org.jspecify.annotations.Nullable;

@VISTALL
VISTALL merged commit 8385e6e into consulo:master Oct 11, 2026
1 check passed
@unv-unv
unv-unv deleted the compilation-fix branch October 14, 2026 20:59
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