refactor: simplify terminal.text.width using luasystem 0.7.0 (#204)#206
refactor: simplify terminal.text.width using luasystem 0.7.0 (#204)#206Gitkbc wants to merge 4 commits intolunarmodules:mainfrom
Conversation
Remove per-character width cache and probe a single ambiguous-width character during initialization. Store the measured value globally and delegate width calculation to LuaSystem. Preserve test and test_write APIs for compatibility. Update documentation and rockspec dependency.
src/terminal/text/width.lua
Outdated
| -- re-run to get the total width, since all widths are known now, | ||
| -- but this time do not write the string, just return the width | ||
| return M.test(str) | ||
| M.initialize() |
There was a problem hiding this comment.
I don't think there is any utility left for this function. It was an optimization of the test function, in case you would also want the text to actually show.
src/terminal/text/width.lua
Outdated
| -- character not in the cache will be passed to `system.utf8cwidth` to determine the width. | ||
| --- Character display width helpers. | ||
| -- Uses LuaSystem width calculations with an optional calibrated | ||
| -- ambiguous-width value for terminal-specific behavior. |
There was a problem hiding this comment.
can we leave some more background in the doc comments? the cache part should go, that's clear, but the ambiguous explanation is still useful I think
src/terminal/draw/init.lua
Outdated
|
|
||
|
|
||
| -- returns a string with all box_fmt characters, to pre-load the character width cache | ||
| -- returns a string with all box_fmt characters |
There was a problem hiding this comment.
this has no utility anymore, it was only here to be able to test the default used characters
src/terminal/init.lua
Outdated
| -- @within Initialization | ||
| function M.preload_widths(str) | ||
| text.width.test((str or "") .. M.progress._spinner_fmt_chars() .. M.draw._box_fmt_chars()) | ||
| if str then |
There was a problem hiding this comment.
this function also has no utility anymore
src/terminal/init.lua
Outdated
| sys.setconsoleflags(io.stdin, sys.getconsoleflags(io.stdin) - sys.CIF_PROCESSED_INPUT) | ||
| end | ||
|
|
||
| text.width.initialize(true) |
There was a problem hiding this comment.
testing should be optional. The default can be to test, but the user should be able to skip it. In case of a pipe-out, instead of a terminal, write the test characters would break the output.
src/terminal/text/width.lua
Outdated
| local ambiguous_char = "·" | ||
| local ambiguous_codepoint = utf8.codepoint(ambiguous_char) | ||
|
|
||
| M.ambiguous_width = nil |
There was a problem hiding this comment.
I think it is save to set a default here and document it. Default should be 1.
src/terminal/text/width.lua
Outdated
|
|
||
|
|
||
| local function detect_ambiguous_width() | ||
| local row, col = t.cursor.position.get() |
There was a problem hiding this comment.
should we use istty() here to see if we have a terminal?
src/terminal/text/width.lua
Outdated
| local query = ambiguous_char .. t.cursor.position.query_seq() .. setpos | ||
|
|
||
| t.text.stack.push({ brightness = 0 }) | ||
| local result, err = t.input.query(query, "^\27%[(%d+);(%d+)R$") |
There was a problem hiding this comment.
don't hardcode the sequence, use this library's functions
|
Thanks for the detailed review and the clear pointers, @Tieske. You're right — the initial submission didn’t fully align with the project’s design and conventions. I’ll rework the implementation to address each of your points and ensure it matches the existing architecture and standards. I’ll push a revised version shortly. Appreciate the thorough feedback. |
… calibration (LuaSystem >= 0.7.0)
|
Addressed the requested changes: Please let me know if any further adjustments are needed. |
2 participants
Implements the simplification proposed in #204.
terminal.initialize() (and preload_widths).
Fixes #204.