[Differential] [Requested Changes To] D3472: API for window tabbing
graesslin (Martin Gräßlin)
noreply at phabricator.kde.org
Fri Nov 25 14:03:39 UTC 2016
graesslin requested changes to this revision.
graesslin added a comment.
This revision now requires changes to proceed.
Overall looks quite good. Please watch the coding style a little bit. That created a few comments.
There are two conceptional things I don't get. One is why you have the tabCount in DecoratedClient, I would assume that is only relevant to the DecorationTabGroup or the Decoration.
The other is all these "Op" things. I don't understand why it would be needed in KDecoration at all and why it's exposed through the DecoratedClient. If you need to know whether a mouse button should start the drag, use the settings.
INLINE COMMENTS
> decoratedclient.h:205-211
> + //Begin Window Tabbing
> + int tabCount() const; // tab count
> + void activateClient(); // activates the tab
> + QList<QPointer<DecoratedClient>> clients() const; // get list of clients in the client group
> + bool isTabDragOp(Qt::MouseButtons btn) const;
> + bool isOperationsOp(Qt::MouseButtons btn) const;
> + //End Window Tabbing
you can turn your comments into documentation. The begin window tabbing and end window tabbing could for example be doxygen sections.
> decoratedclient.h:206
> + //Begin Window Tabbing
> + int tabCount() const; // tab count
> + void activateClient(); // activates the tab
What is tabCount in the context of a DecoratedClient?
> decoratedclient.h:207
> + int tabCount() const; // tab count
> + void activateClient(); // activates the tab
> + QList<QPointer<DecoratedClient>> clients() const; // get list of clients in the client group
The method name and the comment do not match. Either it's activating a client or the tab. In general I wouldn't call a method activateFoo if it's in a Foo class. So just activate would be enough.
> decoratedclient.h:209
> + QList<QPointer<DecoratedClient>> clients() const; // get list of clients in the client group
> + bool isTabDragOp(Qt::MouseButtons btn) const;
> + bool isOperationsOp(Qt::MouseButtons btn) const;
what's an "Op"?
> decoratedclient.h:259
> + //for window tabbing
> + void updateTabGroup(); //emitted when a client is tabbed/untabbed
> +
that doesn't sound like a signal, but more like a method to invoke. Maybe tabGroupChanged?
> decoration_p.h:68
> QVector<DecorationButton*> buttons;
> + DecorationTabGroup *tabGroup;
> QSharedPointer<DecorationShadow> shadow;
I don't see where it's initialized. It's neither set to null nor to a proper value on construction. Please initialize.
> decorationtab.cpp:1
> +#include "decoratedclient.h"
> +#include "decorationtab.h"
please add copyright header
> decorationtab.cpp:8-11
> + :QObject(parent)
> + ,m_client(client)
> + ,m_geometry(QRectF())
> + ,active(client->isActive())
nitpick on coding style: add a space between : and QObject and , and the variables
> decorationtab.cpp:39
> +{
> + return active;
> +}
Once the DecorationTab got activated this method is returning an incorrect value. It caches the active state of the client, but never updates again.
> decorationtab.h:1
> +#ifndef KDECORATION2_DECORATIONTAB_H
> +#define KDECORATION2_DECORATIONTAB_H
please add a copyright header
> decorationtab.h:35
> + bool isActive() const;
> + void setActive();
> +
with the name setActive I would assume a bool argument. Given that it's about activation, call it "activate"
> decorationtab.h:46
> + QRectF m_geometry;
> + bool active;
> +};
naming style: m_active
> decorationtabgroup.cpp:1
> +#include "decoration.h"
> +#include "decoration_p.h"
Please add copyright header
> decorationtabgroup.cpp:14-18
> + :decoration(decoration)
> + ,needTabs(false)
> + ,mouseButtonPressed(false)
> + ,tab(NULL)
> + ,q(parent)
coding style: white spaces
> decorationtabgroup.cpp:26-27
> +DecorationTabGroup::DecorationTabGroup(Decoration *parent)
> + :QObject(parent)
> + ,d(new Private(parent,this))
> +{
coding style: white spaces
> decorationtabgroup.cpp:32-33
> + connect(c, &DecoratedClient::updateTabGroup, this,
> + [this](){
> + setNeedTabs(tabCount()!=0);
> + });
coding style: add whitespaces
> decorationtabgroup.cpp:82-83
> +{
> + auto c = decoration()->client().data();
> + return c->tabCount();
> +}
why do you go through the DecoratedClient? You have the d->windowTabs you can derive the number from?
> decorationtabgroup.cpp:90
> +
> + if(!d->windowTabs.isEmpty()) {
> + qDeleteAll(d->windowTabs);
missing white space
> decorationtabgroup.cpp:95-96
> +
> + if (!needTabs())
> + return;
> +
please use {}
> decorationtabgroup.cpp:109-110
> + int i = 0;
> + QSizeF size = geometry().size();
> + int tabWidth = size.width()/tabCount();
> + auto it = d->windowTabs.begin();
const
> decorationtabgroup.cpp:111-112
> + int tabWidth = size.width()/tabCount();
> + auto it = d->windowTabs.begin();
> + while (it != d->windowTabs.end()) {
> + QRectF tabGeom(geometry().x() + i*tabWidth,0,tabWidth,size.height());
constBegin and constEnd
> decorationtabgroup.cpp:113
> + while (it != d->windowTabs.end()) {
> + QRectF tabGeom(geometry().x() + i*tabWidth,0,tabWidth,size.height());
> + (*it)->setGeometry(tabGeom);
whitespaces
> decorationtabgroup.cpp:113
> + while (it != d->windowTabs.end()) {
> + QRectF tabGeom(geometry().x() + i*tabWidth,0,tabWidth,size.height());
> + (*it)->setGeometry(tabGeom);
const
> decorationtabgroup.h:1
> +#ifndef KDECORATION2_DECORATIONTABGROUP_H
> +#define KDECORATION2_DECORATIONTABGROUP_H
please add copyright header
> decorationtabgroup.h:30
> + void setNeedTabs(bool set);
> + int tabCount();
> + void createTabs(std::function<DecorationTab*(DecoratedClient*, QObject*)> tabCreator);
const and rename just to count
> decorationtabgroup.h:31
> + int tabCount();
> + void createTabs(std::function<DecorationTab*(DecoratedClient*, QObject*)> tabCreator);
> + bool isButtonPressed() const;
It doesn't createTabs, it only creates one tab. But also either call it createDecorationTab or just create
> decorationtabgroup.h:38-43
> +private:
> + bool isTabDragOp(Qt::MouseButtons btn) const;
> + bool isOperationsOp(Qt::MouseButtons btn) const;
> + QPointer<Decoration> decoration() const;
> + QPointer<DecorationTab> tabAt(const QPointF &pos);
> + QList<QPointer<DecoratedClient>> getClientList();
as it's private methods it should go into the Private class
> decorationtabgroup.h:42
> + QPointer<Decoration> decoration() const;
> + QPointer<DecorationTab> tabAt(const QPointF &pos);
> + QList<QPointer<DecoratedClient>> getClientList();
const
> decorationtabgroup_p.h:1
> +#ifndef KDECORATION_DECORATIONTABGROUP_P_H
> +#define KDECORATION_DECORATIONTABGROUP_P_H
please add copyright header
> decoratedclientprivate.h:91-95
> + virtual int tabCount() const { return 0; }
> + virtual void activateClient() { return; }
> + virtual QList<QPointer<DecoratedClient>> clients() const { return QList<QPointer<DecoratedClient>>(); }
> + virtual bool isTabDragOp(Qt::MouseButtons btn) const {Q_UNUSED(btn); return 0;};
> + virtual bool isOperationsOp(Qt::MouseButtons btn) const {Q_UNUSED(btn); return 0;};
please don't inline virtual methods. That can result in problems.
REPOSITORY
rKDECORATION Window Decoration Library
REVISION DETAIL
https://phabricator.kde.org/D3472
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: chinmoyr, graesslin, #kwin, #plasma
Cc: plasma-devel, kwin, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20161125/49a58c18/attachment-0001.html>
More information about the Plasma-devel
mailing list