Support plotting with multibyte characters #99
Conversation
|
Hi @edgar-bonet, very nice! Compiled and ran on Gentoo. Two small things: I get this warning when compiling: # make
cc -Wall -Wextra ttyplot.c `pkg-config --libs ncursesw 2>/dev/null || echo '-lcursesw -ltinfo'` -o ttyplot
ttyplot.c: In function ‘draw_line’:
ttyplot.c:135:9: warning: implicit declaration of function ‘mvvline_set’; did you mean ‘mvvline’? [-Wimplicit-function-declaration]
135 | mvvline_set(ph+1-l1, x, c1, l1-l2 );
| ^~~~~~~~~~~
| mvvlineThis is with GCC 12. The other thing is that the Before
After
|
|
@hartwork: Thanks for testing!
You may be using the wrong include file, or the wrong feature-test macro. On my Ubuntu, /usr/include/curses.h has this: #ifndef NCURSES_WIDECHAR
#if defined(_XOPEN_SOURCE_EXTENDED) || (defined(_XOPEN_SOURCE) && (_XOPEN_SOURCE - 0 >= 500))
#define NCURSES_WIDECHAR 1
#else
#define NCURSES_WIDECHAR 0
#endif
#endif /* NCURSES_WIDECHAR */It then declares all wide character functions (including Could you check how it works on your Gentoo? Is
I also like the “before” better. I'll have to check why this happened... |
@edgar-bonet same here.
A different file, yes: # fgrep -R mvvline_set /usr/include/ncurses*
/usr/include/ncursestw/ncurses.h:extern NCURSES_EXPORT(int) mvvline_set (int, int, const cchar_t *, int); /* generated:WIDEC */
/usr/include/ncursestw/ncurses.h:#define mvvline_set(y,x,c,n) mvwvline_set(stdscr,(y),(x),(c),(n))
/usr/include/ncursestw/curses.h:extern NCURSES_EXPORT(int) mvvline_set (int, int, const cchar_t *, int); /* generated:WIDEC */
/usr/include/ncursestw/curses.h:#define mvvline_set(y,x,c,n) mvwvline_set(stdscr,(y),(x),(c),(n))
/usr/include/ncursesw/ncurses.h:extern NCURSES_EXPORT(int) mvvline_set (int, int, const cchar_t *, int); /* generated:WIDEC */
/usr/include/ncursesw/ncurses.h:#define mvvline_set(y,x,c,n) mvwvline_set(stdscr,(y),(x),(c),(n))
/usr/include/ncursesw/curses.h:extern NCURSES_EXPORT(int) mvvline_set (int, int, const cchar_t *, int); /* generated:WIDEC */
/usr/include/ncursesw/curses.h:#define mvvline_set(y,x,c,n) mvwvline_set(stdscr,(y),(x),(c),(n))None of these seem to be pulled in from $ grep '#.*include' /usr/include/curses.h | sort
#include <libutf8.h>
#include <ncurses_dll.h>
#include <stdarg.h> /* we need va_list */
#include <stdbool.h>
#include <stddef.h> /* we want wchar_t */
#include <stdint.h>
#include <stdio.h>
#include <stdnoreturn.h>
#include <unctrl.h>
#include <wchar.h> /* ...to get mbstate_t, etc. */…so it may need an additional include.
It does, good idea! # pkg-config --cflags ncursesw
-D_DEFAULT_SOURCE -D_XOPEN_SOURCE=600 -I/usr/include/ncurseswSo the
My guess is that that is what |
There was a problem hiding this comment.
@edgar-bonet so it would be great to have the build system add pkg-config --cflags ncursesw to CFLAGS. PS: The pull request title has a small typo "mu[l]tibyte".
edgar-bonet
changed the title
|
@hartwork wrote:
I just pushed a commit with this change. Does it fix the warning you see? |
There was a problem hiding this comment.
I just pushed a commit with this change. Does it fix the warning you see?
@edgar-bonet yes it does, thank you!
|
About the axes ends changing from
Indeed. It appears that two conditions are needed for this to happen:
One way to avoid this change would be to unconditionally define @tenox7: Do your vintage Unix systems support wide characters? Specifically, is |
|
edgar-bonet
force-pushed
the
multibyte
branch
from
6dcb3a4
to
5bdee13
Compare
|
As this pull request's branch conflicted with master, I rebased and force-pushed. @hartwork, could you please test this on Gentoo? I am worried about the include file: the previous version of this pull request added /usr/include/ncursesw to the include path, whereas commit 45e0db8 (which landed in master today) changed the header name to ncurses.h. Rebasing the PR on master changes the include file again: Also, is the last commit's message correct? It says
|
|
feel free to change it to ncursesw or whatever is needed for wide chars, I just wanted to break away from old curses; or revert my change if you think |
|
@tenox7: For me, it makes no difference at all: on Ubuntu, the files
are all symlinks to /usr/include/curses.h. That file declares everything we may need: the classic curses API, the ncurses extensions and – given the proper The relevant question is about portability: What would be the most portable way to get the declarations we want? Are there systems where FWIW, the Debian package ncurses-doc contains man pages that recommend including --- a/usr/lib/x86_64-linux-gnu/pkgconfig/ncurses.pc
+++ b/usr/lib/x86_64-linux-gnu/pkgconfig/ncursesw.pc
@@ -1,19 +1,19 @@
# pkg-config file generated by gen-pkgconfig
# vile:makemode
prefix=/usr
exec_prefix=${prefix}
libdir=/usr/lib/x86_64-linux-gnu
includedir=${prefix}/include
abi_version=6
major_version=6
version=6.3.20211021
-Name: ncurses
+Name: ncursesw
Description: ncurses 6.3 library
Version: ${version}
URL: https://invisible-island.net/ncurses
Requires.private:
-Libs: -L/usr/lib/x86_64-linux-gnu -lncurses -ltinfo
+Libs: -L/usr/lib/x86_64-linux-gnu -lncursesw -ltinfo
Libs.private: -ldl
Cflags: -D_DEFAULT_SOURCE -D_XOPEN_SOURCE=600 |
@edgar-bonet my vote would be for requiring widely available |
|
@hartwork: This PR would not work with traditional curses: it relies on the wchar extensions, which come with ncurses. The thing is, we can usually import the ncurses API by including either Regarding |
@edgar-bonet I'd be happy either way, maybe with a slight preference for
👍 |
|
@hartwork: I'm happy with including I hadn't heard about PDCurses before. Seems niche indeed: it's described as “A curses library for environments that don't fit the termcap/terminfo model” (not what we target), it doesn't provide a Makefile for Unix other than X11 and SDL, and most distro packages describe it as “Curses for Windows/MinGW/X11”. I would say don't bother unless someone volunteers to maintain the port. Long term, it would be nice to have an easy way to run tests on multiple Linux and non-Linux OSes. A Vagrantfile could be an option if/when a list of officially supported systems is established. |
https://github.com/mpartel/bindfs/blob/5c8390f75925797a81973925a14cdf8ab092a3bc/.github/workflows/tests.yml#L108 has a CI using Vagrant based on GitHub Actions, but it's a bit slow whenever a CI runner does not come with KVM acceleration, there seems to be some roulette about it. Free alternatives for running Vagrant inside some CI may include AppVeyor and Cirrus CI, but it would need to be tried/verified. Also Vagrant images for all target OSes and the right driver would be needed, e.g. the VirtualBox Vagrant images only work if the driver is VirtualBox, same for KVM/Qemu. |
|
yeah lets make ncurses a requirement, I'm happy with that; ncurses are available even for very old unixes; I haven't seen or used pdcurses in a very very long time |
|
@tenox7: Did you have a chance to take look at this PR? As far an I am concerned, it is ready, but I would happily amend it if requested. Should I change the arrows at the end of the axes ( |
|
sorry, for slow responses, let me check |
|
I want to test it a little bit in different OSes and terminals. |
@edgar-bonet my vote for any of these over the current arrows, yes please. @tenox7 how do you feel about the topic? |
|
I pushed a commit that unconditionally sets the arrow tips to the ASCII characters |
|
@edgar-bonet good call 👍 |
|
It doesn't build on macOS: (repeated many times) |
|
@tenox7 wrote:
It would seem the wrong
Did you try just typing |
|
yes, just make on macos |
The options -c, -e and -E only support ASCII characters. Make them support multibyte characters too, as box drawing and block elements can make nice plotting glyphs. This requires wide character support from the "ncursesw" version of the ncurses library. Only characters supported by the user's locale can be used, which includes the full Unicode repertoire on UTF-8 locales. As the wide character API of ncurses does not support ACS characters, the default plotting character on locales lacking mutibyte support is now "|" (U+007C vertical line).
On Ubuntu, /usr/include/ncursesw/ncurses.h is a symlink to ../ncurses.h. On Gentoo, however, /usr/include/ncurses.h and /usr/include/ncursesw/ncurses.h are different files, and only the latter defines the wide character functions: we need to give the compiler the proper -I flag. For the sake of portability, ask pgk-config for the correct compiler flags (including -I). Default to the flags used on Gentoo, which also work on Ubuntu.
Enabling multi-byte support changes the arrow tips from the ASCII characters '^'/'>' to the Unicode arrows '↑'/'→' (U+2191 Upwards arrow and U+2192 Rightwards arrow). The ASCII characters look better on the graph: use them unconditionally.
edgar-bonet
force-pushed
the
multibyte
branch
from
055c736
to
3c41c23
Compare
|
@tenox7 wrote:
There is something fishy going on: Could you try to see how many files matching the glob PS: As there was a conflict, I rebased on master and force-pushed. |
|
I think there may be a conflict between curses from the SDK/Xcode and Homebrew. Checking. |
|
I did this and it worked: perhaps something like this? |
This macro enables the wide character API. Although it has been deprecated in favor of _XOPEN_SOURCE, macOS 14 ships a version of ncurses that does not understand the later. Furthermore, pkg-config fails to define it.
edgar-bonet
force-pushed
the
multibyte
branch
from
eb763b2
to
98ae048
Compare
|
@tenox7: Thanks for debugging the issue! I checked |
* Define _XOPEN_SOURCE_EXTENDED on macOS * Always use ASCII characters for the arrow tips * Rely on pkg-config to find the proper ncurses.h * Support plotting with mutibyte characters
This pull request addresses issue #84. Note that, in order for ncurses to support non-ASCII characters, we need to:
Ask for wide character support by defining a suitable macro before including
<curses.h>. I went for_XOPEN_SOURCE, but we could use the more explicitNCURSES_WIDECHARinstead.Link with the version of the library that provides this support. On my system (Ubuntu 22.04), this is ncursesw.so. Some tests should be done on other OSes in order to find the suitable link options.
One drawback of using the wide character version of
mvvline()is that it does not support ACS (alternative character set) characters. Since we cannot draw withACS_VLINE, we default to the visually equivalent “│” (U+2502 box drawings light vertical). This, however, cannot be done on the C locale (or any other 8-bit locale). In this case we use the ASCII character “|” (U+007C vertical line), which unfortunately leaves tiny gaps between successive character cells.With this PR, the command:
{ ./torture | head -75; sleep 1m; } | ./ttyplot -c ╳displays:
I tested with multiple plotting characters, inside and outside the BMP, and also with ASCII characters in the C locale. It works for me, but more testing would be welcome, especially on OSes other than Ubuntu.
Fixes #84.