GNOME Bugzilla – Bug 691972
Block cursor glitches
Last modified: 2014-05-22 23:51:13 UTC
Created attachment 233688 [details] Script to reproduce the cursor glitch The following script shows graphical glitches: #!/bin/sh echo -ne '\e[H' echo -ne '\e[2J' echo -ne 'x' echo -ne '\e[H\t\t' echo -ne '\e[H\t' # 1 sleep 0.5 # 2 echo x # 3 echo xxxxxxxxxxxx # 4 At line #1, the cursor becomes the size of the entire tab stop. Line #2 is necessary from a script to give the cursor time to draw Line #3 overwrites part of the fat cursor but leaves most of it on the screen. Tested with stock gnome-terminal on Ubuntu 12.04 with default config, and roxterm, lxterminal, gnome-terminal on Debian unstable (libvte-2.90-9 1:0.32.2-1).
(Oops, you'll need to use /bin/bash for that script, not /bin/sh, for the "echo -ne" to work)
Created attachment 233691 [details] Screenshot of the resulting glitch in gnome-terminal
Created attachment 254123 [details] [review] Proposed patch Normally vte makes the cursor as wide as the character underneath. This makes perfect sense for double wide (CJK) characters. However, it looks terrible (and is drawn incorrectly for some reason unknown to me) if the underlying char is a TAB (which is 8 characters wide). I think TAB should be handled as an exception and the cursor should be a single character wide.
Egmont, do you want to request commit access to GNOME?
Running the test script shows there are two bugs in vte: * the cursor cell isn't being correctly invalidated when the cursor moves away from the tab cell * the xxxxxxxxxxxx is overdrawing the bottom 1px of the tab's cell Now I don't have a strong preference for or against the change in the patch, but it's independent of the real problem here and just papers over it; I'd rather have the above two problems fixed first.
Behdad: No, thanks, I'd rather just stay an occasional contributor and have my work verified by more experienced developers. ChPe: Agreed, let's catch the underlying bug. But IMHO an 8 char wide cursor looks totally silly, it should be single wide instead.
The problem is that when that 'x' character is replaced then the tab is split into spaces but the cursor is not updated. Previously the cursor was drawn 8 characters wide (as it was above the tab), but later it'll only be invalidated for a single cell (as it stands above the 'x'). A possible solution is to call _vte_invalidate_cursor_once(terminal, FALSE) at the beginning of _vte_terminal_cleanup_tab_fragments_at_cursor(). If you do it at the very beginning, before checking for '\t' then it also takes care of the cursor over the second half of a CJK character. Although overwriting the first half of CJK with an 'x' positions a cursor incorrectly, and overwriting the second half of a CJK only also has bugs with the cursor not getting invalidated. There are plenty of tricky cases with CJK, probably it's worth another bugreport to go through them systematically and to focus on tab only in this one. If we vote for the behavioral change in comment 3 (which matches the behavior of xterm, konsole, putty) then I think that now we understand the underlying cause and we're safe to say that that patch is okay. If we'd like to keep the current behavior of the cursor (which matches the behavior of urxvt) then the solution would be what I've just mentioned: invalidating the cursor before possibly exploding a tab to spaces. /me votes for the first, I find an 8 char wide cursor totally silly. In both cases, addressing all CJK corner cases will be a remaining issue.
I've committed the (partial) fix for the tab character, since it solves the most typical use case, and will be required for the generic fix as well. Leaving open for the CJK case, and for vte-next branch.
I think just calling _vte_invalidate_cursor_once(terminal, FALSE) would be wrong. I haven't tested it, but I guess the same would likely happen if overwriting a reverse-attribute tab (or CJK) with a(ny or narrow) character, not just for the cursor. I think the problem is that cells are invalidated using their attr.columns value *after* the update, while what really needs to be taken is the max of before and after value. Keeping a record of the pre-update attr.column width in vte_terminal_process_incoming() and using it to enlarge the bbox after the update looks promising to fix this. (I guess the overdrawing of the bottom 1px of the previous line should be filed separately.)
Commit 4d6edfb7ecd9bc2743fe507794092ff096d50aac fixes the CJK issues mentioned earlier.