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
m...@milianw.de
http://milianw.de
signature.asc
Description: This is a digitally signed message part.
