Review Request for KDecoration

[Christoph Feck]

On Friday 31 October 2014 08:22:53 Martin Gräßlin wrote:
> today I want to start the review process for the new KDecoration

Hi Martin,

thanks for the work, here are some random comments from my side. I 
hope I am not too late.

* decoration.h

> Rendering whether the Decoration is fully opaque.

This sentence makes no sense (but I understand what you mean).

> The DeocrationSettings used for this Deocration.

spelling

>    void titleBarDoubleClicked();

Maybe it might sense to have the event here, to check keyboard 
modifiers?

>    void titleBarWheelEvent(const QPoint &angleDelta);

Why not simply pass the QWheelEvent? I might be interested in keyboard 
modifiers or the phase().

> explicit Decoration(QObject *parent, const QVariantList &args);

I see that you made the first argument be a map, which can have any
number of optional arguments. But why the list?
I would just remove the indirection, and use QVariantMap here.

decorationbuttongroup.h

Why is this API qreal based but decorationbutton.h not?

decorationdefines.h

> QuickHelp

ContextHelp? This would be consistent with "providesContextHelp"
property, as well as "requestContextHelp" signal. I actually had to
read this file just to check why "ContextHelp" did not work...

> enum class BorderSize {

Does a decoration indicate that it supports different border sizes?
Many old pixmap based themes do not.

decorationsettings.h

> Q_PROPERTY(int gridUnit

Where does this come from? Is this setting per screen? We should
make sure KWin supports multiple screens with different DPI.

Given that you call it "fundamental", I suggest to allow qreal here.
A "millimeter" is really small, and if you only allow integer values, 
the precision might be too coarse.

> Q_PROPERTY(QFont font

When I want to compute pixel sizes for the requested font, how
do I get the correct QPaintDevice needed for the QFontMetrics?
Otherwise, please provide a QFontMetrics (see QStyleOption).

> Q_PROPERTY(int smallSpacing

Which unit is this property in? Pixels?

Christoph Feck (kdepepo)