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.
Details
- Reviewers
zzag - Group Reviewers
KWin - Commits
- R108:b64e67ce7c0c: Remove disabled TabGroup feature
Diff Detail
- Repository
- R108 KWin
- Branch
- tabgroup
- Lint
Lint Errors Excuse: I still don't understand the linter issue, I don't think I caused it. Severity Location Code Message Error kwinbindings.cpp:133 Cppcheck syntaxError - Unit
No Unit Test Coverage - Build Status
Buildable 14970 Build 14988: arc lint + arc unit
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 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.
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.
workspace.cpp | ||
---|---|---|
760 | It ensures that all clients(utilities) are shown. Keep it. |
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 | ||
---|---|---|
1615 | isCurrentTab is without tabs always true, right? So then this should be kept. |
client.cpp | ||
---|---|---|
952 | Keep this one too. We need to restore the skip-taskbar state. |
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.
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?
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.
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.
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?
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
Hold on. advanced.ui, mouse.ui, and actions.ui in kcmkwin/kwinoptions directory still have window tabbing stuff.