[kde-workspace] kwin/data/desktop: Move kwin/data to kwin/data/desktop so that I can add kwin/data/active.

[Martin Gräßlin]

On Tuesday 24 April 2012 05:58:16 Lamarque V. Souza wrote:
> Em Monday 23 April 2012, Martin Gräßlin escreveu:
> > Hi Lamarque,
> 
> 	Hi,
> 
> > I'm quite confused by your three commits [1, 2, 3]. Why are they needed?
> > We
> > nowhere in KWin have folder distinction for "desktop" and "active" and I
> > do
> > not like that at all.
> 
> 	PA2 uses thumbnails as LayoutName, PA3 uses window_strip, we need to
> change that when someone upgrades PA2 to PA3:
> 
> https://bugs.kde.org/show_bug.cgi?id=298285
First of all: if there is a bugfix you should add the bug id to your commit. 
This is really important as I outlined in my latest blog post [1] about how to 
use bugs.kde.org as a KDE developer :-)

It would have turned my mail to a completely different style as I would have 
understood why the change has been done - I still would have disagreed with 
the change, but that's something different.

Now to the question whether the KConf Update is correct in this case and 
needed at all (that's the part which should have been in the review request).
I don't think this change is needed.

The need highlights a severe issue: KWin desktop and KWin active are two 
separate applications which need different configurations and updating the one 
may affect the other. We already identified this issue and have a review 
request [2] for it as Thomas already mentioned. This request has not yet been 
merged as I want to change one more thing and update the request afterwards. 
My understanding from the thread [3] about this change on active ML there is 
no urgent need for it. I could have raised the priority any time.

Another thing is that it's quite obvious that the current mechanism of config 
values does not scale in the long term. Many of the config options just don't 
make sense and having this configurable turns in a risk. For example the 
setting to use placement policy maximized is hardcoded for Plasma Active so 
that no config value is used. This might have been a valid approach for this 
as well (though I prefer keeping the ifdefs low).

Another possibility would have been to extend the D-Bus interface used to 
activate the window switcher from the panel to specify which layout to use. 
This is clearly my preferred solution to this problem and would be useful in 
general and for other things. E.g. I want to have in the netbook shell the 
activation of Present Windows replaced with the new grid based window switcher 
layout. An adjustment here would have fixed this for both use cases.
> 
> > Why did you push directly to master without consulting the KWin
> > development
> > team (no review request, no mail to mailing list, no ack in commits)?
> 
> 	It looked a simple change and has been tested. Sorry for that, I will
> open a review request next time.
<personal experience>I have often thought that a change is small or trivial 
and it bite my a** quite often. I can only recommend to have each of your 
changes reviewed - in all your projects you are involved. Given enough eyes 
all bugs shall be shallow. Having them fixed before hitting the source code is 
much better than afterwards</personal experience>
> 
> > We also had some discussions about the update scripts in the past for
> > active and found a bug in one of the scripts. So in case you just found
> > some issues it would have been better to report a bug to fix it properly.
> 
> 	The commits were in reponse to the bug reported above.
You can always add kwin to CC of bugs for active. That case we can help before 
code gets written :-)
> 
> > Overall I find that pretty uncool :-(
> 
> 	Ok, sorry again.
No problem :-) We all make mistakes and we all are here to learn and improve. 
If you take from this experience that you should add
BUG: 12354
FIXED-IN: 4.9.0
to your git commits, I'm more than happy :-)

Overall I think the change needs to be reverted, as:
* we will have one update script prepared for Beta 1
* it seriously conflicts with a pending review I already did for PA
* it breaks with existing schemes inside KWin
* there are better ways to handle it (d-bus)

I would of course welcome if you work on the addition to d-bus. If you have 
any questions about it, I'm more than happy to give you some pointers about 
the code :-)

Cheers
Martin

[1] http://blog.martin-graesslin.com/blog/2012/04/bko-versions/
[2] https://git.reviewboard.kde.org/r/104299/
[3] http://mail.kde.org/pipermail/active/2012-April/003535.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/active/attachments/20120424/c665f221/attachment.sig>