Improvements for 0.6.0 - #97
Merged
Merged
Conversation
Both checks were negated the wrong way, which disabled the affected code paths exactly while the plugin was enabled: - DocumentMarkupModelScanner.scanForProblemsManuallyInTextEditor passed an empty problem list when the plugin was enabled, so every problem of the scanned file was removed instead of drawn. This broke "Show only highest severity per line" (the MarkupModelListener delegates to this method for that option) and the HighlightProblemListener. - HighlightProblemListener.accept returned early while the plugin was enabled, so the HighlightProblemListener never triggered a scan at all. (cherry picked from commit d2ab348a48ae4d6494b745b64989cb1a6f959998)
scanForProblemsManually collected the problems of all projects into a single list but called updateFromNewActiveProblems inside the project loop. Since that method diffs against all active problems, the first iteration removed every problem belonging to the other projects and the next iteration drew them again - on every scan interval. The update is now applied once, after all open projects have been collected. (cherry picked from commit e0e83b4f7461d199f0a0934cc2395ba42ee93b8e)
The XML unescaping branch was guarded by isEnableHtmlStripping instead of isEnableXmlUnescaping, so the separate checkbox added in 0.5.6 had no effect and unescaping always followed the HTML stripping switch. (cherry picked from commit 3f1ea0c14142aad629eb6363fbfb4f3e604e327b)
- isModified did not compare enableInlineProblemsNotifications, so toggling only that checkbox left the Apply button disabled and the change was discarded when the dialog was closed. - reset did not restore maxFileLines while every other field was restored. - The SettingsComponent constructor passed an Optional to JComboBox.setSelectedItem, which is a no-op for a JComboBox<String>. It now uses the index based setter, which also clamps out of range values. (cherry picked from commit 38d73bfe9197df46b9bc0ed61b95c69f3f65b931)
InlineProblemLabel imported org.jdesktop.swingx.action.ActionManager, so the lookup for ACTION_SHOW_INTENTION_ACTIONS queried the SwingX action registry, which is always empty. The click handler therefore only ever worked through its null fallback, and had the registry ever returned an action the cast to AnAction would have thrown a ClassCastException. The inlay field is also only assigned in paint, so mouseClicked now guards against it being null. (cherry picked from commit bb02730c8cba9d839fd1f62512118e984e050153)
This is hardening, not a fix for an observable bug: the settings use plain
ColorPanel instances and never call setSupportTransparency(true), so the
color picker has no opacity slider and every color that reaches the
converter is opaque. For an opaque color the old implementation was always
correct.
return "#" + Integer.toHexString(value.getRGB()).substring(2);
getRGB() is 0xAARRGGBB, and substring(2) relies on the alpha byte
producing exactly the two leading characters. That only holds for an alpha
of 0x10 or higher; below that toHexString drops the leading zeros and
substring(2) cuts into the red component, or throws
StringIndexOutOfBoundsException for a near black color with a zero alpha.
Delegating to the platforms ColorUtil.toHex, which zero pads every
component, removes that dependency. ColorPanel itself uses the same helper
to render its hex label.
Verified against the old implementation over all 16777216 opaque colors:
identical output and both round trip through Color.decode. They only
differ for an alpha below 0x10, where the old one returned a truncated and
therefore wrong color.
(cherry picked from commit 462125d704379be0eb43287593c94d9f2613ffd8)
The BufferedReader was never closed, so every scanned .csproj leaked a file handle, and the early return on a match left the reader of that file open as well. Use try-with-resources, drop the redundant exists() check and log through the platform logger instead of printStackTrace. (cherry picked from commit d57179d72afda1d222f789bd20f6a9f38af4e585)
drawProblemLabel measured the line text with new Canvas().getFontMetrics(), which builds a heavyweight AWT component per problem and per scan. It is one of the hotspots in the profiler traces of GitHub issue #96. The editor content component provides the same metrics without the allocation, and it is the component the label is actually painted on. (cherry picked from commit 4f9e88a1e32395745ca7a3b82577755c09ef98f5)
Problems identified their drawn elements by storing the hash code of the renderer and of the range highlighter. Removing an element therefore meant walking the whole markup model or every inlay of the document: - undrawErrorLineHighlight iterated markupModel.getAllHighlighters() - undrawInlineProblemLabel asked for all block/after-line-end elements of the entire document - removeGutterIconsForLine iterated getAllHighlighters() again Both traces in GitHub issue #96 hit exactly those loops. Matching by hash code is also ambiguous, which can leave a label or a gutter icon behind or dispose a foreign one - the likely cause of issues #38 and #44. InlineProblem now holds the Inlay and the RangeHighlighter it created, so removal is a single dispose/removeHighlighter call. removeGutterIconsForLine uses processRangeHighlightersOverlappingWith when the markup model supports it, and the dead highlightInfoStartOffset field (which held a hash code despite its name, yet took part in equals) is gone. (cherry picked from commit 884ebd26b021fb67db99ec46ddceb578d957fd08)
The filter list was split and lowercased again for every single problem in
DocumentMarkupModelScanner and for every highlighter event in
MarkupModelProblemListener, which can happen several times per millisecond.
The parsed and normalized list now lives in ProblemTextFilter and is only
rebuilt when the setting itself changes.
This also drops blank filter entries. An empty filter list produced a
single empty entry through split(";"), and startsWith("") matches
everything, so an empty list would have hidden every problem.
(cherry picked from commit e049e34679d60c2e9c8724d5c32b677002cfdba0)
- updateFromNewActiveProblems ran List.contains inside two stream filters and kept the already processed hash codes in an ArrayList, so every scan round was quadratic in the number of problems. All three lookups now go through hash sets. - getProblemsInLineForProblem and getProblemsInLineForProblemSorted were identical except for the sort; the sorted variant now delegates. The returned list is explicitly an ArrayList because InlineDrawer removes an element from it while re-adding the gutter icon. - Collections.synchronizedList(activeProblems) created a fresh wrapper on every add and remove, so it synchronized on an object nobody else could see. It provided no thread safety at all and only cost an allocation per operation - removed. - findActiveProblemByRangeHighlighter now also matches the editor of the listener. A document markup highlighter is shared by all editors of that document, so in a split view the lookup could return the problem of the other editor and remove the wrong element. (cherry picked from commit c6bc02411aff82277bd565e6dbf670d7a1754d06)
- MarkupModelProblemListener kept its disposables in a static list that was only cleared in disposeAll(), so every file that was ever opened left a disposable registered on the ProblemManager for the rest of the session. The disposables are now keyed by TextEditor, are released when the editor is gone (disposeInvalid, called from fileClosed) and setup() no longer installs a second listener on an editor that already has one - which used to duplicate every problem event. - Problems of closed editors were only noticed by the next full scan, and removing them meant undrawing on an already disposed editor. ProblemManager.removeObsoleteProblems drops them without undrawing, and ProjectCloseListener now calls resetForProject while the editors are still alive, for every IDE instead of only Rider. - DocumentMarkupModelScanner.dispose only cancelled the merging queue, so the scheduled scan kept running after a plugin unload and the stale static instance made a reload end up with two scans. It now cancels the future and clears the instance. cancelScheduledFuture no longer retries with cancel(true), which could never succeed after cancel(false) failed, and handles a missing future. (cherry picked from commit 621ea07d11edca9d8e5e637ed77e1933b61e8df3)
Closes GitHub issue #94. The filter list could only match the beginning of a problem text, which does not work for messages that start with the symbol name, e.g. "'variable' is assigned a value but never used". A filter entry is now interpreted as - a regular expression if it starts with "re:" (partial match) - a glob if it contains * or ? (whole text match) - a problem text beginning otherwise Existing filter lists keep working unchanged. Glob entries quote everything but the wildcards, so filter texts may contain regex meta characters, and an invalid regular expression is logged and skipped instead of breaking the whole list. (cherry picked from commit 92f454230d0af3a8e42c24a03a50852b9c6f4d00)
Addresses GitHub issue #61. The drawn elements keep the colors they were created with - the inlay renderer copies them in its constructor and the line highlighter reads the default foreground and background from the color scheme that was active while drawing. When the IDE follows the OS theme, the problems therefore stayed in the old theme. ThemeChangeListener subscribes to EditorColorsListener and LafManagerListener and triggers a reset and rescan, guarded so that a switch which fires both topics only redraws once. Note that the configured problem colors themselves are still absolute values, so a user who wants different colors per theme still has to adjust them by hand. (cherry picked from commit 9937ade3c330617594062b31fcde9f2a1ae0df84)
entities/enums/Listener was a class holding three int constants, so the selected listener travelled through the code as an int and SettingsComponent.getEnabledListener returned the raw combo box index. That only mapped to the right listener because the combo box entries happened to be listed in the same order as the constants - inserting or reordering an entry would have silently switched the listener for every user. Listener is now an enum that owns its persisted id and its display name, the settings expose it through getActiveListener/setActiveListener, and the combo box is built from the enum values so index and value cannot drift apart. The persisted setting stays an int id, so existing settings files keep working, and the NAME constants that only existed to label the combo box are gone. (cherry picked from commit ef5428503a480b8caf27429435a64a79393467ad)
SettingsState.problemLineLengthOffsetPixels existed but was never read; InlineDrawer had the value hardcoded as "+ 50" with the comment that the width calculation is not exact. Since the deviation depends on the font and the editor, the setting is now actually used and exposed in the settings, next to the inlay font size delta. (cherry picked from commit 28e755ac775c169879b46e7a6ac1e0d6997f41ef)
The committed wrapper was still on Gradle 7.5.1 while gradle.properties already claimed 7.6, and 7.5.1 refuses to run on JDK 21 or newer, so the project could not be built with a current default JDK at all. - Gradle 7.5.1 -> 8.10.2 (wrapper regenerated, properties aligned) - org.gradle.unsafe.configuration-cache -> org.gradle.configuration-cache, the old name is gone in Gradle 8 - org.jetbrains.intellij 1.11.0 -> 1.17.4 (last 1.x release) - io.freefair.lombok 6.6 -> 8.6, org.projectlombok:lombok 1.18.24 -> 1.18.34 - org.jetbrains.changelog 2.0.0 -> 2.2.1 - org.jetbrains.qodana 0.1.13 -> 2024.1.5; reportPath, saveReport and showReport no longer exist in the extension, report handling is configured through qodana.yml and the CI action The platform baseline is deliberately untouched: platformVersion stays at 2021.2.4 and the sources still target Java 11, so compilation needs a JDK 17 (or older) to run Gradle. (cherry picked from commit 20c57c7b98ef220d07992b34507b6301ffe32109)
.gitignore additions: - .env, which is currently untracked only by luck and may hold tokens - IMPROVEMENTS.md, local review notes - .claude/, .idea/modules.xml, .idea/copilot*, all generated per machine Workflows: - The build workflow uploaded build/reports/kover/report.xml to Codecov, but the Kover plugin is not applied, so that file never exists. Step removed instead of pretending there is coverage. - Documented that the verify job no longer runs the IntelliJ Plugin Verifier: 9a6304e replaced runPluginVerifier with verifyPlugin, which in gradle-intellij-plugin 1.x only validates the descriptors and the archive structure. The verifier home dir property, its cache step and listProductsReleases have been dead weight since then. Left as a TODO rather than re-enabled, because the commit message suggests it was failing and that needs a look first. - release, beta-release and run-ui-tests still used checkout@v3 and setup-java@v3 while build.yml was already on v4, and the publishing workflows had no Gradle cache. - Added dependabot for gradle and github-actions, the plugin versions had drifted several years behind. (cherry picked from commit 01043256400ecb5f353bf79eabadf7323bf08ec0)
- The license link pointed to LICENSE.txt on the master branch, the file is LICENSE on main - The table of contents link to the beta section had a space instead of a dash, so it did not resolve - Getting Started only said "clone and open"; it now names the JDK requirement that follows from the Java 11 target and the tasks that are actually useful - The roadmap entries that already have issues link to them (cherry picked from commit 885e84905c83387ec66269f5c74499f9388b98f0)
Commit 9a6304e ("Try to fix plugin verification task", 2025-04-10) replaced runPluginVerifier with verifyPlugin in the verify job. With gradle-intellij-plugin 1.x those are different tasks - verifyPlugin only validates the descriptors and the archive structure - so binary compatibility against newer IDE builds has not been checked since then. The verifier home dir property, its cache step and listProductsReleases were left behind as dead weight. The likely reason it was failing is the IDE list rather than the plugin: without an explicit ideVersions, the builds are derived from pluginSinceBuild through listProductsReleases, which with an open untilBuild currently resolves to 14 IDE builds. At 1.8 to 4.1 GB unpacked each that is far more than a runner has after cleanup. The verified build is therefore pinned in gradle.properties (pluginVerifierIdeVersions) and limited to the latest release. The lower bound needs no verifier run: the sources are compiled against platformVersion, so the compiler already guarantees the API exists there. What the verifier adds is catching API that disappeared in a newer IDE. failureLevel is set to the levels that mean real breakage - COMPATIBILITY_PROBLEMS, INVALID_PLUGIN, MISSING_DEPENDENCIES. The deprecation and internal API levels are deliberately not included, because this plugin knowingly builds on internal API and those levels would be permanently red without being actionable; they still show up in the report. The IDE cache key is now the pinned version instead of a hash of gradle.properties, which also holds pluginVersion and would have invalidated a multi gigabyte cache entry on every version bump. listProductsReleases is no longer called since nothing consumes it. Verified locally with the exact command the workflow runs: IU-253.28294.334 Compatible. 3 usages of deprecated API BUILD SUCCESSFUL No compatibility problems. Note that IC-2025.3 resolves to a build that identifies as IU: Community and Ultimate ship as one distribution from 2025.3 on, so the verifier checks against the Ultimate class set. That is harmless here because the plugin only depends on com.intellij.modules.platform. The reported deprecations are HighlightSeverity.INFO in ProblemManager.applyCustomSeverity, ActionUtil.invokeAction in InlineProblemLabel.mouseClicked and the overridden FileEditorManagerListener.fileOpenedSync. (cherry picked from commit 6f96ac9c7838e5d98fa61f2a14fbbca9eed7549a)
applyCustomSeverity assigned HighlightSeverity.INFO.myVal for the
severities configured under "Additional severities" in the info section.
INFO is deprecated in favour of WEAK_WARNING, and it carries the same
value:
INFORMATION = 10
INFO = 200 (deprecated, "use WEAK_WARNING")
WEAK_WARNING = 200
WARNING = 300
ERROR = 400
Every classification in the plugin - shouldProblemBeIgnored, DrawDetails
and SeverityUtil - dispatches on `severity >= WEAK_WARNING.myVal` before
it reaches the INFORMATION branch. A problem remapped through the info
list was therefore rendered as a weak warning: weak warning colors, and
hidden or shown by the weak warning toggle instead of the info one.
INFORMATION is the value the three classifications actually mean by
"info", and it also removes the last deprecation warning from the build.
The plugin was compiled against the IntelliJ Platform 2021.2.4 with
pluginSinceBuild 212.5, which kept every API added after 2021.2 out of
reach and made the platform log a PluginException on every start of a
recent IDE ("Migrate ProjectStartupActivity to ProjectActivity").
New baseline: platformVersion 2025.1.7, pluginSinceBuild 251, Java 21.
2025.1 is the earliest 2025 line, so it keeps the widest user base among
the 2025 releases while unlocking everything this plugin actually wants -
ProjectActivity (2023.1), AnAction.getActionUpdateThread (2022.3), the
non-deprecated FileEditorManagerListener and ActionUtil overloads, and
Java 16+ language features such as pattern matching for instanceof and
records. Nothing in 2025.2 or 2025.3 adds anything for this plugin, and
going higher would only cut users.
That baseline forces two toolchain moves, both required rather than
optional:
- gradle-intellij-plugin 1.x refuses to build against 2024.2+ ("does not
support building plugins against the IntelliJ Platform 2024.2+ (242+)"),
so the build moves to the IntelliJ Platform Gradle Plugin 2.18.1. The
whole build script is restructured accordingly: the intellij block
becomes intellijPlatform with pluginConfiguration, signing, publishing
and pluginVerification, the platform is declared as a dependency, and
patchPluginXml is configured through pluginConfiguration instead.
- The 2.x plugin from 2.14.0 on requires Gradle 9, so the wrapper moves
from 8.10.2 to 9.7.1. Side effect worth having: Gradle 9.7.1 runs on
current JDKs, so the build no longer needs a JAVA_HOME override - it
works with a JDK 25 default.
Task renames that come with the 2.x plugin, applied to the workflow and
the run configuration:
verifyPlugin (1.x) -> verifyPluginStructure
runPluginVerifier (1.x) -> verifyPlugin
listProductsReleases -> printProductsReleases
The verifier no longer needs its own IDE cache either, because 2.x
resolves the verified IDE as a regular Gradle dependency, so it lands in
the Gradle cache the setup-gradle action already handles.
pluginVerifierIdeVersions moves from IC-2025.3 to IU-2025.3: IntelliJ IDEA
Community is not published separately any more starting with 253, the
plugin resolution fails with "IC is no longer published since 2025.3".
Community and Ultimate ship as one distribution now.
Verified with the default JDK 25:
- ./gradlew buildPlugin - BUILD SUCCESSFUL, since-build="251", no until-build
- ./gradlew verifyPluginProjectConfiguration - no issues
- ./gradlew verifyPluginStructure verifyPlugin - IU-253.28294.334
"Compatible. 2 usages of deprecated API", no compatibility problems
The two remaining deprecations are addressed in the next commit, together
with the rest of the modernization the new baseline allows.
Also in this commit because the new tooling requires it: .intellijPlatform
added to .gitignore (asked for by verifyPluginProjectConfiguration),
qodana projectJDK 11 -> 21, and Java 21 in all workflows. The UI test
workflow is marked with a TODO: runIdeForUiTests does not exist in 2.x and
there are no UI tests to run anyway.
Everything here needed either the 2025.1 platform or Java 21, so none of it was possible before the baseline bump. Platform APIs: - ProjectStartupActivity implements ProjectActivity instead of the deprecated StartupActivity. That is what the platform asked for at runtime; a recent IDE logged "PluginException: Migrate org.overengineer.inlineproblems.ProjectStartupActivity to ProjectActivity" on every start. ProjectActivity is a Kotlin interface with a suspending function, so from Java it takes the Continuation and returns Unit.INSTANCE, which completes the activity without suspending. - IPAction overrides getActionUpdateThread() with BGT, which covers all five actions. None of them implements update(), so nothing needs the EDT, and the platform has been logging a warning per action since 2022.3. - FileEditorListener installs the markup listener from fileOpened instead of fileOpenedSync. Both fileOpenedSync overloads are deprecated in 2025.1 - the Pair based one and the List<FileEditorWithProvider> one that replaced it. fileOpened runs once the file is open rather than synchronously during opening, which suits this listener: the editor and its markup model are ready at that point. - InlineProblemLabel invokes the intention action through ActionUtil.invokeAction(AnAction, AnActionEvent, Runnable) built with the non-deprecated AnActionEvent.createEvent, and uses the ActionPlaces.EDITOR_INLAY constant instead of a hand written "EditorInlay" string. Java 21 language level: - InlineProblemProject is a record now, which drops its Lombok annotations. Callers use the record accessors. - Pattern matching for instanceof replaces the instanceof plus cast pairs in GutterRenderer, ListenerManager, DocumentMarkupModelScanner, HighlightProblemListener, MarkupModelProblemListener and InlineDrawer. In HighlightProblemListener that also removes a redundant null check, since instanceof already covers null. - The stream in FileEditorListener filters and casts through TextEditor.class::isInstance and ::cast. Verified: - compileJava with -Xlint:deprecation -Xlint:removal: zero warnings, down from two before this commit and three before the baseline bump. - verifyPluginStructure + verifyPlugin against IU-253.28294.334: "Compatible. 1 usage of deprecated API", no compatibility problems. The one remaining entry is the invokeAction call above, which is only deprecated from 2025.3 on; its successor does not exist in the 2025.1 baseline, so it has to wait for the next bump.
The click handler looked up IdeActions.ACTION_SHOW_INTENTION_ACTIONS and
invoked it through ActionUtil, with a direct ShowIntentionActionsHandler
call as fallback. As of 2025.3 every ActionUtil entry point for running an
action programmatically is deprecated - all three invokeAction overloads
plus performDumbAwareWithCallbacks and performActionDumbAwareWithCallbacks
- so there is no non-deprecated way left to do it that way.
Checked against both SDKs:
ActionUtil.invokeAction(AnAction, Component, ...) deprecated in 251 and 253
ActionUtil.invokeAction(AnAction, DataContext, ...) deprecated in 251 and 253
ActionUtil.invokeAction(AnAction, AnActionEvent, ...) deprecated in 253
ActionUtil.performDumbAwareWithCallbacks(...) deprecated in 253
ShowIntentionActionsHandler.invoke(Project, Editor, PsiFile, boolean)
not deprecated in either
So the action lookup goes away and the handler is called directly. It is
the handler the platform's own "Show Intention Actions" action delegates
to, and it is also the path that actually ran in the released versions:
the lookup used org.jdesktop.swingx.action.ActionManager, which always
returned null, so the fallback was the only code that ever executed.
Added a DumbService guard, because the intentions are computed from the
indexes. The action system used to provide that check.
This drops nine imports from InlineProblemLabel and takes the verifier
verdict against IU-253.28294.334 from "Compatible. 1 usage of deprecated
API" to plain "Compatible". compileJava with -Xlint:deprecation
-Xlint:removal stays at zero warnings.
Reported for a 1000 line file with a warning on every line: deleting a semicolon still spikes the CPU. Three things combined to make one edit cost roughly two thousand inlay operations. 1. Problem identity was unstable. InlineProblem.equals included actualStartffset, actualEndOffset and the RangeHighlighter instance, and excluded the line. A daemon run recreates the highlighters and shifts the offsets of everything behind the edit, so every problem of the file compared as new: the diff removed all of them and added all of them again. Identity is now the editor, the file, the line, the severity and the text, which is stable for an edit inside a line. Offsets are volatile position data and are copied onto the existing problem through refreshPositionFrom instead - the inlay is anchored to the document and moves along on its own, so it must not be recreated, and every inlay change makes the editor recalculate its preferred size (EditorSizeManager.validateSize, visible in the traces of issue #96). 2. Every markup event was handled on its own. MarkupModelProblemListener posted an invokeLater per event and then added or removed a single problem, each time walking the whole active problem list - once in getProblemsInLineForProblem and once in findActiveProblemByRangeHighlighter. With a thousand problems that is on the order of a million comparisons plus a thousand EDT tasks per edit. The events are now coalesced into one rescan of the affected editor through the merging queue, which is the path that "Show only highest severity per line" already used, and the diff decides what actually has to change. That also removes the whole per event add/remove machinery from the listener. 3. The merging queue merged too much and too little at once. The queued update used the constant identity "scan", so with several open editors only one of them was ever rescanned. The identity is the editor now. The merge window went from 10 ms to 100 ms so that a burst of daemon events ends up in a single rescan. Two more things that fell out of it: - updateFromNewActiveProblemsForProjectAndFile filtered the snapshot by project and file, not by editor. As the default path that would make a split view of one file remove the other editors problems on every scan, so it is now updateFromNewActiveProblemsForTextEditor. - The periodic full scan is only a safety net while an event driven listener is active, but ran every 2 seconds, rebuilding every problem of every open editor on the EDT that often. It now runs every 10 seconds. For an edit inside a line the inlay operations should drop from roughly two per problem in the file to roughly the number of problems that really appeared, disappeared or changed line. Editing that changes line numbers (pressing Enter) still redraws everything below the edit; that is the same as before and would need line tracking to improve. Note on identity: two identical problems on the same line collapse into one, which is the behaviour the field comment already described, and the diff now also drops such a duplicate inside one batch instead of drawing two labels on top of each other.
Follow-up after the previous commit did not move the needle on a 1000 line file with a problem per line. Two things were still missing. 1. Inlay changes were applied one by one. The editor recalculates its preferred size for every single added or removed inlay - EditorSizeManager.validateSize, the hotspot in the profiler traces of issue #96. The platform has InlayModel.execute(true, ...) for exactly this: in batch mode the recalculation happens once for the whole operation. Nothing in the plugin used it, so a bulk change of a thousand problems meant a thousand size recalculations, each of which walks the visual lines of the document. The problem diff now runs inside one batch per involved editor, and so do reset, resetForProject and resetForEditor, which remove everything at once. This also matters when the problems genuinely change rather than just move: deleting a semicolon breaks the enclosing method, so its inspections stop reporting and a thousand warnings really do disappear and come back. Removing them is unavoidable, doing it without a thousand size recalculations is not. 2. HighlightProblemListener posted one EDT task per HighlightInfo. accept() is called once per HighlightInfo the daemon produces, and each call posted an invokeLater that ends in the same per file rescan. For a file with a thousand problems that is a thousand EDT tasks per analysis run, all doing the same thing. Only one task per file is queued now, and it is released as soon as it runs. This path matters more than it looks: it was dead until the inverted isEnableInlineProblem check was fixed, so fixing that turned an unused code path into a hot one for anyone who has the HighlightProblemListener selected.
The diff and the reset paths already ran inside InlayModel.execute, but
addProblem and removeProblem did not when called on their own. addProblem
in particular removes and re-adds every problem of the affected line to
keep them ordered by severity, so even a single call can touch several
inlays.
Both public entry points now open a batch for their editor, and
runInInlayBatchMode skips an editor that is already batching because of an
enclosing call, so the nested case stays a no-op instead of re-entering.
Disposed editors are skipped too, rather than relying on the caller to
filter them.
Measured on the reported case - a 1000 line file with a problem on every
line, 1271 problems drawn (instrumentation on perf/instrumented-measurement):
initial draw 5089 markup events -> 1 diff, 1271 inlays, 149 ms
safety net scan 0 events -> 1 diff, 0 inlays, 1.8 ms
the edit 1 event -> 1 diff, 3 inlay ops, 2.0 ms
full daemon run 5087 markup events -> 1 diff, 0 inlay ops, 0.9 ms
The last line is the one that used to hurt: the daemon recreates every
highlighter of the file, and the plugin now coalesces that into a single
diff that recognizes everything as unchanged and touches no inlay at all.
The previous commit batched the single problem entry points as well, which
was the wrong direction. Reading what the platform actually does on batch
mode explains why:
InlayModelImpl.execute -> notifyBatchModeStarting / Finished
EditorSizeManager.onBatchModeStart -> getPreferredSize()
EditorSizeManager.onBatchModeFinish -> reset()
Entering computes the full preferred size and leaving drops the cached
size, so a batch costs roughly two full size computations, while staying
out of it costs one size validation per changed inlay. Batching a single
change is therefore a loss, and batching a diff that changes nothing at
all is pure overhead - which is the most common case by far, because most
scans find nothing to redraw. It showed up in the measurement: a full
daemon run with 5088 events and zero inlay changes went from 0.9 ms to
1.2 ms after the batch was introduced.
The diff is now computed before anything is drawn, so the number of
inlays that will change is known up front:
- nothing to add or remove: return without touching the editor at all
- fewer changes than BATCH_MODE_THRESHOLD: apply them directly
- more: apply them inside one batch per involved editor
addProblem and removeProblem lose their batch wrappers again, so a single
call stays on the cheap incremental path. The nesting guard also went
away, InlayModelImpl.execute already ignores a nested call.
collectEditors is now called with the problems that actually change
instead of the whole snapshot, so an editor that only has unchanged
problems is not put into batch mode either.
Drawing 1268 problems took about 124 ms, roughly 98 microseconds each.
Almost all of that was work that does not depend on the problem at all
but was redone for every single one:
SettingsState.getInstance() three times per problem
(drawProblemLabel, FontUtil.getActiveFont, DrawDetails)
getScrollingModel().getVisibleArea() constant per editor
getColorsScheme().getFont(PLAIN) constant per editor
getContentComponent().getFontMetrics(...) constant per editor
FontInfo.getFontRenderContext(...) constant per editor
UIUtil.getFontWithFallback(...) builds a font with a
fallback chain, per problem
EditorDrawContext holds all of it and is built once per editor per
drawing pass, then handed down through addProblem to drawProblemLabel.
DrawDetails takes the settings as a parameter now instead of looking the
service up itself.
The context is deliberately short lived - built for one pass and dropped -
so it cannot go stale when the color scheme, the settings or the editor
size change. That is also why it is not cached on the drawer.
The expensive metrics construction moved from InlineProblemLabel into
FontUtil.getLabelFontMetrics, and calcWidthInPixels got an overload that
takes the metrics. The renderer still calls the editor based variant when
the platform asks it to measure itself, and both go through the same
helper, so the cached and the uncached path cannot drift apart.
What is left per problem is what genuinely differs per problem: the text
of its line and the inlays already present on that line.
The value compared against BATCH_MODE_THRESHOLD only counts the problems in the diff lists, but addProblem also removes and redraws the problems that already sit in the same line, which the diff does not know about. Measured on the reported case: an edit that adds one problem to a line that already had one is estimated as one operation and stays out of the batch, while it actually performs three inlay operations. At that size batching and not batching cost about the same, so the imprecision does not matter - but the comment claimed an accuracy it does not have.
improvements_offen.md is the second local notes file and idea.log shows up in the project root whenever a sandbox log is copied there for analysis. Neither belongs in the repository, and IMPROVEMENTS.md was already listed.
The scheduled scan is a safety net while one of the event driven listeners is active, but it did the most expensive thing possible: on the EDT, for every open editor of every open project, it rebuilt every problem from the markup model and diffed it - measured at ~1.7 ms for a file with 1272 problems, every interval, no matter whether anything had changed. It now only scans the editors returned by getSelectedEditors(). A background tab is not analyzed by the daemon either, and when it becomes visible the daemon run that follows fires the markup events that trigger a rescan of exactly that editor. With several tabs open this cuts the periodic work proportionally. Scanning per editor also means the global diff variant is gone. It took a snapshot of all active problems and compared it against the problems of all editors at once, which only worked as long as every editor was scanned in the same pass. Two things deliberately not done here: - An early exit for "nothing changed since the last scan". There is no cheap change signal for the markup model: MarkupModelEx has no modification stamp, and the document stamp does not move when the daemon only updates highlighters, which is exactly the case the safety net exists for. A heuristic that can suppress the safety net is worse than the 1.7 ms it would save every 10 seconds. - Moving the collection off the EDT. The drawing has to happen there anyway, and for one visible editor the remaining cost does not justify the threading risk.
getProblemsInLineForProblem scanned the whole active problem list, and it runs once per added problem. For a single edit that is irrelevant, but drawing a file with n problems does it n times, so the initial draw was quadratic - a measurable part of the 81 ms it takes for 1268 problems. The lookup now goes through a Map<TextEditor, Map<Integer, List>> that is kept in step at the three places where activeProblems is mutated: addProblemPrivate, removeProblem and removeObsoleteProblems. The line of a problem is final, so an entry never has to move while the problem is in the list. removeFromIndex is called right before the problem leaves activeProblems, after the gutter icon lookup that removeProblem does at its start, so the order of the existing logic is unchanged. Empty entries are dropped so a closed editor does not stay referenced through the index. getProblemsInLineForProblem returns a copy, as before: InlineDrawer removes the problem that is being undrawn from the list it gets.
DrawDetails only depend on the severity, the settings and the color scheme, all of which are fixed for one drawing pass, so at most four different instances are needed for a whole file instead of one per problem. EditorDrawContext caches them by severity now. The instances are effectively immutable - everything is assigned in the constructor and only read afterwards - so sharing one between problems of the same severity is safe.
drawProblemLabel copied the text of the problems line out of the document
and measured it with the editor font, per problem, only to decide whether
the label still fits behind the line:
String lineText = editor.getDocument().getText(textRange);
... context.getEditorFontMetrics().stringWidth(lineText) ...
The editor already knows where the line ends, so offsetToXY of the line
end offset gives the same number without the string copy and without the
measurement. It is also more correct: measuring the raw text with the
plain editor font ignores tabs, folding and mixed fonts, all of which the
editors own layout accounts for.
With that the editor font and its metrics are no longer needed in the
drawing context at all, which leaves the label metrics as the only thing
it still has to build.
Per problem, what is left of the drawing is the inlays already present on
its line - everything else is either shared through the context or comes
from the editors layout.
The SettingsComponent constructor read 45 values out of the SettingsState,
and SettingsConfigurable.reset() read all of them again - the platform
calls reset() right after createComponent(), so the constructor work was
redundant. Having two places also meant they could drift apart, and they
had: three checkboxes were missing from the constructor.
enableHtmlStripping default true
enableXmlUnescaping default true
showOnlyHighestSeverityPerLine default false
Those three started out unchecked regardless of the stored value. As long
as reset() runs first that is invisible, but any path reaching apply()
without a preceding reset() would have written false for two options that
are enabled by default - the same class of bug as reset() not restoring
maxFileLines, just in the other direction.
The constructor now only builds the widgets, reset() is the single source
of truth for their values. Verified that reset() covers all 49 setters of
the component.
Two problems in the same call, both about how the label font is put together. getFontName() returns the resolved name of the first physical font of a composite font, so passing it to getFontWithFallback drops the fallback chain that the editor font had. Characters the first font does not cover - CJK, emoji, box drawing - then render as boxes. That is what GitHub issue #58 reports. getFamily() keeps the chain intact. The style argument was fontType.ordinal(), the ordinal of an EditorFontType constant used as a java.awt.Font style bitmask. It happens to produce the right numbers only because EditorFontType declares PLAIN, BOLD, ITALIC, BOLD_ITALIC in exactly the order of Font.PLAIN, Font.BOLD, Font.ITALIC and their combination. Reordering that enum, or inserting a constant, would silently change how the labels are rendered. The style is now derived explicitly with Font.BOLD and Font.ITALIC. Both branches were also identical apart from the base font, so they are one expression now. Not verified at runtime: reproducing #58 needs a font that actually depends on the fallback chain.
It removes the problem being undrawn from problemsInLine before redrawing the gutter icon for the ones that remain, which the javadoc did not mention. Callers have to pass a list they own - getProblemsInLineForProblem returns a copy for exactly that reason.
Renamed to actualStartOffset, which also fixes the generated getter getActualStartffset. Purely mechanical, no behaviour change - it was left alone while the work was spread over separate branches because it would have been diff noise in all of them.
compiler.xml, misc.xml and resourceBundles.xml showed up as modified in every git status because each IDE version rewrites them, and the same is true for most of the rest of the directory. The individual exclusions that were there before only covered the files that had already become a nuisance. The whole directory is ignored now and untracked. The shared run configurations stay: they live in .run, not in .idea.
Both the workflow and the run configuration drove the plugin through
runIdeForUiTests, which the IntelliJ Platform Gradle Plugin 2.x does not
have any more. Rather than removing them, they now use what replaced it.
Build:
- intellijPlatform { testFramework(TestFrameworkType.Starter) } pulls in
the IntelliJ Starter framework, which is what drives a real IDE in 2.x -
the old setup started the IDE separately and talked to it through the
robot-server plugin.
- A testIdeUi task is registered through intellijPlatformTesting, so UI
tests get their own task and do not run as part of `test`.
- JUnit 5 is on the test classpath (junitVersion in gradle.properties) and
both test tasks use useJUnitPlatform(), so writing the first test is all
that is left to do.
Workflow: the whole "start the IDE, poll the robot server port, then run
the tests" sequence collapses into `./gradlew testIdeUi`, because the test
code starts the IDE itself now. Linux gets a virtual display through
xvfb-run, and a failed run uploads the test reports together with the IDE
logs and screenshots the Starter framework writes.
The run configuration is renamed from "Run IDE for UI Tests" to "Run UI
Tests" because it no longer just starts an IDE.
Verified: `./gradlew testIdeUi` succeeds. There are no UI tests yet, so it
runs an empty suite - the workflow says so at the top rather than looking
like it checks something.
Everything goes under [Unreleased]; patchChangelog moves it under the version from gradle.properties on release, and publishPlugin depends on that task.
The changelog entry stays under [Unreleased]; patchChangelog turns it into [0.6.0] on release, and publishPlugin depends on that task.
Regression from "Ask the editor for the line width instead of measuring
the text": offsetToXY is not allowed while the inlay model is in batch
mode, and that is exactly where the drawing runs.
java.lang.IllegalStateException: Current operation is not permitted
during batch inlay update
at EditorView.assertNotInBulkMode(EditorView.java:821)
at EditorView.offsetToXY(EditorView.java:199)
...
at InlineDrawer.drawProblemLabel(InlineDrawer.java:51)
Obvious in hindsight: batch mode exists so the editor does not recompute
its layout, so it cannot answer a layout question either. The two
optimizations are mutually exclusive, and the exception aborted the whole
drawing pass, so the remaining problems of the batch were not drawn at
all.
The line is measured with the font metrics again, which only touches the
document and AWT and is therefore safe inside a batch. The editor font and
its metrics move back into EditorDrawContext, so they are still built once
per editor rather than per problem.
Audited the rest of the drawing path against the methods that
EditorView.assertNotInBulkMode guards (offsetToXY, offsetToVisualPosition,
offsetToVisualLine, visualLineToY, xyToVisualPosition,
visualToLogicalPosition and the others): nothing else in the plugin calls
any of them. What the path does touch - Document, InlayModel,
ColorsScheme, MarkupModel and AWT font metrics - is outside that guard.
Qodana for JVM10 new problems were found
💡 Qodana analysis was run in the pull request mode: only the changed files were checked View the detailed Qodana reportTo be able to view the detailed Qodana report, you can either:
To get - name: 'Qodana Scan'
uses: JetBrains/qodana-action@v2024.2.5
with:
upload-result: trueContact Qodana teamContact us at qodana-support@jetbrains.com
|
`./gradlew check` failed in the CI with
Execution failed for task ':instrumentCode'
> 1 >= 1
which is an ArrayIndexOutOfBoundsException inside the plugin itself, at
InstrumentCodeTask.kt:106 where the Ant task for the instrumentation is
defined. Narrowed down locally:
- `buildPlugin`, which only runs instrumentCode, passes cold, repeatedly
- `check` additionally runs instrumentTestCode, and then whichever of the
two runs first fails - in the CI it was instrumentCode, locally
instrumentTestCode
- a warm daemon with a reused configuration cache hides it, which is why
it never showed up here before
So the trigger is having both instrumentation tasks in one build.
instrumentTestCode does nothing useful for this project anyway: it would
only put @NotNull assertions into test classes, and there are no test
sources. It is disabled, which makes `check` deterministic while the
shipped code stays instrumented through instrumentCode.
Also switched the testIdeUi registration from `by ... registering` to
`register("testIdeUi")`. The delegate form is deprecated since Gradle 9.6
and produced three warnings on every configuration.
Verified with the CI's JDK 21, cold daemon and no configuration cache:
`clean check` twice green, plus `buildPlugin`, `testIdeUi` and the plugin
verifier (Compatible).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added
re:is a regular expression, an entry containing*or?is a glob, everything else still matches the beginning of the problem text (#94)Changed
Fixed