Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modification of colours rendering + 256 colours support #104

Merged
merged 43 commits into from
May 7, 2018

Conversation

Lynesth
Copy link
Collaborator

@Lynesth Lynesth commented May 5, 2018

Disclaimer: It isn't finished yet

So this adds support for 256 colors if the terminal supports it (it needs php-school/terminal#8 to work).

For now it only works if such colours are set using their number (0 to 255, see image attached) and doesn't yet fallback to 8 colours if not supported by terminal. I also want to add a full table of names->code association so that we can set them using their name.

It also modifies the way colours escaped sequences are generated so that it only happens as little as possible (when colours are set) and not each time.

It also makes use of the inverted escape sequence to set the colours for selected elements and the reset escape sequence to reset all colours/blod/whatever to their default value.

Let me know what you think so that I continue on this, or not. As with my previous PRs, I'll fix the tests if it's accepted.

List of colours:
ktsqa

Ain't it sexy ?
mintty_2018-05-06_01-17-49

@codecov-io
Copy link

codecov-io commented May 5, 2018

Codecov Report

Merging #104 into master will decrease coverage by 0.37%.
The diff coverage is 95.18%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #104      +/-   ##
============================================
- Coverage      96.7%   96.32%   -0.38%     
- Complexity      297      307      +10     
============================================
  Files            22       23       +1     
  Lines           911      926      +15     
============================================
+ Hits            881      892      +11     
- Misses           30       34       +4
Impacted Files Coverage Δ Complexity Δ
src/CliMenu.php 91.62% <100%> (ø) 70 <0> (-1) ⬇️
src/Input/InputIO.php 98.42% <100%> (-0.03%) 24 <0> (ø)
src/CliMenuBuilder.php 98.69% <100%> (+0.03%) 47 <2> (ø) ⬇️
src/MenuStyle.php 94.69% <100%> (-0.1%) 39 <11> (+2)
src/Dialogue/Flash.php 100% <100%> (ø) 1 <0> (ø) ⬇️
src/Dialogue/Dialogue.php 90% <100%> (ø) 8 <0> (ø) ⬇️
src/Dialogue/Confirm.php 100% <100%> (ø) 4 <0> (ø) ⬇️
src/Util/ColourUtil.php 76.47% <76.47%> (ø) 9 <9> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 06534f7...be09a76. Read the comment docs.

@AydinHassan
Copy link
Member

Just skimmed over it and the ideas all seem sounds to me, please continue! pinging @mikeymike here to checkout also, he wrote most of this stuff!

@Lynesth
Copy link
Collaborator Author

Lynesth commented May 6, 2018

Ok so I spent some time wondering what would be the best way to fallback to 8 colors scheme from a 256 colors code if the terminal did not support it.

Properly (not using RGB) computing the difference between the colors to automatically select the closest one from the default 8 seems to be a bit expensive, so I went with the $fallback argument. It feel that it also gives more control over it.

On another note, do we need some new Exception class to throw those "Invalid colour code" exceptions ?

Note: Travis does not print the inverted lines correctly, but it should just be at the html log level (but they are correctly inverted). I added an issue to their project, hoping they'll be able to add the functionnality.

@AydinHassan
Copy link
Member

AydinHassan commented May 6, 2018

@Lynesth could we just maybe calculate the fallbacks ourself and just store it as code? Like perform the calculations to figure which 8bit colour each of the 256 should map to and store it as a hardcoded array somewhere? that way we don't need to perform expensive computations at runtime

@Lynesth
Copy link
Collaborator Author

Lynesth commented May 7, 2018

@AydinHassan Yeah I guess I can do that.
Many colours will end up as the same one (of course going from 256 to 8) so do we still want to be able to set the fallback manually ?

@Lynesth
Copy link
Collaborator Author

Lynesth commented May 7, 2018

Closing/Reopening to trigger a Travis rebuild since php-school/terminal#9 was merged.

@Lynesth Lynesth closed this May 7, 2018
@Lynesth Lynesth reopened this May 7, 2018
@Lynesth
Copy link
Collaborator Author

Lynesth commented May 7, 2018

There you go: https://gist.github.com/Lynesth/5b8d885a2f3c0bef180c6733813ef156

Where do you want it included ? We would need it both in MenuStyle and in CliMenuBuilder.
Do we make a new class that just has one static function to return the fallback value ?

Let me know.

@AydinHassan
Copy link
Member

@Lynesth this is really cool! Yeah I agree a class with one static method should work. Maybe be dump it in PhpSchool\CliMenu\Style. Name is hard, hmm. Maybe:

class ColourMap {
    public static function map256To8()
    {}
}

?

I'm open for other suggestions though.

We can still fallback manually also, but I guess it will be optional then.

public function setBackgroundColour($colour, string $fallback = null) : self
{
if (is_int($colour)) {
if ($this->terminal->getColourSupport() < 256) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we can extract this validation as it's the same in setForegroundColour. Lets introduce a validateColour method.

@AydinHassan
Copy link
Member

@mikeymike can you take a look at this at some point since you wrote most of this code?

@AydinHassan AydinHassan added this to the 3.0 milestone May 7, 2018
@AydinHassan AydinHassan merged commit 3f2ab1e into php-school:master May 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants