Multiple Splits per Tab
ClosedPublic

Authored by tcanabrava on Dec 17 2018, 7:01 PM.

Details

Summary

This is what I envision for the Konsole Tab / Splits
management. One tab contain a QSPlitter that can contain
multiple TerminalDisplays / Splits.

You can test this behavior by hitting ctrl + shift + 9
and ctrl + shift + 0 to activate the splits, and ctrl +
shift + t to activate a new tab.

Old:

New:

What works:

  • Tab Creation
  • Split Creation (Even Recursive splitting)
  • Terminal Close will close the Split on last split
  • Last last split to close will close the tab
  • Last tab to close will close konsole
  • Detaching
  • Tab Renaming
  • Closing splits after detach
  • Closing windows after detach
  • Detach / Reattach works!

This patch series has the commits of Thomas Surrel, Maciej Niedbdalski and Myself.

Diff Detail

Repository
R319 Konsole
Branch
terminatorStyleTabsRebase
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 8258
Build 8276: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes

This is really awesome work, guys. I gave it a shot today and was really impressed. Here are my only real suggestions after about 15 minutes of using it:

  1. "Close Session" in the File and context menus should be renamed to "Close Pane" or "Close Split View" when there are splits (the fact that a split pane is a session is an implementation detail that users don't need to know about)
  1. I find it difficult to tell which pane is focused when there are many splits. We should do something to make this clearer. One idea I had was to give each split its own header on top that has the pane's title plus a button on it for activating the context menu without needing to right-click. Then the background of this header could change depending on whether or not its pane has focus. Just a thought. :)
mglb added a comment.Jan 25 2019, 1:31 AM
  1. I find it difficult to tell which pane is focused when there are many splits. We should do something to make this clearer. One idea I had was to give each split its own header on top that has the pane's title plus a button on it for activating the context menu without needing to right-click. Then the background of this header could change depending on whether or not its pane has focus. Just a thought. :)

I think standard focus border would be ok:

There is also inactive window dimming, might be nice to apply it to inactive panels.

I'm still testing - I also notice it is hard to see the splitters - often I get another's view scrollbar confused w/ the splitter. I also notice some issues w/ how multiple splits hare calculated; I would think the other view would not change size.
If anyone could review the code, I'd appreciate more people looking now.

desktop/konsoleui.rc
28

When changing rc files, you have to increase the version at the top of file. Also, I'm not sure the 2 detach items should be under View - although I understand why you put them there.

src/Shortcut_p.h
40

Is this related to the splits?

src/ViewContainer.cpp
443

So the tab right click menu no longer has 'Read Only' - do you plan on fixing this before this is committed?

src/ViewManager.cpp
339

?

922

I need to test session restores w/ splits.

minor nitpicks

src/ViewManager.cpp
734–735

container not used

src/ViewSplitter.cpp
156

try not to shadow another variable here

  1. I find it difficult to tell which pane is focused when there are many splits. We should do something to make this clearer. One idea I had was to give each split its own header on top that has the pane's title plus a button on it for activating the context menu without needing to right-click. Then the background of this header could change depending on whether or not its pane has focus. Just a thought. :)

I think we should do that, we could use header to also show icons for "monitor for silence/activity", "copy input", etc. We also discussed possibility to rearrange terminals and header would give us a handle to drag terminals around.

tcanabrava added inline comments.Jan 25 2019, 9:10 AM
src/Shortcut_p.h
40

not really, but the changes where needed to make split resizing work

src/ViewContainer.cpp
443

Yes and no: the read only should apply to a TerminalDisplay, if it's in the Tab does it make sense to apply to all of the TerminalDisplays or just to the currently focused one? I'll add it to the context menu for the TerminalDisplay.

src/ViewManager.cpp
339

there's no notion of 'nextContainer' anymore, so I commented that out. I need to see if the call is userfull at all or remove.

tcanabrava updated this revision to Diff 50216.Jan 25 2019, 9:19 AM
  • Fix updating the detach menus

I also notice some issues w/ how multiple splits hare calculated

This is something that I have given up for the moment, I specifically set just the height or the width of the terminal, but it resizes the other too, and that moves the panes a bit. I looked into the code for quite a lot but failed to pinpoint the cause.

tcanabrava updated this revision to Diff 50269.Jan 25 2019, 3:51 PM
  • Dim unfocused terminals

I would vote against dimming. A common use case for having splitviews in a console is to monitor several terminals at the same time. You wouldn't want dimming to reduce the visibility in the other not-focused terminals in such a case.

I dimmed just a bit to reduce brightness, text is still really readable.
Can you test to see if you like? If not I’m open for suggestions.

Em sex, 25 de jan de 2019 às 17:08, Thomas Surrel <
noreply@phabricator.kde.org> escreveu:

thsurrel added a comment. View Revision
https://phabricator.kde.org/D17643

I would vote against dimming. A common use case for having splitviews in a
console is to monitor several terminals at the same time. You wouldn't want
dimming to reduce the visibility in the other not-focused terminals in such
a case.

*REPOSITORY*
R319 Konsole

*REVISION DETAIL*
https://phabricator.kde.org/D17643

*To: *tcanabrava, Konsole
*Cc: *ngraham, thsurrel, maciejn, mglb, hindenburg, konsole-devel,
maximilianocuria

I'm also not too keen on dimming inactive panes. One of the reasons why I suggested giving each pane its own compact header is so the header itself can be the thing that changes color, which will not only avoid impairing readability of the pane itself, but also mimic what happens with the window's own titlebar. It's a familiar concept.

I dimmed just a bit to reduce brightness, text is still really readable.
Can you test to see if you like? If not I’m open for suggestions.

I have, it looks really nice indeed if you are working on one terminal at a time only. But in the use case I described before, that gets quite in the way.
Personnaly, I would rather keep konsole as visually simple as possible. Changing the border's color as described in @mglb's comment would be my favorite.

mglb added a comment.Jan 25 2019, 4:23 PM

Dimming: I think it should be configurable (dependent on window dimming?), especially when the color is hardcoded (looks bad with e.g. light color schemes)
Headers: Again, configurable. Or at least made with configurability in mind, it can be added later.

Yeah I think dimming would be fine if it's disable-able (like the applicable KWin effect is). Same for headers too.

  • Dim unfocused terminals

Let's leave dimming or not dimming for a later patch please.

Ok. I’ll extract from here.

Em sáb, 26 de jan de 2019 às 05:38, Kurt Hindenburg <
noreply@phabricator.kde.org> escreveu:

hindenburg added a comment. View Revision
https://phabricator.kde.org/D17643

In D17643#399928 https://phabricator.kde.org/D17643#399928, @tcanabrava
https://phabricator.kde.org/p/tcanabrava/ wrote:

  • Dim unfocused terminals

    Let's leave dimming or not dimming for a later patch please.

    *REPOSITORY* R319 Konsole

    *REVISION DETAIL* https://phabricator.kde.org/D17643

    *To: *tcanabrava, Konsole *Cc: *ngraham, thsurrel, maciejn, mglb, hindenburg, konsole-devel, maximilianocuria

I'm also not too keen on dimming inactive panes. One of the reasons why I suggested giving each pane its own compact header is so the header itself can be the thing that changes color, which will not only avoid impairing readability of the pane itself, but also mimic what happens with the window's own titlebar. It's a familiar concept.

Hey I've made some attempt at drawing headers for splits, feel free to check it out in free time, gives some idea how it might work/look. https://phabricator.kde.org/D18587

It appears this causing yakuake to crash when closing a tab/session. I typically try to test the konsolepart using dolphin/yakuake/kdevelop/demo_konsolepart - it can be a pain depending on what distro/setup your using to get them to use the code you're testing.

  1. open yakuake and create a multiple tabs
  2. close a tab via menu or middle mouse
#3  0x00007fffc8c8e590 in Konsole::ViewSplitter::getToplevelSplitter (this=0x0)
    at /home/kurthindenburg/Devel/KDE/src/kde/applications/konsole/src/ViewSplitter.cpp:211
#4  0x00007fffc8c82025 in Konsole::ViewManager::sessionFinished (this=0xb370a0)
    at /home/kurthindenburg/Devel/KDE/src/kde/applications/konsole/src/ViewManager.cpp:469

I have also a patch for Yakuake to fix them too, they are using one kpart
per view instead of using the tabbar that konsole part provides. :/
This week it will be hard for me to touch this code but I’ll see what I can
do.

Em sáb, 2 de fev de 2019 às 20:43, Kurt Hindenburg <
noreply@phabricator.kde.org> escreveu:

hindenburg added a comment. View Revision
https://phabricator.kde.org/D17643

It appears this causing yakuake to crash when closing a tab/session. I
typically try to test the konsolepart using
dolphin/yakuake/kdevelop/demo_konsolepart - it can be a pain depending on
what distro/setup your using to get them to use the code you're testing.

  1. open yakuake and create a multiple tabs
  2. close a tab via menu or middle mouse

    #3 0x00007fffc8c8e590 in Konsole::ViewSplitter::getToplevelSplitter (this=0x0) at /home/kurthindenburg/Devel/KDE/src/kde/applications/konsole/src/ViewSplitter.cpp:211 #4 0x00007fffc8c82025 in Konsole::ViewManager::sessionFinished (this=0xb370a0) at /home/kurthindenburg/Devel/KDE/src/kde/applications/konsole/src/ViewManager.cpp:469

*REPOSITORY*
R319 Konsole

*REVISION DETAIL*
https://phabricator.kde.org/D17643

*To: *tcanabrava, Konsole
*Cc: *ngraham, thsurrel, maciejn, mglb, hindenburg, konsole-devel,
maximilianocuria

tcanabrava updated this revision to Diff 51572.Feb 13 2019, 8:04 AM
  • Remove deprecated code
  • Refactor to reduce the number of boilerplate
  • Fix updating the detach menus
  • Dim unfocused terminals
  • Rebase on master
  • Fix creation of splits in the middle of the grid
  • Remove Dim code: I'll add that to another review
tcanabrava updated this revision to Diff 51585.Feb 13 2019, 1:40 PM
  • Properly count the number of Terminals
maciejn added inline comments.Feb 13 2019, 1:52 PM
src/ViewManager.cpp
298

There is a method ViewSplitter::getTerminalDisplays() which is a lot more complicated than this. It should probably be simplified to this and used here or removed entirely.

tcanabrava added inline comments.Feb 13 2019, 4:26 PM
src/ViewManager.cpp
298

nice catch. that's really easy to remove.

tcanabrava updated this revision to Diff 51605.Feb 13 2019, 4:27 PM
  • Remove unused function - a findChildren is what it's needed
tcanabrava updated this revision to Diff 51607.Feb 13 2019, 4:36 PM
  • Call directly, lambda is uneeded here.
tcanabrava updated this revision to Diff 51608.Feb 13 2019, 4:39 PM
  • don't copy the map

Looking pretty awesome in general! However it's still rather difficult to discern which split view is the active one. Are we addressing that in another patch, or have those changes not been integrated yet>

There are two different patches for that, one from myself that adds a dim
effect (that Kurt asked to remove from this change set) and another one
that implements a header, also in a different review (but not from myself
and I don’t have the review number on my head).

If you want I can create a new review tomorrow with the dimming
implementation I n top of this one.

Em qua, 13 de fev de 2019 às 21:02, Nathaniel Graham <
noreply@phabricator.kde.org> escreveu:

ngraham added a comment. View Revision
https://phabricator.kde.org/D17643

Looking pretty awesome in general! However it's still rather difficult to
discern which split view is the active one. Are we addressing that in
another patch, or have those changes not been integrated yet>

*REPOSITORY*
R319 Konsole

*REVISION DETAIL*
https://phabricator.kde.org/D17643

*To: *tcanabrava, Konsole
*Cc: *ngraham, thsurrel, maciejn, mglb, hindenburg, konsole-devel,
gennad, maximilianocuria

The 19.04 branch will be created around March 14th. What do you think about committing this to master after the branch? That should give everyone 2-3 months in master to resolve any issues and get any apps using the KonsolePart to get fixed.

I’m ok with it.

Em sáb, 16 de fev de 2019 às 16:28, Kurt Hindenburg <
noreply@phabricator.kde.org> escreveu:

hindenburg added a comment. View Revision
https://phabricator.kde.org/D17643

The 19.04 branch will be created around March 14th. What do you think
about committing this to master after the branch? That should give everyone
2-3 months in master to resolve any issues and get any apps using the
KonsolePart to get fixed.

*REPOSITORY*
R319 Konsole

*REVISION DETAIL*
https://phabricator.kde.org/D17643

*To: *tcanabrava, Konsole
*Cc: *gennad, ngraham, thsurrel, maciejn, mglb, hindenburg,
konsole-devel, maximilianocuria

  • Remove deprecated code
  • Refactor to reduce the number of boilerplate
  • Fix updating the detach menus
  • Fix creation of splits in the middle of the grid
  • Properly count the number of Terminals
  • Remove unused function - a findChildren is what it's needed
  • Call directly, lambda is uneeded here.
  • don't copy the map
  • Fix rebase
  • Fix rebase
anthonyfieroni added inline comments.
desktop/konsoleui.rc
3

Increase version

src/MainWindow.cpp
321–322

Can you connect like:

connect(_newTabMenuAction, &KActionMenu::triggered, this, &MainWindow::newTab);
395–396

Same as above

connect(list, &Konsole::ProfileList::profileSelected, this, &MainWindow::newFromProfile);
src/ViewContainer.cpp
88–95
connect(tabBarWidget, &DetachableTabBar::detachTab, this, &TabbedViewContainer::detachTab);
93–96

Connecting signal to signal should work, same as above.

src/ViewManager.cpp
604–608

This should stay, i think.

Have you had time to look at Anthony's comments?

Kurt,

Sorry for the abscense, I was in vacations in brazil for three weeks
without a computer.
I just got back home (like, yesterday), and I also just bougth a new
computer (after 10 years, yey). What I'm doing right now is to setup my KDE
dev environment and compile everything.

So, give me a few days to have everythign correct and I'll enter in
berseker mode on konsole again.

Tomaz

hallas added a subscriber: hallas.Mar 10 2019, 3:42 PM
  • Remove deprecated code
  • Refactor to reduce the number of boilerplate
  • Fix updating the detach menus
  • Fix creation of splits in the middle of the grid
  • Properly count the number of Terminals
  • Remove unused function - a findChildren is what it's needed
  • Call directly, lambda is uneeded here.
  • don't copy the map
  • Fix rebase
  • Fix rebase
  • Update Konsole Rc version
  • Connect signals directly without lambdas
src/Application.cpp
193

It's not work on Wayland, no? Also don't set geometry, KWin will not like it. Use resize and move to coor by parents ones.

19.04 has been split; can you work on Anthony's comment and we can merge to master to get the most testing done on it before 19.08.  Thanks.

Will do that today

Em sáb, 16 de mar de 2019 às 18:30, Kurt Hindenburg <
noreply@phabricator.kde.org> escreveu:

hindenburg added a comment. View Revision
https://phabricator.kde.org/D17643

19.04 has been split; can you work on Anthony's comment and we can merge to master to get the most testing done on it before 19.08. Thanks.

*REPOSITORY*
R319 Konsole

*REVISION DETAIL*
https://phabricator.kde.org/D17643

*To: *tcanabrava, Konsole
*Cc: *hallas, anthonyfieroni, gennad, ngraham, thsurrel, maciejn, mglb,
hindenburg, konsole-devel, maximilianocuria

luc4 added a subscriber: luc4.Mar 17 2019, 10:28 AM

Will do that today

Checking in...

Kurt

not dead, just crazy at life.

src/ViewManager.cpp
604–608

this should actually be removed, the current way to deal with the focus is working without the proxies, also, there's no _viewSplitter anymore.

  • Remove deprecated code
  • Refactor to reduce the number of boilerplate
  • Fix updating the detach menus
  • Fix creation of splits in the middle of the grid
  • Properly count the number of Terminals
  • Remove unused function - a findChildren is what it's needed
  • Call directly, lambda is uneeded here.
  • don't copy the map
  • Fix rebase
  • Fix rebase
  • Update Konsole Rc version
  • Connect signals directly without lambdas
  • Resize instead of setting the geometry
tcanabrava marked 12 inline comments as done.Mar 26 2019, 11:27 AM

Kurt,

I'm using this now for months without problems, I know that people have different usecases but I belive that this is stable enough to land in master, giving us enough time to kill all the bugs for the next release.
and also to minimize the amount of possible breaks.

src/Application.cpp
193

I did not tested on wayland, my system woesn't work with it yet because of nvidia. I accept patches. :)

hindenburg accepted this revision.Mar 27 2019, 1:49 AM

OK thanks - I'm fine with that - I just notice that the shortcut to move tabs doesn't work

This revision is now accepted and ready to land.Mar 27 2019, 1:49 AM
This revision was automatically updated to reflect the committed changes.

Also can you link the yakuake patch so we make sure it gets in

Issues I see so far:

  1. Shortcuts to move tabs don't work
  2. Tab menu doesn't have 'Read Only' entry

I’m in a konsole spree this week so expect patches.

Em sex, 29 de mar de 2019 às 15:39, Kurt Hindenburg <
noreply@phabricator.kde.org> escreveu:

hindenburg added a comment. View Revision
https://phabricator.kde.org/D17643

Issues I see so far:

  1. Shortcuts to move tabs don't work
  2. Tab menu doesn't have 'Read Only' entry

*REPOSITORY*
R319 Konsole

*REVISION DETAIL*
https://phabricator.kde.org/D17643

*To: *tcanabrava, Konsole, hindenburg
*Cc: *luc4, hallas, anthonyfieroni, gennad, ngraham, thsurrel, maciejn,
mglb, hindenburg, konsole-devel, maximilianocuria

Session restore also doesn't work - you just get a blank area w/o a session

The code for sessions is commented out, in the original review you said
that you would help me see the saving the session code as now we have to
deal with splits.

Em sáb, 30 de mar de 2019 às 18:37, Kurt Hindenburg <
noreply@phabricator.kde.org> escreveu:

hindenburg added a comment. View Revision
https://phabricator.kde.org/D17643

Session restore also doesn't work - you just get a blank area w/o a session

*REPOSITORY*
R319 Konsole

*REVISION DETAIL*
https://phabricator.kde.org/D17643

*To: *tcanabrava, Konsole, hindenburg
*Cc: *luc4, hallas, anthonyfieroni, gennad, ngraham, thsurrel, maciejn,
mglb, hindenburg, konsole-devel, maximilianocuria

Thanks for continuing to work on this - did the yakuake patch get committed to avoid crashes?