Remove disabled TabGroup feature
ClosedPublic

Authored by gladhorn on Aug 10 2019, 9:58 AM.

Details

Summary

This has been commented out since 2014, I doubt it will come back.
This is a big amount of code, maintenance will be easier without it.

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.
gladhorn created this revision.Aug 10 2019, 9:58 AM
Restricted Application added a project: KWin. · View Herald TranscriptAug 10 2019, 9:58 AM
Restricted Application added a subscriber: kwin. · View Herald Transcript
gladhorn requested review of this revision.Aug 10 2019, 9:58 AM

This is about window tabs? There was a patch preparing enabling them again at the beginning of the year by @graesslin. Don't know what happened afterwards. There is also m_switchToTabMenu. Is this not also about the same functionality?

I just removed the obviously dead bits behind an if (false). If someone wants to bring it back, it's always in the git history. I don't care either way, but to me reducing lines of code is generally a good thing.

I just removed the obviously dead bits behind an if (false). If someone wants to bring it back, it's always in the git history. I don't care either way, but to me reducing lines of code is generally a good thing.

I agree and I don't see tabs coming back soon either. But removing the control mechanism in UserActionsMenu just partly and then leaving few other bits of potentially now even more confusing zombie code behind I would also try to omit. Also if we want to get it back from git history at some point it would be nice to get it back from a single removing commit and not several ones removing small parts one after the other.

gladhorn updated this revision to Diff 63498.Aug 10 2019, 6:55 PM
gladhorn retitled this revision from Remove dead code to Remove disabled TabGroup feature.
gladhorn edited the summary of this revision. (Show Details)

Attempt to remove entire tabgroup feature.

If this is not wanted, then I won't spend more time on it, but it was
interesting to see how much code this really is.
If this is something that you think should land, then there are one or two
fixmes that need to be addressed.

zzag added a subscriber: zzag.Aug 10 2019, 7:26 PM

It looks like you removed some parts of the tabbox as well.

CMakeLists.txt
301

This option needs to stay.

abstract_client.cpp
31–33

Add it back.

gladhorn added inline comments.Aug 10 2019, 7:32 PM
CMakeLists.txt
301

Thanks, of course, I got mixed up, good catch!

abstract_client.cpp
31–33

Yes, together with it's use below... thanks :)

57

This comes back as well.

zzag added inline comments.Aug 10 2019, 7:45 PM
workspace.cpp
781

It ensures that all clients(utilities) are shown. Keep it.

zzag added a comment.Aug 10 2019, 7:47 PM

You overlooked _KDE_NET_WM_TAB_GROUP in atoms.cpp.

gladhorn marked an inline comment as done.Aug 10 2019, 7:59 PM
In D23069#509945, @zzag wrote:

You overlooked _KDE_NET_WM_TAB_GROUP in atoms.cpp.

Indeed, consider it done...

gladhorn updated this revision to Diff 63507.Aug 10 2019, 8:00 PM
gladhorn removed a subscriber: zzag.

Restored tabbox code

romangg added a subscriber: zzag.Aug 10 2019, 11:03 PM

Wow, didn't thought it would be that much abandoned code. I'm definitely now for removing all of it. Even if we find the time to bring tabbed windows back at some point in the future it will probably be easier to just start again from scratch. And with this commit we can then still consult the old code for inspiration.

So +1, current version also approved by @zzag?

client.cpp
1610

isCurrentTab is without tabs always true, right? So then this should be kept.

zzag added inline comments.Aug 10 2019, 11:14 PM
client.cpp
947

Keep this one too. We need to restore the skip-taskbar state.

gladhorn added inline comments.Aug 11 2019, 7:15 AM
client.cpp
947

Right, I'll grep for all isCurrentTab that I removed, makes a lot of sense :)

1610

Of course, thanks!

gladhorn updated this revision to Diff 63519.Aug 11 2019, 7:29 AM
gladhorn removed a subscriber: zzag.

Fix up a few more places (mostly isCurrentTab related)

gladhorn updated this revision to Diff 63520.Aug 11 2019, 7:42 AM

Also removed shortCaption which was only used by tab group.

zzag added a comment.Aug 11 2019, 2:17 PM

Hmm, I'm not able to apply the patch. Can you rebase it on master?

The patch should apply cleanly on top of https://phabricator.kde.org/D23067

OK, now this should apply :)

Just for the record: the patch to add window decoration back is in D13459 - I still have the code around and uncommitted changes on top. With the relatively small changes I had window tabs kind of working already. So if anyone wants to pick it up: we are almost there and I'm happy to share the code. Personally I doubt that I will find the time to finish it. I currently don't even find the time to push accepted changes. I'd love to do it, but I must be realistic.

On this change: we also have window tabs in the window rule system and also in the configuration system (e.g. middle click to move tabs).

Just for the record: the patch to add window decoration back is in D13459 - I still have the code around and uncommitted changes on top. With the relatively small changes I had window tabs kind of working already. So if anyone wants to pick it up: we are almost there and I'm happy to share the code. Personally I doubt that I will find the time to finish it. I currently don't even find the time to push accepted changes. I'd love to do it, but I must be realistic.

On this change: we also have window tabs in the window rule system and also in the configuration system (e.g. middle click to move tabs).

Hi Martin, it's great to hear from you :)
I wish pushing accepted changes was easier, I find it takes too much effort indeed (gitlab will hopefully make that better). I hope others can pick up your changes.
I wonder how popular the feature is, do we have any idea how many users it had? I have never used it personally, but that doesn't mean anything.
Since I'm so much on the sidelines and won't put major work into KWin either, I'll leave it up to the maintainers to make a call. This was just an attempt to remove about ten lines of code initially, which then turned way bigger than I imagined.

zzag added a comment.Fri, Aug 23, 5:39 PM

Just for the record: the patch to add window decoration back is in D13459 - I still have the code around and uncommitted changes on top. With the relatively small changes I had window tabs kind of working already. So if anyone wants to pick it up: we are almost there and I'm happy to share the code. Personally I doubt that I will find the time to finish it. I currently don't even find the time to push accepted changes. I'd love to do it, but I must be realistic.

Hi,

I would love to continue your work on window tabs, but I think that's a low priority feature right now. We need to deal with more important stuff first.

Given that it's not clear when we start resurrecting window tabs feature and whether we'll be pro-"window tabs" in the future, we can proceed with this change. I don't see any point to keep this code because one can't test it.

On this change: we also have window tabs in the window rule system and also in the configuration system (e.g. middle click to move tabs).

@gladhorn Can you please remove these leftovers?

zzag added a comment.Fri, Aug 23, 5:52 PM

Lint Errors Excuse: help appreciated, I don't know what the problem is... it compiles just fine here...

Heh, looks like cppcheck doesn't have any clue that kwinbindings.cpp is included in another file. Just ignore this error. :-)

On this change: we also have window tabs in the window rule system and also in the configuration system (e.g. middle click to move tabs).

@gladhorn Can you please remove these leftovers?

Any pointers where to look? I just did a quick grep and I'm unsure what's left.

zzag added a comment.Sat, Sep 7, 8:25 AM

Any pointers where to look? I just did a quick grep and I'm unsure what's left.

You need to remove these two options. Relevant code is in kcmkwin/kwinoptions directory. Don't forget to remove corresponding options in Options class too.

zzag added a comment.Sat, Sep 7, 8:43 AM

and also here

GB_2 added subscribers: zzag, GB_2.Sat, Sep 7, 9:11 AM
In D23069#526982, @zzag wrote:

Any pointers where to look? I just did a quick grep and I'm unsure what's left.

You need to remove these two options. Relevant code is in kcmkwin/kwinoptions directory. Don't forget to remove corresponding options in Options class too.

Should I remove them in D23615?

zzag added a comment.Sat, Sep 7, 9:24 AM
In D23069#526993, @GB_2 wrote:

Should I remove them in D23615?

No, because that will be unrelated change. However, one of you will need to resolve merge conflicts.

I can do the UI changes (and I don't mind re-basing the change if the other one goes in first). Should I squash the UI changes into this patch or create a new change?

GB_2 added a comment.Sat, Sep 7, 9:34 AM

I can do the UI changes (and I don't mind re-basing the change if the other one goes in first). Should I squash the UI changes into this patch or create a new change?

I think it's best if the improved UI lands first and then you remove the complete feature with the UI after that in this patch.

I did the UI changes and kept them separate for now. https://phabricator.kde.org/D23767

zzag added a comment.Mon, Sep 9, 7:57 PM

@gladhorn Please update this revision. D23615 has just landed.

gladhorn updated this revision to Diff 65885.Wed, Sep 11, 10:05 PM

Merged in removal of UI

Restricted Application added a project: Documentation. · View Herald TranscriptWed, Sep 11, 10:05 PM
Restricted Application added a subscriber: kde-doc-english. · View Herald Transcript
zzag accepted this revision.Thu, Sep 12, 7:09 AM
zzag added inline comments.
useractions.cpp
441

We'll switch to nullptr in one go. See D23618. Revert this unrelated change.

This revision is now accepted and ready to land.Thu, Sep 12, 7:09 AM
zzag requested changes to this revision.Thu, Sep 12, 7:14 AM

Hold on. advanced.ui, mouse.ui, and actions.ui in kcmkwin/kwinoptions directory still have window tabbing stuff.

This revision now requires changes to proceed.Thu, Sep 12, 7:14 AM
gladhorn updated this revision to Diff 66018.Sat, Sep 14, 8:22 AM
gladhorn removed a reviewer: zzag.
gladhorn removed a project: Documentation.
gladhorn removed subscribers: kde-doc-english, GB_2, zzag.

Removed stuff from .ui files

Restricted Application added a project: Documentation. · View Herald TranscriptSat, Sep 14, 8:22 AM
zzag accepted this revision.Sat, Sep 14, 8:44 AM
This revision is now accepted and ready to land.Sat, Sep 14, 8:44 AM
This revision was automatically updated to reflect the committed changes.