-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Switched CupertinoDynamicColor to implements Color
#153938
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
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
| ) : // The super constructor has to be called with a dummy value in order to mark | ||
| // this constructor const. | ||
| // The field `value` is overridden in the class implementation. | ||
| super(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This! 😠
|
There are a couple of other places where we sublcass Color but they aren't making any assumptions about how Color is implemented so I'm going to try leaving them. |
|
I'm dropping this back into a draft, we are going to land this after flutter/engine#54415 |
goderbauer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
issue: flutter/flutter#127855 integration test: #54415 This does the preliminary work for implementing wide gamut colors in the Flutter framework. Here are the following changes: 1) colors now specify a colorspace with which they are to be interpreted 1) colors now store their components as floats to accommodate bit depths more than 8 The storage of this Color class is weird with float/int storage but that is a temporary solution to support a smooth transition. Here is the plan for landing this: 1) Land this PR 1) Wait for it to roll into the Framework 1) Land flutter/flutter#153938 which will make CupertinoDynamicColor implement Color 1) Land another engine PR that rips out the int storage: #54714 Here are follow up PRs: 1) #54473 - changes DlColor so the wide gamut colors are rendered 1) #54567 - Hooks up these changes to take advantage of wide DlColor 1) flutter/flutter#153319 - the integration test for the framework repo There are some things that have been left as follow up PRs since they are technically breaking: 1) The math on `lerp` hasn't been updated to take advantage of the higher bit depth 1) `operator==` hasn't been updated to take advantage of the higher bit depth 1) `hashCode` hasn't been updated to take advantage of the higher bit depth 1) `alphaBlend` hasn't been updated to take advantage of the higher bit depth 1) `toString` hasn't been updated to take advantage of the higher bit depth ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat
issue: flutter/flutter#127855 integration test: flutter#54415 This does the preliminary work for implementing wide gamut colors in the Flutter framework. Here are the following changes: 1) colors now specify a colorspace with which they are to be interpreted 1) colors now store their components as floats to accommodate bit depths more than 8 The storage of this Color class is weird with float/int storage but that is a temporary solution to support a smooth transition. Here is the plan for landing this: 1) Land this PR 1) Wait for it to roll into the Framework 1) Land flutter/flutter#153938 which will make CupertinoDynamicColor implement Color 1) Land another engine PR that rips out the int storage: flutter#54714 Here are follow up PRs: 1) flutter#54473 - changes DlColor so the wide gamut colors are rendered 1) flutter#54567 - Hooks up these changes to take advantage of wide DlColor 1) flutter/flutter#153319 - the integration test for the framework repo There are some things that have been left as follow up PRs since they are technically breaking: 1) The math on `lerp` hasn't been updated to take advantage of the higher bit depth 1) `operator==` hasn't been updated to take advantage of the higher bit depth 1) `hashCode` hasn't been updated to take advantage of the higher bit depth 1) `alphaBlend` hasn't been updated to take advantage of the higher bit depth 1) `toString` hasn't been updated to take advantage of the higher bit depth ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat
issue: flutter/flutter#127855 integration test: flutter#54415 This does the preliminary work for implementing wide gamut colors in the Flutter framework. Here are the following changes: 1) colors now specify a colorspace with which they are to be interpreted 1) colors now store their components as floats to accommodate bit depths more than 8 The storage of this Color class is weird with float/int storage but that is a temporary solution to support a smooth transition. Here is the plan for landing this: 1) Land this PR 1) Wait for it to roll into the Framework 1) Land flutter/flutter#153938 which will make CupertinoDynamicColor implement Color 1) Land another engine PR that rips out the int storage: flutter#54714 Here are follow up PRs: 1) flutter#54473 - changes DlColor so the wide gamut colors are rendered 1) flutter#54567 - Hooks up these changes to take advantage of wide DlColor 1) flutter/flutter#153319 - the integration test for the framework repo There are some things that have been left as follow up PRs since they are technically breaking: 1) The math on `lerp` hasn't been updated to take advantage of the higher bit depth 1) `operator==` hasn't been updated to take advantage of the higher bit depth 1) `hashCode` hasn't been updated to take advantage of the higher bit depth 1) `alphaBlend` hasn't been updated to take advantage of the higher bit depth 1) `toString` hasn't been updated to take advantage of the higher bit depth ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat
[This PR](#54415) was reverted because it requires a manual roll into the framework. issue: flutter/flutter#127855 integration test: #54415 This does the preliminary work for implementing wide gamut colors in the Flutter framework. Here are the following changes: 1) colors now specify a colorspace with which they are to be interpreted 1) colors now store their components as floats to accommodate bit depths more than 8 The storage of this Color class is weird with float/int storage but that is a temporary solution to support a smooth transition. Here is the plan for landing this: 1) Land this PR 1) Wait for it to roll into the Framework 1) Land flutter/flutter#153938 which will make CupertinoDynamicColor implement Color 1) Land another engine PR that rips out the int storage: #54714 Here are follow up PRs: 1) #54473 - changes DlColor so the wide gamut colors are rendered 1) #54567 - Hooks up these changes to take advantage of wide DlColor 1) flutter/flutter#153319 - the integration test for the framework repo There are some things that have been left as follow up PRs since they are technically breaking: 1) The math on `lerp` hasn't been updated to take advantage of the higher bit depth 1) `operator==` hasn't been updated to take advantage of the higher bit depth 1) `hashCode` hasn't been updated to take advantage of the higher bit depth 1) `alphaBlend` hasn't been updated to take advantage of the higher bit depth 1) `toString` hasn't been updated to take advantage of the higher bit depth
dea2867 to
46a40bd
Compare
|
Now that flutter/engine#54737 has landed and rolled into the framework this will require updating before landed. I'll do that tomorrow. |
Reverts: #54737 Initiated by: chingjun Reason for reverting: Breaking internal tests. See b/363125155 Original PR Author: gaaclarke Reviewed By: {matanlurey, jonahwilliams} This change reverts the following previous change: [This PR](#54415) was reverted because it requires a manual roll into the framework. issue: flutter/flutter#127855 integration test: #54415 This does the preliminary work for implementing wide gamut colors in the Flutter framework. Here are the following changes: 1) colors now specify a colorspace with which they are to be interpreted 1) colors now store their components as floats to accommodate bit depths more than 8 The storage of this Color class is weird with float/int storage but that is a temporary solution to support a smooth transition. Here is the plan for landing this: 1) Land this PR 1) Wait for it to roll into the Framework 1) Land flutter/flutter#153938 which will make CupertinoDynamicColor implement Color 1) Land another engine PR that rips out the int storage: #54714 Here are follow up PRs: 1) #54473 - changes DlColor so the wide gamut colors are rendered 1) #54567 - Hooks up these changes to take advantage of wide DlColor 1) flutter/flutter#153319 - the integration test for the framework repo There are some things that have been left as follow up PRs since they are technically breaking: 1) The math on `lerp` hasn't been updated to take advantage of the higher bit depth 1) `operator==` hasn't been updated to take advantage of the higher bit depth 1) `hashCode` hasn't been updated to take advantage of the higher bit depth 1) `alphaBlend` hasn't been updated to take advantage of the higher bit depth 1) `toString` hasn't been updated to take advantage of the higher bit depth
[This PR](flutter#54415) was reverted because it requires a manual roll into the framework. issue: flutter/flutter#127855 integration test: flutter#54415 This does the preliminary work for implementing wide gamut colors in the Flutter framework. Here are the following changes: 1) colors now specify a colorspace with which they are to be interpreted 1) colors now store their components as floats to accommodate bit depths more than 8 The storage of this Color class is weird with float/int storage but that is a temporary solution to support a smooth transition. Here is the plan for landing this: 1) Land this PR 1) Wait for it to roll into the Framework 1) Land flutter/flutter#153938 which will make CupertinoDynamicColor implement Color 1) Land another engine PR that rips out the int storage: flutter#54714 Here are follow up PRs: 1) flutter#54473 - changes DlColor so the wide gamut colors are rendered 1) flutter#54567 - Hooks up these changes to take advantage of wide DlColor 1) flutter/flutter#153319 - the integration test for the framework repo There are some things that have been left as follow up PRs since they are technically breaking: 1) The math on `lerp` hasn't been updated to take advantage of the higher bit depth 1) `operator==` hasn't been updated to take advantage of the higher bit depth 1) `hashCode` hasn't been updated to take advantage of the higher bit depth 1) `alphaBlend` hasn't been updated to take advantage of the higher bit depth 1) `toString` hasn't been updated to take advantage of the higher bit depth
[This PR](#54415) was reverted because it required customer testing updates. issue: flutter/flutter#127855 integration test: #54415 This does the preliminary work for implementing wide gamut colors in the Flutter framework. Here are the following changes: 1) colors now specify a colorspace with which they are to be interpreted 1) colors now store their components as floats to accommodate bit depths more than 8 The storage of this Color class is weird with float/int storage but that is a temporary solution to support a smooth transition. Here is the plan for landing this: 1) Land this PR 1) Wait for it to roll into the Framework 1) Land flutter/flutter#153938 which will make CupertinoDynamicColor implement Color 1) Land another engine PR that rips out the int storage: #54714 Here are follow up PRs: 1) #54473 - changes DlColor so the wide gamut colors are rendered 1) #54567 - Hooks up these changes to take advantage of wide DlColor 1) flutter/flutter#153319 - the integration test for the framework repo There are some things that have been left as follow up PRs since they are technically breaking: 1) The math on `lerp` hasn't been updated to take advantage of the higher bit depth 1) `operator==` hasn't been updated to take advantage of the higher bit depth 1) `hashCode` hasn't been updated to take advantage of the higher bit depth 1) `alphaBlend` hasn't been updated to take advantage of the higher bit depth 1) `toString` hasn't been updated to take advantage of the higher bit depth ## Reland 2 notes This was reverted because it changes the math on `_lerpDouble`. While those changes were mathematcially equivalent, they had different behaviors when working with non-numbers which created unexpected changes. The change has been reverted and a test added. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat
46a40bd to
10d1160
Compare
|
This seems to be failing in google testing since the changes to Color haven't rolled into google3 yet. |
051eab3 to
3239f67
Compare
issue: flutter/flutter#127855 This depends on flutter/flutter#153938 being landed. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat
issue: #127855
This was an impediment to changing the Color class.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.