diff --git a/.gitignore b/.gitignore index df6966a..11eedbb 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,5 @@ .DS_Store +.claude .gradle .idea .intellijPlatform diff --git a/CHANGELOG.md b/CHANGELOG.md index d9f6bbd..f1a6de5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,39 @@ ## [Unreleased] +### Changed + +- Run / Stop / Rerun / Delete / Collapse buttons now use the declarative inlay + hints API (`com.intellij.codeInsight.hints.declarative.InlayHintsProvider`) + instead of the legacy `InlayHintsProvider` DSL. Click handling + is dispatched through application-level `InlayActionHandler` extensions. + Buttons are text-only (`[Run]`, `[Stop]`, `[Delete]`, `[Collapse]`, + `[Expand]`, `[Rerun]`) because `PresentationTreeBuilder` exposes no icon API + in the public surface. +- Setting moved from `Settings | Editor | Inlay Hints | | Call (Unified)` + to `Settings | Editor | Inlay Hints | Other | Call (Unified)` (the platform's + declarative inlay framework groups providers by `InlayGroup`). + +### Fixed + +- Copy toolbar action now copies the actual console output instead of a hardcoded literal. +- HTTP result panel collapsed into a single console view; the previous three tabs all rendered the same component and two of them were empty. +- Shell execution no longer busy-polls the process handler from a `Task.Backgroundable` worker; lifecycle is delivered through `ProcessHandler` callbacks. +- Dropped optional `` entries that referenced missing config files (`language-json.xml`, `plugin-database.xml`, `plugin-dotenv.xml`). +- Inlay refresh no longer depends on `com.intellij.codeInsight.daemon.impl.InlayHintsPassFactoryInternal` + (an `*Internal` symbol that can be renamed or removed between 2025.x updates). + Refresh now goes through the public `DeclarativeInlayHintsPassFactory.scheduleRecompute(editor, project)` + + `DaemonCodeAnalyzer.restart(file)`. + +### Removed + +- `KotlinLanguageExtractor` (was a no-op; the default `AdapterLanguageExtractor` already covers Kotlin files). +- Debug `println` calls from `ExecutionInlayProvider`. +- Legacy `ExecutionInlayProvider` (`InlayHintsProvider` + `FactoryInlayHintsCollector`). + Replaced by `ExecutionDeclarativeInlayProvider`. The `@Suppress("UnstableApiUsage")` + opt-in is gone from the inlay code path. + + ## [2025.1.0] - 2025-12-16 ### Added diff --git a/docs/plans/01-declarative-inlay-hints.md b/docs/plans/01-declarative-inlay-hints.md new file mode 100644 index 0000000..fa1a210 --- /dev/null +++ b/docs/plans/01-declarative-inlay-hints.md @@ -0,0 +1,402 @@ +# 01 — Migrate to declarative inlay hints / Code Vision + +## Problem + +Today the plugin renders Run/Stop/Rerun/Delete/Collapse buttons via the legacy +`com.intellij.codeInsight.hints.InlayHintsProvider` API. The implementation +lives in +[`src/main/kotlin/com/github/xepozz/inline_call/base/inlay/ExecutionInlayProvider.kt`](../../src/main/kotlin/com/github/xepozz/inline_call/base/inlay/ExecutionInlayProvider.kt), +registered per-language in: + +- `src/main/resources/META-INF/language-kotlin.xml:3` +- `src/main/resources/META-INF/language-php.xml:3` +- `src/main/resources/META-INF/language-xml.xml:3` +- `src/main/resources/META-INF/language-yaml.xml:3` + +Concrete problems: + +1. **Legacy API + `@Suppress("UnstableApiUsage")`.** + `ExecutionInlayProvider.kt:37-38` opts out of stability checks. + `InlayHintsProvider` has been the "old" Kotlin DSL flavour since + 2023.1; the modern replacement is + `com.intellij.codeInsight.hints.declarative.InlayHintsProvider` + (extension point `com.intellij.codeInsight.declarativeInlayProvider`). + +2. **Internal API used to force re-render.** + `ExecutionInlayProvider.kt:13` imports + `com.intellij.codeInsight.daemon.impl.InlayHintsPassFactoryInternal`, and + `ExecutionInlayProvider.kt:266-271` calls + `InlayHintsPassFactoryInternal.forceHintsUpdateOnNextPass()`. + This class is `*Internal` — it is not part of the public plugin SDK and can + be renamed, removed or made `internal` between minor IDE updates. The plugin + already targets `pluginSinceBuild = 251` (`gradle.properties:11`), so any + 2025.x change to this symbol breaks the only refresh path the inlays have. + +3. **Inlay collector does heavy work in `collect`.** + `computeMatches` (`ExecutionInlayProvider.kt:73-98`) walks every extractor + + feature generator per pass. Re-collecting full file matches every keystroke + is wasteful and amplifies the impact of (2). + +4. **Mixed concerns — text presentation and Swing embedding share the same + provider.** The Run button is text-shaped (drawn by the inlay), but the + result panel is a Swing `JPanel` mounted through + [`embedContainerIntoEditor.kt`](../../src/main/kotlin/com/github/xepozz/inline_call/base/inlay/ui/embedContainerIntoEditor.kt) + via `EditorEmbeddedComponentManager`. The provider therefore owns process + lifecycle (`run` / `stop` / `delete` in `ExecutionInlayProvider.kt:184-258`), + which makes migration to *any* declarative API non-trivial, because the + declarative API explicitly forbids Swing inside the hint itself. + +## Options considered + +### (a) Declarative inlay hint for the button row + keep `EditorEmbeddedComponentManager` for the result panel + +Replace `InlayHintsProvider` with +`com.intellij.codeInsight.hints.declarative.InlayHintsProvider`. Each +button becomes a text token built with `PresentationTreeBuilder` +(text + clickable `InlayActionData`). Clicks are dispatched through +`com.intellij.codeInsight.hints.declarative.InlayActionHandler` (registered via +`com.intellij.codeInsight.inlayActionHandler`) with an `InlayActionPayload` +that carries `(featureId, line, action)`. The embedded result `JPanel` stays +exactly as it is — `EditorEmbeddedComponentManager` is the supported way to put +a `JComponent` into the editor and is **independent** of the inlay API. + +- (+) Modern, supported API; no `UnstableApiUsage` opt-in. +- (+) Removes the need to call `InlayHintsPassFactoryInternal` — the + declarative framework re-collects hints on document/PSI change + automatically; for state changes we can call public + `DeclarativeInlayHintsPassFactory.scheduleRecompute(editor, project)` + (note: `impl` package, two arguments) followed by + `DaemonCodeAnalyzer.restart(file)`. +- (+) Sink supports both inline (`InlineInlayPosition`) and end-of-line + (`EndOfLinePosition`) placement and accepts a `tooltip: String` argument + directly on `addPresentation(position, payloads, tooltip, hasBackground, + builder)`. +- (−) **Buttons become text-only.** `PresentationTreeBuilder` exposes only + `text(...)`, `list(...)`, `collapsibleList(...)`, `clickHandlerScope(...)` + and (on `CollapsiblePresentationTreeBuilder`) `toggleButton(...)`. There + is **no `icon(...)` method** — verified by `javap` on + `com.intellij.codeInsight.hints.declarative.PresentationTreeBuilder` in + the 2025.1.1 platform jar (`lib/app-client.jar`). We therefore lose + `factory.icon(AllIcons.Actions.Execute)` and `factory.roundWithBackground` + styling; buttons render as bracketed text tokens such as `[Run]`, + `[Stop]`, `[Delete]`, `[Collapse]`. Background is controlled by the + `hasBackground` boolean on `addPresentation` (or `HintFormat`). +- (−) Click handling becomes EP-routed: we cannot capture a lambda over + `editor` / `match`, so all state for a click must be encoded in the + payload + looked up via `SessionStorage`. + +### (b) Use `DaemonBoundCodeVisionProvider` for the button row + +Code Vision renders a *block* inlay above a PSI anchor, used by IDE for +"references", "implementations", run gutter, etc. Each entry is a clickable +text item handled by `handleClick(editor, element, event)` — the provider owns +the click. Registered via +`com.intellij.codeInsight.daemonBoundCodeVisionProvider`. + +- (+) Public, stable, click handler runs with full `Editor` + `PsiElement` in + scope (closer to today's lambda model). +- (+) `DaemonBound*` variant runs inside the highlighting daemon, so we get + automatic recomputation on PSI changes — no manual refresh hack. +- (−) Code Vision lives **above** the line (block inlay). Today's UX is an + *inline* inlay anchored to the offset of the match (`addInlineElement`, + `ExecutionInlayProvider.kt:68`). Switching to block placement changes the + look noticeably; users may dislike it. +- (−) Each Code Vision entry is one button; rendering Run + Stop + Delete + + Collapse on one line means emitting several entries and they will be + separated by the standard Code Vision spacing/separator. +- (−) Settings UI moves under *Settings | Editor | Inlay Hints | Code Vision*, + not *Editor | Inlay Hints | *. + +### (c) Stay on the old `InlayHintsProvider`, but drop the internal refresh call + +Keep everything in `ExecutionInlayProvider.kt` and only replace +`InlayHintsPassFactoryInternal.forceHintsUpdateOnNextPass()` with the public +`DaemonCodeAnalyzer.getInstance(project).restart(file)` (or +`restart()`, which is already called on the next line — +`ExecutionInlayProvider.kt:269`). + +- (+) Smallest possible diff; one import + two lines change. +- (+) Zero risk of UX regression. +- (−) Does **not** fix the deprecated API surface. The old DSL is still marked + `@ApiStatus.ScheduledForRemoval` in newer 2025.x builds; this only buys + time. +- (−) `DaemonCodeAnalyzer.restart()` does not guarantee the inlay pass actually + *re-runs* the collector when nothing in PSI changed. We may need a + `PsiManager.dropPsiCaches()` or a synthetic + `DaemonCodeAnalyzer.restart(file)` overload to be safe — verify on a + live editor. + +## Recommended approach + +**Two-phase migration: (c) first as a fast hot-fix, then (a) as the proper +migration.** + +Rationale: + +- (c) is small, removable in one PR, and unblocks the immediate "internal API + could disappear" risk. It can ship in the next patch release. +- (a) is the right long-term home: declarative inlays are the only API that + the platform will keep evolving, the Swing embedding requirement is + *orthogonal* to the inlay choice (we keep `EditorEmbeddedComponentManager` + unchanged), and the click-via-EP model maps cleanly onto our existing + `SessionStorage`-keyed state machine. +- (b) is rejected because the inline placement matters: the Run button must + sit next to `// shell: echo hello`, not on a separate line above it. Code + Vision will only be used if user feedback later asks for a more compact + gutter-style affordance. + +## Implementation steps + +### Phase 1 — De-risk: drop internal API (option c) + +1. Edit `ExecutionInlayProvider.kt:266-271` `refreshInlays`: + - Remove the import of + `com.intellij.codeInsight.daemon.impl.InlayHintsPassFactoryInternal` + (`ExecutionInlayProvider.kt:13`). + - Replace the body with + `DaemonCodeAnalyzer.getInstance(project).restart(file)`, where `file` is + captured from `getCollectorFor(file, …)` + (`ExecutionInlayProvider.kt:48-53`). + - **Note.** The legacy (non-declarative) inlay pass for the old + `InlayHintsProvider` does re-run on `restart(file)` — + `InlayHintsPassFactoryInternal.forceHintsUpdateOnNextPass()` is only + needed when the file's PSI/modification stamp has not changed AND we want + to force the cache to be discarded. For our use case the inlay collector + reads off-PSI state (`SessionStorage`); the daemon will still re-run the + pass because `restart(file)` invalidates the daemon's per-file scheduling + state. If this proves insufficient in practice (Phase 1 verification), + fall back to capturing the produced `Inlay` references from the collector + and calling `Inlay.update()` per match — this stays in public API. +2. Verify on the playground project that clicking Run → Stop transitions still + re-render. +3. Keep `@Suppress("UnstableApiUsage")` — it is still required by the old DSL; + it will be removed in Phase 2. + +Deliverable: one commit, no behaviour change, no `*Internal` import. + +### Phase 2 — Migrate to declarative inlay (option a) + +#### 2.1 Introduce the new provider class + +4. Create `src/main/kotlin/com/github/xepozz/inline_call/base/inlay/ExecutionDeclarativeInlayProvider.kt` + implementing `com.intellij.codeInsight.hints.declarative.InlayHintsProvider` + (note: same simple name as the old class — keep them in separate files / + packages until the old one is deleted). + + Required members: + - `createCollector(file: PsiFile, editor: Editor): InlayHintsCollector?` — + returns an `OwnBypassCollector` (we already do bypass collection in + `computeMatches`, `ExecutionInlayProvider.kt:73-98`) **or** a + `SharedBypassCollector` if we want PSI-element-level dispatch. Start with + `OwnBypassCollector` to mirror current behaviour 1:1. + - The collector's `collectHintsForFile(file, sink: InlayTreeSink)` iterates + `matchesByElement` and calls + `sink.addPresentation(InlineInlayPosition(offset, relatedToPrevious = false), hasBackground = false) { buildButtons(...) }`. + +5. Replace the imperative `factory.icon` / `factory.text` / `factory.onClick` + chain (`ExecutionInlayProvider.kt:100-182`) with the declarative + `PresentationTreeBuilder` DSL. The builder is text-only — there is no + `icon(...)` method — so each button is rendered as a labelled token: + - `text("Run", InlayActionData(payload, RUN_HANDLER_ID))` for the Run + button. Use plain ASCII labels: `Run`, `Stop`, `Delete`, `Collapse` / + `Expand`, `Rerun`. Wrap in spaces and/or brackets (`[Run]`) so the click + target is visually obvious. + - Tooltip is supplied via the `tooltip: String` argument of + `sink.addPresentation(position, payloads, tooltip, hasBackground, builder)` + — *not* via `text(...)` (no per-token tooltip is exposed in the public + `PresentationTreeBuilder`). + - Background pill is controlled by `hasBackground = true` on + `addPresentation`. Each button gets its **own** `addPresentation` call so + each one has an independent tooltip and background, and so the platform + does not insert a separator between them inside one presentation tree. + +#### 2.2 Move clicks to `InlayActionHandler` extensions + +6. Define a payload type, e.g. a Kotlin object with `Json`-style encoding: + `data class ExecutionInlayPayload(val featureId: String, val line: Int, val action: Action)`, + where `Action` is `RUN | STOP | DELETE | COLLAPSE`. Wrap into + `StringInlayActionPayload(json)` (simplest) or implement a custom + `InlayActionPayload`. + +7. Create one `InlayActionHandler` per action: + - `RunInlayActionHandler` (`handlerId = "inline_call.run"`) + - `StopInlayActionHandler` + - `DeleteInlayActionHandler` + - `ToggleCollapseInlayActionHandler` + + Each handler reads `editor`, decodes the payload, looks up the session in + [`SessionStorage`](../../src/main/kotlin/com/github/xepozz/inline_call/base/SessionStorage.kt), + and performs the same logic that lives today in `run` / `stop` / `delete` + / `toggleCollapse` (`ExecutionInlayProvider.kt:184-264`). Move that logic + into a new `ExecutionController` service (`Service`) so handlers + stay thin and the controller is reusable. + +8. Register the handlers in `plugin.xml`: + ```xml + + + + + ``` + These are language-agnostic, so they belong in + `src/main/resources/META-INF/plugin.xml` (not in the per-language files). + +#### 2.3 Re-register the provider per language + +9. In each `META-INF/language-*.xml`, replace + ```xml + + ``` + with + ```xml + + ``` + Files to update: + - `src/main/resources/META-INF/language-kotlin.xml:3` + - `src/main/resources/META-INF/language-php.xml:3` + - `src/main/resources/META-INF/language-xml.xml:3` + - `src/main/resources/META-INF/language-yaml.xml:3` + +10. Add the new `inlay.execution.name` and `inlay.execution.description` keys to + `src/main/resources/messages/CallBundle.properties` (referenced from + `plugin.xml:14`). Required because `declarativeInlayProvider` validates + `nameKey` at startup. + +#### 2.4 Refresh on state change without `*Internal` + +11. From `ExecutionController`, after mutating session state, call + `DeclarativeInlayHintsPassFactory.scheduleRecompute(editor, project)` + (public, in `com.intellij.codeInsight.hints.declarative.impl`, + **two arguments** — editor and project). Always follow with + `DaemonCodeAnalyzer.getInstance(project).restart(psiFile)` because session + state lives off-PSI and the daemon would otherwise see no modification + stamp change and skip the pass. Wrap both calls in `invokeLater { … }` — + these are EDT-bound APIs and we may be called from a process listener + thread (see `processTerminated` in the original + `ExecutionInlayProvider.kt:227`). + +#### 2.5 Decommission the old provider + +12. Delete `ExecutionInlayProvider.kt` + (`src/main/kotlin/com/github/xepozz/inline_call/base/inlay/ExecutionInlayProvider.kt`) + once the declarative version is wired up and verified. +13. Remove the `@Suppress("UnstableApiUsage")` annotation — the declarative API + is stable. +14. Update `CHANGELOG.md` under `[Unreleased]` with the API migration note. + +## Risks + +- **Tooltip / icon rendering parity.** The declarative API renders text + icon + in a different visual style than `factory.roundWithBackground(...)` + (`ExecutionInlayProvider.kt:139`). Buttons may look like coloured tokens + instead of pill-shaped affordances. Acceptable, but expect screenshot diff in + README. +- **Click coalescing.** Multiple `addPresentation` calls at the **same offset** + may be merged or separated with a separator dot by the platform. Verify with + the IDLE + FINISHED + RUNNING transitions where we currently chain 1–3 inlay + parts (`ExecutionInlayProvider.kt:111-172`). +- **Payload size limits.** `StringInlayActionPayload` is transported as a + string in the editor model. Keep payload < ~200 bytes. The current key model + (`makeKey(editor, featureId, line)`, `ExecutionInlayProvider.kt:275`) is + already compact, but `editor.hashCode()` won't survive an IDE restart — this + is already tracked as + [`05-session-storage-key.md`](./05-session-storage-key.md). The migration + should *not* fix that key bug, but should also *not* make it worse. + Concretely: encode the payload as `"$featureId|$line"` (action name is + carried by the `handlerId` of `InlayActionData`). The handler must + reconstruct the same `makeKey(editor, featureId, line)` string using the + `editor` argument passed by `InlayActionHandler.handleClick(editor, + payload)` so the hash-based key matches the one written by the collector. +- **Per-project EP vs application EP for handlers.** + `inlayActionHandler` is application-level, while + [`FeatureGenerator.EP_NAME`](../../src/main/kotlin/com/github/xepozz/inline_call/base/api/FeatureGenerator.kt#L36) + is `ProjectExtensionPointName`. Handlers therefore have to fetch the project + from `editor.project` and look up features via + `FeatureGenerator.getApplicable(project)` — same pattern as today + (`ExecutionInlayProvider.kt:101`). +- **PSI-bound collector vs OwnBypassCollector cost.** If the file has many + matches and we re-walk all extractors on every collection + (`computeMatches`, `ExecutionInlayProvider.kt:73-98`), the declarative pass + budget (~50 ms) can be exceeded. Mitigation: cache results per + `(file, modificationStamp)` inside the collector or move to + `SharedBypassCollector` and dispatch by `PsiElement`. +- **Tests.** There are no inlay-level tests in the repo today. Adding + `BasePlatformTestCase`-based tests for the new provider is *strongly* + recommended but covered by a separate plan; this migration only needs the + manual verification below. + +## Out of scope (defer) + +- Fixing the unstable `SessionStorage` key — see + [`05-session-storage-key.md`](./05-session-storage-key.md). +- Replacing `invokeLater` with coroutines in + `ExecutionInlayProvider.kt:237-263` — see + [`03-coroutines-migration.md`](./03-coroutines-migration.md). +- Migrating the **embedded result panel** away from + [`EditorEmbeddedComponentManager`](../../src/main/kotlin/com/github/xepozz/inline_call/base/inlay/ui/embedContainerIntoEditor.kt). + That API is still the supported route for putting Swing into the editor; + there is no declarative replacement. +- Adding a Code Vision provider for users who want a gutter-style "Run" affordance + — possible follow-up, separate plan. +- Configuration UI (the current `ImmediateConfigurable` is empty, + `ExecutionInlayProvider.kt:44-46`). The new declarative API uses + `InlayHintsCustomSettingsProvider` if needed; skip until we have a real + setting to expose. +- Removing the `@Suppress("UnstableApiUsage")` in Phase 1 — only Phase 2 + removes it. + +## Verification + +Manual smoke test on `playground/` (`ls /home/user/inline-call-plugin/playground`): + +1. Build: `./gradlew :buildPlugin` from `/home/user/inline-call-plugin`. +2. Launch sandbox IDE: `./gradlew :runIde`. Open a file from `playground/` + that contains a `// shell: …` or `// https://…` comment. +3. **IDLE state.** Expect a Run button next to the comment. Hover shows the + tooltip "shell: …" (or "http: …"). Cursor turns into a hand. +4. **RUNNING state.** Click Run; the result panel embeds below the line + (`embedContainerIntoEditor`), the inline button row switches to + `Stop` + `Delete` (`ExecutionInlayProvider.kt:124-135` parity). +5. **FINISHED state.** When the process terminates, the row becomes + `Rerun` + `Delete` + `Collapse`. Click `Collapse` — the panel hides; click + again — it shows. Click `Rerun` — the process restarts and the row goes + back to `Stop` + `Delete`. +6. **Delete.** Click `Delete` — the panel disappears and the row resets to + `Run` only. +7. **Per-language coverage.** Repeat steps 3–6 in `.kt`, `.php`, `.xml`, + `.yaml` files to confirm each `language-*.xml` registration was migrated. +8. **No internal-API regressions at startup.** In the sandbox IDE, *Help | + Show Log in Files* — grep for + `InlayHintsPassFactoryInternal`, `NoClassDefFoundError`, + `IllegalAccessError`. None should appear. +9. **Settings UI.** *Settings | Editor | Inlay Hints* — the entry "Call + (Unified)" should appear under each migrated language with the new + `nameKey`. Toggling it off must hide all Run buttons. + +Automated checks: + +10. `./gradlew :verifyPlugin` — IntelliJ Plugin Verifier must not report + references to `*Internal` classes or to the old `inlayProvider` EP. +11. `./gradlew :test` — existing unit tests in `src/test/kotlin` must still + pass (the migration does not touch extractors or feature generators). +12. `./gradlew :runPluginVerifier` against `platformVersion=2025.1.1` and at + least one EAP build to confirm forward compatibility — this is the whole + point of the migration. + +Definition of done: + +- No `InlayHintsPassFactoryInternal` import anywhere in `src/`. +- No `@Suppress("UnstableApiUsage")` referencing the inlay provider. +- All four `language-*.xml` files use `codeInsight.declarativeInlayProvider`. +- All four button states (IDLE / RUNNING / FINISHED + collapse) work in the + sandbox IDE for every supported language. +- Plugin Verifier is green against the current `platformVersion` and the next + EAP. diff --git a/docs/plans/02-run-configurations.md b/docs/plans/02-run-configurations.md new file mode 100644 index 0000000..afcf746 --- /dev/null +++ b/docs/plans/02-run-configurations.md @@ -0,0 +1,425 @@ +# 02 — Register and fix Run Configurations + +## Problem + +The plugin ships two Run Configuration types — HTTP and Shell — fully implemented under: + +- `src/main/kotlin/com/github/xepozz/inline_call/feature/http/run/` +- `src/main/kotlin/com/github/xepozz/inline_call/feature/shell/run/` + +None of them are wired into `META-INF/plugin.xml`. The classes +(`HttpRunConfigurationType`, `ShellRunConfigurationType`, +`HttpRunConfigurationProducer`, `ShellRunConfigurationProducer`) compile but +are never instantiated by the platform — they are effectively dead code. +Naively adding a `` line is not enough: the existing code +has three latent defects that will become visible the moment the IDE starts +constructing these classes for real users. + +Issues to address before registration: + +1. `HttpSettingsEditor` and `ShellSettingsEditor` use unsound binding patterns + that will silently lose user input on the next refactor. +2. Both `ConfigurationFactory` implementations override `getId()` to return + the same string as the parent `ConfigurationType.getId()`. The platform + uses `factory.id` to namespace serialized state inside a type; this + collision triggers a warning and breaks the assumed invariant that the + factory ID is a stable, human-readable name. +3. `HttpProcessHandler.startNotify()` spawns work with `kotlin.concurrent.thread { }`, + bypassing the platform's pooled executor — bad for thread-naming, leak + tracking, and unit-test determinism. +4. The persisted `ConfigurationType` ID becomes a forever-contract once + published on the JetBrains Marketplace. We must lock the IDs in this PR + and never rename them. + +> **Relationship to plan 03 (coroutines migration).** Pre-requisite 3 here is +> an *interim* fix: we move off `kotlin.concurrent.thread` to +> `Application.executeOnPooledThread` so the very first marketplace release +> doesn't ship a non-daemon-thread leak. Plan 03 step 5 supersedes this with +> `runInterruptible(Dispatchers.IO) { ... }` launched on `SessionStorage.cs`. +> Plan 02 deliberately does **not** touch the `invokeLater`-based +> `printToConsole` (that is plan 03 territory) — only the thread-creation +> primitive is replaced. + +## Pre-requisites (must fix BEFORE registration) + +These three fixes are independent and can land in separate commits. They MUST +land before the `plugin.xml` registration commit, because once users have +saved configurations on disk we cannot change persistence shape without a +migration step. + +### 1. SettingsEditor binding fixes + +**HTTP (`HttpRunConfiguration.kt:55-108`)** + +Current shape: `HttpSettingsEditor` declares five `private var` fields +(`url`, `method`, `headers`, `body`, `timeout`) and binds the UI to *those*. +`resetEditorFrom` copies config -> editor fields, `applyEditorTo` copies +editor fields -> config. Two problems: + +- The UI is bound to scratch state, not to the configuration. If a future + refactor removes `applyEditorTo` (or someone adds a new field and forgets + to copy it), the Apply button silently writes defaults instead of the + user's input. +- `bindText(::url)` on a `var url` declared inside the editor compiles but + is semantically wrong: the Kotlin UI DSL expects the binding target to be + the model the dialog represents, so that `isModified()` and `reset()` work + correctly. + +Fix: bind directly to a mutable snapshot model (a plain data class), and let +`applyEditorTo` / `resetEditorFrom` translate between that snapshot and the +`HttpRunConfigurationOptions`. Sketch of the target signatures: + +```kotlin +private data class HttpUiModel( + var url: String = "", + var method: String = "GET", + var headers: String = "", + var body: String = "", + var timeout: Int = 30, +) + +class HttpSettingsEditor : SettingsEditor() { + private val model = HttpUiModel() + private lateinit var panel: DialogPanel + + override fun createEditor(): JComponent { /* panel { ... .bindText(model::url) ... } */ } + override fun resetEditorFrom(s: HttpRunConfiguration) { /* copy s -> model; panel.reset() */ } + override fun applyEditorTo(s: HttpRunConfiguration) { /* panel.apply(); copy model -> s */ } +} +``` + +The key invariant: `panel.apply()` is called before we copy `model -> s`, and +`panel.reset()` is called after we copy `s -> model`. This is the pattern +recommended in the Kotlin UI DSL v2 docs (bindings are applied on +`DialogPanel.apply()`). + +**Shell (`ShellRunConfiguration.kt:69-92`)** + +Current shape: `ShellSettingsEditor` uses raw `JTextField` and +`TextFieldWithBrowseButton` inside `cell(...)` — no binding at all. Values +are read manually in `applyEditorTo`. Also, `TextFieldWithBrowseButton` is +constructed but no `FileChooserDescriptor` is attached, so the Browse button +does nothing useful. + +Fix: + +- Switch to `textField()` / `textFieldWithBrowseButton(...)` factories from + the Kotlin UI DSL v2 so they participate in `isModified()` / `reset()`. +- Bind through a snapshot model as in HTTP. +- Attach a `FileChooserDescriptorFactory.createSingleFolderDescriptor()` to + the working-directory chooser. The DSL helper signature is + `textFieldWithBrowseButton(project: Project? = null, fileChooserDescriptor: FileChooserDescriptor? = null, ...)`; + pass the descriptor as a named argument. + +### 2. `ConfigurationFactory.getId()` rename + +`HttpRunConfigurationType.kt:37` and `ShellRunConfigurationType.kt:38` both +return the same constant as the parent type's ID +(`"HttpInlayRunConfiguration"` / `"ShellInlayRunConfiguration"`). The +platform uses `factory.id` as a child key during run-configuration +serialization. With a single factory per type the collision is technically +tolerable, but: + +- The IDE logs a warning at registration time. +- If we ever add a second factory under the same type, persistence breaks + with no obvious error. + +Fix: return a short, human-readable, stable identifier that is unique inside +its `ConfigurationType`. Proposed values: + +| Type | Factory `getId()` value | Notes | +|-------------------------------|-------------------------|----------------------------------------| +| `HttpRunConfigurationType` | `"HTTP"` | persisted, never rename | +| `ShellRunConfigurationType` | `"Shell"` | persisted, never rename | + +Important: changing `factory.id` *also* changes the serialization key, so +this has to ship in the same PR as the initial registration. After the first +public release the value is frozen forever. + +Note on `ConfigurationFactory.getName()`: by default +`ConfigurationFactory.getName()` falls back to `getId()`, which is what the +*Run/Debug Configurations* wizard shows when a type has more than one +factory. Both of our types have a single factory, so the wizard shows the +type display name (`"HTTP Request"` / `"Shell Command"`) and the factory +name is never user-visible. We therefore leave `getName()` at its default +and let it return `"HTTP"` / `"Shell"` — readable enough if a future feature +adds a second factory under the same type. + +While we are in these files, also fix +`ShellRunConfigurationType.getConfigurationTypeDescription()` — it currently +returns the literal string `"configuration type description"`. + +### 3. ProcessHandler thread management + +`HttpProcessHandler.startNotify()` (HttpRunState.kt:48-54) does: + +```kotlin +override fun startNotify() { + super.startNotify() + thread { executeRequest() } +} +``` + +Problems with `kotlin.concurrent.thread`: + +- Thread is non-daemon by default; it can prevent JVM shutdown in tests. +- No thread name -> hard to identify in profiler / thread dump. +- Not part of the platform's bounded pool -> bypasses limits and leak + detection. + +Fix: use `ApplicationManager.getApplication().executeOnPooledThread { ... }` +and store the returned `Future` so `destroyProcessImpl()` can `cancel(true)` +it. Sketch: + +```kotlin +private var future: Future<*>? = null + +override fun startNotify() { + super.startNotify() + future = ApplicationManager.getApplication().executeOnPooledThread { executeRequest() } +} + +override fun destroyProcessImpl() { + future?.cancel(true) + notifyProcessTerminated(1) +} +``` + +Also wrap the `httpClient.send(...)` call so that an `InterruptedException` +from `cancel(true)` reports a clean "Cancelled" line on the console instead +of a stack trace. The existing `catch (e: Exception)` is broad enough to +catch it; we just need an earlier, more specific `catch (e: InterruptedException)` +(or `catch (e: HttpTimeoutException)` from the platform — `HttpClient.send` +maps thread interruption to `HttpTimeoutException` on Java 21 in some +implementations). Print a single `"\n[Cancelled]\n"` line, then fall +through to the `finally` that calls `notifyProcessTerminated`. + +## Implementation steps + +Each step = one atomic commit. Order matters; later steps depend on earlier +ones being green. + +1. **Commit 1 — Fix `HttpSettingsEditor` binding.** + Introduce `HttpUiModel`, switch UI DSL to bind against it, update + `resetEditorFrom` / `applyEditorTo` to call `panel.reset()` / + `panel.apply()`. No `plugin.xml` change. +2. **Commit 2 — Fix `ShellSettingsEditor` binding.** + Same pattern as HTTP, plus attach a `FileChooserDescriptor` to the + working-directory chooser. +3. **Commit 3 — Rename `ConfigurationFactory.getId()` returns.** + `HttpConfigurationFactory.getId()` -> `"HTTP"`, + `ShellConfigurationFactory.getId()` -> `"Shell"`. Fix the placeholder + description string in `ShellRunConfigurationType`. +4. **Commit 4 — Pool the HTTP request thread.** + Replace `kotlin.concurrent.thread` with `executeOnPooledThread`, store + `Future`, cancel on destroy. Handle `InterruptedException`. +5. **Commit 5 — Register both types and both producers in `plugin.xml`.** + See the XML snippets below. +6. **Commit 6 — Smoke test fixture (deferred to follow-up).** + `LightPlatformTestCase`-based registration smoke tests would guard against + the registration regressing, but adding the first one wires up the entire + platform test fixture (heavy first run, longer CI). Tracked as follow-up + in the next plan revision; out of scope for the PR that registers the + types. `./gradlew :verifyPluginProjectConfiguration` already validates + that the `` and `` extension + declarations are well-formed. + +## plugin.xml registration + +Add the following lines inside the existing +`` block (currently empty on +line 16-17): + +```xml + + + + + + + +``` + +Notes: + +- Both extension points live in the `com.intellij` namespace, so they go in + the existing `defaultExtensionNs="com.intellij"` block (line 16), not in + the second `defaultExtensionNs="com.github.xepozz.inline_call"` block. +- Do NOT add `id=` attributes to the `` lines; the ID + comes from `ConfigurationType.getId()`. +- No `order=` attribute is needed for the producers; `LazyRunConfigurationProducer` + is the canonical pattern and the platform handles ordering. + +## Stable IDs (contract) + +Once this PR is merged and released the following identifiers are frozen +forever. Renaming any of them silently breaks every saved run configuration +of existing users. + +| Surface | Value | Source | +|----------------------------------|--------------------------------------|-----------------------------------------------------------| +| HTTP type ID | `HttpInlayRunConfiguration` | `HttpRunConfigurationType.ID` | +| Shell type ID | `ShellInlayRunConfiguration` | `ShellRunConfigurationType.ID` | +| HTTP factory ID | `HTTP` | `HttpConfigurationFactory.getId()` (new) | +| Shell factory ID | `Shell` | `ShellConfigurationFactory.getId()` (new) | +| HTTP options class FQN | `...feature.http.run.HttpRunConfigurationOptions` | persisted property names: `url`, `method`, `headers`, `body`, `timeout` | +| Shell options class FQN | `...feature.shell.run.ShellRunConfigurationOptions` | persisted property names: `command`, `workingDirectory` | + +The marketplace pre-flight check: search +`grep -r "HttpInlayRunConfiguration\|ShellInlayRunConfiguration"` after +merge — should match only the two `ID` constants and their tests. + +## Manual test plan + +All steps run in a sandbox IDE launched via `./gradlew runIde`. Use a +throwaway project, not the plugin source repo. + +### A. Registration smoke test + +1. Open Run → Edit Configurations… → "+" (Add New Configuration). +2. Expected: "HTTP Request" appears in the list with the Web icon, "Shell + Command" appears with the Run icon. +3. Pick "HTTP Request" → a blank editor opens with fields Method, URL, + Timeout, Headers, Body. No exceptions in `idea.log`. +4. Pick "Shell Command" → editor opens with Command and Working Directory + (Browse button visible). No exceptions in `idea.log`. + +### B. HTTP — empty save / round-trip + +1. Add new HTTP configuration, name it "manual-http-1". +2. Leave defaults (Method=GET, URL empty, Timeout=30). +3. Click Apply → no error dialog, OK to close. +4. Reopen Edit Configurations → "manual-http-1" is still present, all + fields at defaults. +5. Restart the IDE (File → Invalidate Caches → Restart, or kill and + relaunch the sandbox). +6. Reopen Edit Configurations → "manual-http-1" still present and intact. + Confirms `HttpRunConfigurationOptions` round-trips through workspace.xml. + +### C. HTTP — value persistence + +1. Edit "manual-http-1": Method=POST, URL=`https://httpbin.org/post`, + Timeout=15, Headers=`Content-Type: application/json`, Body=`{"x":1}`. +2. Apply, close dialog. +3. Reopen dialog → every field shows the value you just typed. If any + field reverts to the default, the binding fix (step 1) regressed. +4. Modify URL in the dialog but click Cancel. Reopen → URL is unchanged. + Confirms `panel.reset()` correctly discards uncommitted edits. + +### D. HTTP — Modified indicator + +1. With the dialog open and showing a saved config, the Apply button + should be disabled. +2. Type a single character in URL → Apply lights up. +3. Press Ctrl+Z (or revert the character manually) → Apply goes back to + disabled. This proves `bindText(model::url)` correctly drives + `isModified()`. + +### E. HTTP — Run + +1. With the POST config from step C, click Run. +2. Console tab opens, prints `POST https://httpbin.org/post`, then + `Connecting...`, then `HTTP 200` and the response body + (pretty-printed JSON because the response is `application/json`). +3. Click the red Stop button mid-flight (use a slow endpoint like + `https://httpbin.org/delay/10`). Expected: process terminates within + ~1s, console shows the cancellation, no zombie thread in + `jstack`/Thread dump (`Cmd+F1` → Thread dump in the sandbox IDE). +4. Run again — should work after stop, no "process already running" + error. + +### F. HTTP — Producer from comment + +1. In the throwaway project, create `test.kt` with the comment: + `// GET https://httpbin.org/get` +2. Right-click the line containing the comment → "Run …" should offer + to run an HTTP request with name like `GET https://httpbin.org/get`. +3. Run it → console shows the response. +4. Right-click the same line again → instead of creating a new config, + the gutter / context menu reuses the existing one (proves + `isConfigurationFromContext` returns true). +5. Change the URL in the comment, right-click again → a new config is + produced (proves the comparison is exact). + +### G. Shell — Round-trip and Run + +1. Add new "Shell Command" config, name it "manual-shell-1". +2. Command: `echo hello && pwd`. Working Directory: leave empty. +3. Apply, close, reopen → values intact. +4. Run → console prints `hello` followed by the project's base path + (since `ShellRunState.startProcess` falls back to `project.basePath`). +5. Set Working Directory using the Browse button → pick an existing + folder → Apply. Run again → `pwd` now shows the chosen folder. +6. Edit Working Directory to a non-existent path → Run → expected: clean + error from `OSProcessHandler` in the console, no IDE-level exception. + +### H. Shell — Producer from comment + +1. Add the comment `// shell: ls -la` in `test.kt`. +2. Right-click the line → "Run …" offers a "Shell: ls -la" config. +3. Run it → console shows directory listing. + +### I. Negative test — restart safety + +1. Close all sandbox IDE windows. +2. Inspect `.idea/workspace.xml` of the throwaway project: confirm + `` + and `` + are written. The `factoryName` here is the NEW factory `getId()` — + that is the regression guard for pre-requisite 2. +3. Manually corrupt one value (e.g. change `factoryName="HTTP"` to + `factoryName="HttpInlayRunConfiguration"`) → relaunch IDE → confirm + the platform logs a warning that no factory matches. (Then revert.) + +### J. `idea.log` audit + +After running steps A–I, open `Help → Show Log in Files`. Search the +session's log for `WARN` and `ERROR` entries containing +`HttpInlayRunConfiguration`, `ShellInlayRunConfiguration`, `HTTP`, +`Shell`, `configurationType`, `ConfigurationFactory`. Should be empty. + +## Risks + +- **ID freeze.** Once shipped, `HttpInlayRunConfiguration` and + `ShellInlayRunConfiguration` are part of the plugin's persistence + contract. A future rebrand cannot rename them without a + `ConfigurationType` migration shim. Mitigation: add a one-line comment + next to each `const val ID` warning future maintainers. +- **Factory ID change.** Renaming `factory.getId()` from + `HttpInlayRunConfiguration` to `HTTP` will reject saved configurations + that were created with the old factory ID. As of today there are no + such users (the type was never registered), so this is safe — but it + must land in the same release. +- **Bind-model refactor regressions.** The new snapshot-model pattern + is mechanically simple but easy to get subtly wrong (e.g. forgetting + `panel.apply()` before reading the model). Manual test plan steps D + and C are specifically designed to catch this. +- **Pool starvation.** `executeOnPooledThread` shares the platform pool. + If a user fires off many long-running HTTP requests they could starve + other platform work. Acceptable for v1; revisit if reports come in. +- **Producer collisions.** The HTTP pattern is broad + (`(?:VERB\s+)?https?://...`) and may match URLs inside Markdown + comments or KDoc tags. Manual test F validates it works on a typical + case; broader testing during dogfooding will tell. + +## Verification + +After implementing all six commits: + +1. `./gradlew buildPlugin` — must succeed. +2. `./gradlew verifyPlugin` — must succeed with no new warnings for the + two new configurationType / runConfigurationProducer entries. +3. `./gradlew runIde` — execute the full Manual test plan A–J. +4. `grep -n "configurationType\|runConfigurationProducer" src/main/resources/META-INF/plugin.xml` + — must show exactly four lines (two types + two producers). +5. `grep -rn "kotlin.concurrent.thread" src/main/kotlin/com/github/xepozz/inline_call/feature/http` + — must return no matches (confirms pre-requisite 3 landed). +6. `grep -n "configuration type description" src/main/kotlin` + — must return no matches (confirms placeholder description was + replaced). +7. Optional: install the resulting `.zip` from `build/distributions/` + into a clean IDE installation and repeat tests A and E. diff --git a/docs/plans/03-coroutines-migration.md b/docs/plans/03-coroutines-migration.md new file mode 100644 index 0000000..23becbc --- /dev/null +++ b/docs/plans/03-coroutines-migration.md @@ -0,0 +1,604 @@ +# 03 — Migrate invokeLater to Kotlin coroutines + +## Problem + +The plugin currently hops between threads with a mix of low-level primitives: + +- `com.intellij.openapi.application.invokeLater { ... }` is used in 8 places to push UI + mutations onto the EDT. +- The HTTP feature drives `HttpClient.sendAsync(...)` (a `CompletableFuture`) and then + re-enters the EDT from a `whenComplete` callback. +- `HttpRunState` spawns a raw `kotlin.concurrent.thread { ... }` from `startNotify()` to + perform a blocking `httpClient.send(...)`, and reaches back to the EDT through + `invokeLater` for every console line. +- `ShellFeatureAdapter` catches exceptions and pushes the error text to the console via + `invokeLater` — even though the surrounding code is already on the EDT. + +Problems caused by this style: + +1. **No cancellation surface.** Closing the project does not interrupt in-flight HTTP + requests or pending `invokeLater` lambdas. `SessionStorage` keeps references to + `ProcessHandler`s and `CompletableFuture`s that may outlive the project. +2. **No structured concurrency.** Errors in callbacks are swallowed (e.g. the + `whenComplete` block in `HttpFeatureAdapter` only handles success/failure of the + request itself; an exception inside `printResponse` propagates to a fork-join thread). +3. **EDT correctness is hand-wavy.** `mountWrapperIntoContainer` is `invokeLater`-ed, + but every call site is already on the EDT (it is invoked from an inlay click handler + and from `collect()`), so the wrapper is sometimes added on the *next* UI tick — the + inlay refresh races with it. +4. **Inconsistent dispatch.** `embedContainerIntoEditor` schedules its add via + `invokeLater`, while the surrounding `run()` already mutates `Session.state` on the + EDT — so the container becomes visible *after* the inlay refresh has already + reported state `RUNNING`. +5. **No background thread for blocking IO.** `httpClient.send(...)` in `HttpRunState` + runs on a hand-rolled thread instead of `Dispatchers.IO`, so it is invisible to the + platform's progress / cancellation infrastructure. + +The IntelliJ Platform has provided a first-class coroutine integration since 2024.1 +(`Dispatchers.EDT`, `Dispatchers.Default`, suspending `readAction`/`writeAction`, +service-bound `cs: CoroutineScope`, `currentThreadCoroutineScope()` for actions). The +plugin targets `pluginSinceBuild = 251` (IDEA 2025.1), so every API needed for the +migration is available unconditionally. + +## All call sites + +Verified with `grep -rn "invokeLater" src --include='*.kt'`. The table separates +**imports** (4) from **real call sites** (8 lambdas, 9 if we count +`refreshInlays` which contains two consecutive UI calls inside the same +`invokeLater` block). Imports are removed as a side effect of replacing every +call inside the file. + +| # | File | Line | Kind | Context | +|---|------|------|------|---------| +| 1 | `src/main/kotlin/com/github/xepozz/inline_call/base/inlay/ExecutionInlayProvider.kt` | 26 | import | `com.intellij.openapi.application.invokeLater` | +| 2 | `src/main/kotlin/com/github/xepozz/inline_call/base/inlay/ExecutionInlayProvider.kt` | 237 | call | `mountWrapperIntoContainer` — add wrapper to result panel, revalidate, repaint | +| 3 | `src/main/kotlin/com/github/xepozz/inline_call/base/inlay/ExecutionInlayProvider.kt` | 255 | call | `delete()` — remove container from parent after session is dropped | +| 4 | `src/main/kotlin/com/github/xepozz/inline_call/base/inlay/ExecutionInlayProvider.kt` | 263 | call | `toggleCollapse()` — flip `container.isVisible` | +| 5 | `src/main/kotlin/com/github/xepozz/inline_call/base/inlay/ExecutionInlayProvider.kt` | 267–269 | call (+ early-return reference on 269) | `refreshInlays()` — `InlayHintsPassFactoryInternal.forceHintsUpdateOnNextPass()` + `DaemonCodeAnalyzer.restart()` | +| 6 | `src/main/kotlin/com/github/xepozz/inline_call/base/inlay/ui/embedContainerIntoEditor.kt` | 3 | import | — | +| 7 | `src/main/kotlin/com/github/xepozz/inline_call/base/inlay/ui/embedContainerIntoEditor.kt` | 20 | call | Add component to `EditorEmbeddedComponentManager` | +| 8 | `src/main/kotlin/com/github/xepozz/inline_call/feature/http/HttpFeatureAdapter.kt` | 11 | import | — | +| 9 | `src/main/kotlin/com/github/xepozz/inline_call/feature/http/HttpFeatureAdapter.kt` | 109 | call | Print error from `whenComplete` | +| 10 | `src/main/kotlin/com/github/xepozz/inline_call/feature/http/HttpFeatureAdapter.kt` | 114 | call | Print response from `whenComplete` | +| 11 | `src/main/kotlin/com/github/xepozz/inline_call/feature/shell/ShellFeatureAdapter.kt` | 13 | import | — | +| 12 | `src/main/kotlin/com/github/xepozz/inline_call/feature/shell/ShellFeatureAdapter.kt` | 63 | call | `catch (e: Exception)` — print failure to start process | +| 13 | `src/main/kotlin/com/github/xepozz/inline_call/feature/http/run/HttpRunState.kt` | 158 | call (fully-qualified) | `printToConsole` — every console line goes through `invokeLater` | + +Summary: **4 imports + 8 real call lambdas + 1 in-lambda early-return reference** +across 5 files. The often-quoted "12 sites" figure mixes imports and lambdas; +when the task ticket says "12 call sites" it is shorthand for "all 12 matches +returned by `grep -n invokeLater`" (the table above is exhaustive against that +grep). + +Related (not `invokeLater`, but coupled to the migration): + +- `HttpRunState.kt:51` — `kotlin.concurrent.thread { executeRequest() }` driven from + `startNotify()`. Should become a coroutine launched on the service scope (or on a + short-lived `RunProfileState` scope) using `Dispatchers.IO`. +- `HttpFeatureAdapter.kt:105` — `httpClient.sendAsync(...)` returning a + `CompletableFuture`. Worth converting to `withContext(Dispatchers.IO) { httpClient.send(...) }` + for uniformity with cancellation semantics. + +## Migration patterns + +### EDT switching + +Replace every `invokeLater { ... }` that mutates Swing / inlays with a suspending hop +inside an existing coroutine: + +```kotlin +withContext(Dispatchers.EDT) { + container.add(wrapper.component, BorderLayout.CENTER) + container.revalidate() + container.repaint() +} +``` + +Rules of thumb (from `topics/.../coroutine_dispatchers.md`): + +- `Dispatchers.EDT` is the **only** dispatcher that may touch Swing components, + inlays, editor presentation, or `DaemonCodeAnalyzer`. +- The dispatcher captures the right modality state. We do not need `ModalityState` + arguments when launching from a service scope: pass + `ModalityState.defaultModalityState().asContextElement()` if a click handler runs + inside a modal dialog. +- Never use `Dispatchers.Main` from `kotlinx-coroutines-swing` — it is not aware of + IDE modality. + +### Pooled / IO work + +For CPU-bound transformations (regex matching, JSON pretty-printing, +`computeMatches`): + +```kotlin +withContext(Dispatchers.Default) { ... } +``` + +For blocking IO (HTTP, file reads): + +```kotlin +withContext(Dispatchers.IO) { + httpClient.send(request, HttpResponse.BodyHandlers.ofString()) +} +``` + +`Dispatchers.IO` is the IntelliJ-flavored one — it is exposed as an extension +property `val Dispatchers.IO` on `kotlinx.coroutines.Dispatchers` from the +`intellij.platform.coroutines` module. (In code: just write `Dispatchers.IO` +without an explicit `import`; the kotlinx import resolves the receiver, the +platform module supplies the property.) It is unbounded and designed for +blocking calls. The fallback `kotlinx.coroutines.Dispatchers.IO` is bounded +(default `64`) and unsuitable for many concurrent blocking calls — but for our +use (at most a handful of HTTP requests) either is acceptable. + +### Long-running per-execution scope + +Each "Run" click should produce a child `Job` rooted in +`SessionStorage`'s scope so that: + +- Closing the project cancels every in-flight run. +- Clicking *Stop* cancels the run cooperatively (`Job.cancelAndJoin()`). +- Clicking *Rerun* cancels the previous job before starting a new one + (currently the previous `CompletableFuture` is left dangling on error paths). + +Suggested shape: + +```kotlin +session.job = sessionStorage.cs.launch(CoroutineName("inline-call:$key")) { + try { + feature.execute(match, wrapper, project) // suspend + } finally { + withContext(NonCancellable + Dispatchers.EDT) { + session.state = ExecutionState.FINISHED + refreshInlays(editor) + } + } +} +``` + +`FeatureGenerator.execute` should become a `suspend fun` returning a +`ProcessHandler?` (or `Unit` for fire-and-forget features), eliminating the +`onProcessCreated` callback. + +### Cancellation / structured concurrency + +- A coroutine launched inside `sessionStorage.cs` is cancelled automatically when + the project is disposed (the service scope is bound to the project lifecycle). +- For the `HttpFeatureAdapter`, instead of holding a `CompletableFuture` and + cancelling it from `ProcessHandler.destroyProcessImpl`, drive the run from a + coroutine; `destroyProcessImpl` calls `job.cancel()`, the suspending + `httpClient.send` is wrapped in `runInterruptible(Dispatchers.IO) { ... }` so + cancellation maps to thread interrupt. +- Avoid `GlobalScope` and `CoroutineScope(SupervisorJob())` entirely — both detach + from the IDE lifecycle. +- For UI mutations made during a coroutine that is being cancelled (e.g. final + console.print on failure), wrap them in `withContext(NonCancellable + Dispatchers.EDT)`. + +## Implementation steps + +Each step is intended as one atomic commit. Commits build on each other; tests should +pass at every step. + +### Step 1 — Add coroutine scopes to project services + +- Convert `SessionStorage` from `class SessionStorage(val project: Project)` to + `class SessionStorage(val project: Project, val cs: CoroutineScope)`. The platform + injects the scope when the constructor declares one (since 2024.1; we target 2025.1). +- Add a `Session.job: Job?` field (mutable, defaults to `null`) so we can cancel + an in-flight run. +- Expose helper: `fun replaceJob(key: String, job: Job): Job?` returning the + previously stored job (caller cancels it before launching a new one). +- `remove(key)` should also cancel `Session.job` if present (the existing + `try { destroyProcess() }` lives in `ExecutionInlayProvider.delete` — it + stays there; this step only adds the field and helper). +- **Scope is owned by the service**; we do *not* override `dispose()` to call + `cancelAll()` — the platform cancels the injected scope automatically when + the project is disposed (see *Service scope* section below). +- No behaviour change for callers yet (no coroutines launched). Compiles. + +> Cross-plan note: `SessionStorage` is also touched by plan 05 (key derivation, +> `SmartPsiElementPointer`-based anchor, file/editor-listener reaper). This +> plan deliberately limits itself to adding `cs: CoroutineScope` and +> `Session.job: Job?`. Keep the existing `getSession` / `putSession` / +> `remove` signatures unchanged so plan 05 can rebase cleanly. + +### Step 2 — Make `embedContainerIntoEditor` suspending + +- Convert `embedContainerIntoEditor.kt` from `invokeLater { addComponent(...) }` + to a `suspend fun` body that performs `withContext(Dispatchers.EDT) { ... }`. +- Remove the `invokeLater` import from `embedContainerIntoEditor.kt`. +- Guard against editor disposal inside the EDT block: + `if ((editor as? EditorEx)?.isDisposed != false) return@withContext`. +- A dedicated `ui/Edt.kt` helper module is **not** introduced — each call site + already calls `withContext(Dispatchers.EDT) { ... }` directly, and an extra + wrapper would only add a layer of indirection without saving any tokens. + +### Step 3 — Make `FeatureGenerator.execute` suspending + +- Change `FeatureGenerator.execute` signature: + + ```kotlin + suspend fun execute( + match: FeatureMatch, + wrapper: TWrapper, + project: Project, + ): ProcessHandler? + ``` + + The `onProcessCreated` callback disappears — the caller receives the handler + from the return value (or `null` when the feature has no process semantics or + failed to start). +- Update both adapters: + - `ShellFeatureAdapter`: wrap `OSProcessHandler(commandLine)` creation in + `withContext(Dispatchers.IO)` (constructor blocks on `Process.start()`). + On exception, switch to `Dispatchers.EDT` to print the error and return + `null`. Otherwise return the handler after `startNotify()`. + - `HttpFeatureAdapter`: rewrite to: + + ```kotlin + override suspend fun execute(...): ProcessHandler? { + // EDT: initial console.print calls happen on EDT. + val handler = HttpExecAdapterHandler() // ProcessHandler subclass + handler.startNotify() + try { + val response = runInterruptible(Dispatchers.IO) { + httpClient.send(request, HttpResponse.BodyHandlers.ofString()) + } + withContext(NonCancellable + Dispatchers.EDT) { + console.clear() + printResponse(console, response) + } + handler.complete(0) + } catch (ce: CancellationException) { + // Do not print "cancelled" — the caller controls UI feedback. + handler.complete(143) + throw ce + } catch (t: Throwable) { + withContext(NonCancellable + Dispatchers.EDT) { + console.print("[Error: ${t.message}]\n", ConsoleViewContentType.ERROR_OUTPUT) + } + handler.complete(-1) + } + return handler + } + ``` + + The custom `ProcessHandler` becomes much smaller — `destroyProcessImpl` + only calls `notifyProcessTerminated(143)`; the coroutine is cancelled by + the caller via `Session.job?.cancel()`. `CompletableFuture` and + `AtomicBoolean` disappear entirely. +- **Return value semantics.** When `ShellFeatureAdapter` fails to spawn, it + returns `null` (after printing the error). The caller treats `null` the same + way as a successful `ProcessHandler` that has already terminated — set state + to `FINISHED`, do not attach a listener. +- **Caller change** (in `ExecutionInlayProvider.run` — see step 4): replace the + old callback-based `feature.execute(match, wrapper, project) { handler -> ... }` + with `val handler = feature.execute(match, wrapper, project)` plus + `handler?.addProcessListener(...)`. + +### Step 4 — Rewrite `ExecutionInlayProvider.run/stop/delete/toggle/refresh` on coroutines + +- Replace each `invokeLater { ... }` body with a launch on `sessionStorage.cs` + that switches to `Dispatchers.EDT` (UI mutations require it). Click handlers + are entered on the EDT already, but the resulting coroutine outlives the + click and must therefore declare its dispatcher explicitly. +- The `run()` private function becomes: + + ```kotlin + private fun run(...) { + val newJob = sessionStorage.cs.launch(CoroutineName("inline-call:$key")) { + runImpl(...) + } + val prev = sessionStorage.putSession(key, currentSession.also { it.job = newJob }) + // sessionStorage.replaceJob(key, newJob) returns the previous Job + // before overwriting; the caller cancels it. + prev?.cancel() + } + + private suspend fun runImpl(...) { + withContext(Dispatchers.EDT) { + // wrapper / container / mount + session.state = ExecutionState.RUNNING + refreshInlaysOnEdt(editor) + } + val handler = feature.execute(match, wrapper, project) + session.processHandler = handler + if (handler == null) { + withContext(NonCancellable + Dispatchers.EDT) { + session.state = ExecutionState.FINISHED + refreshInlaysOnEdt(editor) + } + return + } + // Bridge ProcessListener -> CompletableDeferred so we can `await()`. + val done = CompletableDeferred() + handler.addProcessListener(object : ProcessListener { + override fun processTerminated(event: ProcessEvent) { done.complete(Unit) } + }) + try { + done.await() + } finally { + withContext(NonCancellable + Dispatchers.EDT) { + session.processHandler = null + session.state = ExecutionState.FINISHED + refreshInlaysOnEdt(editor) + } + } + } + ``` + + Notes: + - `replaceJob` is the cleaner helper exposed in step 1 (`sessionStorage.replaceJob(key, newJob)?.cancel()`). + - `refreshInlaysOnEdt` is a private non-suspending function: it expects to + already be on the EDT (we always are when invoked). +- `mountWrapperIntoContainer` → small helper that runs inline on the EDT + (already inside `withContext(Dispatchers.EDT)`). No more "schedules onto + next UI tick" behaviour. +- `delete(key)` becomes: + + ```kotlin + private fun delete(key: String) { + val session = sessionStorage.remove(key) ?: return + // Cancel the run if any. ProcessHandler.destroyProcess() is still + // useful for OSProcessHandler — the coroutine's finally block will + // see processTerminated → done.complete → coroutine exits. + try { session.processHandler?.destroyProcess() } catch (_: Throwable) {} + session.job?.cancel() + sessionStorage.cs.launch(Dispatchers.EDT) { + session.container?.parent?.remove(session.container) + } + } + ``` + + We do **not** `cancelAndJoin()` from `delete` — it is called from a click + handler on the EDT; blocking the EDT to wait for an HTTP call is exactly + what we're trying to avoid. +- `toggleCollapse(session)` becomes: + + ```kotlin + private fun toggleCollapse(session: Session) { + session.collapsed = !session.collapsed + val visible = !session.collapsed + sessionStorage.cs.launch(Dispatchers.EDT) { session.container?.isVisible = visible } + } + ``` + + (Already on EDT? Yes, but we keep the launch so we can hop back through the + coroutine context — and because the click handler may dispatch us inside a + modal popup where direct mutation needs the right modality state.) +- `refreshInlays(editor)` becomes: + + ```kotlin + private fun refreshInlays(editor: Editor) { + val project = editor.project ?: return + sessionStorage.cs.launch(Dispatchers.EDT) { + @Suppress("UnstableApiUsage") + InlayHintsPassFactoryInternal.forceHintsUpdateOnNextPass() + DaemonCodeAnalyzer.getInstance(project).restart() + } + } + ``` + + > Cross-plan note: `InlayHintsPassFactoryInternal` is removed by plan 01 + > (Phase 1 swaps it for `DaemonCodeAnalyzer.getInstance(project).restart(file)`). + > Plan 03 must **not** remove the call — only move it onto a coroutine + > dispatcher. Plan 01 lands the public-API switch independently. +- `processTerminated` listener inside `runImpl` runs on a process worker + thread; it only completes a `CompletableDeferred` and lets the suspending + coroutine resume — no UI work is done from the worker thread, so no + threading violation. + +### Step 5 — Convert `HttpRunState`/`HttpProcessHandler` + +- `HttpRunState` is a one-shot `RunProfileState` returning an `ExecutionResult`. + It cannot be a `suspend fun` (called from the platform). Inside + `HttpProcessHandler.startNotify()`, replace + `thread { executeRequest() }` with a launch on the project-scoped + `SessionStorage.getInstance(environment.project).cs` and store the + resulting `Job` on the handler so `destroyProcessImpl` cancels it. +- Wire-up: `HttpRunState.execute` constructs `HttpProcessHandler` with the + project (it has `environment.project`). The handler resolves + `SessionStorage` from that project lazily on `startNotify()`. +- Replace `httpClient.send(...)` with `runInterruptible(Dispatchers.IO) { ... }` + so cancellation interrupts the blocking call (JDK 21 — `send` throws + `InterruptedException`/`IOException` on interrupt). +- `printToConsole` no longer needs `invokeLater`. The surrounding coroutine is + on `Dispatchers.IO` while sending the request, and switches to + `Dispatchers.EDT` to print results. For headers + body output we do a + single `withContext(Dispatchers.EDT) { ... }` after the response is received + — printing line-by-line on the EDT does not change observable behaviour but + one bracket cuts log noise. +- `destroyProcessImpl` cancels the stored `Job` and calls + `notifyProcessTerminated(143)` once. `detachProcessImpl` does the same with + exit code `0`. The `finally` block in the coroutine still calls + `notifyProcessTerminated(0)` on natural completion; both paths must guard + with `AtomicBoolean` (existing pattern) so `notifyProcessTerminated` is + invoked exactly once. + +### Step 6 — `computeMatches` off the EDT + +(Stretch — not strictly an `invokeLater` site, but uncovered while reading +`ExecutionInlayProvider.collect`.) + +- `collect` is called by the inlay infrastructure under a read lock on a background + thread; the existing implementation is already safe but performs regex matching + for every visited element. Defer expensive work to: + + ```kotlin + smartReadAction(project) { computeMatches(file) } + ``` + + inside a coroutine started from a custom `InlayHintsCollector` or by moving the + computation into a separate `BackgroundableTask`. Out of scope for this migration, + but track as follow-up. + +### Step 7 — Remove the `invokeLater` import everywhere + +- After steps 1–5 no file imports `com.intellij.openapi.application.invokeLater`. +- Verify with `grep -rn 'invokeLater' src/main/kotlin` — must be empty. +- Adding a CI grep guard is **out of scope** for this PR; track as a follow-up + in `docs/plans/README.md` if desired. The reason it is deferred: this PR + already touches 5 files and a CI step is a separate concern that may need + team review of failure modes (e.g. tests or generated code that legitimately + call `invokeLater`). + +## Service scope (SessionStorage) + +`SessionStorage` is already a `@Service(Service.Level.PROJECT)`. The platform +supports injecting a `CoroutineScope` parameter into the constructor of a +`@Service`-annotated class; it is created from the parent project scope and +cancelled automatically when the project is disposed. + +Target shape: + +```kotlin +@Service(Service.Level.PROJECT) +class SessionStorage( + val project: Project, + val cs: CoroutineScope, +) { + private val sessions = ConcurrentHashMap() + + fun getSession(key: String): Session? = sessions[key] + fun putSession(key: String, s: Session) { sessions[key] = s } + fun remove(key: String): Session? = sessions.remove(key)?.also { it.job?.cancel() } + + /** + * Atomically store [job] on the session for [key] and return any + * previously-stored job so the caller can cancel it. + * Returns `null` if the session no longer exists. + */ + fun replaceJob(key: String, job: Job): Job? { + val s = sessions[key] ?: return null + val prev = s.job + s.job = job + return prev + } +} +``` + +Notes: +- `remove` now also cancels the stored `Job` — but this is a strict superset of + current behaviour (`remove` was only called from `delete(key)` which already + ran `processHandler?.destroyProcess()`). The `Job.cancel()` triggers the + coroutine's `finally` block, which calls `notifyProcessTerminated` exactly + once on the HTTP handler and lets `OSProcessHandler` run its normal + termination. No double-termination occurs because the handler is idempotent + (`AtomicBoolean terminated`). +- The cross-plan note in step 1 still applies: plan 05 expands this class with + per-file lookup helpers and a reaper listener. The `cs` field added here is + orthogonal and survives the plan-05 rewrite unchanged. + +Cancellation chain: + +- Project close → service scope `cs` cancelled → every active `runImpl` coroutine + cancelled → `runInterruptible` propagates cancellation to the IO thread → + HTTP request is interrupted → `finally` block on `NonCancellable + Dispatchers.EDT` + publishes the final state to the inlay (or skips if the editor is already disposed). +- Rerun click → `replaceJob(...)` returns previous, caller calls + `prev?.cancel()` before launching new coroutine. +- Stop click → `session.job?.cancel()`; the existing `destroyProcess()` on the + `OSProcessHandler` remains the canonical way to terminate the shell process — the + coroutine `join`s on the listener's `processTerminated` event. + +## Risks + +1. **Interop with `OSProcessHandler` and `ProcessListener`.** Process handlers + already run their I/O on the platform-managed `BaseDataReader` threads. The + coroutine wrapping must not race with `addProcessListener` — register the + listener before calling `startNotify()` (current code already does this) and + bridge `processTerminated` to a `CompletableDeferred` that the coroutine + awaits. Mistakes here can leak processes. + +2. **`InlayHintsProvider.collect` runs on a pooled read action.** Calling + `runBlocking` inside it is forbidden. Make sure no suspend function is invoked + from `collect` — only from click handlers (which run on the EDT and may launch + coroutines on `sessionStorage.cs`). + +3. **`EditorEmbeddedComponentManager.addComponent` requires the EDT *and* a valid + editor.** When the project closes mid-launch, the editor may already be disposed. + Wrap the `withContext(Dispatchers.EDT)` body in + `if (editor.isDisposed) return@withContext`. + +4. **`Dispatchers.EDT` vs `EDT(modalityState)`.** The default + `Dispatchers.EDT` uses the current coroutine context's modality. For inlay + refreshes triggered from a click in the editor, the default modality is + correct. For UI launched from inside a modal progress dialog, capture + `ModalityState.current().asContextElement()` at launch time. + +5. **`HttpRunState` is invoked by the Run framework, not by us.** Its `execute` + cannot suspend. The launched coroutine must outlive `execute()` and survive + until the process terminates. Anchoring it to `SessionStorage.cs` is fine as + long as the service is created lazily before `startNotify` is called — which it + is, because the run is initiated from a click handler that already touched + `SessionStorage`. + +6. **Behavioural change: `mountWrapperIntoContainer` was previously *async*.** Any + code that read `container.componentCount` immediately after calling + `mountWrapperIntoContainer` would have seen `0`. Search for such callers; the + migration makes it synchronous on the EDT. (`grep -n "componentCount" src` — + none expected.) + +7. **`runInterruptible` requires that the blocking call respects + `Thread.interrupt()`.** `java.net.http.HttpClient.send` does (it throws + `InterruptedException` wrapped in `HttpTimeoutException` / `IOException`). The + `catch (e: Exception)` branch in `HttpFeatureAdapter` must treat + `CancellationException` specially — *do not* print "Cancelled" to the console + if the cancellation came from project shutdown. + +8. **Kotlin coroutines version.** IntelliJ Platform 2025.1 bundles + `kotlinx-coroutines-core` 1.8.x with the platform classloader. The plugin must + not declare its own coroutines dependency; rely on the platform jar to avoid + classloader splits. Confirm `build.gradle.kts` does not introduce a transitive + `kotlinx-coroutines-core`. (Currently it does not.) + +9. **`@Service` constructor with `CoroutineScope`.** This is a platform-managed + parameter and only works on services annotated with `@Service`. The injection + is supported in `pluginSinceBuild = 251` (we target 251) — no compatibility + shim needed. + +10. **`smartReadAction` / `readAction` semantics.** If `computeMatches` is moved + off the EDT in step 6, it must hold a read action. PSI access without a read + action throws on background threads. + +## Verification + +- **Build & inspections:** + - `./gradlew build` succeeds. + - `./gradlew verifyPlugin` (IntelliJ Platform Gradle Plugin) reports no + threading violations. + - `grep -rn "invokeLater" src/main` returns no results. + +- **Existing unit tests:** + - `./gradlew test` — no regressions in `HttpFeatureAdapterTest`, + `ShellFeatureAdapterTest`, `ExecutionInlayProviderTest` (if present). + +- **New tests to add:** + - Coroutine cancellation: launch a "Run", project close → `Session.job.isCancelled` + is true within the test timeout. + - Rerun: clicking Run twice while the first is in flight must cancel the previous + `Job` (assert `prev.isCancelled`). + - `HttpFeatureAdapter` IO cancellation: stub `httpClient` with a slow handler, + cancel the job, assert no console writes happen after cancellation. + +- **Manual smoke (runIde):** + 1. Open `playground/` file with both `shell: echo hi` and `http://example.com` + comments. + 2. Click *Run* on shell, observe inlay state transitions IDLE → RUNNING → FINISHED. + 3. Click *Run* on HTTP, immediately click *Stop* — process handler should + terminate, inlay returns to FINISHED, no exceptions in `idea.log`. + 4. Click *Rerun*, then *Delete* — container is removed from the editor. + 5. Close the project while a request is in flight — `idea.log` shows + `JobCancellationException` (not a thrown `IOException`), no leaked threads in + thread dump. + +- **Threading audit:** + - Run IDE with `-Dide.slow.operations.assertion=true` and + `-Dide.slow.operations.assertion.fast.suspending=true`. Every former + `invokeLater` site should now produce no slow-operation warning. + - Run with `-Dintellij.platform.write.intent.checks=true` to ensure no EDT call + happens without a write-intent lock when required. + +- **Linter / CI guard:** + - Add a one-line CI step: `! grep -rn 'invokeLater' src/main/kotlin` to prevent + regressions. diff --git a/docs/plans/04-offset-mapping.md b/docs/plans/04-offset-mapping.md new file mode 100644 index 0000000..a69804c --- /dev/null +++ b/docs/plans/04-offset-mapping.md @@ -0,0 +1,403 @@ +# 04 — Real OffsetMapping for multi-line and decorated comments + +Status: refined after self-review. +Scope: `base/api/OffsetMapping.kt`, `base/api/BaseLanguageTextExtractor.kt`, +language extractors (`language/{php,xml,yaml}/*Extractor.kt`), feature +adapters (`feature/{shell,http}/*Adapter.kt`). +Out of scope: heredoc/nowdoc, string concatenation, template literals, +XML CDATA, kotlin `""" ... """`. + +## PSI facts verified before implementation + +- `com.jetbrains.php.lang.documentation.phpdoc.psi.PhpDocComment` + extends `com.intellij.psi.PsiDocCommentBase`, which in turn extends + `com.intellij.psi.PsiComment`. So the existing + `PsiTreeUtil.findChildrenOfAnyType(file, PsiComment::class.java)` walk + in `BaseLanguageTextExtractor.extract()` already yields PHPDoc nodes — + no PSI traversal changes needed. +- `com.jetbrains.php.lang.lexer.PhpTokenTypes` exposes both `LINE_COMMENT` + and `C_STYLE_COMMENT`. Per-element dispatch in PHPLanguageExtractor + branches on the PhpDocComment class first (it would otherwise also + match `C_STYLE_COMMENT` because the lexer wraps PHPDoc tokens too). +- `element.text.findTextRange(text)?.shiftRight(element.textOffset)` in + the current base extractor is a tautological `TextRange(0, len)` lookup + followed by a shift, equivalent to `element.textRange`. Plan keeps the + existing call for diff-minimality; cleanup is out of scope. + +## Problem + +`OffsetMapping.Identity` assumes `block.text[i]` corresponds 1:1 to +`document.getText(block.originalRange)[i]`. `BaseLanguageTextExtractor.extract()` +builds the block from `element.text` verbatim (with comment decoration) and +hard-codes `OffsetMapping.Identity`. Both adapters do +`startOriginal = base + block.mapping.toOriginal(m.offset)` — +correct only while the normalized text equals the original slice. + +This is correct today by accident: regexps (`shell:\s*(.+)`, the URL +pattern) match in the *middle* of comment text where decoration has already +been skipped by leading whitespace in the pattern itself, so no +normalization is needed and identity holds. The bug is *latent*: as soon as +we want any of the features below, identity breaks. + +1. **PHPDoc `/** ... */` with `*` line prefix**. To match `shell:` on a + line written `* shell: echo 1`, the natural fix is to strip + `^\s*\*\s?` from each interior line so the regexp sees clean + `shell: echo 1`. Then `m.offset` in the stripped text is N characters + ahead of the document position; identity points the inlay at a `*`. +2. **XML comments ``**. A URL at the very start + (``) needs `mapping.toOriginal(0) == 4`. Identity + highlights the opening bracket. +3. **YAML multi-line `#` blocks**. Several `#` lines glued into one block + need `# ` stripped from every line; offsets shift by `2 * line_index`. +4. **PHP `//cmd` (no space)** — works today by coincidence; relies on the + fact we do not strip `//`. The hidden coupling will fail the moment we + reuse the same normalization pipeline for PHPDoc. + +## Reproduction + +`playground/1.php`: + +```php +/** + * shell: echo 123 + */ +``` + +Steps: +1. Open in IDE; observe shell inlay on line 2. +2. The current matcher sees `* shell: echo 123` and matches starting at + offset 5 (after `* `). Identity maps 5 -> 5 in the document. The + document also has `* ` there, so the inlay lands at the right place by + coincidence. +3. To make this matcher robust we want to **strip the `* ` prefix** so + `shell:\s*(.+)` matches even if the author writes `*shell: cmd` (no + space). After stripping, `m.offset == 0` in normalized text but the + correct document offset is 5. Identity now points the inlay at `*`. + +Fast failure repro once stripping is enabled: + +```php +/** + * Line one + * shell: echo 42 + */ +``` + +Normalized: `Line one\nshell: echo 42`. Match at normalized offset 9; raw +text has `shell` at document offset 16 (3 for `/**`, 1 newline, 4 spaces + +`* `). Identity: 9. Correct: 16. + +`playground/1.yaml`: + +```yaml + # shell: echo 123 +``` + +Single line, identity holds. Adding a sibling `# https://example.com` +produces two independent `PsiComment`s; still fine. Identity breaks only +once we glue them into one block (Step 7) or once we strip `# ` from a +single line (Step 5). + +## Mapping model + +Requirements: +- `toOriginal(normalizedOffset)` cheap and correct for every offset the + matcher emits (match start and `start + value.length`). +- `toNormalized(originalOffset)` defined; clamp right when the original + offset lands on a stripped char. +- Must handle `normalizedOffset == text.length` (end sentinel). + +Chosen shape: **dense piecewise-linear mapping** built from a sorted list +of segments. Each segment is a contiguous run kept in both texts; gaps +between segments are stripped decoration in the *original* text. + +```kotlin +class SegmentedOffsetMapping( + private val segments: List, + private val normalizedLength: Int, + private val originalLength: Int, +) : OffsetMapping { + data class Segment(val normalizedStart: Int, val originalStart: Int, val length: Int) +} +``` + +- Sorted by `normalizedStart`; binary search is `O(log n)`. +- Inside segment `s`: `toOriginal(o) = s.originalStart + (o - s.normalizedStart)`. +- At `o == normalizedLength`: return + `lastSegment.originalStart + lastSegment.length` (or `originalLength` + when the mapping is empty). This makes `endOriginal = base + + toOriginal(start + length)` span exactly the matched substring even + when the original text continues with stripped decoration + (e.g. PHPDoc closer). +- `toNormalized` on a stripped char clamps to the next segment's + `normalizedStart`. + +### Length handling + +Today `endOriginal = startOriginal + m.value.length`. That stays correct +while each match lies inside one segment **and** `m.value` equals the +full match. Phase 1 patterns use `(.+)` (no newline), and all decoration +we strip is at line boundaries — so matches never cross gaps. Still, +**migrate adapters to** +`endOriginal = base + mapping.toOriginal(m.offset + m.value.length)` so +multi-line patterns are not a future trap. Add a debug assertion that +`endOriginal - startOriginal >= m.value.length`. + +Caveat (latent, separate fix): `RegexpMatcher.findMatches` builds each +`MatchResult` as `(matcher.start(), matcher.group(1).trim())`. For the +shell pattern `shell:\s*(.+)`, `group(0)` is `shell: cmd` and `group(1)` +is `cmd`, so `m.offset` points at `shell:` while `m.value.length` is +only the trailing command length. End offset arithmetic +`m.offset + m.value.length` therefore lands mid-match today regardless +of this plan. Plan 04 does **not** fix this: it routes both ends through +the mapping unchanged. A follow-up should align `m.offset` with +`group(1)` or emit `valueRange` directly; the current adapters render an +inlay only at `startOriginal`, so the wrong `endOriginal` is invisible +in the UI but will bite anything that highlights the matched range +(actions, intentions, navigation). + +### Why not a function or parallel `IntArray`s? + +Function form cannot answer the inverse direction; serialization-unfriendly; +hard to debug. Parallel `IntArray`s are faster, worth the swap later +if profiling demands it — the public API does not change. + +## Per-language strategy + +Each language extractor returns the raw `PsiComment` text; a per-language +`CommentNormalizer` strips decoration and emits both the normalized text +and the segment list. + +```kotlin +interface CommentNormalizer { fun normalize(rawText: String): NormalizedComment } +data class NormalizedComment(val text: String, val mapping: SegmentedOffsetMapping) +``` + +`BaseLanguageTextExtractor` ctor accepts a normalizer (default +`IdentityNormalizer`). + +### XML (``) + +Strip leading `` (3 chars). Optionally +strip one space after ``. One segment for the inner +range. Multi-line XML comments have no per-line decoration, so one segment +is enough; interior indentation stays in normalized text and the regexp +handles `\s*` itself. CDATA is not a `PsiComment` (separate plan). + +### PHP (`/** ... */`, `/* ... */`, `//`, `#`) + +Dispatch on PSI element class inside `PHPLanguageExtractor.extract()`: + +- `PhpDocComment` -> `CStyleBlockNormalizer(stripStarPrefix = true)` +- `PhpTokenTypes.C_STYLE_COMMENT` (`/* ... */`) -> `CStyleBlockNormalizer(stripStarPrefix = false)` +- line comment -> `SingleLineCommentNormalizer("//|#")` + +PHPDoc normalization: +1. Strip leading `/**` and any whitespace up to and including the first + `\n` (drop the `\n` too; first content line starts at normalized + offset 0). +2. Strip trailing whitespace + `*/`. +3. Each interior line: strip `^\s*\*\s?` — at most one space after `*`, + so indentation inside `@example` stays. +4. Emit each surviving `\n` as a 1-char segment between content segments. + +Example. Raw `"/**\n * Line one\n * shell: echo 42\n */"` becomes +normalized `"Line one\nshell: echo 42"` with segments (illustrative): + +``` +(0, 7, 8) // "Line one" +(8, 15, 1) // "\n" +(9, 20, 14) // "shell: echo 42" +``` + +Implementation: line-by-line scan, regex `^\s*\*\s?` anchored, two +cursors `originalCursor` and `normalizedCursor`. `\r\n` and `\n` both count +as one line terminator for stripping purposes; the original bytes still +contribute to `originalCursor` so document offsets stay accurate. + +`//` and `#`: strip leading marker plus at most one space. One segment. + +### YAML (`# ...`) + +`SingleLineCommentNormalizer("#")`. Strip leading `\s*#\s?`. One segment. + +**Do not glue** sibling `#` comments in Phase 1 — each `PsiComment` is one +block. Gluing is Step 7 (optional). + +### Adapter (fallback) + +`AdapterLanguageExtractor` uses `IdentityNormalizer`: single segment +`(0, 0, text.length)`. Behavior identical to today. + +## Implementation steps + +Each step is one commit; CI must pass at every step. + +### Step 1 — `SegmentedOffsetMapping` (no behavior change) + +Add `base/api/SegmentedOffsetMapping.kt` with `Segment`, binary-search +`toOriginal`/`toNormalized`, factory `SegmentedOffsetMapping.identity(length)`. +Unit tests in `SegmentedOffsetMappingTest`: +- identity round-trip; +- single-segment with leading gap (XML); +- multi-segment with per-line gaps (PHPDoc); +- boundary offsets (0, `length`, gap edges); +- `toNormalized` clamping on stripped chars. + +`OffsetMapping.Identity` stays (back-compat). + +### Step 2 — `CommentNormalizer` SPI + normalizers + +Files: `base/api/CommentNormalizer.kt`, `base/normalizers/{Identity,SingleLine,CStyleBlock,XmlComment}Normalizer.kt`. + +`SingleLineCommentNormalizer(markerRegex: String)`: strips +`^\s*(?:)\s?`. Note the non-capturing group around the marker — +the original `^\s*\s?` form has wrong precedence under +alternation (`//|#` would bind as `(^\s*//)|(#\s?)`). The normalizer +wraps the caller's marker in `(?:...)` to keep call sites simple. +`CStyleBlockNormalizer(stripStarPrefix: Boolean)`: handles `/* ... */` +and PHPDoc as described. +`XmlCommentNormalizer`: strips `` with one optional inner +space. + +Unit tests per normalizer with hand-written fixtures plus a property test: +for every preserved index `i`, `original[mapping.toOriginal(i)] == normalized[i]`. + +### Step 3 — Wire normalizer into `BaseLanguageTextExtractor` + +Change ctor to `BaseLanguageTextExtractor(normalizer: CommentNormalizer = IdentityNormalizer)`. +In `extract()` call `normalizer.normalize(element.text)` and pass +`normalized.text` / `normalized.mapping` into `ExtractedBlock`. +`AdapterLanguageExtractor` keeps the default; behavior unchanged. + +### Step 4 — Migrate adapters to map end-offsets via mapping + +In `ShellFeatureAdapter` and `HttpFeatureAdapter`: + +```kotlin +val startOriginal = base + block.mapping.toOriginal(m.offset) +val endOriginal = base + block.mapping.toOriginal(m.offset + m.value.length) +``` + +Debug assertion: `endOriginal - startOriginal >= m.value.length`. + +### Step 5 — Per-language wiring + +- `XmlLanguageExtractor`: pass `XmlCommentNormalizer` to base ctor. +- `YamlLanguageExtractor`: pass `SingleLineCommentNormalizer("#")`. +- `PHPLanguageExtractor`: override `extract()` to dispatch by PSI class + (PHPDoc / C-style / line) and pick the matching normalizer per element. + +### Step 6 — Tidy up Identity usages + +Mark `OffsetMapping.Identity` `@Deprecated("Use IdentityNormalizer")`. +Grep production code; no direct call sites must remain outside normalizers +and tests. + +### Step 7 — Optional: YAML comment gluing + +Detect contiguous same-column `PsiComment` siblings (no blank line, no +non-comment node between), build one block spanning all of them, normalize +each line. Defer until a real use case appears; touches the inlay +provider's per-comment anchoring assumption. + +## Test plan + +testData layout: + +``` +src/test/testData/extractors/ + php/ + line_comment.php + line_comment.expected.txt + block_comment.php + block_comment.expected.txt + phpdoc_single_line.php + phpdoc_multi_line.php + phpdoc_no_space_after_star.php + phpdoc_crlf.php + xml/ + single_line.xml + multi_line.xml + url_no_space.xml + yaml/ + single.yaml + indented.yaml +``` + +`*.expected.txt` records one block per fixture: + +``` +ORIGINAL: "/** shell: echo 1 */" +NORMALIZED: "shell: echo 1" +MATCH: feature=shell value="echo 1" + normalized=[0,13) original=[4,17) +``` + +Tests: + +1. **Normalizer tests** (`*NormalizerTest`): parametrized over fixtures; + assert normalized text and segment list. +2. **Extractor tests** (`PhpExtractorTest`, `XmlExtractorTest`, + `YamlExtractorTest`) on `BasePlatformTestCase`: load fixture, run + extractor, assert `block.text` and `block.originalRange` are correct. +3. **Adapter end-to-end** (`ShellAdapterMappingTest`, + `HttpAdapterMappingTest`): load fixture, run adapter, + assert `editor.document.getText(match.originalRange) == match.value` — + this is the invariant Identity violates. +4. **Property test** in `SegmentedOffsetMappingTest`: generate random + inputs with `*`/`#`/CRLF decoration, verify round-trip on every + preserved index plus clamping on every gap index. +5. **Regression** for `playground/1.php`: load file, assert two shell + matches at expected document offsets. + +No harness changes; `MyPluginTest` already drives the existing fixture +folder. Add new fixtures next to it. + +CRLF fixtures: do not check `phpdoc_crlf.php` in with literal `\r\n` — +local git autocrlf / `.gitattributes` configurations can rewrite it. +Construct the CRLF buffer in test code (`text.replace("\n", "\r\n")` on +a normal fixture) and feed it via `myFixture.configureByText`. Same +treatment for any YAML CRLF case if added later. + +## Compatibility with sibling plans + +- **01 (declarative inlay hints)**: provider reads + `match.originalRange.startOffset`, calls + `editor.document.getLineNumber(start)`, and addresses inlays via + `addInlineElement(offset, ...)`. As long as `startOriginal` lands on a + real document offset, the new mapping is transparent. No file in + `base/inlay/` is touched. +- **02 (run configurations)**: feature adapters' `execute()` body is + untouched; only `match()` end-offset arithmetic is rewritten. Run + configs that consume `match.value` see no change. +- **03 (coroutines)**: no `invokeLater`/coroutine site is modified. +- **05 (session storage key)**: key derivation uses + `editor.document.getLineNumber(start)`. The mapping makes `start` + *more* correct (no longer pointing at decoration), but on existing + passing fixtures the line number is identical — sessions stay sticky. + +## Risks + +- **Greedy `.+` and segment boundaries**. Phase 1 patterns never cross + `\n`, so matches never cross stripped gaps. If a future pattern uses + `(?s)` or `(.+?)\n.../s`, `m.value.length` can drift from + `endOriginal - startOriginal`. Mitigation: Step 4 already routes end + through the mapping; debug assertion catches regressions. +- **PHPDoc layouts in the wild are messy**. No space after `*`, multiple + spaces, deep indent inside `@example`. Regex `^\s*\*\s?` strips at most + one space; verify with Symfony / Laravel / Yii snippets in testData. +- **Performance**. Each `PsiComment` allocates a fresh + `SegmentedOffsetMapping`. Acceptable for typical files; swap to + `IntArray`-backed segments if profiling shows pressure. Single-segment + cases can intern an `IdentityMapping` per length cache. +- **YAML gluing (Step 7) touches the inlay provider**. The provider + anchors on a single `PsiComment`; a glued block spans multiple. Either + pick the first comment as the anchor or teach the provider about + multi-element blocks. Defer. +- **CRLF**. `PhpDocComment.text` on Windows-checked-out files contains + `\r\n`. Normalizer must treat `\r\n` and `\n` uniformly for stripping + but keep both bytes in `originalCursor`. Cover with `phpdoc_crlf.php`. +- **`toNormalized` contract**. For a stripped original offset we clamp + right. Document this; callers must not assume round-trip equality on + arbitrary inputs. diff --git a/docs/plans/05-session-storage-key.md b/docs/plans/05-session-storage-key.md new file mode 100644 index 0000000..ca1bf74 --- /dev/null +++ b/docs/plans/05-session-storage-key.md @@ -0,0 +1,508 @@ +# 05 — Stable session key and lifecycle + +Status: refined (post self-review) +Owner: inline-call plugin +Touches: `base/SessionStorage.kt`, `base/inlay/Session.kt`, +`base/inlay/ExecutionInlayProvider.kt`, new +`base/lifecycle/SessionLifecycleListener.kt`, +`base/lifecycle/SessionPsiChangeListener.kt`, +`base/lifecycle/SessionEditorFactoryListener.kt`, +`META-INF/plugin.xml` + +## Problem + +Current key derivation: + +```kotlin +fun makeKey(editor: Editor, featureId: String, line: Int): String = + "${editor.hashCode()}_${featureId}_$line" +``` + +This composite key is wrong on four dimensions: + +1. **Does not survive file reopen.** `editor.hashCode()` is the Swing/identity hash of + the live `Editor` instance. Closing the tab disposes the `Editor`; reopening the + same `VirtualFile` creates a new `Editor` with a different identity hash. The + orphaned `Session` stays in the project-level `ConcurrentHashMap` but no + future lookup will ever match it — the result panel still renders below the + line (because the inlay was added to the old editor) but the user-visible + button collapses back to `IDLE`. + +2. **Does not survive split editor / multiple windows.** Opening the same file + in a vertical split or in a detached window yields two distinct `Editor` + objects with distinct hash codes. Clicking *Run* in one split does not + change anything in the other — each pane has its own ghost session. + +3. **`line` is computed from the live document offset.** In + `buildActionsPresentation` we do + `val line = editor.document.getLineNumber(start)`. If the user types a new + line *above* the comment, every match below shifts down. The next + recomposition computes a *different* key for the *same* logical match, so + the existing `RUNNING` session becomes invisible and the button resets to + `IDLE` — while the underlying `ProcessHandler` is still alive and writing + into a detached panel. + +4. **No cleanup.** `SessionStorage` is a `@Service(PROJECT)` holding + `ConcurrentHashMap`. Nothing ever removes entries except the + explicit *Delete* button. Closing an editor, closing a file, closing the + project (until dispose), deleting the comment from source — all leak the + `Session`, its `JPanel`, its `Wrapper`, and potentially a live + `ProcessHandler`. + +## Desired semantics + +> "The same session" = the same logical *invocation site* in the user's source. + +Concretely: if the user has `// https://api.example.com` at line 10 and clicks +*Run*, then: + +- inserting a blank line above (now line 11) — **same session**, button stays + *Stop* / *Rerun*; +- closing and reopening the file — **same session** if the comment text is + unchanged and the process is still alive (or `FINISHED`); +- opening a vertical split — **both** splits show the same state, both *Stop* + buttons control the one process; +- editing the URL to `https://api.other.com` — **new** logical site → fresh + `IDLE` session; the old session (if still running) is auto-terminated + because its anchor is gone; +- deleting the comment — old session terminated and removed; +- closing the project — everything terminated. + +So the identity of a session is `(VirtualFile.url, featureId, normalized value)`, +not `(Editor instance, line)`. + +## Options considered + +### A) Key = `VirtualFile.url + featureId + hash(match.value)` + +Pros: +- Survives editor reopen, splits and additional windows (shared by file). +- Stable under any edit that doesn't change the comment payload itself + (inserting lines above, reformatting, renaming surrounding symbols). +- No PSI work — trivial to compute in `collect()`. + +Cons: +- Changing the URL/command text (`https://a` → `https://b`) yields a new key; + the old session orphans unless we actively reap it. +- Duplicate comments (same URL twice in the same file) collide. Need a + disambiguator (occurrence index within file). +- Moving the project to a different path changes `VirtualFile.url` → sessions + reset. Acceptable: rebuild is cheap, and we never had to persist anything. + +### B) Key = `VirtualFile.url + featureId + PSI startOffset` + +Same shape as (A) but uses offset instead of value hash. + +Pros: +- Disambiguates two identical comments naturally. + +Cons: +- Offset is *exactly* the thing that drifts under edits — equivalent to the + current `line`-based bug. Strictly worse than (A) for our use case. + +### C) `SmartPsiElementPointer` as the anchor + +Pros: +- IntelliJ tracks the PSI element across edits, including rename refactors + and most reformatters. The pointer resolves to the same element as long as + it physically exists. +- Naturally survives line shifts. + +Cons: +- Doesn't help across file reopen unless we re-acquire pointers on file open + (extra wiring with `FileEditorManagerListener`). +- After the comment is deleted, the pointer goes stale but the `Session` map + entry persists — still need a reaper. +- More expensive to maintain than a string key; we'd carry pointers in + values, not in keys (keys still need to be serializable for `Map` lookup). +- Doesn't naturally share state across split editors unless backed by a + project-level store anyway. + +### D) `Editor.putUserData(KEY, Map)` + +Pros: +- Automatically GC'd with the `Editor` — no leak, no listener wiring. +- Trivial implementation. + +Cons: +- Defeats the entire goal: state vanishes on reopen, splits don't share, + multi-window doesn't share. We'd be solving the leak by amplifying bug #1 + and bug #2. + +### E) `FileEditorManagerListener` + `EditorFactoryListener` as a janitor + +Not an alternative key strategy — a complementary lifecycle layer. Listens +for `fileClosed`, `editorReleased`, `selectionChanged`, `beforeDocumentChange` +and tells `SessionStorage` to: + +- terminate orphaned processes, +- dispose orphaned panels (the embedded `JComponent` is anchored to the + disposed `Editor` and would NPE on touch otherwise), +- evict entries whose anchor PSI is gone. + +Required regardless of which key option we pick. + +## Recommended approach + +**File-scoped key (A) + reaper listener (E), with `SmartPsiElementPointer` +inside the `Session` value as the anchor.** + +Key shape: + +``` +${virtualFile.url}::${featureId}::${valueHash}#${occurrenceIndex} +``` + +- `virtualFile.url` survives editor lifecycle and is the same across splits + and additional windows in the same project. +- `featureId` keeps `http` and `shell` matches with the same payload + separate. +- `valueHash` — stable hash of `FeatureMatch.value` (the normalized + command/URL text), not of the surrounding line. We keep this as a short + hex hash (`Integer.toHexString(value.hashCode())`) for compactness; the + paired `occurrenceIndex` and per-file scoping make accidental collisions + innocuous (two distinct values with the same hash in the same file with + the same `featureId`). See Risk #1 for the trade-off and the explicit + decision to *not* embed the raw value in the key. +- `occurrenceIndex` — 0-based index of this match among matches in the same + file with the same `(featureId, valueHash)`. Computed during + `computeMatches` while we already iterate the file. Disambiguates + duplicates without depending on offsets. Order is determined by + **global** ascending `originalRange.startOffset` across the whole file + (not per-PsiElement) so the index is deterministic regardless of which + `PsiElement` contains the match. + +Session value gains: + +```kotlin +data class Session( + val virtualFileUrl: String, + val featureId: String, + val valueHash: String, + val occurrenceIndex: Int, + var anchor: SmartPsiElementPointer?, // for liveness checks + val container: JPanel?, + var wrapper: Wrapper?, + var state: ExecutionState = ExecutionState.IDLE, + var processHandler: ProcessHandler? = null, + var collapsed: Boolean = false, +) +``` + +`anchor` is used only by the reaper to decide "does this session still have +a comment in the file?". It is NOT part of the key — the key stays a pure +string so `ConcurrentHashMap` lookup remains O(1) and free of PSI access +from `collect()`. + +Why this combo: +- Solves problems 1, 2, 3 (key independent of `Editor` and of physical line). +- Solves problem 4 via listener-driven cleanup, not via key cleverness. +- Keeps `collect()` pure-CPU (no PSI pointer dereference per repaint). +- Anchor-based reaping handles the "user deleted the comment while + `RUNNING`" corner case cleanly. + +## Implementation steps + +1. **Extend `Session`** with `virtualFileUrl`, `featureId`, `valueHash`, + `occurrenceIndex`, `anchor`. `container` is already nullable. Add + `fun isAlive(): Boolean` that resolves the anchor under a read action + (`SmartPsiElementPointer.element` requires read access) and returns + `element != null && element.isValid`. Make the new mutable fields + (`container`, `wrapper`, `state`, `processHandler`, `collapsed`, + `anchor`) `@Volatile` — `PsiTreeChangeListener` (EDT) and + `processTerminated` (worker thread) write concurrently; see Risk #4. + +2. **Rewrite `makeKey`** as a top-level helper on `(VirtualFile, FeatureMatch, + Int)` — drop the `Editor` parameter entirely: + + ```kotlin + fun makeKey(file: VirtualFile, featureId: String, valueHash: String, occ: Int): String = + "${file.url}::${featureId}::${valueHash}#${occ}" + + fun hashValue(value: String): String = + Integer.toHexString(value.hashCode()) // collision rate is fine for our scale + ``` + +3. **Compute `occurrenceIndex` in `computeMatches`.** After building + `matchesByElement`, flatten everything into a single list sorted by + ascending `originalRange.startOffset`. Walk it and assign each entry a + running 0-based index keyed by `(featureId, valueHash)`. Store the + result in an `IdentityHashMap` (sharing class with + `matchesByElement` so `collect()` can look it up by reference). + `FeatureMatch` stays unmodified — it's a public data class shared with + future plans. + +4. **`buildActionsPresentation`** no longer reads `editor.document.getLineNumber` + for key computation. Line is still needed for `lineEndOffset` (where to + mount the panel), but that's a UI concern, not an identity concern. + +5. **`run()` upgrade path.** When mounting the result container, we now need + to know which `Editor` to embed into. Two situations: + - Click came from one of N split editors — embed into *that* editor. The + other splits will pick up the session on next `collect()` and show + *Stop* / *Rerun*, but will not get their own panel. (Acceptable for v1; + see Risks.) + - File reopened with `FINISHED` session — `container` is non-null but its + parent `Editor` is disposed. Detect via `container.parent == null` or a + dispose flag; rebuild the panel into the new `Editor` and re-mount the + existing `wrapper.component`. + +6. **`SessionStorage` API broadening:** + + ```kotlin + fun getSession(key: String): Session? + fun putSession(key: String, session: Session) + fun remove(key: String): Session? + + // new + fun sessionsForFile(url: String): List + fun removeAllForFile(url: String): List + fun forEach(action: (String, Session) -> Unit) + ``` + + Backing map can stay `ConcurrentHashMap`. The + per-file queries scan; volume is small (one session per Run-click, not + per match), so an O(N) scan is fine. If it isn't, add a secondary + `ConcurrentHashMap>` (url → keys). + +7. **`FileEditorManagerListener` reaper** (new class + `base/lifecycle/SessionLifecycleListener.kt`, wired via + `` in `plugin.xml`, topic + `FileEditorManagerListener.FILE_EDITOR_MANAGER`): + + - `fileClosed(source, file)` — **first check** `source.getAllEditors(file)`. + If the file is still open in another tab/window of the same project, + do nothing (split close, not file close). Otherwise, for each session + with that url: + - if `state == RUNNING` → call `processHandler?.destroyProcess()` and + set `state = FINISHED`. **Do not remove the entry** — user may + reopen and want to see the result. + - if `state == IDLE` → remove the entry; it never ran, nothing to + preserve. + - dispose the `JPanel` (it's anchored to a dead `Editor`), null out + `container`. Next reopen will rebuild it from the still-attached + `wrapper`. + - `fileOpened(file)` — no-op; `collect()` will naturally restore the + buttons via key match. + +8. **`EditorFactoryListener.editorReleased`** — covers the split-close case + without closing the underlying file. If this was the *only* editor for + the file, behave like `fileClosed`. Otherwise just unbind any panel + anchored to that specific editor (panels are 1-per-session today, so + typically nothing to do). + +9. **PSI-deletion reaper.** New `SessionPsiChangeListener` extending + `PsiTreeChangeAdapter`, registered via + `PsiManager.getInstance(project).addPsiTreeChangeListener(listener, + sessionStorage)` so it disposes with the project service. On + `childRemoved` / `childReplaced`, derive the affected file (`event.file + ?: event.parent?.containingFile`), then iterate sessions for that + file's url and check `session.isAlive()` under a read action. For any + dead anchor: + - terminate the process, + - dispose the panel, + - `sessionStorage.remove(key)`. + + This is also where the "user edited the URL" case is handled: the old + anchor still points to the same `PsiComment`, but `collect()` on the + next pass will compute a new `valueHash` and therefore a new key. The + old key has no matching match anymore — sweep it via "session has no + match in latest `computeMatches`" check (step 10), not via PSI delete. + +10. **Stale-key sweep at end of `computeMatches`.** Cheap defence in depth: + after computing all matches and `occurrenceIndex` values for the file, + build the set of *valid* keys for this file and call + `sessionStorage.sweepFile(url, validKeys)` which, for each session + keyed by `url` not present in `validKeys`, terminates the process and + disposes the panel before removing the entry. Runs on each inlay + pass; the per-file scan is cheap (one session per Run-click, not per + match). The sweep MUST be confined to keys for this single file's + `url` — never touch sessions belonging to other files. + +11. **`SessionStorage.dispose()`** — `SessionStorage` is a + `@Service(PROJECT)` and therefore already disposed with the project. + Implement `Disposable` and in `dispose()` terminate every session's + process and drop the map. This is the last-resort cleanup; we don't + need a separate `ProjectManagerListener.projectClosing` because + project disposal disposes project services. + +12. **Delete `makeKey(editor, featureId, line)`** signature, update all + call sites. Compile error will catch any miss. + +### Listener registration (plugin.xml) + +```xml + + + +``` + +`PsiTreeChangeListener` and `EditorFactoryListener` are wired +programmatically inside `SessionStorage` (or a tiny "starter" service +triggered on project open) because they need a `Disposable` parent +parameter and benefit from sharing the `SessionStorage` instance. The +`SessionStorage` service serves as the `Disposable` parent so that +unregistration is automatic on project close. + +## Cross-plan dependencies + +- **Plan 03 (coroutines).** Plan 03 will convert `SessionStorage` into + `class SessionStorage(project, cs: CoroutineScope)` and add `Session.job: + Job?`. To minimise merge friction this plan deliberately leaves the + primary constructor untouched and adds *all* new mutable fields as `var` + so plan 03 can append `var job: Job? = null` without rewriting any field + in the data class. The `dispose()` we add here will, after plan 03, + cancel the `cs` as well; for now it just terminates processes. +- **Plan 01 (declarative inlay hints).** Plan 01 owns the inlay UI + pipeline. This plan only changes `makeKey` and the listener wiring + inside `ExecutionInlayProvider`; the per-state UI builder code remains + identical bytes-wise. +- **Plan 02 (run configurations).** No overlap. Run configurations don't + touch `SessionStorage`. +- **Plan 04 (offset mapping).** This plan reads `FeatureMatch.value` (the + normalised payload) which is exactly what plan 04 strengthens. Once + plan 04 lands, `valueHash` becomes even more stable across + PHPDoc-comment-style reformatting; no API change needed here. + +## Migration + +There is no persisted state — `SessionStorage` is in-memory only and dies with +the project. A plugin upgrade naturally drops all sessions, which is the +desired behaviour: nothing to migrate, nothing to corrupt. + +If at some future point sessions become persistent (e.g. result history +across restarts), the file-scoped key from this plan serializes fine: +`url::feature::hash#occ` is a stable string. The PSI anchor would not be +persistable and would have to be re-acquired on project open by re-running +`computeMatches` for each cached key's file. + +In-flight processes during upgrade: plugin reload kills the entire JVM +classloader scope for the plugin. Any `ProcessHandler` we hold disappears +along with us, and the spawned OS processes inherit nobody. This is a +pre-existing condition, not new with this change. Document it as known +limitation; do not attempt to re-attach on plugin reload. + +## Lifecycle rules + +| Event | Behaviour | +| --- | --- | +| **File closed while session is RUNNING** | Process is **terminated**, `state` set to `FINISHED`, `container` disposed, session kept in storage. Rationale: user expects "close the tab, stop the work"; standard IDE behaviour for run tools. Keeping the entry lets reopen show a *Rerun* button immediately. | +| **File closed while session is FINISHED** | Container disposed, session kept in storage. Reopen rebuilds the panel from the cached `wrapper` (still holding the output). | +| **File closed while session is IDLE** | Session removed (it never ran, nothing to preserve). | +| **File reopened (any state)** | `collect()` finds the matching key, shows the right button. On *Rerun*, `run()` detects null/orphan `container`, rebuilds it into the new editor, re-mounts the existing wrapper component if possible (else a fresh wrapper). | +| **Editor split (same file, second pane)** | Both panes share the session via the file-scoped key. The result panel exists in whichever editor first mounted it (typically the one where the user clicked *Run*). The other pane shows *Stop* / *Rerun* with no inline panel — clicking *Stop* affects the shared process. v1 limitation: no duplicate panel in the second pane (see Risks). | +| **Second IDE window for same file** | Same project → same `SessionStorage` → same behaviour as split. Different project (same file opened via "Open in New Window" into a different project) → different `SessionStorage`, sessions are independent. Acceptable. | +| **Comment deleted from file** | `PsiTreeChangeListener` detects the missing anchor → terminate process → dispose panel → `remove(key)`. Next `collect()` pass would also evict it via the stale-key sweep (defence in depth). | +| **Comment edited (URL/command changed)** | New `valueHash` → new key. The stale-key sweep at the end of `computeMatches` finds the old key has no current match → terminate process → dispose panel → remove. The new key shows up as `IDLE`. | +| **Comment moved (cut here, paste 20 lines down)** | If done as a single PSI-level move, `SmartPsiElementPointer` may follow it; the key is value-based so it stays the same regardless. Session preserved. If done as delete+retype, see "comment deleted" then "new IDLE session appears". | +| **Project closed** | `projectClosing` terminates every process, then service disposal drops the map. | +| **Plugin reloaded / IDE restarted** | All sessions lost (in-memory storage). Processes spawned by the plugin become detached OS processes; pre-existing limitation. | +| **Duplicate identical comments in one file** | `occurrenceIndex` (0, 1, 2…) disambiguates. Order is determined by `originalRange.startOffset` during `computeMatches` so it's stable across edits as long as relative order of matches doesn't change. Reordering identical matches will swap their sessions — acceptable edge case; the user would have to deliberately reorder identical URLs. | + +## Risks + +1. **Hash collisions on `valueHash`.** `String.hashCode()` collisions in a + single file are vanishingly rare for realistic URL/shell payloads, but a + hostile or unlucky case would alias two sessions. Mitigation: use full + string in the key (`...::${value}#${occ}`) instead of a hash. Cost: key + string length; benefit: zero collisions. Recommend going full-string + unless we ever embed sessions into log lines where length matters. + +2. **`occurrenceIndex` instability under reorder.** If the user swaps two + identical lines, their sessions swap too. Trade-off is intrinsic to any + index-disambiguated scheme; PSI-pointer disambiguation would fix it at + the cost of much more bookkeeping. Acceptable for v1. + +3. **Split editor: panel only in one pane.** Inlay model supports adding the + same inline element in both editors, but `EditorEmbeddedComponentManager` + binds the component to one specific editor. Fully replicating panels + across splits requires per-editor `JPanel` instances driven from a shared + `Wrapper` model — bigger refactor. v1: button state shared, panel + single-pane. + +4. **Reaper races.** `PsiTreeChangeListener` fires on EDT; `processTerminated` + fires on a process worker thread. Concurrent `state` updates on the + `Session` are already racy in current code. Add `@Volatile` to mutable + `Session` fields and confine all map mutations to `SessionStorage`'s + `ConcurrentHashMap` calls. + +5. **`VirtualFile.url` for in-memory / scratch files.** Light virtual files + produce synthetic URLs that change across sessions; some are + `temp:///...`. Sessions in scratches won't survive IDE restart. Document; + not worth fighting. + +6. **Renamed / moved files.** `VirtualFile.url` changes on move; pointer + identity does not. Sessions detach on file rename. Optional follow-up: + subscribe to `VirtualFileListener.fileMoved` and rewrite keys. Probably + not worth it — the user can just click *Rerun*. + +## Verification + +Manual scenarios; each should be reproduced before merging. + +1. **Line shift.** + - Open a PHP file with `// https://example.com/users` on line 10. + - Click *Run*; verify *Stop* appears. + - Press Enter at line 1 five times. + - Expected: button on line 15 is still *Stop* (or *Rerun* if finished), + panel is still mounted under the comment. + +2. **File reopen, FINISHED.** + - Run a quick request, wait for *Rerun* to appear. + - Close the editor tab. + - Reopen the same file. + - Expected: *Rerun* (and *Delete*) buttons present, panel re-renders + beneath the comment with the previous response body. + +3. **File reopen, RUNNING.** + - Run a long shell command (`sleep 30`). + - Close the editor tab. + - Expected: command is **killed** within ~1s (`ps` shows no `sleep`). + - Reopen file → button shows *Rerun*, panel shows whatever output was + captured before termination. + +4. **Split editor.** + - Open file, run a long shell command. + - Right-click tab → Split Right. + - Expected: both panes show *Stop* at the comment. + - Click *Stop* in either pane → process dies, both panes show *Rerun*. + +5. **Comment deleted while running.** + - Run `sleep 30`. + - Select the comment line and delete it. + - Expected: process killed within ~1s, no orphan panel, no inlay. + +6. **Comment edited.** + - Run for `https://a.example.com`. + - Edit URL to `https://b.example.com`. + - Expected: old session terminated, panel removed, new *Run* button + appears for the new URL, old session entry evicted from storage + (verify via debug log of `sessionStorage` size). + +7. **Duplicate comments.** + - Put two identical `// shell: echo hi` lines in one file. + - Run the first only. + - Expected: first shows *Rerun*, second still shows *Run*. Run the + second, both have independent panels. + +8. **Multiple windows, same project.** + - Window > New Window for the same project. + - Open the same file in both. + - Run in window A. + - Expected: window B's button updates to *Stop* on next inlay refresh + (may need a keystroke to trigger). + +9. **Project close with active process.** + - Run `sleep 60`. + - Close project. + - Expected: `sleep` process gone within ~1s; no leaked Swing components + in `ReferenceQueue` (verify with `Diagnostics > Find Memory Leaks` in + IDE devkit). + +10. **IDLE not persisted.** + - Open file with five matchable comments; click none. + - Close file. + - Expected: `sessionStorage.sessionsForFile(url)` returns empty list + (verify via temporary log). diff --git a/docs/plans/README.md b/docs/plans/README.md new file mode 100644 index 0000000..4614f2e --- /dev/null +++ b/docs/plans/README.md @@ -0,0 +1,13 @@ +# Out-of-scope plans + +Эти задачи были вынесены за рамки PR с гигиеническими правками +(см. ветку `claude/plugin-capabilities-analysis-u3S7Y`). Каждый файл — +независимый план изменения; берётся в работу отдельным PR. + +| Файл | Тема | +|------|------| +| `01-declarative-inlay-hints.md` | Миграция с `InlayHintsProvider` + `InlayHintsPassFactoryInternal` на declarative API (или Code Vision) | +| `02-run-configurations.md` | Регистрация `HttpRunConfigurationType` / `ShellRunConfigurationType` после починки `SettingsEditor` биндингов | +| `03-coroutines-migration.md` | Замена `invokeLater` / `Task.Backgroundable` на корутины (`Dispatchers.EDT`, `withContext`, `readAction`) | +| `04-offset-mapping.md` | Реальная реализация `OffsetMapping` для языков с экранированием/heredoc/конкатенацией | +| `05-session-storage-key.md` | Стабильный ключ `SessionStorage` (сейчас `editor.hashCode()` теряется при reopen) | diff --git a/src/main/kotlin/com/github/xepozz/inline_call/base/api/BaseLanguageTextExtractor.kt b/src/main/kotlin/com/github/xepozz/inline_call/base/api/BaseLanguageTextExtractor.kt index 1f7255b..b9f900d 100644 --- a/src/main/kotlin/com/github/xepozz/inline_call/base/api/BaseLanguageTextExtractor.kt +++ b/src/main/kotlin/com/github/xepozz/inline_call/base/api/BaseLanguageTextExtractor.kt @@ -1,26 +1,45 @@ package com.github.xepozz.inline_call.base.api +import com.github.xepozz.inline_call.base.normalizers.IdentityNormalizer import com.intellij.psi.PsiComment +import com.intellij.psi.PsiElement import com.intellij.psi.PsiFile import com.intellij.psi.util.PsiTreeUtil import com.intellij.util.text.findTextRange -abstract class BaseLanguageTextExtractor : LanguageTextExtractor { +/** + * Walks every [PsiComment] in [file] and emits one [ExtractedBlock] per + * element. The block's `text` and `mapping` are produced by the + * [normalizer]; the `originalRange` always spans the whole raw comment + * so adapters can map normalized offsets back into the document via + * `block.originalRange.startOffset + block.mapping.toOriginal(...)`. + * + * Subclasses that need per-element dispatch (PHPDoc vs `//` vs `#`) + * should override [normalizerFor] instead of [extract]. + */ +abstract class BaseLanguageTextExtractor( + private val defaultNormalizer: CommentNormalizer = IdentityNormalizer, +) : LanguageTextExtractor { + + /** + * Picks the normalizer for the given PSI element. Defaults to the + * constructor-supplied [defaultNormalizer]; override to dispatch on + * element type / class. + */ + protected open fun normalizerFor(element: PsiElement): CommentNormalizer = defaultNormalizer + override fun extract(file: PsiFile): List = PsiTreeUtil .findChildrenOfAnyType(file, PsiComment::class.java) .mapNotNull { element -> val text = element.text val textRange = element.text.findTextRange(text)?.shiftRight(element.textOffset) - - if (textRange == null) { - null - } else { - ExtractedBlock( - element = element, - originalRange = textRange, - text = text, - mapping = OffsetMapping.Identity - ) - } + ?: return@mapNotNull null + val normalized = normalizerFor(element).normalize(text) + ExtractedBlock( + element = element, + originalRange = textRange, + text = normalized.text, + mapping = normalized.mapping, + ) } -} \ No newline at end of file +} diff --git a/src/main/kotlin/com/github/xepozz/inline_call/base/api/CommentNormalizer.kt b/src/main/kotlin/com/github/xepozz/inline_call/base/api/CommentNormalizer.kt new file mode 100644 index 0000000..c7e1366 --- /dev/null +++ b/src/main/kotlin/com/github/xepozz/inline_call/base/api/CommentNormalizer.kt @@ -0,0 +1,29 @@ +package com.github.xepozz.inline_call.base.api + +/** + * Strips per-language comment decoration (markers, leading stars, + * indentation) and emits the surviving text alongside the offset + * mapping that ties every normalized character back to its original + * position inside the raw comment text. + * + * Implementations must satisfy, for every preserved index `i`: + * + * normalized.text[i] == rawText[mapping.toOriginal(i)] + * + * and `mapping.toOriginal(normalized.text.length) == rawText.length`. + * + * Implementations are pure functions of [rawText] and must not call + * into PSI; the per-element dispatch happens in the extractor. + */ +interface CommentNormalizer { + fun normalize(rawText: String): NormalizedComment +} + +/** + * The result of [CommentNormalizer.normalize]: the normalized text plus + * the mapping back to the original offsets inside the raw comment. + */ +data class NormalizedComment( + val text: String, + val mapping: SegmentedOffsetMapping, +) diff --git a/src/main/kotlin/com/github/xepozz/inline_call/base/api/ExtractedBlock.kt b/src/main/kotlin/com/github/xepozz/inline_call/base/api/ExtractedBlock.kt index 7cfbc14..d2fdfa3 100644 --- a/src/main/kotlin/com/github/xepozz/inline_call/base/api/ExtractedBlock.kt +++ b/src/main/kotlin/com/github/xepozz/inline_call/base/api/ExtractedBlock.kt @@ -5,10 +5,16 @@ import com.intellij.psi.PsiElement /** * A block of text extracted from PSI that can be processed by features. + * `text` is the normalized comment body; `mapping` ties every offset in + * `text` back to a document offset relative to `originalRange.startOffset`. + * + * The default mapping is a degenerate identity sized to the supplied + * text — convenient for synthetic blocks in tests; production + * extractors always pass a real [SegmentedOffsetMapping]. */ data class ExtractedBlock( val element: PsiElement, val originalRange: TextRange, val text: String, - val mapping: OffsetMapping = OffsetMapping.Identity, + val mapping: OffsetMapping = SegmentedOffsetMapping.identity(text.length), ) diff --git a/src/main/kotlin/com/github/xepozz/inline_call/base/api/FeatureGenerator.kt b/src/main/kotlin/com/github/xepozz/inline_call/base/api/FeatureGenerator.kt index ba54803..9cda300 100644 --- a/src/main/kotlin/com/github/xepozz/inline_call/base/api/FeatureGenerator.kt +++ b/src/main/kotlin/com/github/xepozz/inline_call/base/api/FeatureGenerator.kt @@ -22,12 +22,27 @@ interface FeatureGenerator { /** * Execute the given match and stream output to the provided wrapper. + * + * Implementations must start their own background work (process, async + * IO, etc.) and deliver the [ProcessHandler] (or `null` on failure) + * via [onProcessCreated]. The callback is invoked on the same thread + * that started the work, OR off-thread if the implementation defers + * the handler creation. UI code is responsible for marshalling back + * to EDT in the callback when needed. + * + * This signature is intentionally NOT `suspend`. An earlier attempt + * to wrap the call chain in coroutines deadlocked in production + * click paths because `withContext(Dispatchers.EDT/IO)` from a + * coroutine launched on the service-scope's default dispatcher + * never resumed. The callback shape mirrors the platform's own + * `ProcessHandler` / `Task.Backgroundable` paradigm and does not + * require any dispatcher juggling. */ fun execute( match: FeatureMatch, wrapper: TWrapper, project: Project, - onProcessCreated: (ProcessHandler?) -> Unit = {} + onProcessCreated: (ProcessHandler?) -> Unit = {}, ) fun createWrapper(): TWrapper @@ -37,6 +52,5 @@ interface FeatureGenerator { fun getApplicable(project: Project): List> = EP_NAME.getExtensions(project).filter { it.isEnabled(project) } - } -} \ No newline at end of file +} diff --git a/src/main/kotlin/com/github/xepozz/inline_call/base/api/OffsetMapping.kt b/src/main/kotlin/com/github/xepozz/inline_call/base/api/OffsetMapping.kt index 80e28c0..4bf07ec 100644 --- a/src/main/kotlin/com/github/xepozz/inline_call/base/api/OffsetMapping.kt +++ b/src/main/kotlin/com/github/xepozz/inline_call/base/api/OffsetMapping.kt @@ -2,12 +2,22 @@ package com.github.xepozz.inline_call.base.api /** * Maps between normalized text offsets and original PSI text offsets. - * For Phase 1 we keep identity mapping; extractors may upgrade it later. + * + * The production implementation is [SegmentedOffsetMapping]; the legacy + * [Identity] singleton is kept only for binary compatibility and for + * tests that need a trivial mapping with no length context. */ interface OffsetMapping { fun toOriginal(normalizedOffset: Int): Int fun toNormalized(originalOffset: Int): Int + @Deprecated( + "Use SegmentedOffsetMapping.identity(length) or IdentityNormalizer instead", + ReplaceWith( + "SegmentedOffsetMapping.identity(length)", + "com.github.xepozz.inline_call.base.api.SegmentedOffsetMapping", + ), + ) object Identity : OffsetMapping { override fun toOriginal(normalizedOffset: Int) = normalizedOffset override fun toNormalized(originalOffset: Int) = originalOffset diff --git a/src/main/kotlin/com/github/xepozz/inline_call/base/api/SegmentedOffsetMapping.kt b/src/main/kotlin/com/github/xepozz/inline_call/base/api/SegmentedOffsetMapping.kt new file mode 100644 index 0000000..12a75df --- /dev/null +++ b/src/main/kotlin/com/github/xepozz/inline_call/base/api/SegmentedOffsetMapping.kt @@ -0,0 +1,160 @@ +package com.github.xepozz.inline_call.base.api + +/** + * Piecewise-linear [OffsetMapping] backed by a sorted list of preserved + * segments. A *segment* is a contiguous run of characters kept verbatim + * in both the normalized and the original text; the gaps between + * segments in the original text are stripped decoration + * (PHPDoc opener, leading `* `, XML `` delimiters of an XML comment, plus at most + * one space immediately after the opener and before the closer. Interior + * whitespace and indentation are preserved verbatim — the matcher + * regexes already handle `\s*` themselves, and stripping inner indent + * would lose offsets without buying anything. + * + * Multi-line XML comments have no per-line decoration, so the result is + * always a single segment. + */ +object XmlCommentNormalizer : CommentNormalizer { + private const val OPEN = "" + + override fun normalize(rawText: String): NormalizedComment { + if (!rawText.startsWith(OPEN) || !rawText.endsWith(CLOSE) || rawText.length < OPEN.length + CLOSE.length) { + // Defensive: not a well-formed XML comment text, behave like identity. + return IdentityNormalizer.normalize(rawText) + } + var start = OPEN.length + var end = rawText.length - CLOSE.length + if (start < end && rawText[start] == ' ') start++ + if (start < end && rawText[end - 1] == ' ') end-- + val length = end - start + val text = rawText.substring(start, end) + val segments = if (length == 0) { + emptyList() + } else { + listOf(SegmentedOffsetMapping.Segment(0, start, length)) + } + return NormalizedComment( + text = text, + mapping = SegmentedOffsetMapping(segments, length, rawText.length), + ) + } +} diff --git a/src/main/kotlin/com/github/xepozz/inline_call/feature/http/HttpFeatureAdapter.kt b/src/main/kotlin/com/github/xepozz/inline_call/feature/http/HttpFeatureAdapter.kt index 3b8d5fa..8843cd2 100644 --- a/src/main/kotlin/com/github/xepozz/inline_call/feature/http/HttpFeatureAdapter.kt +++ b/src/main/kotlin/com/github/xepozz/inline_call/feature/http/HttpFeatureAdapter.kt @@ -8,7 +8,7 @@ import com.intellij.execution.process.ProcessHandler import com.intellij.execution.ui.ConsoleView import com.intellij.execution.ui.ConsoleViewContentType import com.intellij.icons.AllIcons -import com.intellij.openapi.application.invokeLater +import com.intellij.openapi.application.ApplicationManager import com.intellij.openapi.project.Project import com.intellij.openapi.util.TextRange import java.net.URI @@ -22,7 +22,9 @@ import java.util.regex.Pattern import javax.swing.Icon /** - * Feature adapter that delegates matching and execution to existing HttpExecutionHandler. + * HTTP feature adapter. Async via [HttpClient.sendAsync] (no coroutines — + * see [com.github.xepozz.inline_call.base.api.FeatureGenerator.execute] + * for why the suspend chain was reverted). */ class HttpFeatureAdapter(val project: Project) : FeatureGenerator { override val id: String = "http" @@ -41,7 +43,10 @@ class HttpFeatureAdapter(val project: Project) : FeatureGenerator val startOriginal = base + block.mapping.toOriginal(m.offset) - val endOriginal = startOriginal + m.value.length + val endOriginal = base + block.mapping.toOriginal(m.offset + m.value.length) + assert(endOriginal - startOriginal >= m.value.length) { + "endOriginal($endOriginal) - startOriginal($startOriginal) < value.length(${m.value.length})" + } FeatureMatch( featureId = id, block = block, @@ -56,12 +61,10 @@ class HttpFeatureAdapter(val project: Project) : FeatureGenerator Unit + onProcessCreated: (ProcessHandler?) -> Unit, ) { val value = match.value val console = wrapper.console - val bodyConsole = wrapper.bodyConsole - val headersConsole = wrapper.headersConsole console.print("GET $value\n", ConsoleViewContentType.SYSTEM_OUTPUT) console.print("Connecting...\n\n", ConsoleViewContentType.LOG_INFO_OUTPUT) @@ -79,7 +82,6 @@ class HttpFeatureAdapter(val project: Project) : FeatureGenerator - console.clear() if (throwable != null) { - invokeLater { + ApplicationManager.getApplication().invokeLater { console.print("[Error: ${throwable.message}]\n", ConsoleViewContentType.ERROR_OUTPUT) } handler.complete(-1) } else if (response != null) { - invokeLater { - printResponse(console, headersConsole, bodyConsole, response) + ApplicationManager.getApplication().invokeLater { + console.clear() + printResponse(console, response) } handler.complete(0) } } } - private fun printResponse( - console: ConsoleView, - headersConsole: ConsoleView, - bodyConsole: ConsoleView, - response: HttpResponse, - ) { + private fun printResponse(console: ConsoleView, response: HttpResponse) { response.headers().map().entries.take(10).forEach { (k, v) -> - headersConsole.print("$k: ${v.firstOrNull()}\n", ConsoleViewContentType.LOG_INFO_OUTPUT) console.print("$k: ${v.firstOrNull()}\n", ConsoleViewContentType.LOG_INFO_OUTPUT) } console.print("\n", ConsoleViewContentType.LOG_DEBUG_OUTPUT) console.print(response.body(), ConsoleViewContentType.NORMAL_OUTPUT) - - bodyConsole.print(response.body(), ConsoleViewContentType.NORMAL_OUTPUT) } override fun createWrapper() = HttpWrapperPanel(project) -} \ No newline at end of file +} diff --git a/src/main/kotlin/com/github/xepozz/inline_call/feature/http/HttpWrapperPanel.kt b/src/main/kotlin/com/github/xepozz/inline_call/feature/http/HttpWrapperPanel.kt index 3b44019..7633498 100644 --- a/src/main/kotlin/com/github/xepozz/inline_call/feature/http/HttpWrapperPanel.kt +++ b/src/main/kotlin/com/github/xepozz/inline_call/feature/http/HttpWrapperPanel.kt @@ -4,21 +4,10 @@ import com.github.xepozz.inline_call.base.api.Wrapper import com.intellij.execution.filters.TextConsoleBuilderFactory import com.intellij.execution.ui.ConsoleView import com.intellij.openapi.project.Project -import com.intellij.ui.components.JBTabbedPane import javax.swing.JComponent -import javax.swing.JTabbedPane class HttpWrapperPanel(project: Project) : Wrapper { - private val builder = TextConsoleBuilderFactory.getInstance().createBuilder(project) - val bodyConsole: ConsoleView = builder.console - val headersConsole: ConsoleView = builder.console + val console: ConsoleView = TextConsoleBuilderFactory.getInstance().createBuilder(project).console - val console: ConsoleView = builder.console - - override val component: JComponent = JBTabbedPane(JTabbedPane.TOP).apply { - addTab("Raw", console.component) - addTab("Headers", headersConsole.component) - addTab("Body", bodyConsole.component) -// addTab("Preview", bodyConsole.component) - } -} \ No newline at end of file + override val component: JComponent = console.component +} diff --git a/src/main/kotlin/com/github/xepozz/inline_call/feature/http/run/HttpRunConfiguration.kt b/src/main/kotlin/com/github/xepozz/inline_call/feature/http/run/HttpRunConfiguration.kt index 090c394..61342ef 100644 --- a/src/main/kotlin/com/github/xepozz/inline_call/feature/http/run/HttpRunConfiguration.kt +++ b/src/main/kotlin/com/github/xepozz/inline_call/feature/http/run/HttpRunConfiguration.kt @@ -9,6 +9,7 @@ import com.intellij.execution.configurations.RunProfileState import com.intellij.execution.runners.ExecutionEnvironment import com.intellij.openapi.options.SettingsEditor import com.intellij.openapi.project.Project +import com.intellij.openapi.ui.DialogPanel import com.intellij.ui.dsl.builder.bindIntText import com.intellij.ui.dsl.builder.bindItem import com.intellij.ui.dsl.builder.bindText @@ -52,58 +53,77 @@ class HttpRunConfiguration( HttpRunState(this, environment) } -class HttpSettingsEditor : com.intellij.openapi.options.SettingsEditor() { +/** + * Mutable snapshot the Kotlin UI DSL binds against. Translated to/from + * [HttpRunConfiguration] in [HttpSettingsEditor.applyEditorTo] / + * [HttpSettingsEditor.resetEditorFrom]. + */ +private data class HttpUiModel( + var url: String = "", + var method: String = "GET", + var headers: String = "", + var body: String = "", + var timeout: Int = 30, +) - private var url = "" - private var method = "GET" - private var headers = "" - private var body = "" - private var timeout = 30 +class HttpSettingsEditor : SettingsEditor() { + + private val model = HttpUiModel() + private lateinit var panel: DialogPanel private val methods = listOf("GET", "POST", "PUT", "PATCH", "DELETE", "HEAD", "OPTIONS") - override fun createEditor(): JComponent = panel { - row("Method:") { - comboBox(methods) - .bindItem({ method }, { method = it ?: "GET" }) - } - row("URL:") { - textField() - .resizableColumn() - .bindText(::url) - } - row("Timeout (sec):") { - intTextField(1..300) - .bindIntText(::timeout) - } - row("Headers:") { - textArea() - .rows(3) - .resizableColumn() - .bindText(::headers) - .comment("Format: Header-Name: value (one per line)") - } - row("Body:") { - textArea() - .rows(5) - .resizableColumn() - .bindText(::body) + override fun createEditor(): JComponent { + panel = panel { + row("Method:") { + comboBox(methods) + .bindItem({ model.method }, { model.method = it ?: "GET" }) + } + row("URL:") { + textField() + .resizableColumn() + .bindText(model::url) + } + row("Timeout (sec):") { + intTextField(1..300) + .bindIntText(model::timeout) + } + row("Headers:") { + textArea() + .rows(3) + .resizableColumn() + .bindText(model::headers) + .comment("Format: Header-Name: value (one per line)") + } + row("Body:") { + textArea() + .rows(5) + .resizableColumn() + .bindText(model::body) + } } + return panel } override fun applyEditorTo(s: HttpRunConfiguration) { - s.url = url - s.method = method - s.headers = headers - s.body = body - s.timeout = timeout + // Flush UI -> model first, then copy model -> options. + panel.apply() + s.url = model.url + s.method = model.method + s.headers = model.headers + s.body = model.body + s.timeout = model.timeout } override fun resetEditorFrom(s: HttpRunConfiguration) { - url = s.url - method = s.method - headers = s.headers - body = s.body - timeout = s.timeout + // Copy options -> model first, then refresh UI from model. + model.url = s.url + model.method = s.method + model.headers = s.headers + model.body = s.body + model.timeout = s.timeout + if (::panel.isInitialized) { + panel.reset() + } } -} \ No newline at end of file +} diff --git a/src/main/kotlin/com/github/xepozz/inline_call/feature/http/run/HttpRunConfigurationType.kt b/src/main/kotlin/com/github/xepozz/inline_call/feature/http/run/HttpRunConfigurationType.kt index c0c53ca..69bb0ff 100644 --- a/src/main/kotlin/com/github/xepozz/inline_call/feature/http/run/HttpRunConfigurationType.kt +++ b/src/main/kotlin/com/github/xepozz/inline_call/feature/http/run/HttpRunConfigurationType.kt @@ -13,10 +13,13 @@ import org.jetbrains.annotations.Nls import javax.swing.Icon class HttpRunConfigurationType : ConfigurationType { - + companion object { + // Persistence contract: this value is written into workspace.xml as + // `type="..."` and is part of the plugin's public surface once + // shipped. Do NOT rename without a migration shim. const val ID = "HttpInlayRunConfiguration" - + fun getInstance(): HttpRunConfigurationType = ConfigurationTypeUtil.findConfigurationType(HttpRunConfigurationType::class.java) } @@ -33,9 +36,15 @@ class HttpRunConfigurationType : ConfigurationType { } class HttpConfigurationFactory(type: ConfigurationType) : com.intellij.execution.configurations.ConfigurationFactory(type) { - - override fun getId(): String = HttpRunConfigurationType.ID - + + companion object { + // Persistence contract: written into workspace.xml as `factoryName="..."`. + // Must be unique within `HttpRunConfigurationType`. Do NOT rename. + const val FACTORY_ID = "HTTP" + } + + override fun getId(): String = FACTORY_ID + override fun createTemplateConfiguration(project: Project): RunConfiguration = HttpRunConfiguration(project, this, "HTTP") diff --git a/src/main/kotlin/com/github/xepozz/inline_call/feature/shell/ConsoleWrapperPanel.kt b/src/main/kotlin/com/github/xepozz/inline_call/feature/shell/ConsoleWrapperPanel.kt index 2d6efc1..029f03b 100644 --- a/src/main/kotlin/com/github/xepozz/inline_call/feature/shell/ConsoleWrapperPanel.kt +++ b/src/main/kotlin/com/github/xepozz/inline_call/feature/shell/ConsoleWrapperPanel.kt @@ -3,6 +3,7 @@ package com.github.xepozz.inline_call.feature.shell import com.github.xepozz.inline_call.base.api.Wrapper import com.github.xepozz.inline_call.base.inlay.ui.createToolbar import com.intellij.execution.filters.TextConsoleBuilderFactory +import com.intellij.execution.impl.ConsoleViewImpl import com.intellij.execution.ui.ConsoleView import com.intellij.openapi.project.Project import java.awt.BorderLayout @@ -16,7 +17,7 @@ class ConsoleWrapperPanel(project: Project) : Wrapper { val console: ConsoleView = builder.console - val toolbar = createToolbar() + val toolbar = createToolbar { (console as? ConsoleViewImpl)?.editor?.document?.text.orEmpty() } val toolbarWrapper = JPanel(FlowLayout(FlowLayout.RIGHT, 5, 5)).apply { isOpaque = false alignmentX = 1.0f diff --git a/src/main/kotlin/com/github/xepozz/inline_call/feature/shell/ShellFeatureAdapter.kt b/src/main/kotlin/com/github/xepozz/inline_call/feature/shell/ShellFeatureAdapter.kt index 4596264..2f8f4e7 100644 --- a/src/main/kotlin/com/github/xepozz/inline_call/feature/shell/ShellFeatureAdapter.kt +++ b/src/main/kotlin/com/github/xepozz/inline_call/feature/shell/ShellFeatureAdapter.kt @@ -10,17 +10,24 @@ import com.intellij.execution.process.ProcessHandler import com.intellij.execution.process.ProcessTerminatedListener import com.intellij.execution.ui.ConsoleViewContentType import com.intellij.icons.AllIcons -import com.intellij.openapi.application.invokeLater -import com.intellij.openapi.progress.ProgressIndicator -import com.intellij.openapi.progress.ProgressManager -import com.intellij.openapi.progress.Task +import com.intellij.openapi.application.ApplicationManager import com.intellij.openapi.project.Project import com.intellij.openapi.util.TextRange import java.util.regex.Pattern import javax.swing.Icon /** - * Feature adapter that delegates matching and execution to existing ShellExecutionHandler. + * Feature adapter that runs a shell command via [OSProcessHandler]. + * + * The implementation is intentionally callback-based (no coroutines) + * because the inlay click path runs on EDT and any suspension between + * EDT and the service scope was found to silently swallow the work + * (see the design notes on + * [com.github.xepozz.inline_call.base.inlay.ExecutionController]). + * + * `Process.start()` is fast — running it on the caller's thread (EDT) + * is acceptable for a one-off click. The actual process IO happens + * off-thread inside OSProcessHandler's notification machinery. */ class ShellFeatureAdapter(val project: Project) : FeatureGenerator { override val id: String = "shell" @@ -34,7 +41,10 @@ class ShellFeatureAdapter(val project: Project) : FeatureGenerator val startOriginal = base + block.mapping.toOriginal(m.offset) - val endOriginal = startOriginal + m.value.length + val endOriginal = base + block.mapping.toOriginal(m.offset + m.value.length) + assert(endOriginal - startOriginal >= m.value.length) { + "endOriginal($endOriginal) - startOriginal($startOriginal) < value.length(${m.value.length})" + } FeatureMatch( featureId = id, block = block, @@ -49,52 +59,27 @@ class ShellFeatureAdapter(val project: Project) : FeatureGenerator Unit + onProcessCreated: (ProcessHandler?) -> Unit, ) { val value = match.value val console = wrapper.console - // Run with progress indicator similar to HTTP feature - ProgressManager.getInstance().run(object : Task.Backgroundable(project, "Executing shell command", true) { - override fun run(indicator: ProgressIndicator) { - indicator.isIndeterminate = true - indicator.text = "Running shell command" - indicator.text2 = value - - val commandLine = GeneralCommandLine("/bin/sh", "-c", value) - .withRedirectErrorStream(true) - val processHandler = OSProcessHandler(commandLine) - ProcessTerminatedListener.attach(processHandler) - console.attachToProcess(processHandler) - onProcessCreated(processHandler) - - try { - processHandler.startNotify() - // Keep progress indicator alive until process finishes or user cancels - while (!processHandler.isProcessTerminated) { - if (indicator.isCanceled) { - // User canceled via progress UI; destroy the process - processHandler.destroyProcess() - break - } - // Slight sleep to avoid busy waiting; progress stays visible - try { - Thread.sleep(50) - } catch (_: InterruptedException) { - // If interrupted, attempt to stop process and exit - processHandler.destroyProcess() - break - } - } - } catch (e: Exception) { - invokeLater { - onProcessCreated(null) - console.print("\n[Error: ${e.message}]\n", ConsoleViewContentType.ERROR_OUTPUT) - } - } + try { + val commandLine = GeneralCommandLine("/bin/sh", "-c", value) + .withRedirectErrorStream(true) + val processHandler = OSProcessHandler(commandLine).also { + ProcessTerminatedListener.attach(it) } - }) + console.attachToProcess(processHandler) + onProcessCreated(processHandler) + processHandler.startNotify() + } catch (e: Exception) { + ApplicationManager.getApplication().invokeLater { + console.print("\n[Error: ${e.message}]\n", ConsoleViewContentType.ERROR_OUTPUT) + } + onProcessCreated(null) + } } override fun createWrapper() = ConsoleWrapperPanel(project) -} \ No newline at end of file +} diff --git a/src/main/kotlin/com/github/xepozz/inline_call/feature/shell/run/ShellRunConfiguration.kt b/src/main/kotlin/com/github/xepozz/inline_call/feature/shell/run/ShellRunConfiguration.kt index 3c5b862..0d93203 100644 --- a/src/main/kotlin/com/github/xepozz/inline_call/feature/shell/run/ShellRunConfiguration.kt +++ b/src/main/kotlin/com/github/xepozz/inline_call/feature/shell/run/ShellRunConfiguration.kt @@ -13,12 +13,13 @@ import com.intellij.execution.process.OSProcessHandler import com.intellij.execution.process.ProcessHandler import com.intellij.execution.process.ProcessTerminatedListener import com.intellij.execution.runners.ExecutionEnvironment +import com.intellij.openapi.fileChooser.FileChooserDescriptorFactory import com.intellij.openapi.options.SettingsEditor import com.intellij.openapi.project.Project -import com.intellij.openapi.ui.TextFieldWithBrowseButton +import com.intellij.openapi.ui.DialogPanel +import com.intellij.ui.dsl.builder.bindText import com.intellij.ui.dsl.builder.panel import javax.swing.JComponent -import javax.swing.JTextField class ShellRunConfiguration( project: Project, @@ -53,7 +54,7 @@ class ShellRunState( override fun startProcess(): ProcessHandler { val commandLine = GeneralCommandLine("/bin/sh", "-c", configuration.command) .withRedirectErrorStream(true) - + if (configuration.workingDirectory.isNotBlank()) { commandLine.withWorkDirectory(configuration.workingDirectory) } else { @@ -66,27 +67,50 @@ class ShellRunState( } } +/** + * Mutable snapshot the Kotlin UI DSL binds against. Translated to/from + * [ShellRunConfiguration] in [ShellSettingsEditor.applyEditorTo] / + * [ShellSettingsEditor.resetEditorFrom]. + */ +private data class ShellUiModel( + var command: String = "", + var workingDirectory: String = "", +) + class ShellSettingsEditor : SettingsEditor() { - - private val commandField = JTextField() - private val workingDirField = TextFieldWithBrowseButton() - override fun createEditor(): JComponent = panel { - row("Command:") { - cell(commandField).resizableColumn() - } - row("Working directory:") { - cell(workingDirField).resizableColumn() + private val model = ShellUiModel() + private lateinit var panel: DialogPanel + + override fun createEditor(): JComponent { + panel = panel { + row("Command:") { + textField() + .resizableColumn() + .bindText(model::command) + } + row("Working directory:") { + textFieldWithBrowseButton( + fileChooserDescriptor = FileChooserDescriptorFactory.createSingleFolderDescriptor() + ) + .resizableColumn() + .bindText(model::workingDirectory) + } } + return panel } override fun applyEditorTo(s: ShellRunConfiguration) { - s.command = commandField.text - s.workingDirectory = workingDirField.text + panel.apply() + s.command = model.command + s.workingDirectory = model.workingDirectory } override fun resetEditorFrom(s: ShellRunConfiguration) { - commandField.text = s.command - workingDirField.text = s.workingDirectory + model.command = s.command + model.workingDirectory = s.workingDirectory + if (::panel.isInitialized) { + panel.reset() + } } -} \ No newline at end of file +} diff --git a/src/main/kotlin/com/github/xepozz/inline_call/feature/shell/run/ShellRunConfigurationType.kt b/src/main/kotlin/com/github/xepozz/inline_call/feature/shell/run/ShellRunConfigurationType.kt index 5e8e600..656021f 100644 --- a/src/main/kotlin/com/github/xepozz/inline_call/feature/shell/run/ShellRunConfigurationType.kt +++ b/src/main/kotlin/com/github/xepozz/inline_call/feature/shell/run/ShellRunConfigurationType.kt @@ -13,10 +13,13 @@ import org.jetbrains.annotations.Nls import javax.swing.Icon class ShellRunConfigurationType : ConfigurationType { - + companion object { + // Persistence contract: this value is written into workspace.xml as + // `type="..."` and is part of the plugin's public surface once + // shipped. Do NOT rename without a migration shim. const val ID = "ShellInlayRunConfiguration" - + fun getInstance(): ShellRunConfigurationType = ConfigurationTypeUtil.findConfigurationType(ShellRunConfigurationType::class.java) } @@ -24,7 +27,7 @@ class ShellRunConfigurationType : ConfigurationType { override fun getId(): String = ID override fun getDisplayName(): String = "Shell Command" override fun getConfigurationTypeDescription(): @Nls(capitalization = Nls.Capitalization.Sentence) String? { - return "configuration type description" + return "Run shell command from comment" } override fun getIcon(): Icon = AllIcons.Actions.Execute @@ -34,9 +37,15 @@ class ShellRunConfigurationType : ConfigurationType { } class ShellConfigurationFactory(type: ConfigurationType) : ConfigurationFactory(type) { - - override fun getId(): String = ShellRunConfigurationType.ID - + + companion object { + // Persistence contract: written into workspace.xml as `factoryName="..."`. + // Must be unique within `ShellRunConfigurationType`. Do NOT rename. + const val FACTORY_ID = "Shell" + } + + override fun getId(): String = FACTORY_ID + override fun createTemplateConfiguration(project: Project): RunConfiguration = ShellRunConfiguration(project, this, "Shell") diff --git a/src/main/kotlin/com/github/xepozz/inline_call/language/kotlin/KotlinLanguageExtractor.kt b/src/main/kotlin/com/github/xepozz/inline_call/language/kotlin/KotlinLanguageExtractor.kt deleted file mode 100644 index 7f71476..0000000 --- a/src/main/kotlin/com/github/xepozz/inline_call/language/kotlin/KotlinLanguageExtractor.kt +++ /dev/null @@ -1,9 +0,0 @@ -package com.github.xepozz.inline_call.language.kotlin - -import com.github.xepozz.inline_call.base.api.BaseLanguageTextExtractor -import com.intellij.psi.PsiFile - -class KotlinLanguageExtractor : BaseLanguageTextExtractor() { -// override fun isApplicable(file: PsiFile): Boolean = file is KtFile - override fun isApplicable(file: PsiFile): Boolean = false -} diff --git a/src/main/kotlin/com/github/xepozz/inline_call/language/php/PHPLanguageExtractor.kt b/src/main/kotlin/com/github/xepozz/inline_call/language/php/PHPLanguageExtractor.kt index 425bdfe..7a8de3d 100644 --- a/src/main/kotlin/com/github/xepozz/inline_call/language/php/PHPLanguageExtractor.kt +++ b/src/main/kotlin/com/github/xepozz/inline_call/language/php/PHPLanguageExtractor.kt @@ -1,9 +1,42 @@ package com.github.xepozz.inline_call.language.php import com.github.xepozz.inline_call.base.api.BaseLanguageTextExtractor +import com.github.xepozz.inline_call.base.api.CommentNormalizer +import com.github.xepozz.inline_call.base.normalizers.CStyleBlockNormalizer +import com.github.xepozz.inline_call.base.normalizers.SingleLineCommentNormalizer +import com.intellij.psi.PsiComment +import com.intellij.psi.PsiElement import com.intellij.psi.PsiFile +import com.jetbrains.php.lang.documentation.phpdoc.psi.PhpDocComment +import com.jetbrains.php.lang.lexer.PhpTokenTypes import com.jetbrains.php.lang.psi.PhpFile +/** + * Per-element comment dispatch for PHP: + * + * - `PhpDocComment` (`/** ... */`) -> [CStyleBlockNormalizer] with + * `stripStarPrefix = true`, so leading `* ` is dropped on every line. + * `PhpDocComment` extends `PsiDocCommentBase` which extends + * `PsiComment`, so the base extractor walks it without changes. + * - `C_STYLE_COMMENT` token (`/* ... */`, no PHPDoc) -> + * [CStyleBlockNormalizer] with `stripStarPrefix = false`. + * - line comments (`//`, `#`) -> [SingleLineCommentNormalizer]. + */ class PHPLanguageExtractor : BaseLanguageTextExtractor() { + + private val phpDocNormalizer = CStyleBlockNormalizer(stripStarPrefix = true) + private val cStyleNormalizer = CStyleBlockNormalizer(stripStarPrefix = false) + private val lineNormalizer = SingleLineCommentNormalizer("//|#") + override fun isApplicable(file: PsiFile): Boolean = file is PhpFile + + override fun normalizerFor(element: PsiElement): CommentNormalizer { + if (element is PhpDocComment) return phpDocNormalizer + if (element is PsiComment) { + val type = element.tokenType + if (type == PhpTokenTypes.C_STYLE_COMMENT) return cStyleNormalizer + if (type == PhpTokenTypes.LINE_COMMENT) return lineNormalizer + } + return super.normalizerFor(element) + } } diff --git a/src/main/kotlin/com/github/xepozz/inline_call/language/xml/XmlLanguageExtractor.kt b/src/main/kotlin/com/github/xepozz/inline_call/language/xml/XmlLanguageExtractor.kt index 1c26960..67413bb 100644 --- a/src/main/kotlin/com/github/xepozz/inline_call/language/xml/XmlLanguageExtractor.kt +++ b/src/main/kotlin/com/github/xepozz/inline_call/language/xml/XmlLanguageExtractor.kt @@ -1,9 +1,10 @@ package com.github.xepozz.inline_call.language.xml import com.github.xepozz.inline_call.base.api.BaseLanguageTextExtractor +import com.github.xepozz.inline_call.base.normalizers.XmlCommentNormalizer import com.intellij.psi.PsiFile import com.intellij.psi.xml.XmlFile -class XmlLanguageExtractor : BaseLanguageTextExtractor() { +class XmlLanguageExtractor : BaseLanguageTextExtractor(XmlCommentNormalizer) { override fun isApplicable(file: PsiFile): Boolean = file is XmlFile } diff --git a/src/main/kotlin/com/github/xepozz/inline_call/language/yaml/YamlLanguageExtractor.kt b/src/main/kotlin/com/github/xepozz/inline_call/language/yaml/YamlLanguageExtractor.kt index 237891e..436aa1d 100644 --- a/src/main/kotlin/com/github/xepozz/inline_call/language/yaml/YamlLanguageExtractor.kt +++ b/src/main/kotlin/com/github/xepozz/inline_call/language/yaml/YamlLanguageExtractor.kt @@ -1,9 +1,10 @@ package com.github.xepozz.inline_call.language.yaml import com.github.xepozz.inline_call.base.api.BaseLanguageTextExtractor +import com.github.xepozz.inline_call.base.normalizers.SingleLineCommentNormalizer import com.intellij.psi.PsiFile import org.jetbrains.yaml.psi.YAMLFile -class YamlLanguageExtractor : BaseLanguageTextExtractor() { +class YamlLanguageExtractor : BaseLanguageTextExtractor(SingleLineCommentNormalizer("#")) { override fun isApplicable(file: PsiFile): Boolean = file is YAMLFile } diff --git a/src/main/resources/META-INF/plugin.xml b/src/main/resources/META-INF/plugin.xml index 1abe02c..6852b6b 100644 --- a/src/main/resources/META-INF/plugin.xml +++ b/src/main/resources/META-INF/plugin.xml @@ -9,14 +9,20 @@ com.intellij.modules.xml com.jetbrains.php - com.intellij.modules.json org.jetbrains.plugins.yaml - com.intellij.database - ru.adelf.idea.dotenv messages.CallBundle + + + + + diff --git a/src/main/resources/messages/CallBundle.properties b/src/main/resources/messages/CallBundle.properties index e69de29..a83327d 100644 --- a/src/main/resources/messages/CallBundle.properties +++ b/src/main/resources/messages/CallBundle.properties @@ -0,0 +1,2 @@ +inlay.execution.name=Call (Unified) +inlay.execution.description=Renders Run / Stop / Rerun / Delete / Collapse buttons next to inline-call comments (// shell: ..., // https://...). diff --git a/src/test/kotlin/com/github/xepozz/inline_call/MyPluginTest.kt b/src/test/kotlin/com/github/xepozz/inline_call/MyPluginTest.kt index 1bf01db..493739e 100644 --- a/src/test/kotlin/com/github/xepozz/inline_call/MyPluginTest.kt +++ b/src/test/kotlin/com/github/xepozz/inline_call/MyPluginTest.kt @@ -1,14 +1,27 @@ package com.github.xepozz.inline_call import com.intellij.ide.highlighter.XmlFileType +import com.intellij.openapi.vfs.newvfs.impl.VfsRootAccess import com.intellij.psi.xml.XmlFile import com.intellij.testFramework.TestDataPath import com.intellij.testFramework.fixtures.BasePlatformTestCase import com.intellij.util.PsiErrorElementUtil +import java.io.File @TestDataPath("\$CONTENT_ROOT/src/test/testData") class MyPluginTest : BasePlatformTestCase() { + override fun setUp() { + super.setUp() + // The test fixture sandbox VFS only whitelists a few roots + // (project base, idea-sandbox, etc.); our testData lives in the + // module's src/test/testData which is outside those roots when + // the build runs from /home/user/inline-call-plugin. Allow it + // explicitly for the duration of the test. + val abs = File(testDataPath).absolutePath + VfsRootAccess.allowRootAccess(testRootDisposable, abs) + } + fun testXMLFile() { val psiFile = myFixture.configureByText(XmlFileType.INSTANCE, "bar") val xmlFile = assertInstanceOf(psiFile, XmlFile::class.java) diff --git a/src/test/kotlin/com/github/xepozz/inline_call/base/api/SegmentedOffsetMappingTest.kt b/src/test/kotlin/com/github/xepozz/inline_call/base/api/SegmentedOffsetMappingTest.kt new file mode 100644 index 0000000..923a8fa --- /dev/null +++ b/src/test/kotlin/com/github/xepozz/inline_call/base/api/SegmentedOffsetMappingTest.kt @@ -0,0 +1,173 @@ +package com.github.xepozz.inline_call.base.api + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertThrows +import org.junit.Test + +/** + * Pure-Kotlin tests for SegmentedOffsetMapping — no IntelliJ platform + * harness needed. + */ +class SegmentedOffsetMappingTest { + + @Test + fun `identity round-trips every offset`() { + val mapping = SegmentedOffsetMapping.identity(10) + for (i in 0..10) { + assertEquals(i, mapping.toOriginal(i)) + assertEquals(i, mapping.toNormalized(i)) + } + } + + @Test + fun `identity of length zero is a valid empty mapping`() { + val mapping = SegmentedOffsetMapping.identity(0) + assertEquals(0, mapping.toOriginal(0)) + assertEquals(0, mapping.toNormalized(0)) + } + + @Test + fun `single-segment with leading gap maps to original start of segment`() { + // Original: "", normalized: "foo". + val mapping = SegmentedOffsetMapping( + segments = listOf(SegmentedOffsetMapping.Segment(0, 4, 3)), + normalizedLength = 3, + originalLength = 10, + ) + assertEquals(4, mapping.toOriginal(0)) + assertEquals(5, mapping.toOriginal(1)) + } + + @Test + fun `end sentinel returns position right after last preserved char`() { + // Original: "", normalized: "foo". After 'foo' the + // original position is 7 (the start of "-->"), not the document + // end 10 — so a match spanning the whole body lands at [4, 7). + val mapping = SegmentedOffsetMapping( + segments = listOf(SegmentedOffsetMapping.Segment(0, 4, 3)), + normalizedLength = 3, + originalLength = 10, + ) + assertEquals(7, mapping.toOriginal(3)) + } + + @Test + fun `end sentinel falls back to originalLength when no segments`() { + val mapping = SegmentedOffsetMapping(emptyList(), 0, 5) + assertEquals(5, mapping.toOriginal(0)) + } + + @Test + fun `identity end sentinel still returns length`() { + // For identity mappings lastSegment.originalStart + length == length. + val mapping = SegmentedOffsetMapping.identity(7) + assertEquals(7, mapping.toOriginal(7)) + } + + @Test + fun `multi-segment maps across PHPDoc-style gaps`() { + // Original: "/**\n * Line one\n * shell: echo 42\n */" + // Normalized: "Line one\nshell: echo 42" + val segments = listOf( + SegmentedOffsetMapping.Segment(0, 7, 8), // "Line one" + SegmentedOffsetMapping.Segment(8, 15, 1), // "\n" + SegmentedOffsetMapping.Segment(9, 19, 14), // "shell: echo 42" + ) + val mapping = SegmentedOffsetMapping(segments, normalizedLength = 23, originalLength = 38) + + assertEquals(7, mapping.toOriginal(0)) // 'L' + assertEquals(14, mapping.toOriginal(7)) // 'e' in "one" + assertEquals(15, mapping.toOriginal(8)) // '\n' + assertEquals(19, mapping.toOriginal(9)) // 's' in "shell" + assertEquals(32, mapping.toOriginal(22)) // last char '2' + // End sentinel returns position right after last preserved char: + // last segment (9, 19, 14) -> 19 + 14 = 33. The original text + // continues with "\n */" past that point. + assertEquals(33, mapping.toOriginal(23)) + } + + @Test + fun `toNormalized clamps right on stripped char`() { + // Original: "", normalized: "foo" (gap at [0,4) and [7,10)). + val mapping = SegmentedOffsetMapping( + segments = listOf(SegmentedOffsetMapping.Segment(0, 4, 3)), + normalizedLength = 3, + originalLength = 10, + ) + // Inside gap [0,4): clamp to next segment start (normalized 0). + assertEquals(0, mapping.toNormalized(0)) + assertEquals(0, mapping.toNormalized(2)) + // Inside segment. + assertEquals(0, mapping.toNormalized(4)) + assertEquals(1, mapping.toNormalized(5)) + assertEquals(2, mapping.toNormalized(6)) + // Trailing gap [7,10): no next segment, clamp to normalizedLength. + assertEquals(3, mapping.toNormalized(7)) + assertEquals(3, mapping.toNormalized(9)) + assertEquals(3, mapping.toNormalized(10)) + } + + @Test + fun `out-of-bounds normalized offset throws`() { + val mapping = SegmentedOffsetMapping.identity(5) + assertThrows(IllegalArgumentException::class.java) { mapping.toOriginal(-1) } + assertThrows(IllegalArgumentException::class.java) { mapping.toOriginal(6) } + } + + @Test + fun `out-of-bounds original offset throws`() { + val mapping = SegmentedOffsetMapping.identity(5) + assertThrows(IllegalArgumentException::class.java) { mapping.toNormalized(-1) } + assertThrows(IllegalArgumentException::class.java) { mapping.toNormalized(6) } + } + + @Test + fun `overlapping segments rejected at construction`() { + assertThrows(IllegalArgumentException::class.java) { + SegmentedOffsetMapping( + segments = listOf( + SegmentedOffsetMapping.Segment(0, 0, 5), + SegmentedOffsetMapping.Segment(3, 10, 5), + ), + normalizedLength = 10, + originalLength = 20, + ) + } + } + + @Test + fun `segment exceeding originalLength rejected`() { + assertThrows(IllegalArgumentException::class.java) { + SegmentedOffsetMapping( + segments = listOf(SegmentedOffsetMapping.Segment(0, 5, 10)), + normalizedLength = 10, + originalLength = 10, + ) + } + } + + @Test + fun `preserved-index property holds across many segments`() { + // Build a 100-segment ladder: each segment 5 chars long, 2-char gap between. + val segments = mutableListOf() + var norm = 0 + var orig = 0 + repeat(100) { + segments += SegmentedOffsetMapping.Segment(norm, orig, 5) + norm += 5 + orig += 5 + 2 + } + // The final 2-char gap after the last segment is *not* preserved + // content. originalLength can include it (e.g. trailing `*/`) + // without changing the per-index mapping. + val mapping = SegmentedOffsetMapping(segments, norm, orig) + // Every preserved index should map correctly and round-trip. + for (i in 0 until norm) { + val o = mapping.toOriginal(i) + assertEquals(i, mapping.toNormalized(o)) + } + // End sentinel: position right after last preserved char. + val last = segments.last() + assertEquals(last.originalStart + last.length, mapping.toOriginal(norm)) + } +} diff --git a/src/test/kotlin/com/github/xepozz/inline_call/base/inlay/ExecutionInlayProviderTest.kt b/src/test/kotlin/com/github/xepozz/inline_call/base/inlay/ExecutionInlayProviderTest.kt new file mode 100644 index 0000000..066ec87 --- /dev/null +++ b/src/test/kotlin/com/github/xepozz/inline_call/base/inlay/ExecutionInlayProviderTest.kt @@ -0,0 +1,115 @@ +package com.github.xepozz.inline_call.base.inlay + +import com.github.xepozz.inline_call.base.api.FeatureGenerator +import com.github.xepozz.inline_call.base.api.LanguageTextExtractor +import com.intellij.codeInsight.hints.NoSettings +import com.intellij.openapi.fileTypes.FileTypeManager +import com.intellij.testFramework.utils.inlays.InlayHintsProviderTestCase + +/** + * Verifies that [ExecutionInlayProvider] actually renders inlay hints + * next to `shell:` comments and `https://...` comments in real files. + * + * Uses the platform's [InlayHintsProviderTestCase.doTestProvider] — + * the test content embeds inline hint expectations as `/*<# ... #>*/`, + * the framework strips them, runs the provider through a real + * FactoryInlayHintsCollector and renders the presentations into a + * text dump. Actual vs expected dumps are compared char-by-char. + * + * The dump format the platform uses: an icon presentation renders + * as `` and a `roundWithBackground` wraps its content with + * `[ ... ]`. So the Run button (icon + " Run " with rounded background) + * comes out as `[ Run ]` — two spaces because the inner + * `text(" Run ")` already has a leading space and the icon precedes it. + * + * Headless. No Swing rendering required. + */ +@Suppress("UnstableApiUsage") +class ExecutionInlayProviderTest : InlayHintsProviderTestCase() { + + fun `test shell run button appears next to shell comment in YAML`() { + val content = """ + services: + # /*<# [ Run ] #>*/shell: echo 123 + """.trimIndent() + doTestProvider( + "test.yaml", + content, + ExecutionInlayProvider(), + NoSettings(), + ) + } + + fun `test http run button appears next to URL in YAML`() { + val content = """ + apis: + # /*<# [ Run ] #>*/https://example.com + """.trimIndent() + doTestProvider( + "test.yaml", + content, + ExecutionInlayProvider(), + NoSettings(), + ) + } + + fun `test no inlay in YAML file without matchable comments`() { + val content = """ + services: + # just a regular comment + # nothing here either + """.trimIndent() + doTestProvider( + "test.yaml", + content, + ExecutionInlayProvider(), + NoSettings(), + ) + } + + fun `test shell and http inlays coexist on adjacent lines`() { + val content = """ + # /*<# [ Run ] #>*/shell: echo one + # /*<# [ Run ] #>*/https://api.example.com/two + """.trimIndent() + doTestProvider( + "test.yaml", + content, + ExecutionInlayProvider(), + NoSettings(), + ) + } + + /** + * Smoke check: the provider's underlying extractor + feature chain + * actually finds the comment block and produces a FeatureMatch for + * "shell:". If this passes, the chain that feeds the inlay sink is + * intact even if the rendering test happens to be off by a + * presentation-tree detail. + */ + fun `test extractor + feature chain produces a match for shell comment`() { + val yamlType = FileTypeManager.getInstance().getFileTypeByExtension("yaml") + val psiFile = myFixture.configureByText(yamlType, "# shell: echo plumbing\n") + + val extractors = LanguageTextExtractor.getApplicable(psiFile) + assertTrue("at least one extractor must be applicable for YAML", extractors.isNotEmpty()) + + val blocks = extractors.flatMap { it.extract(psiFile) } + assertFalse("the comment must produce at least one ExtractedBlock", blocks.isEmpty()) + + val features = FeatureGenerator.getApplicable(project) + assertTrue("shell feature must be enabled", features.any { it.id == "shell" }) + + // Iterate every extractor's blocks against the shell feature; collect + // unique matches by value. Multiple extractors may overlap (yaml + // language-specific + fallback adapter); we just want at least one + // distinct match with the right command. + val shellFeature = features.first { it.id == "shell" } + val shellMatches = blocks.flatMap { shellFeature.match(it, project) } + assertFalse("shell feature must match the comment", shellMatches.isEmpty()) + assertTrue( + "shell feature must extract 'echo plumbing' from the comment, got: ${shellMatches.map { it.value }}", + shellMatches.any { it.value == "echo plumbing" }, + ) + } +} diff --git a/src/test/kotlin/com/github/xepozz/inline_call/base/normalizers/CommentNormalizerTest.kt b/src/test/kotlin/com/github/xepozz/inline_call/base/normalizers/CommentNormalizerTest.kt new file mode 100644 index 0000000..ff19fbd --- /dev/null +++ b/src/test/kotlin/com/github/xepozz/inline_call/base/normalizers/CommentNormalizerTest.kt @@ -0,0 +1,171 @@ +package com.github.xepozz.inline_call.base.normalizers + +import com.github.xepozz.inline_call.base.api.CommentNormalizer +import com.github.xepozz.inline_call.base.api.NormalizedComment +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * Pure-Kotlin tests for the four [CommentNormalizer] implementations. + * Each test asserts both the normalized text and the "preserved index" + * property: for every i in normalized.text.indices, + * normalized.text[i] == rawText[mapping.toOriginal(i)]. + */ +class CommentNormalizerTest { + + @Test + fun `IdentityNormalizer is a no-op`() { + val raw = "hello world" + val n = IdentityNormalizer.normalize(raw) + assertEquals(raw, n.text) + assertPreservedIndices(raw, n) + } + + @Test + fun `SingleLine strips slashes and one space`() { + val raw = "// echo 1" + val n = SingleLineCommentNormalizer("//|#").normalize(raw) + assertEquals("echo 1", n.text) + assertPreservedIndices(raw, n) + assertEquals(3, n.mapping.toOriginal(0)) // 'e' + assertEquals(raw.length, n.mapping.toOriginal(n.text.length)) + } + + @Test + fun `SingleLine strips hash and one space`() { + val raw = "# shell: echo 1" + val n = SingleLineCommentNormalizer("#").normalize(raw) + assertEquals("shell: echo 1", n.text) + assertPreservedIndices(raw, n) + } + + @Test + fun `SingleLine respects leading indentation`() { + val raw = " # shell: foo" + val n = SingleLineCommentNormalizer("#").normalize(raw) + assertEquals("shell: foo", n.text) + assertEquals(4, n.mapping.toOriginal(0)) // 's' after " # " + assertPreservedIndices(raw, n) + } + + @Test + fun `SingleLine with no-space marker keeps everything after slash`() { + val raw = "//cmd" + val n = SingleLineCommentNormalizer("//|#").normalize(raw) + assertEquals("cmd", n.text) + assertPreservedIndices(raw, n) + } + + @Test + fun `XmlComment strips brackets and optional spaces`() { + val raw = "" + val n = XmlCommentNormalizer.normalize(raw) + assertEquals("https://example.com", n.text) + assertPreservedIndices(raw, n) + assertEquals(5, n.mapping.toOriginal(0)) + } + + @Test + fun `XmlComment with no inner spaces is identity on content`() { + val raw = "" + val n = XmlCommentNormalizer.normalize(raw) + assertEquals("https://example.com", n.text) + assertEquals(4, n.mapping.toOriginal(0)) + assertPreservedIndices(raw, n) + } + + @Test + fun `XmlComment empty body`() { + val raw = "" + val n = XmlCommentNormalizer.normalize(raw) + assertEquals("", n.text) + assertEquals(7, n.mapping.toOriginal(0)) + } + + @Test + fun `CStyleBlock single-line block strips opener and trailing whitespace`() { + val raw = "/* shell: echo 1 */" + val n = CStyleBlockNormalizer(stripStarPrefix = false).normalize(raw) + assertEquals("shell: echo 1", n.text) + assertPreservedIndices(raw, n) + assertEquals(3, n.mapping.toOriginal(0)) + } + + @Test + fun `CStyleBlock single-line PHPDoc with stripStarPrefix`() { + val raw = "/** shell: echo 1 */" + val n = CStyleBlockNormalizer(stripStarPrefix = true).normalize(raw) + assertEquals("shell: echo 1", n.text) + assertPreservedIndices(raw, n) + assertEquals(4, n.mapping.toOriginal(0)) + } + + @Test + fun `CStyleBlock multi-line PHPDoc strips per-line star prefix`() { + // raw[7]='L', raw[15]='\n', raw[19]='s', total length 38. + val raw = "/**\n * Line one\n * shell: echo 42\n */" + val n = CStyleBlockNormalizer(stripStarPrefix = true).normalize(raw) + assertEquals("Line one\nshell: echo 42", n.text) + assertPreservedIndices(raw, n) + assertEquals(7, n.mapping.toOriginal(0)) // 'L' + assertEquals(15, n.mapping.toOriginal(8)) // '\n' + assertEquals(19, n.mapping.toOriginal(9)) // 's' + // End sentinel: position right after last preserved char ('2'). + // Last segment is (9, 19, 14) -> 33 (original `\n */` starts here). + assertEquals(33, n.mapping.toOriginal(n.text.length)) + } + + @Test + fun `CStyleBlock multi-line without stripStarPrefix preserves stars`() { + val raw = "/*\n * hello\n */" + val n = CStyleBlockNormalizer(stripStarPrefix = false).normalize(raw) + // Without star-stripping, each interior line keeps its leading + // whitespace and `*`. Trailing decoration-only line is dropped. + assertEquals(" * hello", n.text) + assertPreservedIndices(raw, n) + } + + @Test + fun `CStyleBlock CRLF treated as single line terminator`() { + val raw = "/**\r\n * Line one\r\n * shell: echo 42\r\n */" + val n = CStyleBlockNormalizer(stripStarPrefix = true).normalize(raw) + assertEquals("Line one\nshell: echo 42", n.text) + assertPreservedIndices(raw, n) + // 'L' lives at offset: "/**\r\n " = 0..5; '*' at 5, ' ' at 6, 'L' at 7? Let's count. + // '/'=0 '*'=1 '*'=2 '\r'=3 '\n'=4 ' '=5 '*'=6 ' '=7 'L'=8 + assertEquals(8, n.mapping.toOriginal(0)) + } + + @Test + fun `CStyleBlock PHPDoc with no space after star still works`() { + // raw[5]='*', raw[6]='s' (no space). Should map normalized 0 -> 6. + val raw = "/**\n *shell: echo 1\n */" + val n = CStyleBlockNormalizer(stripStarPrefix = true).normalize(raw) + assertEquals("shell: echo 1", n.text) + assertPreservedIndices(raw, n) + assertEquals(6, n.mapping.toOriginal(0)) + } + + @Test + fun `CStyleBlock preserves indentation inside content`() { + // After the per-line `* ` prefix is stripped, only one space is + // consumed; further indentation stays as content. + val raw = "/**\n * indented\n */" + val n = CStyleBlockNormalizer(stripStarPrefix = true).normalize(raw) + assertEquals(" indented", n.text) + assertPreservedIndices(raw, n) + } + + private fun assertPreservedIndices(raw: String, n: NormalizedComment) { + for (i in n.text.indices) { + val o = n.mapping.toOriginal(i) + assertTrue("toOriginal($i)=$o out of bounds for raw length ${raw.length}", o in raw.indices) + assertEquals( + "normalized[$i] (${n.text[i]}) != raw[toOriginal($i)=$o] (${raw[o]})", + n.text[i], + raw[o], + ) + } + } +}