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)
