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

Feature/lab color representation #12935

Merged

Conversation

RubenFricke
Copy link
Contributor

@RubenFricke RubenFricke commented Aug 27, 2021

Summary of the Pull Request

Added CIE LAB and CIE XYZ color space to the Color Picker.

What is include in the PR:

This pull request adds the CIE LAB color space, also referred to as L* a* b* and the CIE XYZ color space to the Color Picker. To calculate the CIELAB space from RGB, a conversion from RGB to CIEXYZ had to be made, this conversion is also included in this pull request.

How does someone test / validate:

Open Color Picker, and validate that the CIE LAB and CIE XYZ color space are visible, if these color spaces are enabled via the settings.
afbeelding
afbeelding

Validate that the output of the color is correct (use online available color conversion websites if you don't have any tool available)

Quality Checklist

Contributor License Agreement (CLA)

A CLA must be signed. If not, go over here and sign the CLA. ✅

@github-actions
Copy link

github-actions bot commented Aug 27, 2021

@check-spelling-bot Report

Unrecognized words, please review:

  • chromaticities
  • cielab
  • CIEXYZ
  • companding
  • debian
  • mindaro
Previously acknowledged words that are now absent Accessible coc djsoref dogancelik estdir Fody ftps gmx htt inprivate itsme mfreadwrite mfuuid Nefario nitroin null nunit Radiobuttons sidepanel ulazy XSmall xunit
Some files were were automatically ignored

These sample patterns would exclude them:

^src/modules/previewpane/UnitTests-MarkdownPreviewHandler/HelperFiles/MarkdownWithHTMLImageTag\.txt$

You should consider adding them to:

.github/actions/spell-check/excludes.txt

File matching is via Perl regular expressions.

To check these files, more of their words need to be in the dictionary than not. You can use patterns.txt to exclude portions, add items to the dictionary (e.g. by adding them to allow.txt), or fix typos.

To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the git@github.com:RubenFricke/PowerToys.git repository
on the feature/lab-color-representation branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spell-check/expect.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spell-check/expect.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
(cat '.github/actions/spell-check/excludes.txt' - <<EOF
$should_exclude_patterns
EOF
) |grep .|
sort -f |
uniq > '.github/actions/spell-check/excludes.txt.temp' &&
mv '.github/actions/spell-check/excludes.txt.temp' '.github/actions/spell-check/excludes.txt'
}

comment_json=$(mktemp)
curl -L -s -S \
  --header "Content-Type: application/json" \
  "https://api.github.com/repos/microsoft/PowerToys/issues/comments/907407158" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
  

patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
  

should_exclude_patterns=$(perl -e '$/=undef;
$_=<>;
exit unless s{(?:You should consider excluding directory paths|You should consider adding them to).*}{}s;
s{.*These sample patterns would exclude them:}{}s;
s{.*\`\`\`([^`]*)\`\`\`.*}{$1}m;
print' < "$comment_body" | grep . || true)

update_files
rm $comment_body
git add -u
If you see a bunch of garbage

If it relates to a ...

well-formed pattern

See if there's a pattern that would match it.

If not, try writing one and adding it to the patterns.txt file.

Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

Note that patterns can't match multiline strings.

binary-ish string

Please add a file path to the excludes.txt file instead of just accepting the garbage.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

@martinchrzan martinchrzan removed the request for review from martinchrzan-msft August 28, 2021 13:39
@RubenFricke
Copy link
Contributor Author

@martinchrzan , As I stated above in the pull request description To calculate L* a* b*, CIEXYZ must also be calculated. This is also a color space that already gets calculated right now. Is there also a need to add the XYZ color format to the color picker for the user? If so, can I add it to this issue/pr? Or does this require a new issue/pr?

Do you happen to know the answer to this? Or someone who can answer this? Thanks again!

@martinchrzan
Copy link
Contributor

martinchrzan commented Aug 28, 2021 via email

@martinchrzan
Copy link
Contributor

@RubenFricke I would guess that you can include that color format in this PR as well since you have already added a code to calculate it, but I would create a new issue and refer to it from this PR, so we can keep a track of it.

@github-actions
Copy link

github-actions bot commented Aug 28, 2021

@check-spelling-bot Report

Unrecognized words, please review:

  • SRGB
Previously acknowledged words that are now absent Accessible coc djsoref dogancelik estdir Fody ftps gmx htt inprivate itsme mfreadwrite mfuuid Nefario nitroin null nunit Radiobuttons sidepanel ulazy XSmall xunit
Some files were were automatically ignored

These sample patterns would exclude them:

^src/modules/previewpane/UnitTests-MarkdownPreviewHandler/HelperFiles/MarkdownWithHTMLImageTag\.txt$

You should consider adding them to:

.github/actions/spell-check/excludes.txt

File matching is via Perl regular expressions.

To check these files, more of their words need to be in the dictionary than not. You can use patterns.txt to exclude portions, add items to the dictionary (e.g. by adding them to allow.txt), or fix typos.

To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the git@github.com:RubenFricke/PowerToys.git repository
on the feature/lab-color-representation branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spell-check/expect.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spell-check/expect.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
(cat '.github/actions/spell-check/excludes.txt' - <<EOF
$should_exclude_patterns
EOF
) |grep .|
sort -f |
uniq > '.github/actions/spell-check/excludes.txt.temp' &&
mv '.github/actions/spell-check/excludes.txt.temp' '.github/actions/spell-check/excludes.txt'
}

comment_json=$(mktemp)
curl -L -s -S \
  --header "Content-Type: application/json" \
  "https://api.github.com/repos/microsoft/PowerToys/issues/comments/907656539" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
  

patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
  

should_exclude_patterns=$(perl -e '$/=undef;
$_=<>;
exit unless s{(?:You should consider excluding directory paths|You should consider adding them to).*}{}s;
s{.*These sample patterns would exclude them:}{}s;
s{.*\`\`\`([^`]*)\`\`\`.*}{$1}m;
print' < "$comment_body" | grep . || true)

update_files
rm $comment_body
git add -u
If you see a bunch of garbage

If it relates to a ...

well-formed pattern

See if there's a pattern that would match it.

If not, try writing one and adding it to the patterns.txt file.

Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

Note that patterns can't match multiline strings.

binary-ish string

Please add a file path to the excludes.txt file instead of just accepting the garbage.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

@github-actions
Copy link

github-actions bot commented Aug 28, 2021

@check-spelling-bot Report

Unrecognized words, please review:

  • SRGB
Previously acknowledged words that are now absent Accessible CIEXYZ coc djsoref dogancelik estdir Fody ftps gmx htt inprivate itsme mfreadwrite mfuuid Nefario nitroin null nunit Radiobuttons sidepanel ulazy XSmall xunit
Some files were were automatically ignored

These sample patterns would exclude them:

^src/modules/previewpane/UnitTests-MarkdownPreviewHandler/HelperFiles/MarkdownWithHTMLImageTag\.txt$

You should consider adding them to:

.github/actions/spell-check/excludes.txt

File matching is via Perl regular expressions.

To check these files, more of their words need to be in the dictionary than not. You can use patterns.txt to exclude portions, add items to the dictionary (e.g. by adding them to allow.txt), or fix typos.

To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the git@github.com:RubenFricke/PowerToys.git repository
on the feature/lab-color-representation branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spell-check/expect.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spell-check/expect.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
(cat '.github/actions/spell-check/excludes.txt' - <<EOF
$should_exclude_patterns
EOF
) |grep .|
sort -f |
uniq > '.github/actions/spell-check/excludes.txt.temp' &&
mv '.github/actions/spell-check/excludes.txt.temp' '.github/actions/spell-check/excludes.txt'
}

comment_json=$(mktemp)
curl -L -s -S \
  --header "Content-Type: application/json" \
  "https://api.github.com/repos/microsoft/PowerToys/issues/comments/907660132" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
  

patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
  

should_exclude_patterns=$(perl -e '$/=undef;
$_=<>;
exit unless s{(?:You should consider excluding directory paths|You should consider adding them to).*}{}s;
s{.*These sample patterns would exclude them:}{}s;
s{.*\`\`\`([^`]*)\`\`\`.*}{$1}m;
print' < "$comment_body" | grep . || true)

update_files
rm $comment_body
git add -u
If you see a bunch of garbage

If it relates to a ...

well-formed pattern

See if there's a pattern that would match it.

If not, try writing one and adding it to the patterns.txt file.

Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

Note that patterns can't match multiline strings.

binary-ish string

Please add a file path to the excludes.txt file instead of just accepting the garbage.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

@mykhailopylyp
Copy link
Contributor

It looks like we have rounding/precision issues here
image

@RubenFricke
Copy link
Contributor Author

Thanks for all your feedback @mykhailopylyp . I will look into it!

@crutkas
Copy link
Member

crutkas commented Sep 20, 2021

@RubenFricke / @mykhailopylyp what needs to happen so we can roll this improvement into the 0.47 release at the end of the month.

@jaimecbernardo
Copy link
Collaborator

@crutkas , from the feedback, looks like the calculations are still not quite right for the color format.

@crutkas
Copy link
Member

crutkas commented Sep 20, 2021

@jaimecbernardo is this something we can just do a quick fix for?

@RubenFricke
Copy link
Contributor Author

I will fix the calculations, sorry! Only question, what format do you want to output?

@jaimecbernardo
Copy link
Collaborator

@RubenFricke ,

I'm not sure which is the standard here.
You are one of the original proposals for these formats. @Ayanari3 as well.
@TheDecryptor also suggested the CSS 4 format.

Which would be useful to you?
I suppose more lines can be added if needed later.

@htcfreek
Copy link
Collaborator

@RubenFricke
There seems to be a round bug. Pipeline is failing because of test results.

@crutkas
Copy link
Member

crutkas commented Sep 23, 2021

@jaimecbernardo / @dedavis6797 do we have time for this to be in .47?

Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

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

This looks good to me.
Thank you for the contribution.
I do have some questions regarding presentation of the values.
Looking for those in other sites show these values presented with some decimal places. Why are we not showing these as well?

@jaimecbernardo
Copy link
Collaborator

@jaimecbernardo / @dedavis6797 do we have time for this to be in .47?

Reviewing. Let's try to get this in, then.

@jaimecbernardo
Copy link
Collaborator

@RubenFricke ,
In case you didn't add the decimal places because they didn't fit, I have another PR that can go in after yours to increase the size of the window:
#13420

image

Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

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

I've added the decimal places and committed the changes.
Thank you for the contribution!

@jaimecbernardo jaimecbernardo merged commit 3358fd9 into microsoft:master Sep 24, 2021
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.

7 participants