Port TabGroup from Client to AbstractClient
ClosedPublic

Authored by graesslin on May 6 2018, 7:17 PM.

Details

Summary

First step towards a return of window tabbing.

Move TabGroup functionality from Client to AbstractClient

Only setClientShown remains in Client. This might need a dedicated
implementation for ShellClient.

Use AbstractClient in UserActionsMenu for tab functionality

Drop no longer needed cast to Client in TabGroup related code

Test Plan

hacked in tabs in useraction's menu and tried whether basic functionality still works

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
graesslin created this revision.May 6 2018, 7:17 PM
Restricted Application added a project: KWin. · View Herald TranscriptMay 6 2018, 7:17 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
graesslin requested review of this revision.May 6 2018, 7:17 PM
romangg accepted this revision as: KWin, romangg.May 11 2018, 12:06 AM
romangg added a subscriber: romangg.

Compiles and since it mostly moves code looks fine to me. I added inline comments to some non-blocking issues, which you can address as you please before pushing.

client.cpp
169 ↗(On Diff #33725)

Better style to use &AbstractClient::tabGroupChanged?

Are you sure it's ok to delay the code execution below by the signal? In the old code this was done before the signal was emitted. Maybe instead a virtual function with override is needed?

tabgroup.cpp
193 ↗(On Diff #33725)

fix coding style: brackets

305 ↗(On Diff #33725)

The keyword decltype was new to me and so I read up on it. This took multiple hours because it has quite complex semantics depending on the expression it evaluates. Still I'm happy I had to do it, so I learned about Rvalue references and so on in modern C++.

Still (and although it should be the simple case here by evaluating a simple expression, which gives just the type of the variable back) I would refrain from using it here and name the type explicitly instead.

To be more general from what I've read on it I would prefer to use decltype only when it provides a large benefit, i.e. in certain templated library functions, and not just to replace explicitly writing out a type in simple sequential code.

In the end it's your decision but I don't see how the usage of decltype simplifies the code here. I think it makes it more difficult to understand, most certainly for potential contributors, who know basic C++, but not the advanced tools for library writers introduced in modern C++.

This revision is now accepted and ready to land.May 11 2018, 12:06 AM
zzag added a subscriber: zzag.May 11 2018, 12:31 AM
zzag added inline comments.
abstract_client.h
1145 ↗(On Diff #33725)

Naming nitpick: why not m_tabGroup?

tabgroup.cpp
193 ↗(On Diff #33725)

Why not range-based for loop?

313 ↗(On Diff #33725)

IMHO, auto should not be used here because the type of c is not obvious.

graesslin added inline comments.May 14 2018, 3:45 PM
abstract_client.h
1145 ↗(On Diff #33725)

for new code yes. I wanted to do the refactoring least invasive so most of the code is just moved. I'll do a follow up commit changing the name.

client.cpp
169 ↗(On Diff #33725)

I used Client::tabGroupChanged as it's in the context of Client.

In this case it doesn't matter when the change property happens. It's async to X anyway.

tabgroup.cpp
193 ↗(On Diff #33725)

answer for both comments: I tried to do a least invasive refactoring. Thus only the change from ClientList to whatever the current type is. I did not want to change the logic of the loop by using range-based for and also did not want to fix the coding style.

For new code it would of course be a range based for and {}

305 ↗(On Diff #33725)

I'll change it. I used decltype as I expected the type to change during the refactoring and just forgot to switch to a proper type.

313 ↗(On Diff #33725)

According to Scott Meyer's Effictive modern C++ one should "Prefer auto to explicit type declarations".

I think it's totally obvious what the type is when using a good IDE like kdevelop. IMHO there is no other way to read code than using KDevelop.

zzag added inline comments.May 14 2018, 4:34 PM
tabgroup.cpp
313 ↗(On Diff #33725)

Not all people use IDEs. Also, if a piece of code is readable only in IDEs, maybe, that's a sign that something is wrong.

Sure, refactoring is much easier with auto, but it's harder to read code. IMHO, auto should be used wisely. E.g. when I call list.begin(), I know that the return type of this call is some kind of iterator so I can use auto, the code is still readable. But when I do auto* foo = (*i);, the type of foo is not really clear, the code is not readable at all.

This revision was automatically updated to reflect the committed changes.