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
signature.asc
Description: This is a digitally signed message part.
