storing color in variable

[Lecrapouille]

Hi ! this is not an issue. I get inspired by your library. I need to store style and fg color in a variable for a postponed usage. Here is the idea:

rang::color mycolor = std::pair(rang::red, rang::italic);
std::cout << mycolor << "hello" << std::endl;

So why not adding in your lib something like:

struct color
  {
    color(rang::style style,
          rang::fg fg,
          rang::bg bg = rang::bg::reset)
      : m_style(style), m_fg(fg), m_bg(bg)
    {
    }

    rang::style m_style;
    rang::fg m_fg;
    rang::bg m_bg;
  };

inline std::ostream& setColor(std::ostream &os, rang::color const c)
  {
    return os << "\033["
      << static_cast<int>(c.m_fg) << ';'
      << static_cast<int>(c.m_bg) << ';'
      << static_cast<int>(c.m_style) << "m";
  }

A simple test code could be:

#define ERROR_COLOR  rang::color(rang::style::bold, rang::fg::red)
rang::color c = ERROR_COLOR;
std::cout << c << "hello" << std::endl;

I dunno what could be the best: std::pair (ugly and long syntax but can be used with constexpr) or struct (drawback: make copy). personally I would prefer something like red | italic (but can achieve easily). Other solution ?

(Maybe your lib already can do this, in this case ignore my comment)

[agauniyal]

This is a great idea, will definitely reduce noise in those cout statements :+1:.

I dunno what could be the best: std::pair (ugly and long syntax but can be used with constexpr) or struct (drawback: make copy).

Afaik the rang::color struct/class can be constexpr as well and I wouldn't worry about copies here(they're so small :stuck_out_tongue:). I'll look into implementing this through this weekend but there's a higher priority task pending in version4 branch(bringing cursor movements to library) so can't gurantee when it'll land. Anyways awesome suggestion :100:

[Lecrapouille]

Ok but beware:

• in the setColor(), given in this example, I probably get wrong with the order of style/fg/bg: the order impacts a lot on the display (I gave up adding bg color in the structure)
• you will probably have to adapt your using enableStd = typename std::enable_if< ... for adding the color structure on it, else I just noticed that g++-5.4 got problem with operator<< (with my code).
• Also as alternative for rang::setControlMode(x) you can overload operator<< to pass it like:

std::cout << control::Off << green << std::endl;

But I'm not sure this is thread safe.

Here is my attempt with all cited points: https://github.com/Lecrapouille/SimTaDyn/blob/development/src/common/utils/TerminalColor.hpp unit tests: https://github.com/Lecrapouille/SimTaDyn/blob/development/tests/common/utils/TerminalColorTests.cpp

[HemilTheRebel]

@Lecrapouille , may you please to take a look at my pull request? It implements exactly that. May be you could help me with naming and (good) design?

[Lecrapouille]

@HemilTheRebel is this one https://github.com/agauniyal/rang/pull/107 ? Ok I'll give a try. (note: I just updated my previous comment just to add unit tests because last time I said I have not made one)

[HemilTheRebel]

Yes. It is the one i am talking about. Even i forgot to reference my PR.

[Lecrapouille]

@HemilTheRebel I've never deal with PR, but it seems you changed indentation of the original code (I had to activate the option "hide whitespace changes" to see you real changes). You should have create a special PR just for fixing indentation, else it will difficult for people understanding your change once in git history. Also try to follow the author coding style "if(" vs "if (" same for the "for" and for comments.

Except that I see nothing wrong with design when reading your code. Just some minor improvements:

• I don't think that your PR is not related with issue #52: we are saving colors in a variable while in their issue they save a colorful stream. In this I would not use the name RangBuffer but simply Color.
• I would name bit position with a private enum (less risk of coding error).
• I dunno if it is faster to "concatenate" all your colors into a single variable when calling your functions set*Color() in the aim to avoid all the if-then-else in the << operator. This would save CPU for this function. Here the idea:

class RangBuffer
{
private:
  void updateFinalColor();
  std::string m_final_color;
...
};

void RangBuffer::setFgBColor(rang::fgB fgB)
{
  // be careful with variables member initialization this code can be wrong, else remove this if
  if (fgBColor != fgB)
  {
    fgBColor = fgB;
    elementSet[4] = true;
    updateFinalColor();
  }
}

// Naive code
void RangBuffer::updateFinalColor()
{
  m_final_color.clear();
  if(r.elementSet.test(0))
    m_final_color += r.style;
  if(r.elementSet.test(1))
    m_final_color += r.bgColor;
  if(r.elementSet.test(2))
    m_final_color += r.bgBColor;
  if(r.elementSet.test(3))
    m_final_color += r.fgColor;
  if(r.elementSet.test(4))
    m_final_color += r.fgBColor;
}

friend std::ostream& operator<<(std::ostream& os, const RangBuffer& r)
{
  os << r.m_final_color;
}

• I would restore your older clear() function. I think it was a good idea. But probably rename it as reset(). Like this you have both reset() and reset(os).
• Method names are a little too long in my opinion. For example "bg(const rang::bg color)" instead of "setBgColor" and why not renaming ignoreMySettings() by noColor() ? But deal with the original author my english is poor.
• You should not write your feelings in comments "an interesting function" stay neutral :)

I'm not the author of this library so the final world does not belong to me.

[HemilTheRebel]

Thank you so much @Lecrapouille .

I made the following changes:

1. Removed the bitset. After seeing your code, i realized all i needed was a std::string in my class. That was it. Removed every bit of unnecessary thing!
2. Concatenated all colors into a string variable. The operator << is clean of unnecessary if-else.
3. I don't think we need two clear functions but added them anyway. One function takes no parameters and resets the m_final_color to "", resetting the buffer; the other resets the buffer as well as the std::ostream.
4. Set come parameters const as per you recommendation
5. Shortened the functions to bg(const rang::bg) as you said. Renamed ingoreMySettings to ignore(). Sorry, i didn't make it noColor() because I felt the function needs to have "ignore" in it. Just fell inline with my intuition. (If you wish I will change that).
6. Removed "an interesting function" from the comment.

[Lecrapouille]

@HemilTheRebel seems good to be ! Maybe m_finalColor can be directly be a stream, this could avoid conversion stream -> string -> stream

[HemilTheRebel]

Done. How can I not figure that out? Stupid me. Thanks a lot