Minutes Monday Plasma hangout

[Sebastian Kügler]

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