allwpilib icon indicating copy to clipboard operation
allwpilib copied to clipboard

Improve `Color#toString()`/`Color8Bit#toString()` in Java

Open jonahsnider opened this issue 2 years ago • 8 comments

Is your feature request related to a problem? Please describe.

Colors can be stringified in a human-friendly way, but they aren't. They use the default Java Object#toString() behavior of returning the simple class name with the memory address appended (easy to mistake for a hex color code!).

Describe the solution you'd like

Named colors should get special #toString() values:

Color.kBlack.toString(); // "kBlack"

Other colors should get

new Color8Bit(0xff, 0xbb, 0xcc); // "#ffbbcc"

Additional context

https://discord.com/channels/176186766946992128/368993897495527424/960415849259368468

jonahsnider avatar Apr 04 '22 21:04 jonahsnider

Mapping named colours back to their names would probably be a lot of work. Would just returning the hex code be reasonable too?

On a similar note, in RobotPy we have a __repr__ implementation for colours, which give something like Color8Bit(255, 187, 204) (i.e. it returns an expression to reproduce the object, as is standard behaviour for repr() in Python). I think a differing toString would be reasonable too.

I don't think this should be exclusive to Java - no reason C++ shouldn't get a similar method.

auscompgeek avatar Apr 05 '22 03:04 auscompgeek

The PR that closed this fixed the confusion, but doesn't deal with names for specialized colors.

Starlight220 avatar Jun 03 '22 08:06 Starlight220

It's just a variable, you can't really do that without reflection (not even sure if you can do it with that).

prateekma avatar Jun 03 '22 15:06 prateekma

You could compare the color components with kBlack, for example, then print "kBlack" if they match. That could get expensive if there's a lot of predefined colors though. Maybe have a hash map from color components to pre-defined colors?

calcmogul avatar Jun 03 '22 15:06 calcmogul

If you write a big switch statement, the compiler will do most of the work for you. But I agree that it might be more expensive than you might want to pay if you're e.g. logging the color to a CSV file every iteration or something like that. Maybe not include it in toString(), but add a function like toPrettyString()?

PeterJohnson avatar Jun 03 '22 15:06 PeterJohnson

I was thinking of adding a string constructor parameter that defaults to the RGB hex. That way it's cached for all objects, and not expensive at all.

Starlight220 avatar Jun 05 '22 17:06 Starlight220

Yeah that would work. Main downside is you’re forcing the overhead for hex conversion every time a color is constructed. Avoiding that might be micro-optimization, but could be done eg by storing a blank/null string for hex values and converting/caching it only on toString.

PeterJohnson avatar Jun 06 '22 14:06 PeterJohnson

Sounds like a good idea.

Starlight220 avatar Jun 06 '22 15:06 Starlight220