feat: add Artwork::color and tests #4

Open
wlm-iii wants to merge 1 commit from liam/colors into dev/ben
Collaborator

Uses ANSI terminal colors and allows per-character customization to the
foreground and background. Includes a new Artwork constructor to color
automatically.

Uses ANSI terminal colors and allows per-character customization to the foreground and background. Includes a new `Artwork` constructor to color automatically.
Uses ANSI terminal colors and allows per-character customization to the
foreground and background. Includes a new `Artwork` constructor to color
automatically.
b0x207 requested review from b0x207 2026-06-23 22:34:42 +00:00
b0x207 left a comment

The rest of the code base has been transitioning to using namespace std in .cpp files. The artwork.cpp and artwork_test.cpp files should do so too. As an added benefit, it will help address some of the long line lengths.

The rest of the code base has been transitioning to `using namespace std` in `.cpp` files. The `artwork.cpp` and `artwork_test.cpp` files should do so too. As an added benefit, it will help address some of the long line lengths.
@ -82,0 +89,4 @@
* first -> foreground color
* second -> bacground color
*/
const std::map<char, const std::pair<const int, const int>> colors = {
Owner

Since this is a constant, it should be in SCREAMING_SNAKE case like the others.

Since this is a constant, it should be in SCREAMING_SNAKE case like the others.
b0x207 marked this conversation as resolved
src/artwork.cpp Outdated
@ -21,0 +79,4 @@
lines = colored_lines;
}
Artwork::Artwork(const std::string &raw, int offset, const char *raw_fg, const char *raw_bg)
Owner

This should to be grouped with the other constructor at the top of the file

This should to be grouped with the other constructor at the top of the file
b0x207 marked this conversation as resolved
@ -0,0 +49,4 @@
TEST(Artwork, AllBackgrounds) {
Artwork art { "01234567\n89ABCDEF\n", 8};
art.color(" \n ", "01234567\n89ABCDEF");
std::string expected {"\x1b[39;40m0\x1b[39;41m1\x1b[39;42m2\x1b[39;43m3\x1b[39;44m4\x1b[39;45m5\x1b[39;46m6\x1b[39;47m7\x1b[0m\n\x1b[39;100m8\x1b[39;101m9\x1b[39;102mA\x1b[39;103mB\x1b[39;104mC\x1b[39;105mD\x1b[39;106mE\x1b[39;107mF\x1b[0m"};
Owner

Perhaps look into splitting this over multiple lines? Maybe something like

    std::string expected {
        "\x1b[39;40m0\x1b[39;41m1\x1b[39;42m2\x1b[39;43m3\x1b[39;44m4"
        "\x1b[39;45m5\x1b[39;46m6\x1b[39;47m7\x1b[0m\n\x1b[39;100m8"
        "\x1b[39;101m9\x1b[39;102mA\x1b[39;103mB\x1b[39;104mC\x1b[39;105mD"
        "\x1b[39;106mE\x1b[39;107mF\x1b[0m"
    };

All of these expected testing strings could benefit from a similar approach (pretty much just wrapping at 80 columns).

Perhaps look into splitting this over multiple lines? Maybe something like ```cpp std::string expected { "\x1b[39;40m0\x1b[39;41m1\x1b[39;42m2\x1b[39;43m3\x1b[39;44m4" "\x1b[39;45m5\x1b[39;46m6\x1b[39;47m7\x1b[0m\n\x1b[39;100m8" "\x1b[39;101m9\x1b[39;102mA\x1b[39;103mB\x1b[39;104mC\x1b[39;105mD" "\x1b[39;106mE\x1b[39;107mF\x1b[0m" }; ``` All of these expected testing strings could benefit from a similar approach (pretty much just wrapping at 80 columns).
b0x207 marked this conversation as resolved
b0x207 left a comment

The main changes I'm looking for are just moving most error checking to guard clauses and cleaning up various sections of color() as a result of that. I'm also a bit confused about some of your test cases which seem to break the expected rules that delimiters must be a length exact (line and total) mask over the uncolored artwork.

The main changes I'm looking for are just moving most error checking to guard clauses and cleaning up various sections of `color()` as a result of that. I'm also a bit confused about some of your test cases which seem to break the expected rules that delimiters must be a length exact (line and total) mask over the uncolored artwork.
src/artwork.cpp Outdated
@ -21,0 +35,4 @@
char curr_fg {' '};
char curr_bg {' '};
string_view fg {raw_fg};
string_view bg {raw_bg};
Owner
vector<string> fg { split_lines(raw_fg) };
vector<string> bg { split_lines(raw_bg) };

See the comment on line 53.

```cpp vector<string> fg { split_lines(raw_fg) }; vector<string> bg { split_lines(raw_bg) }; ``` See the comment on line 53.
src/artwork.cpp Outdated
@ -21,0 +38,4 @@
string_view bg {raw_bg};
ZipRange<string_view, string_view> color_delims {fg, bg};
ZipRange<string_view, string_view>::iterator iter_delims {color_delims.begin()};
ZipRange<string_view, string_view>::iterator end_delims {color_delims.end()};
Owner

This is one of those places where auto is really nice and we have plenty of context to know what the real type is. I would also suggest making using of template parameter inference and just use ZipRange.

This is one of those places where `auto` is really nice and we have plenty of context to know what the real type is. I would also suggest making using of template parameter inference and just use `ZipRange`.
src/artwork.cpp Outdated
@ -21,0 +47,4 @@
for (int col {}; col < line_width; ++col, ++iter_delims) {
pair<char, char> color_pair {*iter_delims};
stringstream color_esc_seq {};
color_esc_seq << "\x1b[";
Owner

This could be easily moved to the if statement where the rest of the escape code is built and might be in a better context for readers.

This could be easily moved to the if statement where the rest of the escape code is built and might be in a better context for readers.
src/artwork.cpp Outdated
@ -21,0 +50,4 @@
color_esc_seq << "\x1b[";
if (color_pair.first == '\n' || color_pair.second == '\n') {
throw runtime_error("Unexpected newline");
Owner

I'd recommend pulling as many error/invalid states to the top of your logic as possible. Here, the uncolored artwork and the delimiters share the same basic processing which is splitting by newlines. I'd recommend making a private helper method that splits lines.

vector<string> Artwork::split_lines(string_view raw) {
    int j = 0;
    vector<string> result;

    for (int i = 0; (i = raw.find("\n", i)) != string::npos; ++i) {
        result.push_back(string { raw.substr(j, i - j) });
        j = i + 1;
    }

    return result;
}

Then I would split the delimiters before processing. This means that no unexpected newlines will appear and checking for the right format can simply be reduced to:

vector<string> fg { split_lines(raw_fg) };
vector<string> bg { split_lines(raw_bg) };
ZipRange color_delims {fg, bg};

auto split_size = [](int ac, string &s) { return ac + s.size(); };
int fg_size = accumulate(fg.begin(), fg.end(), 0, split_size);
int bg_size = accumulate(bg.begin(), bg.end(), 0, split_size);
int lines_size = accumulate(lines.begin(), lines.end(), 0, split_size);
if (fg_size != bg_size)
  throw runtime_error("Delimiters are not of equal size");
if (fg_size != lines_size || bg_size != lines_size)
  throw runtime_error("Delimiters are not equal in size to the artwork");

Once the input has passed all of your checks, then you can proceed with your logic. By handling things this way, you should be able to clean up some of the logic code as well.

I'd recommend pulling as many error/invalid states to the top of your logic as possible. Here, the uncolored artwork and the delimiters share the same basic processing which is splitting by newlines. I'd recommend making a private helper method that splits lines. ```cpp vector<string> Artwork::split_lines(string_view raw) { int j = 0; vector<string> result; for (int i = 0; (i = raw.find("\n", i)) != string::npos; ++i) { result.push_back(string { raw.substr(j, i - j) }); j = i + 1; } return result; } ``` Then I would split the delimiters before processing. This means that no unexpected newlines will appear and checking for the right format can simply be reduced to: ```cpp vector<string> fg { split_lines(raw_fg) }; vector<string> bg { split_lines(raw_bg) }; ZipRange color_delims {fg, bg}; auto split_size = [](int ac, string &s) { return ac + s.size(); }; int fg_size = accumulate(fg.begin(), fg.end(), 0, split_size); int bg_size = accumulate(bg.begin(), bg.end(), 0, split_size); int lines_size = accumulate(lines.begin(), lines.end(), 0, split_size); if (fg_size != bg_size) throw runtime_error("Delimiters are not of equal size"); if (fg_size != lines_size || bg_size != lines_size) throw runtime_error("Delimiters are not equal in size to the artwork"); ``` Once the input has passed all of your checks, then you can proceed with your logic. By handling things this way, you should be able to clean up some of the logic code as well.
src/artwork.cpp Outdated
@ -21,0 +56,4 @@
if (!artwork::COLORS.contains(color_pair.first) ||
!artwork::COLORS.contains(color_pair.second)
) {
throw runtime_error("Invalid color delimiter");
Owner

This too can be checked for at the beginning, however, that would involve a complete iteration over the delimiters so I think it is acceptable to consider it here. Since it can only be triggered when someone is manually editing the source code or future config files, knowing what character caused the error is important (and a time saver for the user). A simple addition of ": (" + color_pair.first + ", " + color_pair.second + ")" might do the trick. Feel free to find a better way to express this of course.

This too can be checked for at the beginning, however, that would involve a complete iteration over the delimiters so I think it is acceptable to consider it here. Since it can only be triggered when someone is manually editing the source code or future config files, knowing what character caused the error is important (and a time saver for the user). A simple addition of `": (" + color_pair.first + ", " + color_pair.second + ")"` might do the trick. Feel free to find a better way to express this of course.
src/artwork.cpp Outdated
@ -21,0 +77,4 @@
pair<char, char> color_pair {*iter_delims};
if (color_pair.first == '\n' || color_pair.second == '\n') {
if (color_pair.first != color_pair.second) {
throw runtime_error("Foreground/background not the same length.");
Owner

See the comment on line 53. In general, it means that this set of nested if statements is not needed which is good because it makes little sense to me. I'll point out that the condition color_pair.first == '\n' || color_pair.second == '\n' can only fail if the delimiter lines are of a different length than the artwork. This seems like an error state to me and is something I would want to check for before I've already done a bunch of processing (see the aforementioned comment for an example).

With the other changes in mind, this whole block seems to be reducible to just ++iter_delims.

See the comment on line 53. In general, it means that this set of nested if statements is not needed which is good because it makes little sense to me. I'll point out that the condition `color_pair.first == '\n' || color_pair.second == '\n'` can only fail if the delimiter lines are of a different length than the artwork. This seems like an error state to me and is something I would want to check for before I've already done a bunch of processing (see the aforementioned comment for an example). With the other changes in mind, this whole block seems to be reducible to just `++iter_delims`.
@ -0,0 +25,4 @@
{
Artwork art { "foo\nbar\nbaz\n", 3};
ASSERT_EQ(str(art), "foo\nbar\nbaz");
art.color( " \n \n ", " \n \n ");
Owner

The provided delimiters don't seem quite right to me. Since the source artwork has 3 newlines the delimiters should too. This appears to be a problem in all of the test cases but not with the actual delimiters in the artwork namespace.

The provided delimiters don't seem quite right to me. Since the source artwork has 3 newlines the delimiters should too. This appears to be a problem in all of the test cases but not with the actual delimiters in the `artwork` namespace.
@ -0,0 +79,4 @@
ASSERT_EQ(str(art), "foo\nbar\nbaz");
ASSERT_THROW(
{art.color( " \n \n \n ", " \n \n ");},
runtime_error
Owner

Why not also check for the correct type of error message?

Why not also check for the correct type of error message?
This pull request can be merged automatically.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin liam/colors:liam/colors
git switch liam/colors

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch dev/ben
git merge --no-ff liam/colors
git switch liam/colors
git rebase dev/ben
git switch dev/ben
git merge --ff-only liam/colors
git switch liam/colors
git rebase dev/ben
git switch dev/ben
git merge --no-ff liam/colors
git switch dev/ben
git merge --squash liam/colors
git switch dev/ben
git merge --ff-only liam/colors
git switch dev/ben
git merge liam/colors
git push origin dev/ben
Sign in to join this conversation.
No reviewers
No labels
No milestone
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
b0x207/peterfetch!4
No description provided.