Code review comment for lp:~zsombi/ubuntu-ui-toolkit/76-refactoring

Revision history for this message
Zsombor Egri (zsombi) wrote :

> > > > void UCListItem::setHighlightColor(const QColor &color)
> > > > {
> > > > Q_D(UCListItem);
> > > > if (d->highlightColor == color) {
> > > > return;
> > > > }
> > > > d->highlightColor = color;
> > > > // no more theme change watch
> > > > d->customColor = true;
> > > > update();
> > > > Q_EMIT highlightColorChanged();
> > > > }
> > > > void UCListItem::resetHighlightColor()
> > > > {
> > > > Q_D(UCListItem);
> > > > d->customColor = false;
> > > > d->highlightColor = getPaletteColor("selected", "background");
> > > > update();
> > > > Q_EMIT highlightColorChanged();
> > > > }
> > > >
> > > >
> > > > if you call setHighlightColor(color) // color ==
> > getPaletteColor("selected",
> > > > "background") then customColor is not set to true, and you are still
> > > watching
> > > > for palette updates, while it may be purely coincidence that you set the
> > > color
> > > > to the palette color. So customColor = true should be moved before the
> > if()
> > > > (maybe update() too).
> > >
> > > Well, if a property's default value is set again as value, there will be
> no
> > > change done either.
> >
> > But the link to the theme color should be broken, when you set a manual
> color
> > (same as the theme color), the color should not be updated automatically any
> > more when the theme color changes.
>
> We need to check how font.family is handled. That one gets its value from the
> default value set - the one we patched in the plugin. I need to check what
> happens if I set that default value in C++ to something else, while having the
> same value assigned to the property value in QML. If changing the default
> value changes the Text font, then we're good as we are now, e behave the same
> way. If it doesn't change, then we fix our code as well.

Seems the font.family was a bad example, as it does not change the font of the text even if the font.family is not having any static value bound. So let's do the way that we mark it as custom color even if it is the same as the default one.

« Back to merge proposal