Allow bgcolor to be set to any color, not just string

[kjordahl]

Setting the bgcolor trait of a Plot instance (or an instance of its parent DataView class) only works if the argument is a string. This change allows it to take any color value. This also adds a colors_equal() function to chaco.api to allow comparison of colors that may be in different formats.

Without this change, the following will fail, raising KeyError:

from chaco.api import Plot
plot = Plot(bgcolor=(1, 1, 1))

Tests that cover the new function and the bgcolor bug are included.

In general, colors should be settable to any ColorTrait allowed value, not just strings. There are a few more comparisons in the codebase that could be cleaned up to allow more general color definitions as well.

[enbuilder]

Can one of the admins verify this patch?

[prabhuramachandran]

please test this

[prabhuramachandran]

test this please

[prabhuramachandran]

ok to test

[kjordahl]

Travis says OK

[prabhuramachandran]

Yes there is one Linux test failure. It is green on OS X and Windows (both 32 and 64bit). The failure is in: chaco.tests.border_test_case.DrawBorderTestCase.test_draw_border_simple

[prabhuramachandran]

This was a test setup error and is now fixed. All is well on all 6 platforms.

[jonathanrocher]

LGTM. Merging tomorrow if there are no objections.

[itziakos]

bgcolor is a ColorTrait (a mapped trait type). The RGBA value of all ColorTraits is available as self.<trait>_ . if we change this line if self.bgcolor_ == color_table[grid_color] it would be enough to fix this bug. In general comparing between mapped traits should always happen through the shadow <trait>_ attribute.

[itziakos]

I would suggest to rework the code as suggested above. The colors_equal can stay since it might be useful somewhere else.

[kjordahl]

@itziakos That's not quite sufficient, because the user could set the bgcolor to a 3-tuple with no alpha. Even if there is an alpha value, we probably don't want to compare it in this case. Using if self.bgcolor_[:3] == color_table[grid_color][:3]: works, but IMO is not as clean as using the colors_equal function.

We probably want something like colors_almost_equal as well, because it could be set to a color indistinguishable by eye. I think that's overkill in this case, though. In fact, I am tempted to take out this check altogether and let the user be responsible for grid color. A simple check here does no harm, though, I suppose.

[itziakos]

I am tempted to take out this check altogether and let the user be responsible for grid color.

I agree, I think that it would be better if grid_color is an attribute in DataView with some basic default value given the bgcolor.

[jonathanrocher]

👍 for making the grid_color an attribute controlled by the user. I don't see a strong necessity for the check then.

[kjordahl]

OK, I won't have time to do this for a couple weeks, so if anyone feels like taking it on, feel free.

[jonathanrocher]

We should finish and close this PR... I haven't had the time to help. @kjordahl do you think you will have some time soon?

[kjordahl]

@jonathanrocher Thanks for the ping, obvioussly this had dropped off my radar. I'll try to pick it up again.