'Re: Review Request: new KWin effect: Dashboard'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kwin
Subject:    Re: Review Request: new KWin effect: Dashboard
From:       "Andreas Demmer" <ademmer () opensuse ! org>
Date:       2010-06-17 6:45:35
Message-ID: 20100617064535.608.12368 () localhost
[Download RAW message or body]

> 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
> 
> 

_______________________________________________
kwin mailing list
kwin@kde.org
https://mail.kde.org/mailman/listinfo/kwin

[prev in list] [next in list] [prev in thread] [next in thread] 