PR: Fix opts.title Behaviors of _make_floating_popup_size
Background
I discovered this issue while browsing through the Neovim issue tracker.
Understanding the Problem
The issue reported unexpected behavior with opts.title in _make_floating_popup_size. My investigation revealed that while opts.title worked correctly with simple string values, it failed to handle the [string, string][] format (an array of text-highlight pairs).
After discussing with the issue author, I learned that vim.lsp.util.open_floating_preview internally uses vim.api.nvim_open_win, which supports both title formats according to the Neovim API specification. The bug was clear: _make_floating_popup_size needed to calculate width correctly for both title formats.
Implementation Journey
Initial Attempt
My first implementation was functional but had critical flaws:
if opts.title then
local title = opts.title
local title_length = 0
if type(title) == 'string' then
title_length = vim.fn.strdisplaywidth(title)
else
if title ~= nil then
for i = 1, #title do
title_length = title_length + string.len(title[i][1])
end
end
end
width = math.max(width, title_length)
Issues with this approach:
- Incorrect CJK handling: Using
string.len()instead ofvim.fn.strdisplaywidth()resulted in wrong width calculations for CJK (Chinese, Japanese, Korean) characters. - Code complexity: The nested conditionals made the logic harder to follow and maintain.
Refined Solution
@luukvbaal suggested a much more elegant approach:
local title_length = 0
local chunks = type(opts.title) == 'string' and { { opts.title } } or opts.title or {}
for _, chunk in
ipairs(chunks --[=[@as [string, string][]]=])
do
title_length = title_length + vim.fn.strdisplaywidth(chunk[1])
Why this is better:
- Unified handling: Normalizes both title formats (
stringand[string, string][]) into a consistent array structure - Cleaner logic: Eliminates nested conditionals with a concise ternary expression
- Type safety: Uses Lua's type annotation syntax
--[=[@as [string, string][]]=]to help with LSP type checking
This elegant solution taught me a new Lua pattern that I hadn't encountered before.
Testing
Before submitting for review, I added a unit test following the existing test patterns:
it('considers [string,string][] title when computing width', function()
eq(
{ 17, 2 },
exec_lua(function()
return {
vim.lsp.util._make_floating_popup_size(
{ 'foo', 'bar' },
{ title = { { 'A very ', 'Normal' }, { 'long title', 'Normal' } } }
),
}
end)
)
end)
The test validates that the function correctly computes the width when given a title with multiple styled chunks.
Outcome
After pushing the changes and requesting review, @zeertzjq merged the PR successfully.
Key Takeaways
-
Communication is crucial: Discussing with the issue author helped me understand the underlying API design and expected behavior.
-
Learn from reviewers:
@luukvbaal's suggestion introduced me to more idiomatic Lua patterns and the type annotation syntax for better code quality. -
Don't be intimidated: Contributing to large codebases like Neovim is more approachable than it seems. Starting with well-defined issues and learning from reviewer feedback makes the process manageable and educational.