Review Request for KDecoration

[Martin Gräßlin]

On Monday 03 November 2014 23:33:15 Milian Wolff wrote:
> On Friday 31 October 2014 08:22:53 Martin Gräßlin wrote:
> > 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.
> 
> Hey Martin,
> 
> I'll list a few things that I spotted, in no special order. Is there maybe a
> possibility to get some tooling help in the review phase here?
> Copy'n'pasting code and filenames is cumbersome to say the least ;-) Maybe
> we could create a (git read-only) kdereview repository to reviewboard. Then
> one can add the whole repo as a huge diff to that repository and review
> stuff there? Admins, is this feasible?
> 
> DecorationShadow::Private
> 	int paddingTop;
>     int paddingRight;
>     int paddingBottom;
>     int paddingLeft;
> -> QMargins? Might also simplify DecorationShadow then, though I don't know
> whether QMargins is transparently useable from within QML.

uh nice, I didn't know about it. Will have a look whether it can be used.

> 
> DecorationButton::Private and other places
>     QPointer<Decoration>
> -> is the lifetime really undefined? I.e. can a button outlive a decoration?
> Or put differently, why do you need a QPointer here? Also, if you really
> want to use it, pass it by const& like Albert mentioned.

Yes a DecorationButton could outlive a Decoration as the Decoration is not the 
parent for the DecorationButton. And it's also a valid use case: one might 
only be interested in the DecorationButton, but not in the Decoration 
(configuring the order of the buttons in the KCM).

> 
> Decoration::Private
> -> unrelated question: why do you add getters and setters for private data?
> just because or do you see a value in that? Personally, I'd get rid of all
> that code and use plain structs where possible. This is private API after
> all. With Q_D or similar you'll also get a const/non-const d_ptr which
> gives you const-correctness as well. But this is certainly not something
> you must change. I'm genuinely interested why you decided to go that route.

> 
> DecorationSettings
>     onAllDesktopsAvailableChanged
> -> remove the "on" prefix

No, maybe the name is bad, but if the on is removed the meaning changes. It's 
whether one can put DecoratedClients "on all desktops" (at lest two virtual 
desktops). I'll think about a better name.

> 
> DecorationButtonGroup::Private
>     buttons() const
> -> returning a const& is usually a bad idea. And due to NRVO it won't be
> faster than returning a non-const. Rather, you should bind to a const& on
> the callee side if you want to optimize, see also
> http://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-> const/ Note how this "issue" would not exist, if you'd let the public class
> access the data members of the ::Private classes directly.
> 
> DecorationButton .cpp
>     auto updateSlot = [this]() { update(); };
>     connect(this, &DecorationButton::hoveredChanged, this, updateSlot);
>     connect(this, &DecorationButton::pressedChanged, this, updateSlot);
>     connect(this, &DecorationButton::checkedChanged, this, updateSlot);
>     connect(this, &DecorationButton::enabledChanged, this, updateSlot);
> -> remove the named lambda and replace it with &DecorationButton::update

done in  c864731

> -> also further down in ::addButton

I don't see how I could do that as the invoked method is in the Private class 
which is not derived from QObject.

> 
> DecoarationButton
>     #define DELEGATE
>     #undef
>     #define
> -> why not have one macro, called DEFINE_GETTER_SETTER?
> 
> DecorationButtonGroup.cpp
>     for (auto it = m_buttons.constBegin(); it != m_buttons.constEnd(); ++it)
> { -> for (auto button : m_buttons)
> also in other places

in some places it's intended, because it's using the iterator API. Other 
places adjusted in a189fe2

> 
> while (!d->buttons().isEmpty()) {
>                 delete d->buttons().takeFirst();
>             }
> -> qDeleteAll + clear. Also note how this loop has very bad performance as
> you takeFirst. A QList<T*> is - more or less - a QVector<T*>.

thanks good point, adjusted in b4aacef

> 
> DecorationButtonGroup::hasButton
> -> you iterate over a list of QPointer<...> but do not check for the
> validity of the buttons. Why do you use QPointer then?

ah oversight. I introduced the QPointer later on and apparently forgot to 
adjust some places. Will adjust after adding auto test for 
DecorationButtonGroup.

> 
> DecorationButtonGroup::removeButton
> -> I'd suggest you use std::remove_if + std::erase. 

Will adjust after adding auto test for DecorationButtonGroup.

> 
> Generally: Ceterum censeo QList esse delendam

:-D

> -> I suggest you remove QList and use QVector by default everywhere.

 Will adjust after adding auto test for all usages of QList.

> 
> DecoratedClient::DC
>     , d(std::move(bridge->createClient(this, parent)))
> -> is the move here really required? I don't think so.
> 
> DecoratedClient::decoration
> -> why wrap it in a QPointer? If the caller wants a QPointer, he can wrap it
> manually, no?

No, the point I wanted to make by using a QPointer is that the API doesn't 
guarantee that the returned pointer stays valid. Kind of I want to enforce 
correct usage (note this are plugins loaded into KWin, I don't want a plugin 
to crash KWin by accessing a dangling pointer. Better save in the API than 
sorry).

> 
> Decoration.cpp
> 
>     ::findBridge
> 
> -> this should go into an anonymous namespace or made static

could you please explain why or point me to a web resource describing it?

> -> the check for isValid is not really required, .toMap will give you an
> empty map in such cases
> -> the find etc. below can be replaced by this code:
>     b = map.value(QStringLiteral("bridge"));
> .value() of a QMap/QHash will return a default-constructed value of T. And a
> T*() == Q_NULLPTR.
> So overall this can be written as:
> 
> namespace {
> DecorarationBridge* findBridge(const QVariantList &args)
> {
>     for (const auto &arg : args) {
>         if (auto bridge = arg.toMap().value(QStringLiteral("bridge"))) {
>             return bridge;
>         }
>     }
>     Q_UNREACHABLE();
> }
> }

almost, but it needs to be:
arg.toMap().value(QStringLiteral("bridge")).value<DecorationBridge*>()

toMap() returns a QVariantMap, so the value is a QVariant.

Adjusted in cb429f8.

> 
> Decoration::Private::set(Extended)Borders
> -> QMargins?

will have a look whether it's a solution.

> 
> DecoarationBridge::DB
>     qRegisterMetaType<Qt::MouseButton>();
> -> upstream that to Qt? Or why is this required here?

I'll have a look on why I added this.

> 
> Overall I can say very clean code. Nice work Martin! Hope the above nitpicks
> help you a bit.

thanks.

> 
> Cheers!
> --
> Milian Wolff
> mail at milianw.de
> http://milianw.de
-------------- 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/07f62566/attachment.sig>