> sebas:
> - sebas/themeswitch branch allows switching themes, almost good to merge:
>         - Blur should depend on theme values
>         - styled icons don't change on theme changes (notmart will have a 
> look)
>

I did a code review of the branch.
A few nitpicks

Missing & in src/plasma/package.cpp
+ foreach (const QString pack, entries) {

---------------------------

1fedbb587c7a94674484df0fe57d24299d000a06

avoid committing commented out code (the qDebugs)

---------------------------

+ if (!themeName.isEmpty()) {

+ Plasma::Theme *t = new Plasma::Theme(this);

+ t->setThemeName(themeName);

+ }

This API sucks a bit. (not the fault of your branch)

Changing a member of an object, should not affect lots of instances of
that object.

Can I make this a static method in Plasma::Theme instead?

Whilst I'm here:
 - setUseGlobalSettings does nothing. It unsets the theme name.. but
then if you call setThemeName you'll then change things globally as
it's still the same d pointer.

 - why do we do our own ref counting? QHash<QString,
QSharedPtr<ThemePrivate>> themes; would make all code simpler.

David
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Reply via email to