Akib Azmain Turja writes: > Eli Zaretskii writes: > >>> From: Akib Azmain Turja >>> Cc: 57607@debbugs.gnu.org, gerd.moellmann@gmail.com >>> Date: Tue, 04 Oct 2022 13:22:51 +0600 >>> >>> > Thanks. But could you please describe the idea of the patch in some >>> > comment? It is hard to follow the code, especially since the diff has >>> > many pure whitespace changes. >>> >>> The idea is that you write the string just like before (for example, you >>> want to write "hello" in a five columns width terminal, so you write >>> only "hell", so that the line shows "hell "), then move a character >>> backward and write the last glyph (write "o", so that the line shows >>> "helo "), move a character backward again and arrange that after writing >>> the next glyph, the character on the current position will be pushed >>> towards right and write the glyph before the last one (write "l", now >>> the line shows "hello"). >>> >>> Should I add the explanation to the function as comment? >> >> Yes, please. It would also help if the respective parts of the code >> were annotated with comments that explain their role in the algorithm. > > OK. Done. > >> >>> > Also, this: >>> > >>> >> + /* Go to the previous position. */ >>> >> + cmgoto (tty, curY (tty), curX (tty) - 1); >>> >> + cmplus (tty, 1); >>> > >>> > Seem to assume the last character takes just one column? What about >>> > characters whose width is 2 columns? >>> >>> Yes, I assume that. I don't think any multi-char width glyph reach this >>> function (I think they are converted to single column width glyphs, for >>> example "^L" is converted to "^" and "L"). The reason of the assumption >>> is the following code: >>> >>> --8<---------------cut here---------------start------------->8--- >>> if (AutoWrap (tty) >>> && curY (tty) + 1 == FRAME_TOTAL_LINES (f) >>> && (curX (tty) + len) == FRAME_COLS (f)) >>> len --; >>> --8<---------------cut here---------------end--------------->8--- >>> >>> Which also assumes the same. >> >> Please try with some 2-column CJK characters, I'm not sure the example >> with ^L is relevant here. You can find the list of wide characters in >> characters.el (search for "width"); for example, characters in the >> U+FF00 block can be useful. > > Thanks, "CJK STROKE D" doesn't work as expected, and the result depends > on terminal in use. How can I determine the width of a glyph? > >> (And the existing code could have bugs, >> no need to assume it is always correct.) > > Yes, it can, but in most cases, my brain has more bugs. > >> >>> > And finally, it would be nice to avoid so much code duplication >>> > between tty_write_glyphs and tty_write_glyphs_with_face. Is that >>> > feasible? >>> >>> Yes, I think so. I think it possible to just add a new argument >>> "face_id" to tty_write_glyphs would the trick. tty_write_glyph will try >>> to use "face_id" if it is specified, otherwise fallback to the face_id >>> in the string. But how to "not" specify a face_id? Would a NULL work? >> >> face_id is an integer, and zero is a valid value (it means the default >> face), so NULL won't do. But you can use -1 to mean "no face ID". >> Just make sure you never pass it to FACE_FROM_ID etc. >> >> Thanks. > > OK, I'll try that. Thanks. Done. Looks like Emacs still works. Here are the patches: