Eventviews: properly fall back to color-generator if ColorAttribute is absent

Authored by dfaure on Oct 29 2018, 10:14 AM.



This was introduced by 1ec2e08 which only called the color-generator
code when there *is* a ColorAttribute on the collection (with an
invalid color, which seems unlikely), rather than when the attribute
isn't there.

I thought this was the bug that made all new resources red (with caldav)
but actually that seems to come from the resource itself. So now I'm not
100% sure about this commit anymore, although it still seems to make
sense to me -- for other resources, possibly.

Test Plan

Disabling the code that reads from the ColorAttribute
and removing all "Resources Colors" entries from ~/.config/eventviewsrc,
the colors are properly generated.

Diff Detail

2018_10_color_generation (branched from Applications/18.08)
No Linters Available
No Unit Test Coverage
Build Status
Buildable 4308
Build 4326: arc lint + arc unit
dfaure requested review of this revision.Oct 29 2018, 10:14 AM
dfaure created this revision.

Just to be sure: you just factored both treatment in one? Or do I miss something?

If it is just factoring, you can ship it. If not, I'd like to be sure not to miss anything in the change.

The idea was to always do:

1) load resource
 1a) if color, use it
 1b) if invalid color,
   1b1) try to use the one from the dav
   1b2) if none in the dav, define one in resources and load resource

It seems you do exactly that (which I thought I was doing, but with a clumsy way)

dkurz accepted this revision.Oct 31 2018, 11:33 AM

I just discovered by accident that I was named reviewer for the first time. Didn't see it coming!

Please do not drop this patch, as it deduplicates code AND fixes a bug, even if it isn't the one you aimed for.

Every declaration in this method could be const-auto'd, especially the one in line 72, but that would be mixing style changes with functional ones, I guess.


You might want to invert the condition here and return early, to reduce the indentation of the following lines, but I guess this is just a matter of taste.

This revision is now accepted and ready to land.Oct 31 2018, 11:33 AM

If it is just factoring, you can ship it. If not, I'd like to be sure not to miss anything in the change.

The old code returns an invalid color if the preference color is invalid and the collection doesn't have a color attribute (as the big if is never entered).

The new code generates a new color in this case.

ochurlaud accepted this revision.Oct 31 2018, 5:45 PM

OK fine, ship it

dfaure added inline comments.Nov 1 2018, 11:01 PM

That would duplicate the call to resourceColor:

if (!hasAttribute)
    return preferences->resourceColor(id);

if (colorAttr && colorAttr->color().isValid())
    return colorAttr->color();

return preferences->resourceColor(id);

Not great IMHO.

dfaure closed this revision.Nov 1 2018, 11:07 PM