Don't duplicate the tabs when Spliting the View
ClosedPublic

Authored by tcanabrava on Nov 30 2018, 4:59 PM.

Details

Summary

Instead of iterating over the sessions to fetch the view and
duplicate the current screen once for each split, ignore all
of them and just create a new terminal view. The only thing
this terminal view shares with the previous current widget
is the profile.

use createView instead of manually trying to create the view

Fixes bug while closing the split

Don't duplicate tabs in the splits

BUG: 385697
BUG: 380455
FIXED-IN: 19.04

Diff Detail

Repository
R319 Konsole
Branch
dontDuplicateTabs
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 5747
Build 5765: arc lint + arc unit
tcanabrava created this revision.Nov 30 2018, 4:59 PM
Restricted Application added a project: Konsole. · View Herald TranscriptNov 30 2018, 4:59 PM
Restricted Application added a subscriber: konsole-devel. · View Herald Transcript
tcanabrava requested review of this revision.Nov 30 2018, 4:59 PM
tcanabrava edited the summary of this revision. (Show Details)Nov 30 2018, 5:02 PM
tcanabrava added reviewers: thsurrel, hindenburg, gennad.

Well done on fixing the crash !

I don't know how hard that would be, but another cool feature would be to be able to move tabs from one split to another with drag & drop. Right now if you do that, your tab just vanishes, we probably don't want that.

I plan to remove the hability to have tabs per split in the future, I
believe an implementation like terminator makes more sense. One Tabbar,
splits inside of the tabs. But konsole code is really tied and I’m doing
many cleanup commits just to be able to change that in the future.

Em sex, 30 de nov de 2018 às 21:46, Thomas Surrel <
noreply@phabricator.kde.org> escreveu:

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

Well done on fixing the crash !

I don't know how hard that would be, but another cool feature would be to
be able to move tabs from one split to another with drag & drop. Right now
if you do that, your tab just vanishes, we probably don't want that.

*REPOSITORY*
R319 Konsole

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

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

thsurrel accepted this revision.Dec 1 2018, 8:29 PM

I like the goal, good luck!
I am much less knowledgeable in konsole than you, so I'd recommend to wait for someone else's approval too.

Consider adding BUG: 225202 so that we keep track when it got fixed.

This revision is now accepted and ready to land.Dec 1 2018, 8:29 PM
tcanabrava updated this revision to Diff 46759.Dec 2 2018, 10:08 PM
  • Allow to move a tab between splitters
thsurrel added inline comments.Dec 3 2018, 9:11 AM
src/ViewManager.cpp
669

sourceViewManager is unused now.

tcanabrava updated this revision to Diff 46771.Dec 3 2018, 9:21 AM
  • Simplify Check
tcanabrava marked an inline comment as done.Dec 3 2018, 9:25 AM

There's still one bug when I try to drag a tab from a window *back* into another window when I just habe one tab. I need to fix that before having an accept.

tcanabrava updated this revision to Diff 46782.Dec 3 2018, 11:56 AM
  • Fix tab management

Ready for review, I belive I killed all the bugs related to this.

Small glitch: when we move one tab from one side of a split view to the other, the tab content is not re-drawn correctly.

untill you click on it. I'v realized that but I belive this is not a
blocker. The TerminalDisplay has *too many* issues not redrawing correctly
after a move untill someone clicks on it or it gains focus.

tcanabrava updated this revision to Diff 46875.Dec 4 2018, 9:30 PM
  • currentTabChanged was never connected
  • Fix the redraw after focus
ngraham accepted this revision.Dec 4 2018, 11:35 PM
ngraham added a subscriber: ngraham.

Code seems sane to me and it does what it says it does. :)

thsurrel accepted this revision.Dec 5 2018, 8:21 AM

Works great! Well done on fixing some existing bugs along the way!

considering that this is a behavioral change I'll wait for @hindenburg ok to merge this.

w/o the Konsole reviewer, it doesn't show up on https://phabricator.kde.org/project/37/item/view/637/ which is what I use

Ok let me look at this - I notice if you double-click on an empty spot in the non-focused view's tabbar, the new tab is created in the focused view. Which is a bit confusing.

This fixes https://bugs.kde.org/show_bug.cgi?id=385697
https://bugs.kde.org/show_bug.cgi?id=380455

Ok, I'll take a look.
Seems easy to fix.

Em qua, 5 de dez de 2018 às 15:52, Kurt Hindenburg <
noreply@phabricator.kde.org> escreveu:

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

Ok let me look at this - I notice if you double-click on an empty spot in
the non-focused view's tabbar, the new tab is created in the focused view.
Which is a bit confusing.

This fixes https://bugs.kde.org/show_bug.cgi?id=385697
https://bugs.kde.org/show_bug.cgi?id=380455

*REPOSITORY*
R319 Konsole

*BRANCH*
dontDuplicateTabs

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

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

tcanabrava updated this revision to Diff 46960.Dec 6 2018, 2:46 PM
  • Allow to move a tab between splitters
  • Simplify Check
  • Fix tab management
  • currentTabChanged was never connected
  • Fix the redraw after focus
  • Create the default container on constructor
  • Return the current tabWidget from the view
  • Be explicit in where we are creating the new session
  • Always pass the tab that the session will be created
  • Export the ViewContainer
  • Expand the signals / slots to use the tabWidget

The last fix (opening a tab by double clicking on the empty space on the tabbar) was a bit more intrusive than what I wanted (as the signal must be forwarded to the MainWindow / KPart, to be forwarded back and the pointer to the TabWidget that was clicked needed to be in the callchain. To make it more safe I removed the signal-casts in the connect changing the names of the signals, keeping compile-time correctness, so I'm pretty sure I didn't broke code by doing that. The calls that could use the ~default~ tab now just ask for activeContainer() as a parameter. in the end the code is a bit safer and more explicit on what's happening.

hindenburg edited the summary of this revision. (Show Details)Dec 7 2018, 3:35 PM

I've noticed a few issues

  1. detach split tabs maintain the same size (ie not the full window) but the size the tab was - is this expected behavior?
  2. close one side of the view causing the menus to not have all the menus until switching tabs
  3. opening a new tab on the non-focus side will not update the tab text (ie should be "src:zsh", it is ":" until you click on tab).

As usual thanks for the really good review.
I’ll address the Points tomorrow as today I’m traveling.

The detach tabs I believe that it takes the size of the terminal display;
probably it’s something that’s broken with the current behavior already.

The point two seems easy to fix.
And I believe the fix for point three is the same for point two.

I need to focus the newly created tab for point 3. And I need to focus the
current tab for point 2.

Em sáb, 8 de dez de 2018 às 18:16, Kurt Hindenburg <
noreply@phabricator.kde.org> escreveu:

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

I've noticed a few issues

  1. detach split tabs maintain the same size (ie not the full window) but the size the tab was - is this expected behavior?
  2. close one side of the view causing the menus to not have all the menus until switching tabs
  3. opening a new tab on the non-focus side will not update the tab text (ie should be "src:zsh", it is ":" until you click on tab).

*REPOSITORY*
R319 Konsole

*BRANCH*
dontDuplicateTabs

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

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

tcanabrava updated this revision to Diff 47179.Dec 9 2018, 12:58 PM
  • Allow to move a tab between splitters
  • Simplify Check
  • Fix tab management
  • currentTabChanged was never connected
  • Fix the redraw after focus
  • Create the default container on constructor
  • Return the current tabWidget from the view
  • Be explicit in where we are creating the new session
  • Always pass the tab that the session will be created
  • Export the ViewContainer
  • Expand the signals / slots to use the tabWidget
  • After cretaing the view, focus it
  • If a side of the split is close, focus another tab
  • Fix size of the new Tab
hindenburg edited the summary of this revision. (Show Details)Dec 10 2018, 12:10 AM

This is really good - I noticed that once you detach/close views, the menus View->Close/Expand/Shrink are still active (they don't do anything when selected).

I'm fine w/ committing this to get more testing and allow other fixes to be smaller.

Then please commit, I’ll work on the other bugs as soon as this hits master.

Em seg, 10 de dez de 2018 às 01:11, Kurt Hindenburg <
noreply@phabricator.kde.org> escreveu:

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

This is really good - I noticed that once you detach/close views, the
menus View->Close/Expand/Shrink are still active (they don't do anything
when selected).

I'm fine w/ committing this to get more testing and allow other fixes to
be smaller.

*REPOSITORY*
R319 Konsole

*BRANCH*
dontDuplicateTabs

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

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

hindenburg accepted this revision.Dec 10 2018, 3:01 PM
This revision was automatically updated to reflect the committed changes.

FYI

ViewContainer.cpp:133:44: warning: Missing emit keyword on signal call Konsole::TabbedViewContainer::newViewWithProfileRequest

Might be nice to backport this (and a few other fixes) to the 18.12 branch. See https://bugs.kde.org/show_bug.cgi?id=396689#c7

Might be nice to backport this (and a few other fixes) to the 18.12 branch. See https://bugs.kde.org/show_bug.cgi?id=396689#c7

I rather see this as a feature but I will look at it and other commits to be backported.

What would be the level of effort to make the split view behavior change optional. This change forced me to downgrade to version 18 since it seriously broke my workflow.

Seriously much. As the splits in master are in no way near what they where
before. But you might consider trying master, you might like the current
split behavior.
(It actually splits).
For the old behavior there was just 2 complaints, your and someone that
talked to me on email, and I’ll ask you the same thing I asked him. Try to
help with patches to have the old behavior back as a mirror tabs feature.
I’m trying to solve as many bugs as I can, but I can’t do that alone :)

Em ter, 7 de mai de 2019 às 21:07, Alex Olshansky <
noreply@phabricator.kde.org> escreveu:

aolshansky added a comment. View Revision
https://phabricator.kde.org/D17267

What would be the level of effort to make the split view behavior change
optional. This change forced me to downgrade to version 18 since it
seriously broke my workflow.

*REPOSITORY*
R319 Konsole

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

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

Thanks for reply. I did try the new behavior and it seriously ruins my workflow, but I do understand why this was patched and approved. Unfortunately I don't have the skills to help with patches, but I would be happy to reward a developer with time/skills that would be willing to bring back the old (mirroring) behavior as an option. Does any mechanism exist to put a bounty on a wanted feature?