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. > >> > 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. -- Akib Azmain Turja Find me on Mastodon at @akib@hostux.social. This message is signed by me with my GnuPG key. Its fingerprint is: 7001 8CE5 819F 17A3 BBA6 66AF E74F 0EFA 922A E7F5