Opened 14 years ago
Closed 14 years ago
#3653 closed defect (fixed)
dojo.Color.setColor(colorObj) returns invalid object (should work?)
Reported by: | guest | Owned by: | Eugene Lazutkin |
---|---|---|---|
Priority: | high | Milestone: | 0.9 |
Component: | General | Version: | 0.9 |
Keywords: | Cc: | Adam Peller | |
Blocked By: | Blocking: |
Description
It seems like passing dojo.Color.setColor a dojo.Color object should return a valid dojo.Color object in response; regardless, the object returned has a null _cache, resulting in weird responses to all calls.
Attachments (2)
Change History (9)
comment:1 Changed 14 years ago by
Owner: | changed from anonymous to Eugene Lazutkin |
---|
comment:2 Changed 14 years ago by
Milestone: | → 0.9 |
---|---|
Status: | new → assigned |
comment:3 Changed 14 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:4 Changed 14 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
Eugene, please check out some ideas for reducing the code size. See patch, attached.
comment:5 Changed 14 years ago by
Cc: | Adam Peller added |
---|
Changed 14 years ago by
Attachment: | color-reduce.patch added |
---|
use an array as internal impl, reduce some code
comment:6 Changed 14 years ago by
Some fixes are good and sound, and I want to roll them in, but using an array as a primary data holder is a questionable move.
- From my conversation with Alex I got that we need to implement an object because color.r, color.a are more mnemonic (and less error-prone) than color.rgba[0], color.rgba[3].
- Using an array reduces performance in most cases. Namely Color.sanitize() is significantly slower, Color.toCss() is slower (both versions with or without alpha), the simulated Color.toRgb() is slower too. The slowdown comes from three primary things:
- Array.slice() is expensive because it allocates memory and copies the array.
- Short loops are slower than unrolled loops.
- Array.forEach() is slower than the for-loop. Again see #2 above.
- I understood the idea that we can get away with toRgb() and toRgba(). The latter will be much faster than the original version. But both toRgb() and toRgba() are not used in our code at all. I used toRgba() in some tests, that's all.
- Color.sanitize() was meant to be a public method, so user can assign stuff manually, and sanitize it. But you are right --- it can be moved to the core.
- The preamble of dojo.blendColors() is slower with "new" operator, because it will copy. The for-loop you proposed is faster than dojo.forEach() I use. But unrolled assignments will be even faster.
- The private confine() function is slower than my original sanitize. In the case of non-numeric c (e.g., "3") it will convert c to Number on every comparison. Remember that c is the raw datum, which came from a user, while high and low are 100% numeric --- we control them.
- dojo.colorFromRgb() was named like that because it doesn't handle a very frequent case of CSS color --- hex ("#000000"). There is a separate function for that. I understand that it becomes inconsistent in dojo.colors, where new dojo.colorFromRgb() handles HSL as well. I did it purely for performance reasons. Probably the best way to handle it is to name it differently (in dojo.colors) and re-define dojo.colorFromString() as well. Actually it was the original intent.
- I agree with you that dojo.colorFromArray() can be shortened.
I'll attach a file, which profiles different implementations mentioned in #2 above. Run it in FF + Firebug and see results in the console window.
Changed 14 years ago by
Attachment: | color_perf.html added |
---|
Profiles several pieces of the proposed dojo.Color changes. Put it in dojo/tests.
comment:7 Changed 14 years ago by
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
Fixed in [9657].