👍 Add a priority property to Decoration#295
Conversation
WalkthroughAdds an optional per-decoration Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Buffer as buffer/decoration
participant Backend as Vim/Neovim API
Caller->>Buffer: decorate([{ highlight, priority?, ... }])
Note over Buffer: derive prop-type key = (prefix, priority, highlight)
alt prop-type missing
Buffer->>Backend: prop_type_add / prop_type_create (include priority)
end
Buffer->>Backend: prop_add_list / nvim_buf_set_extmark (attach decorations with priority)
Caller->>Buffer: listDecorations()
Backend-->>Buffer: prop_type_get / extmark info (includes priority)
Buffer-->>Caller: Decoration[] (each includes priority, default 0)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
buffer/decoration.ts (2)
38-43: Clarify JSDoc: negative values allowedCall out that negative priorities are allowed (per Vim’s behavior) to set expectations for consumers.
Apply this diff to the JSDoc:
- /** - * Priority for the decoration (higher numbers are displayed on top) - * Defaults to 0 if not specified - */ + /** + * Priority for the decoration (higher numbers are displayed on top). + * Negative values are allowed. Defaults to 0 when omitted. + */
260-276: Make Vim listing faster and more robust (fallback + cache + safe highlight parse)
- Avoid 1 RPC per prop by caching type->priority.
- Fallback to 0 when
prop_type_get()returns no priority.- Parse highlight without relying on
.split(':').pop()so it won’t break if a highlight ever contains a colon.Apply this diff:
- return Promise.all( - props - .filter((prop) => prop.type.startsWith(`${prefix}:`)) - .map(async (prop) => { - const propType = await vimFn.prop_type_get(denops, prop.type) as { - priority: number; - }; - return ({ - line: prop.lnum, - column: prop.col, - length: prop.length, - highlight: prop.type.split(":").pop() as string, - priority: propType.priority, - }); - }), - ); + const filtered = props.filter((prop) => prop.type.startsWith(`${prefix}:`)); + const typePriorityCache = new Map<string, number>(); + return Promise.all(filtered.map(async (prop) => { + let prio = typePriorityCache.get(prop.type); + if (prio == null) { + const t = await vimFn.prop_type_get(denops, prop.type) as { priority?: number }; + prio = t.priority ?? DEFAULT_DECORATION_PRIORITY; + typePriorityCache.set(prop.type, prio); + } + const head = `${prefix}:${prio}:`; + const highlight = prop.type.startsWith(head) + ? prop.type.slice(head.length) + : prop.type; // safe fallback + return { + line: prop.lnum, + column: prop.col, + length: prop.length, + highlight, + priority: prio, + }; + }));buffer/decoration_test.ts (1)
57-66: Avoid asserting internal Vim prop type namingAsserting the exact
typestring (including embedded priority) tightly couples tests to an internal naming scheme. Prefer verifying behavior vialistDecorationsor by checking theprioritythroughprop_type_get()for the relevant type(s).Example alternatives:
- Use
listDecorationsto assert highlight range and omit type internals.- Or, derive the prop type name, call
prop_type_get(), and assertpriority: 0.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
buffer/decoration.ts(7 hunks)buffer/decoration_test.ts(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
buffer/decoration_test.ts (2)
function/buffer.ts (1)
bufnr(301-307)buffer/decoration.ts (2)
decorate(78-92)listDecorations(153-166)
buffer/decoration.ts (1)
batch/batch.ts (2)
batch(67-73)batch(187-203)
🔇 Additional comments (8)
buffer/decoration.ts (5)
12-12: Good default constantDEFAULT_DECORATION_PRIORITY = 0 aligns with Vim’s default behavior.
63-63: Nice exampleIncluding an explicit priority in the example helps demonstrate ordering semantics.
175-208: Vim prop types keyed by (priority, highlight): LGTMType naming and caching strategy make priority part of the prop-type identity; batch creation looks correct.
352-353: Default priority fallback (Neovim) looks goodSafe defaulting to 0 when the detail is missing.
298-299: Extmark priority supported since Neovim v0.7/API level 7
Thepriorityoption onnvim_buf_set_extmarkis available since API level 7 (Neovim ≥ 0.7), so no change is needed if your minimum supported version is v0.7 or newer.buffer/decoration_test.ts (3)
126-172: Priority coexistence test: LGTMValidates multiple decorations of the same highlight with different priorities across hosts.
185-215: listDecorations priority defaults and propagation: LGTMCovers default priority (0) and explicit priority (10).
230-247: Undecorate tests with priorities: LGTMBoth full-buffer and ranged clears behave correctly with prioritized decorations.
Also applies to: 262-287
|
I'm sorry but we first need to fix CI (it's our fault.) |
|
@blyoa Could you rebase from the latest main and force push? CI would be fixed (prob. CI fails to generate coverage report on Windows but we will ignore that.) |
blyoa
force-pushed
the
add-priority-property-to-decoration
branch
from
2f703e0 to
664a39b
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #295 +/- ##
==========================================
+ Coverage 84.77% 84.82% +0.04%
==========================================
Files 64 64
Lines 2864 2879 +15
Branches 277 281 +4
==========================================
+ Hits 2428 2442 +14
- Misses 434 435 +1
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@lambdalisue |
2 participants
This Pull Request adds a
priorityproperty to theDecorationinterface, allowing users to control the display order of multiple decorations.The default priority is set to zero to align with the
priorityproperty ofprop_type_add.https://github.com/vim/vim/blob/fdeb721251481c26c230a0cd793cb89c2534c206/runtime/doc/textprop.txt#L426-L438
Summary by CodeRabbit
New Features
Tests