Hi David,
Thanks for the review!
On Monday, February 03, 2014 18:07:51 David Edmundson wrote:
> > 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) {
Ah, missed that. Will fix.
> ---------------------------
>
> 1fedbb587c7a94674484df0fe57d24299d000a06
>
> avoid committing commented out code (the qDebugs)
I'll clean this up.
> ---------------------------
>
> + 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.
In this case, it's a little more involved. Plasma::Theme::themeName() here
changes the default theme, but the setter can also affect a custom theme
(which is why there's the refcounting). In
> Can I make this a static method in Plasma::Theme instead?
I don't think that'll work, see above.
> 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.
I'm not familiar with this part of the internals of Theme, maybe someone else
can weigh in.
Cheers,
--
sebas
http://www.kde.org | http://vizZzion.org | GPG Key ID: 9119 0EF9
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
