Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix test failure due to not displaying invisible characters #1065

Merged
merged 1 commit into from
Jan 31, 2021

Conversation

krobelus
Copy link
Contributor

test/main/emoji-test fails for me at emoji-commit-titles-col-300 because
this line is not rendered correctly:

2009-04-06 01:44 +0000 Committer o [master] πŸŒπŸ’§βœ‹πŸ•‹πŸ—‘πŸš€πŸœβ˜€οΈπŸŒ‘πŸŒΆπŸ’―πŸš±β³πŸŒ…πŸŒ‘πŸ˜‘πŸ’‰πŸ˜±πŸ˜ˆπŸ’€πŸ’₯πŸŒ›πŸŒ™πŸ­πŸ’₯πŸšΆπŸ»γ€°πŸ›οΈβŒ›οΈπŸ‘³πŸ™πŸ’₯πŸ˜΄πŸ›ŒπŸ˜³πŸ’₯πŸ›πŸ’₯πŸ‘Šβš”πŸ‘‘

The line in emoji-test contains two πŸ›emojis.
However, the first bug is followed by an invisible "Variation Selector".

Here are the two bugs with a \uFE0F in the middle:

πŸ›οΈπŸ›

It looks like Tig renders the first bug without \uFE0F, but the test
expects it.

The same goes for the next visible character after the first bug: βŒ› is
also followed by \uFE0F which is not rendered.

Fix the test failure by removing both invisible characters from the expected output.

This approach will probably break the test on other systems. I don't know
if the character should be rendered. I want to find out why it fails on my
system - maybe ncurses 6.2.1 doesn't render those control characters anymore?

@krobelus
Copy link
Contributor Author

krobelus commented Jan 5, 2021

Travis fails (https://travis-ci.org/github/jonas/tig/jobs/752292336) so this seems wrong.
EDIT: this change is sufficient for the tests to pass in a fresh container running Ubuntu 20.04.

@koutcher
Copy link
Collaborator

koutcher commented Jan 8, 2021

With your PR, the test also fails on OS X 10.11... Did Ubuntu 20.04 fix something ? Or did it break something ? We need to find out before deciding what to do.

@krobelus
Copy link
Contributor Author

krobelus commented Jan 9, 2021

Turns out this is because glibc's wcwidth in Ubuntu 16 assumes that πŸ› has a display width of 1, while later versions, like from Ubuntu 18 return 2:

#define _XOPEN_SOURCE
#include <wchar.h>
#include <stdio.h>
#include <locale.h>

int main() {
        if (setlocale(LC_CTYPE, "en_US.UTF-8"))
                printf("%d\n", wcwidth(0x1F41B));
}

For some reason, a single 0x1F41B ist still rendered as two cells on either system; I haven't figured out exactly why that's the case even if wcwidth is 1.

In our test, we want to render a string of two codepoints: {0x1F41B, 0xFE0F}
On old Ubuntu, ncurses would render two cells, with exactly those characters. Since the variation selector is nonprintable, the end result is correct.

On new Ubuntu, 0x1F41B is of width two, and therefore rendered in two cells with the same value. The variation selector is then attached to the second cell only. I'll test with a character where the variation selector actually makes a difference to see if "attaching to the last cell" can work.

So I think we can make the test cases a bit more interesting, but probably only if we know how wcwidth will behave. Maybe a check at configuration time is warranted.
The simplest fix would be to drop the variation selectors from the test input.

@koutcher
Copy link
Collaborator

koutcher commented Jan 9, 2021

Yes, I agree with you, I don't think it's worth having a complex test case just to make sure we don't break the display of a caterpillar in Tig...

test/main/emoji-test fails when using a recent version of glibc, where
wcwidth(πŸ›) equals 2 (used to be 1 which is less correct).

This test used to include πŸ› and other emoji followed by the 
["Variation Selector"](https://unicode-table.com/en/FE0F/).

With the old wcwidth, Tig would render both characters, one cell each.
With the new wcwidth, Tig only renders the first one twice, in two cells.

As a result, the output of :save-display varies across systems.  Solving this
is not really in scope for Tig, since this is the domain of wcwidth and
Ncurses. Hence, remove the variation selectors. As far as I can tell they
are useless here anyway because the characters render the same way after
removing them.
@krobelus
Copy link
Contributor Author

krobelus commented Jan 9, 2021

OK I've removed the variation selectors and updated the commit message. This seems to work on Ubuntu 16-20 and Mac EDIT: nvm, I didn't try on Mac.

@koutcher
Copy link
Collaborator

It runs fine on Mac as well.

@koutcher koutcher merged commit a4e569d into jonas:master Jan 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants