This patch adds the support for window tabbing to the Kdecoration2 API.
decorationtab* - This is the actual tab clicking on which switches the client.
decorationtabgroup* - This is the tab manager, responsible for creation of tabs, setting their geometry and calling their respective paint methods.
build only
| Lint Skipped |
| Unit Tests Skipped |
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.
| src/decoratedclient.h | ||
|---|---|---|
| 205–211 | you can turn your comments into documentation. The begin window tabbing and end window tabbing could for example be doxygen sections. | |
| 206 | What is tabCount in the context of a DecoratedClient? | |
| 207 | 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. | |
| 209 | what's an "Op"? | |
| 259 | that doesn't sound like a signal, but more like a method to invoke. Maybe tabGroupChanged? | |
| src/decoration_p.h | ||
| 68 | I don't see where it's initialized. It's neither set to null nor to a proper value on construction. Please initialize. | |
| src/decorationtab.cpp | ||
| 2 | please add copyright header | |
| 9–12 | nitpick on coding style: add a space between : and QObject and , and the variables | |
| 40 | Once the DecorationTab got activated this method is returning an incorrect value. It caches the active state of the client, but never updates again. | |
| src/decorationtab.h | ||
| 2 | please add a copyright header | |
| 36 | with the name setActive I would assume a bool argument. Given that it's about activation, call it "activate" | |
| 47 | naming style: m_active | |
| src/decorationtabgroup.cpp | ||
| 2 | Please add copyright header | |
| 15–19 | coding style: white spaces | |
| 27–28 | coding style: white spaces | |
| 33–34 | coding style: add whitespaces | |
| 83–84 | why do you go through the DecoratedClient? You have the d->windowTabs you can derive the number from? | |
| 91 | missing white space | |
| 96–97 | please use {} | |
| 110–111 | const | |
| 112–113 | constBegin and constEnd | |
| 114 | whitespaces | |
| 114 | const | |
| src/decorationtabgroup.h | ||
| 2 | please add copyright header | |
| 31 | const and rename just to count | |
| 32 | It doesn't createTabs, it only creates one tab. But also either call it createDecorationTab or just create | |
| 39–44 | as it's private methods it should go into the Private class | |
| 43 | const | |
| src/decorationtabgroup_p.h | ||
| 2 | please add copyright header | |
| src/private/decoratedclientprivate.h | ||
| 91–95 | please don't inline virtual methods. That can result in problems. | |
On a general note: this terribly looks like doubling all (or a lot of) the core logics reg. tabs - which we had in initial KDE 4 tabbing and which made tabbing *incredibly* buggy.
I'd suggest to forward the cores tab logics and not keep local states, counters etc. around, you'll easily get out of sync.
what's an "Op"?
Op = Operation
If you need to know whether a mouse button should start the drag, use the settings.
By "use the settings" do you mean moving "*Op" methods to DecorationSettings??
why do you go through the DecoratedClient? You have the d->windowTabs you can derive the number from?
If you think the logic of "createDecorationTab" is fine then the "tabCount" method can be safely removed.
Once the DecorationTab got activated this method is returning an incorrect value. It caches the active state of the client, but never updates again.
When say 'n' clients are tabbed, 'n' tabs are painted on the titlebar of each client (i.e. total of n^2 tabs are created). In each client one tab which represents the client has m_active set to true and for rest of the tabs m_active is false.
So updating m_active is not required.