Calculate the width of each displayed character #185

mptre · 2017-04-16T18:43:32Z

In an attempt to fix #184 caused by lines including Unicode characters
that occupy more than one column.

While at it, fix another bug related to the column counter when a line
contains more than one escape sequence.

Reviews and testing would be much appreciated I haven't discovered any
regression nor performance decrease running this diff so far.

/cc @calleerlandsson @mike-burns @teoljungberg.

teoljungberg

Is there a test we can add for this change? Or is the testing setup we have now non-applicable for a change such as this?

teoljungberg · 2017-04-16T19:45:19Z

pick.c

@@ -667,19 +667,23 @@ tty_restore(void)
 }

 void
-print_line(const char *string, size_t length, int so, ssize_t ulso, ssize_t uleo)
+print_line(const char *str, size_t len, int so, ssize_t ulso, ssize_t uleo)


I, for one, am of the view to write out as many acronyms as possible. But since we have ssize_t unt so weiter already, I'd say this falls in line with that style better.

mptre · 2017-04-17T08:26:49Z

Is there a test we can add for this change? Or is the testing setup we have now non-applicable for a change such as this?

Unfortunately no. Since this change only concerns the displayed
TTY-output, it's not testable.

I, for one, am of the view to write out as many acronyms as possible. But since we have ssize_t unt so weiter already, I'd say this falls in line with that style better.

Agree, just renamed the arguments. Thanks!

calleluks

Thanks for working on this @mptre, I really appreciate it!

I added some questions inline in the diff.

I did find a regression but haven't had time to figure out exactly why it's happening. Steps to reproduce:

echo "🤔" | ./pick
The standout spills over to the next line by one column. (see first screenshot below)
Type a non-printable character (for example arrow keys)
All lines (query and choice) are pushed down by one line (see second screenshot below)

I'm currently running bash 4.4.5 on macOS 10.12.4 with LC_CTYPE=UTF-8.

calleluks · 2017-04-18T07:32:07Z

configure.ac

+case "${host_os}" in
+linux*) build_linux=yes ;;
+esac
+AM_CONDITIONAL([LINUX], [test "$build_linux" = "yes"])


Is $build_linux used solely to define LINUX so that it can be picked up in Makefile.am or is it used elsewhere?

calleluks · 2017-04-18T07:36:06Z

configure.ac

+AC_CANONICAL_HOST
+build_linux=no
+case "${host_os}" in
+linux*) build_linux=yes ;;


I haven't used AC_CANONICAL_HOST before and didn't find any documentation detailing the formatting of host_os. Is linux* a reliable pattern for catching all Linux-based OSs?

calleluks · 2017-04-18T07:37:22Z

Makefile.am

@@ -2,6 +2,10 @@ AUTOMAKE_OPTIONS=foreign

 AM_CFLAGS=-pedantic -Wall -Werror -Wextra

+if LINUX


Could defining _XOPEN_SOURCE on non-linux platforms cause problems? If not, what do you think of always defining it? That way we could avoid the complexities of testing for the host os.

Replying to all comments above here. Unfortunately no, defining it on
*BSD makes BSD-specific functions like pledge and reallocarray
unavailable.

Maybe @mike-burns could shed some insight on how-to solve this?

The OS detection is taken from here 1 2.

I see. Thanks for the links! I'm happy to try this and correct course if we run into issues with some distro.

I don't see why, from a quick skim of the repo, pledge disappears when defining _XOPEN_SOURCE. But I do see this in glibc: https://github.com/bminor/glibc/blob/963394a22b38c4ec92b6875a6c06d3b15d5c0d21/include/features.h#L186-L208

So maybe: skip the OS check (which should be a glibc check ... somehow?) and instead do:

AM_CPPFLAGS+=-D_GNU_SOURCE

Works, thanks!

calleluks · 2017-04-18T07:38:11Z

pick.c

-	/* If uleo is greater than columns the underline attribute will spill
-	 * over on the next line unless all attributes are exited. */
+	/*
+	 * If uleo is greater than columns the underline attribute will spill


Should this comment say exit_underline instead of uleo that the parameter names have been changed?

Nice catch, fixed!

calleluks · 2017-04-18T07:43:45Z

pick.c

+		 * mbtowc(3) returns 0 is not handled here.
+		 */
+		if ((nbytes = mbtowc(&wc, &str[i], MB_CUR_MAX)) == -1) {
+			mbtowc(NULL, NULL, MB_CUR_MAX);


Is this for resetting internal state of mbtowc?

mptre · 2017-04-18T09:32:01Z

I did find a regression but haven't had time to figure out exactly why it's happening. Steps to reproduce:

Can't reproduce on OpenBSD but I will try it on macOS this evening.

calleluks · 2017-04-18T10:04:10Z

Thanks @mptre. Let me know if I can help you in any way!

In an attempt to fix a bug caused by lines including Unicode characters that occupy more than one column. While at it, fix another bug related to the column counter when a line contains more than one escape sequence.

Required by posix_openpt(3) and wcwidth(3) on Linux.

mptre · 2017-04-18T17:49:14Z

Turns out the passing that specific emoji to wcwidth(3) returns 0 on
macOS. This is fortunately not true for all emojis. I don't think we
should bother handling this edge-case.

OK to merge?

calleluks · 2017-04-19T08:09:08Z

Interesting. Searching for fixes, I found a lot of tickets and issues discussing this problem and a lot of homegrown solutions special casing certain characters. I'm of the opinion that pick(1) should solve these problems.

Looks good to merge.

mptre · 2017-04-19T08:25:02Z

Interesting. Searching for fixes, I found a lot of tickets and issues discussing this problem and a lot of homegrown solutions special casing certain characters. I'm of the opinion that pick(1) should solve these problems.

Can you create a separate issue and share your findings there?

Looks good to merge.

Done! Thanks for the feedback. Time to release?

calleluks · 2017-04-19T08:29:50Z

Sure thing!

mptre mentioned this pull request Apr 16, 2017

Texts are not aligned and position change while I scroll down... #184

Closed

teoljungberg reviewed Apr 16, 2017

View reviewed changes

calleluks mentioned this pull request Apr 18, 2017

Remove usage of sysexits.h #183

Merged

calleluks reviewed Apr 18, 2017

View reviewed changes

mptre added 2 commits April 18, 2017 16:54

Calculate the width of each displayed character

369350b

In an attempt to fix a bug caused by lines including Unicode characters that occupy more than one column. While at it, fix another bug related to the column counter when a line contains more than one escape sequence.

Define _GNU_SOURCE

ffb4ca4

Required by posix_openpt(3) and wcwidth(3) on Linux.

mptre force-pushed the width branch from 0d1b611 to ffb4ca4 Compare April 18, 2017 14:55

mptre merged commit ffb4ca4 into master Apr 19, 2017

mptre deleted the width branch April 19, 2017 08:22

calleluks mentioned this pull request Apr 19, 2017

Some emojis cause line overflow on macOS #186

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Calculate the width of each displayed character #185

Calculate the width of each displayed character #185

mptre commented Apr 16, 2017

teoljungberg left a comment

teoljungberg Apr 16, 2017

mptre commented Apr 17, 2017

calleluks left a comment

calleluks Apr 18, 2017

calleluks Apr 18, 2017

calleluks Apr 18, 2017

mptre Apr 18, 2017

calleluks Apr 18, 2017

mike-burns Apr 18, 2017

mptre Apr 18, 2017

calleluks Apr 18, 2017

mptre Apr 18, 2017

calleluks Apr 18, 2017

mptre Apr 18, 2017

mptre commented Apr 18, 2017

calleluks commented Apr 18, 2017

mptre commented Apr 18, 2017

calleluks commented Apr 19, 2017

mptre commented Apr 19, 2017

calleluks commented Apr 19, 2017

		@@ -2,6 +2,10 @@ AUTOMAKE_OPTIONS=foreign

		AM_CFLAGS=-pedantic -Wall -Werror -Wextra

		if LINUX

Calculate the width of each displayed character #185

Calculate the width of each displayed character #185

Conversation

mptre commented Apr 16, 2017

teoljungberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mptre commented Apr 17, 2017

calleluks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mptre commented Apr 18, 2017

calleluks commented Apr 18, 2017

mptre commented Apr 18, 2017

calleluks commented Apr 19, 2017

mptre commented Apr 19, 2017

calleluks commented Apr 19, 2017