Review Request: new KWin effect: Dashboard

Thu Jun 17 08:45:35 CEST 2010

> On 2010-06-16 18:12:50, Martin Gräßlin wrote:
> > I must agree with Aaron: I'm really glad we finally get a Dashboard effect (I remember that I wrote one about 4.1, but at that time Dashboard was so broken that I stopped working on it :-P)
> > 
> > I'm not sure about the config options. Having them in effect config makes it a poweruser feature nobody will find. It's fine for development but for the release it's kind of useless. Such option might make sense in the dashboard config, but in general it looks quite a bit KDE 3-ish to change the level of gray ;-)
> > 
> > The dashboard effect has the potential to be astonishing and I think it needs some Nuno love. Something special like we did for the logout effect. A kind of decent blur, so that we could enable it without harming the users who want to read something in the window. I need to ask Nuno...
> > 
> > And if Plasma devs split Plasmoids from the wallpaper (needs KWin help) we can even do more awesome stuff like fading the Plasmoids to the top through all windows for same widget sets and a nice animation for dashboard widget set.

I agree that animations would really enhance the effect. I propose to polish the current version according to your comments below and to push this into trunk when it opens again for 4.6. Then I have plenty of time to implement animations etc. in the 4.6 development cycle. What do you think?

> On 2010-06-16 18:12:50, Martin Gräßlin wrote:
> > trunk/KDE/kdebase/workspace/kwin/effects/dashboard/dashboard.cpp, line 27
> > <http://reviewboard.kde.org/r/4332/diff/1/?file=28777#file28777line27>
> >
> >     Where does this magic number come from? And why is it required?

This is set in window data to let the blur effect handle the blurring of the background.

> On 2010-06-16 18:12:50, Martin Gräßlin wrote:
> > trunk/KDE/kdebase/workspace/kwin/effects/dashboard/dashboard.cpp, line 44
> > <http://reviewboard.kde.org/r/4332/diff/1/?file=28777#file28777line44>
> >
> >     we have reconfigure for reading the configuration values :-)

Ah, crap, you are right. I'll change this to a reconfigure call.

> On 2010-06-16 18:12:50, Martin Gräßlin wrote:
> > trunk/KDE/kdebase/workspace/kwin/effects/dashboard/dashboard.cpp, line 52
> > <http://reviewboard.kde.org/r/4332/diff/1/?file=28777#file28777line52>
> >
> >     In the dtor it's required to remove the property again. Please have a look at other effects which use xatoms.

Ok, I'll implement the DTOR. Otherwise, the effect is not unloaded during the desktop session when deactivating in effect settings.

> On 2010-06-16 18:12:50, Martin Gräßlin wrote:
> > trunk/KDE/kdebase/workspace/kwin/effects/dashboard/dashboard.cpp, line 70
> > <http://reviewboard.kde.org/r/4332/diff/1/?file=28777#file28777line70>
> >
> >     I would use percentage as brightness/saturation is such a nice [0.0, 1.0] interval

Agreed.

> On 2010-06-16 18:12:50, Martin Gräßlin wrote:
> > trunk/KDE/kdebase/workspace/kwin/effects/dashboard/dashboard.cpp, line 77
> > <http://reviewboard.kde.org/r/4332/diff/1/?file=28777#file28777line77>
> >
> >     Don't set this in each painting step. Set it in window activated. And as region you should use w->geometry(). The region is the area which is painted in the current rendering step. If another effect increases the region only parts of the dashboard would be blurred.

Ah, I see. BTW: You just explained an issue, I have while patching the plasmoid-smooth-tasks. :-)

> On 2010-06-16 18:12:50, Martin Gräßlin wrote:
> > trunk/KDE/kdebase/workspace/kwin/effects/dashboard/dashboard.desktop, line 7
> > <http://reviewboard.kde.org/r/4332/diff/1/?file=28778#file28778line7>
> >
> >     No need to translate, scripty will automatically overwrite it.

I was unsure how KDE handles translations. I will read some information about it and change the desktop files accordingly.

- Andreas

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4332/#review6145
-----------------------------------------------------------

On 2010-06-15 20:31:15, Andreas Demmer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4332/
> -----------------------------------------------------------
> 
> (Updated 2010-06-15 20:31:15)
> 
> 
> Review request for kwin and Plasma.
> 
> 
> Summary
> -------
> 
> This patch adds a new KWin effect that allows to modify the appearance of the Plasma dashboard. It has a KCM configuration dialog where you can adjust brightness, saturation and blur of the dashboard background. Blur depends on the loaded blur plugin.
> 
> I also patched the Plasma dashboard to recognize the loaded effect: If the effect is loaded, the dashboard draws its background fully translucent. In order for the Dashboard to recognize wether the effect is loaded, I added support for the effect in Plasma::WindowEffects from kdelibs.
> 
> The dashboard detection in the effect itself is hackish right now. As soon as Plasma adds a proper class to the dashboard window, I will replace the hack with a class-check.
> 
> 
> This addresses bugs dashboard, detection and hackish.
>     https://bugs.kde.org/show_bug.cgi?id=dashboard
>     https://bugs.kde.org/show_bug.cgi?id=detection
>     https://bugs.kde.org/show_bug.cgi?id=hackish
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdebase/workspace/kwin/effects/CMakeLists.txt 1138357 
>   trunk/KDE/kdebase/workspace/kwin/effects/configs_builtins.cpp 1138357 
>   trunk/KDE/kdebase/workspace/kwin/effects/dashboard/CMakeLists.txt PRE-CREATION 
>   trunk/KDE/kdebase/workspace/kwin/effects/dashboard/dashboard.h PRE-CREATION 
>   trunk/KDE/kdebase/workspace/kwin/effects/dashboard/dashboard.cpp PRE-CREATION 
>   trunk/KDE/kdebase/workspace/kwin/effects/dashboard/dashboard.desktop PRE-CREATION 
>   trunk/KDE/kdebase/workspace/kwin/effects/dashboard/dashboard_config.desktop PRE-CREATION 
>   trunk/KDE/kdebase/workspace/kwin/effects/dashboard/dashboard_config.h PRE-CREATION 
>   trunk/KDE/kdebase/workspace/kwin/effects/dashboard/dashboard_config.cpp PRE-CREATION 
>   trunk/KDE/kdebase/workspace/kwin/effects/dashboard/dashboard_config.ui PRE-CREATION 
>   trunk/KDE/kdebase/workspace/kwin/effects/dashboard/dashboardeffectconfig.ui PRE-CREATION 
>   trunk/KDE/kdebase/workspace/plasma/desktop/shell/dashboardview.cpp 1138357 
>   trunk/KDE/kdelibs/plasma/windoweffects.h 1138355 
>   trunk/KDE/kdelibs/plasma/windoweffects.cpp 1138355 
> 
> Diff: http://reviewboard.kde.org/r/4332/diff
> 
> 
> Testing
> -------
> 
> Code compiles, plugin loads, plugin configuration dialog is registered in KCM Workspace module under "all effects". If the plugin is enabled, its settings apply to the dashboard.
> 
> 
> Screenshots
> -----------
> 
> configuration dialog
>   http://reviewboard.kde.org/r/4332/s/434/
> dashboard with modified background
>   http://reviewboard.kde.org/r/4332/s/435/
> 
> 
> Thanks,
> 
> Andreas
> 
>