Skip to content

Adding missing @Override annotations. Some refactoring#56

Merged
VISTALL merged 2 commits into
consulo:masterfrom
unv-unv:overrides
Jul 6, 2026
Merged

Adding missing @Override annotations. Some refactoring#56
VISTALL merged 2 commits into
consulo:masterfrom
unv-unv:overrides

Conversation

@unv-unv

@unv-unv unv-unv commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@unv-unv

unv-unv commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

@claude review pr with inline comments

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown

Claude finished @unv-unv's task in 7m 52s —— View job


PR Review — @OverRide annotations & refactoring

  • Read PR diff & changed files (19 files, +244/-212)
  • Analyze for correctness / regressions
  • Post inline review comments

Overall: LGTM 👍 This is a clean, well-scoped mechanical cleanup and I found no correctness issues. Build CI is green ✅, so nothing here breaks compilation. The changes fall into consistent, low-risk buckets:

  • Adding missing @Override annotations (the main intent)
  • Adding @RequiredReadAction / @RequiredWriteAction / @RequiredUIAccess contract annotations
  • Diamond operators (new Option<Integer>()new Option<>()), pattern-matching instanceof ((DTDName)pp instanceof DTDName n)
  • Dropping redundant final on locals/params, interface types for locals (ArrayListList)
  • Javadoc modernization (User:/Date:@author/@since), fixing uknownunknown
  • Localization: RefactoringBundle.message(...)RefactoringLocalize.*(...) in BnfInlineViewDescriptor

I posted 3 inline comments on the specific lines. All are minor / optional — none block merge:

  1. LivePreviewStructureViewFactory.java:63 — leftover final on a param, inconsistent with the rest of the PR.
  2. DTDModelLoader.java:340 — raw Map/HashMap could be parameterized.
  3. ChooseModulesDialog.java:66 — heads-up that a couple of runtime API swaps (icons) are bundled in.
Copy-paste suggestions for the two code nits

LivePreviewStructureViewFactory.java:63 — drop the leftover final (other params/locals in this PR had final removed, e.g. BnfStringImpl.updateText):

public StructureViewBuilder getStructureViewBuilder(PsiFile psiFile) {

DTDModelLoader.java:340 — parameterize the raw types:

Map<Object, Object> map = new HashMap<>(dtd.elements);

Notes (non-blocking)

  • Java 21 dependency (confirmed OK): DTDModelLoader / XSDModelLoader change several LinkedList<...> plist locals to List<...> while still calling plist.removeFirst() / eList.addFirst(). Those methods only exist on the List interface since Java 21 (SequencedCollection). Green CI confirms the project targets 21+, so this is fine — just noting the implicit floor.
  • ChooseModulesDialog bundles genuine behavior changes with the annotation cleanup: Messages.getQuestionIcon()UIUtil.getQuestionIcon() and AllIcons.Nodes.ModulePlatformIconGroup.nodesModule(). Reasonable modernizations; flagged only because they're functional, not cosmetic.
  • Optional/out-of-scope: BnfInlineViewDescriptor.getProcessedElementsHeader() still returns a hardcoded "Rule" while the sibling methods were just migrated to RefactoringLocalize — a future candidate for localization.

Nice cleanup overall.
· branch overrides

@Nullable
@Override
@RequiredReadAction
public StructureViewBuilder getStructureViewBuilder(final PsiFile psiFile) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Consistency nit: this PR removes the redundant final modifier from locals/params elsewhere (e.g. BnfStringImpl.updateText, the ChooseModulesDialog constructors), but this parameter keeps it. Consider dropping final here too for consistency: getStructureViewBuilder(PsiFile psiFile).

StringBuffer sb = new StringBuffer("Empty root: possible elements: ");
HashMap map = new HashMap(dtd.elements);
StringBuilder sb = new StringBuilder("Empty root: possible elements: ");
Map map = new HashMap(dtd.elements);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Optional cleanup: now that this line is touched, the raw Map / HashMap could be parameterized to drop the unchecked warning, e.g. Map<Object, Object> map = new HashMap<>(dtd.elements);


myCandidateModules = candidateModules;
myIcon = Messages.getQuestionIcon();
myIcon = UIUtil.getQuestionIcon();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Beyond the @OverRide additions, this line also swaps runtime behavior: Messages.getQuestionIcon() -> UIUtil.getQuestionIcon() (and AllIcons.Nodes.Module -> PlatformIconGroup.nodesModule() below). These are reasonable API modernizations, just flagging that they are functional changes bundled into an "annotations/refactor" PR — worth a quick visual confirm that the dialog icon is unchanged.

@VISTALL
VISTALL merged commit 401f6f5 into consulo:master Jul 6, 2026
1 check passed
@unv-unv
unv-unv deleted the overrides branch July 6, 2026 22:21
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