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.
Disabling the code that reads from the ColorAttribute
and removing all "Resources Colors" entries from ~/.config/eventviewsrc,
the colors are properly generated.
| No Linters Available |
| No Unit Test Coverage |
| Buildable 4308 | |
| Build 4326: arc lint + arc unit |
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)
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.
| src/helper.cpp | ||
|---|---|---|
| 71 | 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. | |
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.
| src/helper.cpp | ||
|---|---|---|
| 71 | 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. | |