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

Attachment: signature.asc
Description: This is a digitally signed message part.

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

Reply via email to