Expose original colour metadata/palette indices to users

[Andreas Stöckel]

This patch essentially implements this blueprint:

https://blueprints.launchpad.net/libvterm/+spec/palette-index

Note that my design decision was to not change the size of the VTermColor struct, which before (including alignment) was four bytes. Correspondingly, as of this patch, VTermColor has become a union and either contains a palette index or an RGB colour. All colours can always be converted to RGB colours by either calling

void vterm_state_get_rgb_color(VTermColor *col)

or

void vterm_screen_get_rgb_color(VTermColor *col)

depending on the interface that is being used. If this function is called before any VTermColor
is accessed, the behaviour of libvterm should not change.

Note that VTermColor also exposes metadata regarding whether the given colour is the default foreground/background colour via VTERM_COLOR_IS_DEFAULT_FG/VTERM_COLOR_IS_DEFAULT_BG. This is important when implementing terminal multiplexers or things like transparent backgrounds.

[PATCH] Expose original colour indices through `VTermColor`
https://github.com/astoeckel/libvterm/commit/35cc1afea7a100c4fc7511be0f60e7a2bd3a38bc.patch

[Paul "LeoNerd" Evans (leonerd)]

Overall this is looking very nice - I like that you've nicely documented the new functions in comments, and updated the tests.

A couple of small points on API naming though:

 * I dislike the name `get` in `vterm_state_get_rgb_color()` (and its `screen` variant). A `get` function really ought to have no mutation side-effect, other than perhaps on a pointer argument that the caller didn't first need to initialise. I think a better name for this would probably draw attention to its conversion nature, maybe one of

    void vterm_state_color_to_rgb();
    void vterm_state_convert_color_rgb();
    void vterm_state_convert_color_to_rgb();

   Either that, or change its behaviour so it takes a `const VTermColor` and returns its result by side-effecting another `VTermColor` which might just be a copy. That feels inefficient though.

 * Since it's now an API-exposed function, `vterm_color_equal()` should have a verb in its name to be properly named; I'd suggest

    bool vterm_color_is_equal(...);

One other point - I expect that now you should be able to delete the `fg_index` and `bg_index` members of VTermState and associated management code, without breaking anything.

[Andreas Stöckel (andreas-stoeckel)]

Thank you for your feedback! I've incorporated your suggestions into a new patch that can be found here:

[PATCH] Expose original colour indices through `VTermColor`
https://github.com/astoeckel/libvterm/commit/1f7a8e195e58cf30ba0e742e36b224254c2f7699.patch

Regarding renaming, I went with the longest variant, vterm_state_convert_color_to_rgb(), since I think that it is important to indicate that there is a conversion from an arbitrary color instance *to* an RGB colour happening.

I thought about the two-argument version of vterm_state_convert_color_to_rgb(), but I don't think that this is a good idea. With the current function signature for vterm_*_convert_color_to_rgb() adapting code from the old API to the new API should boil down to adding this function call before accessing the RGB colour components. The two-argument version would at least require another variable declaration.

I also renamed vterm_color_equal to vterm_color_is_equal as you suggested and got rid of the fg_index and bg_index variables.

In case you want to have a look at my changes since the last patch have a look at the "color" branch in my GitHub repository:

https://github.com/astoeckel/libvterm/commits/color

[Paul "LeoNerd" Evans (leonerd)]

Yeah that looks about right. I'll merge the patch, give or take some minor alterations:

* Code formatting - function opening braces should be on their own line
                  - uncuddled "} else {" braces

* vterm_state_getpen_color can be a static function, no need for INTERNAL.

727: Paul "LeoNerd" Evans 2018-12-22 Expose cell colour index as API value (LP1805035)