Review Request for KDecoration

[Martin Gräßlin]

On Monday 03 November 2014 22:29:22 Albert Astals Cid wrote:
> El Divendres, 31 d'octubre de 2014, a les 08:22:53, Martin Gräßlin va
> 
> escriure:
> > Hi KDE core developers,
> > 
> > today I want to start the review process for the new KDecoration
> > library[1]. This library is intended to replace the window decoration
> > library used by KWin and addresses some of the shortcomings we hit with
> > the existing library.
> > 
> > The library is a tier 1 in frameworks speak. It consists of two parts:
> > * a public library to create decorations
> > * a private library to create the backend (used by KWin)
> 
> I had a look at kdecoration, i could not find anything big, so i'm just
> commenting on minor stuff

thanks for having a look.

> 
> Some of your classes use int for positioning e.g. DecorationShadow, but
> other use real like DecorationButtonGroup. Is that on purpose or a bug?

I'll look again, but I'm sure it's intended. DecorationButtonGroup is inside 
the decoration so it's up to the implementation on how it is supposed to work. 
In order to not restrict it, it's in real. On the other hand DecorationShadow 
is the interface to the outside world (e.g. KWin) where real pixel sizes don't 
make sense.

> 
> Some of your classes use QScopedPointer<Private> d; and other
> std::unique_ptr<DecorationSettingsPrivate> d; Why?

the std::unique_ptr is used in two places where the instance gets created by 
the DecorationBridge. It could also be a QScopedPointer but I changed to make 
the methods in DecorationBridge return a std::unique_ptr to indicate that the 
ownership is passed to the caller of the method.

> 
> If you change
>   void addButton(QPointer<DecorationButton> button);
> to
>   void addButton(const QPointer<DecorationButton> &button);
> You'll save a copy constructor of QPointer

thanks, adjusted in e2ee8d9

> 
> 
> QList<QPointer<DecorationButton> > buttons() const;
> afaik if you're doing C++11 you can type
> QList<QPointer<DecorationButton>> buttons() const;

yes, one can do and I do everywhere except in one place ;-) Adjusted in 
9ae0f04

> 
> Cheers,
>   Albert
> 
> > After the review the repository will be moved under KDE/Workspace and see
> > its first release in Plasma 5.2. In theory it would also be a fit for
> > frameworks, but KDecoration uses C++11 features not allowed in frameworks.
> > 
> > If you want to give the whole thing a try you need some further
> > development
> > branches and repositories:
> > 
> > * kdecoration-viewer provides a small tool to preview a decoration. Usage:
> >   #> kdecorationviewer plugin [theme]
> > 
> > * Breeze [3] provides a new KDecoration theme. Once installed one can use
> > it> 
> > in kdecoration-viewer:
> >  #> kdecorationviewer org.kde.breeze
> > 
> > * KWin [4] is ported to use the new KDecoration library and tries to load
> > org.kde.breeze by default. Also the Aurorae theme engine is ported and the
> > configuration module. You can switch the decoration used by KWin in
> > systemsettings5.
> > 
> > What's still missing:
> > * A window tab API. I have some ideas on how to do it, but want to wait
> > for
> > the first drafts for DWD to not stupidly block some possibilities. My
> > ideas
> > on how to integrate window tabs can be done in a ABI-stable way for the
> > public library.
> > * Better support for OpenGL based decorations (e.g. QtQuick). This needs
> > to
> > wait till Qt 5.4. Qt 5.4 will finally allow to share with non
> > QOpenGLContext and that will hopefully allow to make better use. Once I
> > have a Qt 5.4 setup and experimented with it I'll draft an API for this.
> > Most likely this will end in a specific subclass exposing new virtuals.
> > * Various pieces in KWin, especially in the configuration area. Help is
> > more than welcome :-)
> > 
> > Best Regards
> > Martin Gräßlin
> > 
> > [1] kde:kdecoration
> > [2] kde:kdecoration-viewer
> > [3] kde:breeze (branch graesslin/kdecoration2)
> > [4] kde:kwin (branch graesslin/kdecoration2)
-------------- 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/20141104/8842faa4/attachment.sig>