Closed djcas9 closed 8 years ago
Relates to #72.
@mephux please take into account that after submitting the review I edited/removed some of my initial comments.
@jroimartin all changes made - the only one Im confused on is the escape part. It seems the logic that's already in parseOne works for 256 support.
The only annoying part is 38 is used for FG and 48 for BG and if we use a case for catching 0...256 it will call into that.
Any ideas?
The lib i was using for 256 output was github.com/aybabtme/rgbterm. Maybe we should just add helpers to gocui for writing color strings? then we can use:
ESC[ … 38;2;<r>;<g>;<b> … m Select RGB foreground color
ESC[ … 48;2;<r>;<g>;<b> … m Select RGB background color
and pass in straight r/g/b values vs a converted rgb value.
@jroimartin, Last comment for tonight. I changed my code just to use the 256 terminal code vs. trying to convert RGB to the closest match. https://github.com/mephux/komanda-cli/blob/256/komanda/color/color.go
Still, have issues with the colors being wrong.. i.e., Red for some reason doesn't want to work.. haha dunno, will continue in the morning. It's getting closer at least.
@jroimartin ok, it's ready to rock&roll - also added a 256 color example. Guess it just comes down to how the mode is set and making the code not so ugly.
Great! I'll give it a look in the next days.
@jroimartin awesome, thanks - small update, fixed the _examples. See https://github.com/mephux/komanda-cli/tree/256 for screenshots of a super colorful gocui app. haha
BTW I love the screenshots of komanda-cli with 256-colors support! :D
@jroimartin thanks
Ok, I fixed everything outside the support for Normal with 256. Maybe I'm missing something but I'm not sure that will be possible?
@mephux 256 with normal support should be possible though.
@libeclipse @jroimartin just added that per the comments above.. it works.. just tested it in my project.
@jroimartin ok, that should be it -- sorry for making this such a pain in the ass. I had to learn all about terminal escaping. lol
Not at all! It's a great PR :)
@jroimartin fixed
Cool! I think we are ready to merge. Can you squash your commits?
@jroimartin sure - one sec
@jroimartin done - thanks!
Perfect! Let me do a couple of style changes (removing some empty lines) and update the AUTHORS file and will merge :)
@jroimartin roger, thanks for the help along the way!
Merged! Thanks a lot for your contribution! :D
BTW I did a couple of style changes and added 8-colors escape sequences to _examples/colors256.go
.
@jroimartin no problem! Thanks for accepting it. :)
Known issue with gray-scale colors