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

List:       kwin
Subject:    Re: [Kwin] KWin Decoration HOWTO Review Request
From:       Lubos Lunak <l.lunak () suse ! cz>
Date:       2003-10-13 14:41:29
[Download RAW message or body]

On Sunday 12 of October 2003 22:12, David Johnson wrote:
> I've updated my KWin theming HOWTO for the 3.2 API, and am requesting
> those more knowledgable than I to review it for accuracy. Please look
> at the both the code and the tutorial. Thanks,
>
> Tarball can be found at http://www.usermode.org/code/example-0.2.tar.gz,
> which includes the HOWTO.

- Maybe you should mention somewhere that the KDecoration* classes that are 
used when writing decorations are documented (unlike before), so people can 
check it if they need more details about the API.

- event filter - you only mention the event filter shortly when describind 
ExampleClient::init(), so maybe I should explain it in more detail: With 
KDE3.1, the ExampleClient class itself was a widget, it was the whole window. 
But now, ExampleClient is not a widget at all. It only creates the main 
widget of the decoration, which will be placed by the KWin core behind the 
real window, according to border sizes given by borders(), thus making it 
look like the real window is inside the decoration. There were several 
reasons for this change, e.g. avoiding clashes between the KDecoration and 
QWidget API.
 Event filter is installed on the main widget mainly because it made porting 
the existing styles easier. Since before the Client class used to be a 
widget, installing event filter on the real widget and processing them in 
Client reduces necessary changes. I actually personally consider this to be 
better also because this way Client handles everything, the QWidget 
representing the main widget is dumb, but strictly speaking you could create 
a real QWidget inherited class, and you wouldn't have to use event filter at 
all.

- in tutorial text only - activeChange() doesn't take bool - it used to 
before, but now you should simply use isActive().

- borders() - perhaps it should be stressed that the values returned from 
borders() are what really matters, and therefore should match the painting 
methods - if borders() returns values that don't match the reality, they'll 
be obeyed by KWin core, possibly resulting in improperly painted decoration

- It seems me to you left borders() method in ExampleButton class definition 
by a mistake.

- Your ExampleClient class has many methods as private:, even though they're 
not in fact, as they're reimplementation of virtual methods from KDecoration. 
I think it'd be better if you left those methods as public, and explicitly 
repeated the virtual keyword to make it more obvious they're 
reimplementations. To be specific, I'm talking about init(), and 
activeChange() till mousePosition() methods.

- ExampleClient::showEvent() - don't call widget()->show() there.

- For the tips'n'tricks section ;) : The decoration is also shown in the 
KControl module, so it might be simpler to check it there first, instead of 
restarting KWin. Also, the simplest (and safest) way to restart KWin is to 
run 'kwin --replace' from Alt+F2.

- Small nitpick - you don't need to write ' if(foo) delete foo;', just 'delete 
foo;' is enough - delete does that check.

 Otherwise, well, the code works for me, and I don't see anything else that 
should be said. Maybe others have something to say too.

>
> p.s. I mention that a bug in my discussion of mousePressEvent(), where I
> have to translate the mouse clicks to LeftButton. Am I correct in
> assuming that this is a bug?

 Depends on the way you look at it. QButton reacts only on left mouse events. 
Therefore you can either consider this to be a QButton feature, or you can 
consider basing the button class on QButton to be a bug. Whoever was the 
first one to do this probably considered using QButton to be the better 
choice.

 BTW, it'd be good if you explicitly mentioned that most buttons should check 
and react only if it was the left button that was pressed - see http://
bugs.kde.org/show_bug.cgi?id=58220 .

-- 
Lubos Lunak
KDE developer
---------------------------------------------------------------------
SuSE CR, s.r.o.  e-mail: l.lunak@suse.cz , l.lunak@kde.org
Drahobejlova 27  tel: +420 2 9654 2373
190 00 Praha 9   fax: +420 2 9654 2374
Czech Republic   http://www.suse.cz/

_______________________________________________
Kwin mailing list
Kwin@mail.kde.org
http://mail.kde.org/mailman/listinfo/kwin
[prev in list] [next in list] [prev in thread] [next in thread]