Skip to content
This repository has been archived by the owner on Aug 31, 2021. It is now read-only.

[Bug 7344] Correct gray RGB values in "GrayX" colors #4290

Merged
merged 8 commits into from Aug 15, 2016

Conversation

meiradarocha
Copy link
Contributor

"GrayX" series (where X is a number from 0 to 100) returned by the colorNames function has wrong values.

"GrayX" series (where X is a number from 0 to 100) returned by the colorNames function has wrong values.
@livecode-vulcan
Copy link
Contributor

📝 Hi @meiradarocha, I haven't been able to verify that you've signed our LiveCode Contributor's Agreement.

If you have previously signed the Contributor's Agreement, I may not be able to detect it because you haven't linked your GitHub account to your LiveCode account.

Please see the information for contributors for more information.

@montegoulding
Copy link
Member

@meiradarocha thanks for the contribution! If you want to sort out the contributor agreement I'll check with the team whether we can include it from a backwards compatibility standpoint as these colors have been incorrect for a loooong time and I guess there could be people depending on their incorrect value.

@montegoulding
Copy link
Member

This is the bug report for this issue http://quality.livecode.com/show_bug.cgi?id=7344

This patch will therefore need a bugfix note in the docs/notes directory.

@montegoulding
Copy link
Member

@meiradarocha While there is some concern that projects may look different in the version that this patch is applied to that it would be beneficial to fix these values and move forward as long as the change is well documented. On that note we will need you to add a bugfix-7344.md file to the docs/notes directory explaining the change. This will appear in the release notes for the version this patch is merged into. I think we also will need the colorNames dictionary entry updated noting the change in version 8.1.

@meiradarocha
Copy link
Contributor Author

Thank you, @montegoulding I will do that.

@montegoulding
Copy link
Member

@meiradarocha We really need all the changes for this patch to be included in the one pull request. This means any further changes you make related to the GrayX colors needs to be on this branch https://github.com/meiradarocha/livecode/tree/patch-1

Just for clarity the lcdoc change will need to add a Changes: element. Something like this:

Changes:
In LiveCode 8.1 the Gray1-100 colors were corrected to match the shades of gray from the [X11 color names](https://en.wikipedia.org/wiki/X11_color_names).

We also need a docs/notes/bugfix-7344.md file containing something like:

# Correct named shades of gray (Gray1-100) to match the X11 color names

@mwieder
Copy link
Contributor

mwieder commented Jul 25, 2016

Just a question here, since the new values seem correct (and thank you for fixing this - they've been wrong forever)...

Would it make sense to eliminate the magic numbers and just use round((NN / 100) * 255) for all the GrayNN values? I mean, as long as this is compile-time assignment and not runtime function calling.

Note about bug 7344 fix. Changes in names of shades of gray (Gray1-100) to match the X11 color names
@montegoulding
Copy link
Member

@meiradarocha Thanks for the updates to the PR. The merge conflicts will be on the lcdoc file due to the hard wrapping PR we merged the other day. Unfortunately there isn't really an easy way to resolve this without using git locally rather than via the github web interface. Let me know if you need talking through that. If you want to use a git gui I recommend sourcetree.

@meiradarocha
Copy link
Contributor Author

OK, I will do that.
Thank you.

@meiradarocha
Copy link
Contributor Author

meiradarocha commented Jul 25, 2016

@mwieder, it would probably make more sense to not use magic numbers, but I have no skills in C ++ to do this.

@peter-b peter-b mentioned this pull request Jul 25, 2016
@mwieder
Copy link
Contributor

mwieder commented Jul 25, 2016

@meiradarocha no worries. I was just suggesting something like
instead of
{"Gray11", 28, 28, 28},
using the form
{"Gray11", round((11 / 100) * 255), round((11 / 100) * 255), round((11 / 100) * 255)},

@peter-b peter-b changed the title Correct gray RGB values in "GrayX" colors [BUg 7344] Correct gray RGB values in "GrayX" colors Jul 26, 2016
@peter-b peter-b changed the title [BUg 7344] Correct gray RGB values in "GrayX" colors [Bug 7344] Correct gray RGB values in "GrayX" colors Jul 26, 2016
@montegoulding montegoulding added this to the 8.1.0-dp-4 milestone Aug 10, 2016
@montegoulding
Copy link
Member

@meiradarocha we would like to include this in LiveCode 8.1 but need to resolve the conflict first. Do you need help doing that?

@meiradarocha
Copy link
Contributor Author

Yes, @montegoulding. Currently, I have no spare time to learn local git to resolve this. If you could make this patch would be fine.

@montegoulding
Copy link
Member

@livecode-vulcan review ok be92d8e

@livecode-vulcan
Copy link
Contributor

💙 review by @montegoulding ok be92d8e

livecode-vulcan added a commit that referenced this pull request Aug 15, 2016
[Bug 7344] Correct gray RGB values in "GrayX" colors

"GrayX" series (where X is a number from 0 to 100) returned by the colorNames function has wrong values.
@livecode-vulcan
Copy link
Contributor

😎 test success be92d8e

@montegoulding montegoulding merged commit 530afc9 into livecode:develop Aug 15, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants