Take advantage of all terminal colours (#10) #22

StefansM · 2018-03-30T16:34:26Z

This commit lets cpipes take advantage of all the colours available to the terminal (mostly).

On terminals that support the initc terminfo capability, the terminal colours are set to a colour palette. The colours of the palette may be specified with multiple -c options. The default is a full sweep of HSL space at maximum saturation and half lightness (i.e. the most vivid colours available), split into as many colours as are available. Some terminals with a broken oc capability (cough urxvt cough) do not reset colours when delscreen is called, which breaks the look of ls, vim and any other programs that use terminal colours. The --backup-colors switch can be used to try and query the current colours from the terminal and restore them before exiting. This has to work with raw escape codes, because terminfo doesn't support initc queries, so it's very terminal-dependent and should be avoided on terminals with a working oc.

On terminals that support direct colour (determined via the RGB terminfo capability, so this requires ncurses >= 6), direct colour is used to write colours, so initc is not used. The number of colours in the palette is still limited by the number of colour pairs available in ncurses, because ncurses does not support setting colours outside of colour pairs. Practically speaking, this is not a problem: terminals modern enough to support direct colour probably have 65535 colour pairs.

This pull requests adds some autoconf hackery to detect the features available to ncurses. We prefer init_extended_color over init_color, and alloc_pair over init_extended_pair over init_pair. Interestingly, using alloc_pair allows 65535 colour pairs to be used, but init_extended_pair and init_pair only ever allow half that, because the classic curses uses a signed short to store COLOR_PAIRS. At least, I think that's the reason: it's a mess of features shoehorned into a backwards-compatible ABI. I think this may be the only project on GitHub actually using alloc_pair. The others I found seem to be either implementing LISP interpreters or copies of the ncurses source.

Here's a gif in action with the parameters "$(for i in {0..25}; do printf -- "-c 0000%02X " $((i * 10)); done)":

It suffers a bit from conversion to a gif.

It even works on the Linux terminal. It looks rubbish because the chars don't line up correctly, but the colours work (all eight of them):

StefansM · 2018-03-30T16:36:00Z

I've just noticed that any error messages to do with the colour palette are cleared when the program exits because they are printed before delscreen is called. I won't merge this until I fix that.

livibetter · 2018-03-31T03:14:27Z

src/cpipes.c

@@ -24,6 +25,7 @@

 void interrupt_signal(int param);
 void parse_options(int argc, char **argv);
+static void find_num_colours(int argc, char **argv);


Before this commit grep -o color src/*.{c,h} | wc -l shows 2 occurrences for color and 18 for colour; as of 2dc1121, 136 for color and 88 for colour.

I think it's better you use only one spelling in your code regardless how ncurses spells it.

livibetter · 2018-03-31T04:14:09Z

src/render.c

+    // 8 colours.
+    int max_pipe_colors = min(COLORS, COLOR_PAIRS);
+
+#if !HAVE_ALLOC_PAIR


Here is the output I got as of 4b143e5:

src/render.c: In function 'palette_size': src/render.c:84:6: warning: "HAVE_ALLOC_PAIR" is not defined [-Wundef] #if !HAVE_ALLOC_PAIR ^~~~~~~~~~~~~~~ src/render.c:92:6: warning: "HAVE_EXTENDED_COLOR" is not defined [-Wundef] #if !HAVE_EXTENDED_COLOR ^~~~~~~~~~~~~~~~~~~ src/render.c: In function 'set_pair': src/render.c:245:5: warning: "HAVE_ALLOC_PAIR" is not defined [-Wundef] #if HAVE_ALLOC_PAIR ^~~~~~~~~~~~~~~ src/render.c:248:7: warning: "HAVE_EXTENDED_COLOR" is not defined [-Wundef] #elif HAVE_EXTENDED_COLOR ^~~~~~~~~~~~~~~~~~~ src/render.c: In function 'set_color_pair_indirect': src/render.c:275:5: warning: "HAVE_EXTENDED_COLOR" is not defined [-Wundef] #if HAVE_EXTENDED_COLOR ^~~~~~~~~~~~~~~~~~~

Perhaps using the following check instead for these -Wundef warnings?

#if !defined(HAVE_ALLOC_PAIR)

or just using ifndef and ifdef since we only check one by one.

livibetter · 2018-03-31T04:18:11Z

src/render.c

 void render_pipe(struct pipe *p, char **trans, char **pipe_chars,
        int old_state, int new_state){

    move(p->y, p->x);
-    attron(COLOR_PAIR(p->colour));
+    attr_set(A_NORMAL, 1, &p->colour);


In file included from src/render.c:6:0: src/render.c: In function 'render_pipe': /usr/include/ncursesw/curses.h:1301:13: warning: right-hand operand of comma expression has no effect [-Wunused-value] #define wattr_set(win,a,p,opts) (((win) \ ~~~~~~~~~ ? ((win)->_attrs = ((a) & ~A_COLOR), \ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ (win)->_color = (p)) \ ~~~~~~~~~~~~~~~~~~~~~~ : OK), \ ~~~~~^~~ OK) ~~~ /usr/include/ncursesw/curses.h:1206:26: note: in expansion of macro 'wattr_set' #define attr_set(a,c,o) wattr_set(stdscr,(a),(c),(o)) ^~~~~~~~~ src/render.c:453:5: note: in expansion of macro 'attr_set' attr_set(A_NORMAL, 1, &p->colour); ^~~~~~~~

I think all warnings should be treated as errors, that is -Werror, but configure.ac has something for a specific warning and I don't know how we can work around these three diagnoses (-Wunused-value and two notes), when the problem is in ncurses' code. Well, by we, I meant you will figure it out, I am sure. :)

I wasn't able to replicate this on my system, using either gcc 7.3.1 or clang 6.0.0 with ncurses 6.1. I don't know about clang, but I'm fairly sure gcc will suppress warnings from system library headers. Could you give me some info about your system so I can try and reproduce this?

Regarding errors as warnings, configure.ac was supposed to set -Werror for non-release builds---that is, builds that aren't exactly on a tag. However, I forgot to add --tags to the git describe command line that gets the version, and the GitHub uses lightweight tags for releases, so git describe --always was just giving the abbreviated commit ID! This obviously doesn't contain a hyphen, so configure thought it was always building a release. Frankly, I have no idea how I manged to build the 1.0.0 tarball with make dist: if I check out v1.0.0 now and run make dist I get pipes-c-d161b9c.tar.gz. I will fix this in a separate PR immediately and rebase this PR onto it.

Finally, I agree that most warnings should be treated as errors, but not all. I currently have a couple of unused parameters that I don't care about in functions that are called by function pointers, but those functions need to have a consistent signature. I'll leave the compiler flags as they are for the moment: it should be more useful after fixing configure.ac.

Your mention about ncurses was very helpful, I was using ncurses 6.0 and so I updated to 6.1, now GCC 6.4.0 compiles it without any warnings (curses.h and the five -Wundef warnings)

This is made clear to me, I think we should require ncurses 6.1+ even though pipes.c can be compiled and run with ncurses 6.0 and those warnings.

Forgot to also mention that clang 3.9.1 doesn't show the warnings on curses.h of ncurses 6.0.

(3.9.1 is still in Gentoo Portage tree as stable, the latest stable is 5.0.1, but I don't have it installed. Travis CI has 5, but ncurses version is unknown from the log)

livibetter · 2018-03-31T05:01:01Z

st doesn't seem to support can_change_color (== false) if with TERM=st-256color, even changed to xterm-256color, only first -c color registers, I didn't trace into, so don't what happened to the rest.

~~We might need to add a notice for -c, indicating what terminal emulators would work properly.~~ (this exactly what @StefansM's first comment is about)

livibetter · 2018-03-31T16:11:12Z

src/render.c

+ * Note that the color should be zero-indexed. This function internally
+ * adds 1 to it to avoid overwriting any of the default colours.
+ */
+int set_color_pair_indirect(int color_index, int color) {


I can see color has 24 bits, so unsigned long should be completely as int is only guaranteed to be at least 16 bits and unsigned as your note above states it's zero-indexed.

livibetter · 2018-04-03T07:54:44Z

@StefansM

This is quick and dirty, but you get the idea.

#define fprintf(...)  endwin(); fprintf(__VA_ARGS__); refresh()
#define perror(...)   endwin(); perror(__VA_ARGS__);  refresh()

I am relearning C recently, and learned that the currently expanded macro is temporarily disabled to avoid infinite recursion, so this seems to be a good use of it. They might have serious flaw or risk as my C isn't good, and might not work nicely with ncurses and in pipes.c. Please do get me some pointers.

I think a specific message printing function to use within ncurses would be a saner option.

(Off-topic: just accidentally discovered user hovercard, this didn't show up in my homepage)

StefansM · 2018-04-03T18:03:41Z

Thanks @livibetter - I'm been afk for a few days but will take a look sometime this week when I get the chance. All very good points in your comments though!

In preparation for using `init_color`.

This commit adds a colour palette. Our "colours" are ncurses "pairs". We define a palette as an ordered list of pair IDs. We define it like this because the pair IDs returned by `alloc_pair` are not necessarily consecutive, and we want to know the relationship between color `i` and `i+1`. On terminals that allow it, we redefine the actual colours used by the terminal. Alternatively, on terminals that support direct colour we use that. The default palette is a sweep of HSL space at maximum lightness and saturation.

Some terminals support querying the current colours of the terminal. We can use this to reset the colours of the terminal on broken terminals that don't obey the "oc" (original colors) capability.

This was based on a misreading of the `init_color` man page.

The American cultural juggernaut wins again. This commit changes all "colours" to "colors". While this obviously makes everything much worse, the previous commits on this branch have used ncurses' "*color*" routines a lot, and the inconsistencies were starting to become confusing. Since we can't convert ncurses to the correct spelling, I've moved to the American spelling everywhere. 😄

This commit fixes the width of a "color" to a 32-bit unsigned integer. In practise, this is unlikely to matter: any system this is likely to compile on is likely to have an int greater than 24 bits. There's no harm in being more rigorous, though. At the same time, we also differentiate functions that return a "size" (e.g. number of colours), and return a size_t rather than an int. Again, this is unlikely to matter, but there is no harm.

Added routines for recording and displaying errors. These are useful because a simple "fprintf(stderr, ...)" will be obscured by the ncurses window. To record an error, call "set_error()" with a "cpipes_errno" as an argument. A cpipes_errno is just a typedef for an element of the enum PIPES_C_ERRORS. C doesn't make the distinction, so these can all be treated as negative integers. Some errors take arguments, which should be passed into set_error(): see the "PIPES_C_ERROR_STRINGS" array in "err.c". Information can be added to an error message to provide contextualising information as a kind of poor man's stack trace.

When --backup-colors was passed on a terminal that doesn't support querying colors, the loop index could underflow and attempt to free an arbitrary location.

The previous test always returned an exit status of zero, and the test driver wasn't actually interpreting the TAP output so the test always passed. This commit removes the attempt at parsing the TAP output, and makes sure to set the exit value correctly upon test failure.

Where possible, we use the "format" function attribute to tell the compiler that we are wrapping printf. When not possible, we disable the warning via a pragma.

On Mac, terminfo functions use a "char *" rather than a "const char *" for capability names. This commit explicitly copies the string literal capability name into a char buffer.

StefansM · 2018-04-14T20:40:08Z

Phew! Thanks Travis!

Issues resolved, but I do not read the changes.

livibetter · 2018-04-15T05:30:26Z

Now I can see the error message and I realized that I need to run with TERM=st-direct in order to "take advantage of all terminal colours."

livibetter reviewed Mar 31, 2018

View reviewed changes

livibetter previously requested changes Mar 31, 2018

View reviewed changes

livibetter reviewed Mar 31, 2018

View reviewed changes

StefansM mentioned this pull request Apr 6, 2018

Fixes for build system (notably the version number) #23

Merged

StefansM added 9 commits April 7, 2018 12:22

Add checks for extended ncurses functions

66de05f

In preparation for using `init_color`.

Save and restore colour codes when possible

2515c31

Some terminals support querying the current colours of the terminal. We can use this to reset the colours of the terminal on broken terminals that don't obey the "oc" (original colors) capability.

Add option to save/restore colours

5ef6ee8

Add option to specify palette

82b5402

There is no need to preserve the default 8 colours

6270f07

This was based on a misreading of the `init_color` man page.

Use fabfs to avoid double promotion

6939b22

StefansM force-pushed the feature-colors branch from 4b143e5 to d07feff Compare April 14, 2018 19:30

StefansM added 4 commits April 14, 2018 20:37

Fix possible segfault with --backup-colors

c3a308a

When --backup-colors was passed on a terminal that doesn't support querying colors, the loop index could underflow and attempt to free an arbitrary location.

Fix -Wundef preprocessor warnings

eaa8a22

StefansM force-pushed the feature-colors branch from d07feff to eaa8a22 Compare April 14, 2018 19:37

StefansM added 4 commits April 14, 2018 20:53

Fix -Wabsolute-value warnings in clang

45f321b

Fix size of "in" buffer in test

d201555

Fix or ignore warnings for -Wformat-nonliteral

bb2dc56

Where possible, we use the "format" function attribute to tell the compiler that we are wrapping printf. When not possible, we disable the warning via a pragma.

Address warnings on Mac for warnings with terminfo

3e5b59f

On Mac, terminfo functions use a "char *" rather than a "const char *" for capability names. This commit explicitly copies the string literal capability name into a char buffer.

StefansM merged commit bcaa6d5 into pipeseroni:master Apr 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Take advantage of all terminal colours (#10) #22

Take advantage of all terminal colours (#10) #22

StefansM commented Mar 30, 2018

StefansM commented Mar 30, 2018

livibetter Mar 31, 2018

livibetter Mar 31, 2018

livibetter Mar 31, 2018

livibetter Mar 31, 2018

StefansM Apr 6, 2018

livibetter Apr 7, 2018

livibetter Apr 7, 2018

livibetter commented Mar 31, 2018 •

edited

Loading

livibetter Mar 31, 2018

livibetter commented Apr 3, 2018

StefansM commented Apr 3, 2018

StefansM commented Apr 14, 2018

livibetter commented Apr 15, 2018

Take advantage of all terminal colours (#10) #22

Take advantage of all terminal colours (#10) #22

Conversation

StefansM commented Mar 30, 2018

StefansM commented Mar 30, 2018

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

livibetter commented Mar 31, 2018 • edited Loading

Choose a reason for hiding this comment

livibetter commented Apr 3, 2018

StefansM commented Apr 3, 2018

StefansM commented Apr 14, 2018

livibetter commented Apr 15, 2018

livibetter commented Mar 31, 2018 •

edited

Loading