fix(width): align display width handling with LuaSystem 0.7.0#208
fix(width): align display width handling with LuaSystem 0.7.0#208tanmayrajurkar wants to merge 2 commits intolunarmodules:mainfrom
Conversation
tanmayrajurkar
force-pushed
the
fix/width-ambiguous-probe
branch
from
525b6af to
c4bddcf
Compare
Tieske
left a comment
Tieske
left a comment
There was a problem hiding this comment.
This is clearly AI generated, and it suffers from the same problems as #204 by @Gitkbc .
If you use AI then you are responsible for checking its results. The AI probably assumed backward compatibility, which we not yet need, since we do not have a release yet. Inconsistencies as getter/setter being non-symmetrical, is something that's easy to pick up, but was missed.
| --- Stored width for ambiguous-width characters (1 or 2). Set by `detect_ambiguous_width`. | ||
| -- When nil, width functions use 1 (safe default). Do not set directly; use | ||
| -- `detect_ambiguous_width` or `set_ambiguous_width`. | ||
| M.ambiguous_width = nil |
There was a problem hiding this comment.
should we export this?
maybe just local AMBIGUOUS_WIDTH = 1
|
|
||
| local function ambiguous_width() | ||
| return M.ambiguous_width or 1 | ||
| end |
There was a problem hiding this comment.
since we have a setter, should this be prefixed with get_? also, the getter and setter should live next to eachother.
| -- @tparam number aw 1 or 2 | ||
| -- @within Initialization | ||
| function M.set_ambiguous_width(aw) | ||
| assert(aw == 1 or aw == 2, "ambiguous_width must be 1 or 2, got " .. tostring(aw)) |
There was a problem hiding this comment.
A performance thing; this statement will always do the string-concatenation, even if the assertion passes. Lua will evaluate all parameters before calling assert.
Hence it is more performant if you do:
if aw ~= 1 and aw ~= 2 then
error("ambiguous_width must be 1 or 2, got " .. tostring(aw))
endSince the string concatenation will only be executed in the error-path.
btw; if you had this generated, then maybe this is something to add to the AI steering documents (a to-be added style guide maybe)
There was a problem hiding this comment.
btw; there are probably other occurrences in the code base, I don't think I have been very consistent so far.
| if M.ambiguous_width ~= nil then | ||
| return M.ambiguous_width |
There was a problem hiding this comment.
should we do this check? imho this is a user responsibility; if they request an AW test, we execute an AW test, that's it.
| if not t.ready() then | ||
| M.ambiguous_width = 1 | ||
| return 1 |
There was a problem hiding this comment.
doing a test on an unitnitialized terminal should be an error condition I think, not a silent failure/default.
| -- @treturn[2] nil | ||
| -- @treturn[2] string error message (only if detection was run and failed) | ||
| -- @within Testing | ||
| function M.test(str) |
There was a problem hiding this comment.
I don't think this function has any utility, we can remove it
| -- @treturn number the width of the string in columns | ||
| -- @within Testing | ||
| function M.test_write(str) | ||
| M.detect_ambiguous_width() |
There was a problem hiding this comment.
this one also has no utility anymore, can also be removed
| -- @within Initialization | ||
| function M.preload_widths(str) | ||
| text.width.test((str or "") .. M.progress._spinner_fmt_chars() .. M.draw._box_fmt_chars()) | ||
| text.width.detect_ambiguous_width() |
There was a problem hiding this comment.
this function name no longer covers what it does. This can be removed all together. We can just call the detection from the terminal initialization.
| -- @return true | ||
| -- @within Initialization | ||
| function M.preload_widths(str) | ||
| text.width.test((str or "") .. M.progress._spinner_fmt_chars() .. M.draw._box_fmt_chars()) |
There was a problem hiding this comment.
since this is removed, the 2 functions; _spinner_fmt_chars() and _box_fmt_chars() have lost their utility and can be removed from their respective modules.
| --- Returns the current output stream (e.g. for isatty checks). | ||
| -- @treturn file the stream set by `set_stream` or the default | ||
| function M.get_stream() | ||
| return t |
There was a problem hiding this comment.
an alternative could be to add t.output.isatty() (and to keep it symmetrical also t.input.isatty())
2 participants
Fixes #204
LuaSystem 0.7.0 introduces support for configurable ambiguous-width handling.
This removes the per-character width cache and runtime probing logic and replaces it with a single ambiguous-width detection during initialization, all width calculations now delegate directly to
system.utf8cwidth/system.utf8swidthluasystem >= 0.7.0test/test_writefor compatibility