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.

« Back to merge proposal