mRs- / HexColors

HexColors is an extension for UIColor and NSColor to support for creating colors from a hex strings
MIT License
457 stars 102 forks source link

Hex string with alpha not converting correctly #32

Closed billburgess closed 8 years ago

billburgess commented 8 years ago

I'm using this Pod in a new Mac app (targeting 10.11) and I discovered a weird issue while writing tests for my color extension.

NSColor.hx_colorWithHexString("#99999955")

This should return an RGB of 153,153,153. When it gets to hx_colorWith8BitRed it is trying to create an NSColor colorWithCalibratedRed using 153,153,85. This seems to work whenever I leave off the alpha characters, but is returning the wrong value for blue.

billburgess commented 8 years ago

I've tracked the issue down to the hx_colorWithHexString class method. When it detects there is an alpha length string it is stripping the incorrect range for alpha (which I assume are the final 2 characters) and is generating the incorrect alpha as well as incorrect blue.

billburgess commented 8 years ago

After further testing it appears the alpha is expected in the first 2 positions, not the last 2. This works correctly if I flip my alpha around. It would be helpful if this was documented some place either in the comments or in the Readme to explain this behavior. I'm going to close this issue as I don't think flipping the position is viable for the library given the amount of users using it. But some clarification or explanation would be helpful to new users.

mRs- commented 8 years ago

I think most of the user are just not using ARGB values with the library. The common usage is done via RGB and not ARGB.

Maybe we can start a discussion if we should use ARGB or RGBA in this library.

billburgess commented 8 years ago

I agree. It is rare for users to use RGBA or ARGB, most apply it separately. As well as other platforms. I blindly assumed it was RGBA as that is how the default implementation in Android is. I assumed it was standard practice to have it at the end. My fault entirely. I only noticed it due to the color and alpha as they were very close when I wrote some unit tests. (Hooray for unit testing, it works!)

If I were voting, I'd prefer RGBA as it fits with the order currently available in iOS and Mac for the colorWithRed alpha methods. Either way just indicating in the docs will go a long way in preventing issues down the road.

mRs- commented 8 years ago

Okay maybe we should create something to use on both sides, because for better cross platform support.

But this should be handled with care!

billburgess commented 8 years ago

Sounds good. I'll take a look when I have a little time to see if there is a way to address both sides.

mRs- commented 8 years ago

the first thing is to deprecate the old call, with information about the changes, create the new call to move on for the users of this extension.

A step more in the future drop the deprecated call with the new one. Then we are ready to go.

billburgess commented 8 years ago

I agree, that sounds like a good step in the right direction.