Review Request for KDecoration

Christoph Feck christoph at maxiom.de
Fri Nov 28 11:52:44 GMT 2014


On Friday 28 November 2014 12:00:43 Martin Gräßlin wrote:
> 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.
> [...]

> > > The DeocrationSettings used for this Deocration.
> > 
> > spelling
> 
> fixed (4895e46)

only 50% fixed... ;)

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

Ah, okey, thanks for the clarification.

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

... and since you also fixed the input handling to use the QPointF, 
this actually makes sense now. Merci :)

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

If I use the default constructor, no information about which screen to 
use for the DPI (point->pixel) conversion is passed, and it will 
probably use the default screen, which is fine for applications (which 
most probably never span multiple screens), but is fatal for KWin, 
which is supposed to provide readable decorations on multiple screens.

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

Christoph Feck (kdepepo)




More information about the kde-core-devel mailing list