Review Request for KDecoration

[Martin Gräßlin]

On Sunday 16 November 2014 23:30:35 Christoph Feck wrote:
> 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.

thanks for your comments and sorry that it took me so long to address them.

> 
> * decoration.h
> 
> > Rendering whether the Decoration is fully opaque.
> 
> This sentence makes no sense (but I understand what you mean).

fixed (4895e46)

> 
> > The DeocrationSettings used for this Deocration.
> 
> spelling

fixed (4895e46)

> 
> >    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().

on second thought: both signals should not be emitted at all. I'll try to 
change this to have the backend detecting the double click and the wheel 
events.

> 
> > 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.

It's because of KPluginFactory, the create method expects QObject, 
QvariantList as arguments

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

as you are not the first one asking that: DecorationButton changed to qreal:
* kdecoration: 2e49336
* breeze: f5952af

> 
> 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...

Fixed:
* kdecoration:  91e2bda
* kdecoration-viewer: a8f4199
* kwin: 0cc8665

> 
> > enum class BorderSize {
> 
> Does a decoration indicate that it supports different border sizes?
> Many old pixmap based themes do not.

No, it's only meant as a recommendation for the decoration. If it cannot 
support it that's just fine.

> 
> 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.

The setting is provided by the backend, e.g. KWin. It's not per screen (yet) 
but certainly the architecture allows to pass a different value per screen.

> 
> 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.

The documentation is copied from Plasma. I don't know anything about high DPI, 
so maybe sebas can answer that.

> 
> > 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).

sorry, but I don't get what you want. There is a ctor in QFontMetrics not 
needing a QPaintDevice.

> 
> > Q_PROPERTY(int smallSpacing
> 
> Which unit is this property in? Pixels?

sebas?

Cheers
Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20141128/0a72e7f8/attachment.sig>