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)8285 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
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ Active mailing list Active@kde.org https://mail.kde.org/mailman/listinfo/active
