Review Request for KDecoration

[Milian Wolff]

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.

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.

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

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
-> also further down in ::addButton

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

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

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?

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

Generally: Ceterum censeo QList esse delendam
-> I suggest you remove QList and use QVector by default everywhere.

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?

Decoration.cpp
    ::findBridge
-> this should go into an anonymous namespace or made static
-> 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();
}
}

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

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

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

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/20141103/464c72e5/attachment.sig>