Re: [PATCH] emoji on TUI emacs
Eli Zaretskii <[email protected]>
| Newsgroups | gmane.emacs.devel |
|---|---|
| Message-ID | <[email protected]> |
> From: Kai Ma <[email protected]> > Cc: [email protected] > Date: Sun, 16 Aug 2026 07:45:29 +0200 > > Kai Ma <[email protected]> writes: > >> Eli Zaretskii <[email protected]> writes: >> >>>> However, the column number change due to C-f, C-b, etc. is still the >>>> total sum of CHARACTER_WIDTH (not necessarily 2). Do we want to change >>>> it as well? >>> >>> Yes, definitely. current-column and move-to-column should both work >>> correctly in these cases. >> >> Fixed. Previously in composition_update_it, the width is computed by >> summing character widths. I'm not entirely sure whether it's correct to >> sum glyph widths here. > > Apparently, this is incorrect, and it crashes for me occasionally > because nglyphs != nchars in general. > > Here is a new patch that supersedes the wrong one. It reuses > 'composition_gstring_width' as cmp_it->width for automatic compositions. Thanks, I have a couple of nits and one comment, and then there are a few problems that still need to be fixed: > --- a/src/composite.c > +++ b/src/composite.c > @@ -1535,12 +1535,12 @@ composition_update_it (struct composition_it *cmp_it, ptrdiff_t charpos, ptrdiff > glyph = LGSTRING_GLYPH (gstring, cmp_it->from); > cmp_it->nchars = LGLYPH_TO (glyph) + 1 - from; > cmp_it->nbytes = 0; > - cmp_it->width = 0; > + cmp_it->width = composition_gstring_width (gstring, cmp_it->from, cmp_it->to, NULL); > + > for (i = cmp_it->nchars - 1; i >= 0; i--) > { > c = XFIXNUM (LGSTRING_CHAR (gstring, from + i)); > cmp_it->nbytes += CHAR_BYTES (c); > - cmp_it->width += CHARACTER_WIDTH (c); > } > } > return c; Hmmm... is this guaranteed to produce correct results for both Emoji and non-Emoji compositions? Did you verify we are not breaking anything here, for example with compositions like 'a' followed by an accent? It would help if you could talk me through the code and explain how what composition_gstring_width does is equivalent to summing CHARACTER_WIDTH for TTY frames, in cases other than Emoji. We need to convince ourselves that we don't introduce regressions here. If we are not sure, perhaps special-casing Emoji would be safer. > * admin/unidata/Makefile.in: Changed. This should describe the change. > * src/composite.c (composition_update_it): set the width of an automatic > composition using composition_gstring_width. The description of the change should begin with a capital letter ("Set", not "set"). I applied the patches and tried to use Emacs in a -nw session. I got assertion violation when scrolling through the HELLO file ("C-h h"). See the backtrace and some data from GDB below. As result, I think this code is incorrect: if (it->cmp_it.from + 1 >= it->cmp_it.to) return false; first = XFIXNUM (LGSTRING_CHAR (gstring, it->cmp_it.from)); second = XFIXNUM (LGSTRING_CHAR (gstring, it->cmp_it.from + 1)); See the doc string of composition-get-gstring regarding the structure of a gstring object. My reading of that is that you always need to use 0 and 1 to access the first and the second codepoint of a composed sequence; FROM and TO are indices of the _glyphs_ in the composed grapheme cluster, which is not what you want. As you can see from the GDB session of an Emacs that hit assertion violation, this gstring has only 2 composed characters, but FROM is 2 and TO is 4, so you are accessing a 3-component vector with indices 2+1 = 3 and 3+1 = 4, which is not right. And I'm not sure you need the condition about FROM+1 >= TO, is that perhaps some kind of defense against crashes you've seen? If not, why do you need this condition there? I think the above fragment should be replaced with this: first = XFIXNUM (LGSTRING_CHAR (gstring, 0)); if (LGSTRING_CHAR_LEN (gstring) <= 1) second = 0; else second = XFIXNUM (LGSTRING_CHAR (gstring, 1)); because I've seen cases that the function gets a composition with only one codepoint (which is strange, but we should not crash or hit assertions). After fixing these two problems, I still see some incorrect compositions in the various admin/unidata/emoji-*.txt files. First, cursor movement through Emoji sequences around line 3600 of emoji-test.txt is wrong: it seems that the terminal thinks the "heart" Emoji is two separate characters, or that its width is not 2. The second problem is with the Keycap sequences around line 550 of emoji-sequences: it seems Emacs is not composing these sequences for some reason? Do you see this on your terminal? Apart of the above two issues, the display seems correct, which is a nice improvement. Here's the backtrace from the assertion violation: gdb) bt #0 0x7526ad23 in KERNELBASE!DebugBreak () from C:\WINDOWS\SysWOW64\KernelBase.dll #1 0x00bbf015 in emacs_abort () at w32fns.c:12178 #2 0x009b116f in terminate_due_to_signal (sig=22, backtrace_limit=2147483647) at emacs.c:480 #3 0x00a73efd in die ( msg=0xc96d08 <o_fwd+2184> "0 <= idx && idx < gc_asize (array)", file=0xc96c60 <o_fwd+2016> "lisp.h", line=1966) at alloc.c:7353 #4 0x009885d2 in AREF (array=XIL(0xa00000000b3d9318), idx=3) at lisp.h:1966 #5 0x0098d9f4 in composite_glyph_is_emoji_sequence (it=0x63fa710) at term.c:1874 #6 0x0098dc1b in produce_composite_glyph (it=0x63fa710) at term.c:1925 #7 0x0098ce29 in produce_glyphs (it=0x63fa710) at term.c:1657 #8 0x008d3c10 in display_line (it=0x63fa710, cursor_vpos=40) at xdisp.c:26153 #9 0x008c35f7 in try_window (window=XIL(0xa00000000a72b480), pos=..., flags=0) at xdisp.c:21699 #10 0x008be8bd in redisplay_window (window=XIL(0xa00000000a72b480), just_this_one_p=true) at xdisp.c:20785 #11 0x008b5a67 in redisplay_window_1 (window=XIL(0xa00000000a72b480)) at xdisp.c:18547 #12 0x00ab2666 in internal_condition_case_1 ( bfun=0x8b5a0f <redisplay_window_1>, arg=XIL(0xa00000000a72b480), handlers=XIL(0xc000000009e0787c), hfun=0x8b57e7 <redisplay_window_error>) at eval.c:1736 #13 0x008b474c in redisplay_internal () at xdisp.c:18053 #14 0x008b1a98 in redisplay () at xdisp.c:17071 #15 0x009bed8e in read_char (commandflag=1, map=XIL(0xc00000000b4f4580), prev_event=XIL(0), used_mouse_menu=0x63fecff, end_time=0x0) at keyboard.c:2711 #16 0x009d8be6 in read_key_sequence (keybuf=0x63fefd8, prompt=XIL(0), dont_downcase_last=false, can_return_switch_frame=true, fix_current_buffer=true, prevent_redisplay=false, disable_text_conversion_p=false) at keyboard.c:11224 #17 0x009ba6ca in command_loop_1 () at keyboard.c:1424 #18 0x00ab257c in internal_condition_case (bfun=0x9ba06b <command_loop_1>, handlers=XIL(0x90), hfun=0x9b90c7 <cmd_error>) at eval.c:1712 #19 0x009b9ad3 in command_loop_2 (handlers=XIL(0x90)) at keyboard.c:1163 #20 0x00ab1721 in internal_catch (tag=XIL(0x12ff0), func=0x9b9a9c <command_loop_2>, arg=XIL(0x90)) at eval.c:1392 #21 0x009b9a3e in command_loop () at keyboard.c:1141 #22 0x009b8b15 in recursive_edit_1 () at keyboard.c:749 #23 0x009b8dc5 in Frecursive_edit () at keyboard.c:832 #24 0x009b3dbf in main (argc=3, argv=0x8172350) at emacs.c:2635 Lisp Backtrace: "redisplay_internal (C function)" (0x0) (gdb) fr 5 #5 0x0098d9f4 in composite_glyph_is_emoji_sequence (it=0x63fa710) at term.c:1874 1874 first = XFIXNUM (LGSTRING_CHAR (gstring, it->cmp_it.from)); (gdb) p it->cmp_it.from $1 = 2 (gdb) p gstring $2 = XIL(0xa00000000b3f6078) (gdb) pp gstring [[cp65001 43438 43446] 133 [0 0 32 43438 1 0 0 1 0 nil] [0 0 43438 43438 0 0 0 1 0 nil] [1 1 32 43446 1 0 0 1 0 nil] [1 1 43446 43446 0 0 0 1 0 nil]] (gdb) pp AREF(gstring,0) [cp65001 43438 43446] (gdb) p/x 43438 $3 = 0xa9ae (gdb) p it->cmp_it $4 = { stop_pos = 2399, id = 133, ch = 43446, rule_idx = 0, lookback = 1, nglyphs = 4, reversed_p = false, parent_it = 0x63fa710, charpos = 2400, nchars = 1, nbytes = 3, from = 2, to = 4, width = 1 } (gdb)