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

Fixes hexColorDWORDToRGBA for transparent colors #11557

Merged
merged 1 commit into from Jan 11, 2018

Conversation

Projects
None yet
4 participants
@josimi
Contributor

josimi commented Jan 2, 2018

Fixes issue #11556

Uses 0-padding and a set width 8 characters of ensure that RGBA hexadecimal colors string representations are unambiguous.

e.g. (A: 00, R: 0, G: 0, B: 255) is now unambiguously printed as 0000FF00, not FF00.

Replaces janky string substring and concatenation based ARGB -> RGBA shifting logic with bitwise operations to fix several bugs.

The previous code resulted in colors with low-alpha or R-channel values being printed incorrectly. For example, previously

(A: 0, R: 0, G: 160, B: 0) (ARGB: 0x0000A000) would be rendered as RGBA 00A0 which is not correct. Meanwhile, (A: 0, R: 0, G: 0, B: 0) would not render at all in RGBA leading to an exception. Please review the previous code's use of string operations to understand why.

@josimi josimi requested a review from electron/reviewers as a code owner Jan 2, 2018

@welcome

This comment has been minimized.

welcome bot commented Jan 2, 2018

💖 Thanks for opening this pull request! 💖

Here is a list of things that will help get it across the finish line:

  • Follow the JavaScript, C++, and Python coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes you've made following the documentation styleguide.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.
    We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.
@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Jan 2, 2018

Could you briefly explain what you are doing in this PR? I might still be in holiday mode but I can't clearly see what's going on 😄

@josimi

This comment has been minimized.

Contributor

josimi commented Jan 2, 2018

@MarshallOfSound added an explanation

@josimi

This comment has been minimized.

Contributor

josimi commented Jan 2, 2018

@MarshallOfSound I don't understand the test failures. Can you point out what's wrong?

@josimi

This comment has been minimized.

Contributor

josimi commented Jan 2, 2018

@MarshallOfSound Any ideas about where test coverage for this should be added?

@jkleinsc

This comment has been minimized.

Contributor

jkleinsc commented Jan 8, 2018

@josimi in regards to the test failure, I reran the failing CI and it passed, so there is nothing to worry about there.

@josimi

This comment has been minimized.

Contributor

josimi commented Jan 8, 2018

@jkleinsc Thank you. I am happy to see this. Any ideas about when this can be reviewed?

@josimi

This comment has been minimized.

Contributor

josimi commented Jan 9, 2018

@juturu Can you look at this?

@lee-dohm lee-dohm changed the title from hexColorDWORDToRGBA is broken for transparent colors to Fixes hexColorDWORDToRGBA for transparent colors Jan 9, 2018

@ckerr

LGTM with the exception of the placement of the iomanip header

@@ -5,6 +5,7 @@
#ifndef ATOM_BROWSER_API_ATOM_API_SYSTEM_PREFERENCES_H_
#define ATOM_BROWSER_API_ATOM_API_SYSTEM_PREFERENCES_H_
#include <iomanip>

This comment has been minimized.

@ckerr

ckerr Jan 9, 2018

Member

This should go in atom/browser/api/atom_api_system_preferences_win.cc

@josimi

This comment has been minimized.

Contributor

josimi commented Jan 9, 2018

Thanks @ckerr

@josimi

This comment has been minimized.

Contributor

josimi commented Jan 11, 2018

@ckerr I did make the changes you requested. Perhaps it was a mistake to squash the commits?

@ckerr

ckerr approved these changes Jan 11, 2018

LGTM. Thanks for this fix!

@ckerr ckerr merged commit fa43cb6 into electron:master Jan 11, 2018

6 checks passed

ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
@welcome

This comment has been minimized.

welcome bot commented Jan 11, 2018

Congrats on merging your first pull request! 🎉🎉🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment