Use QTabBar: drop tons of code
ClosedPublic

Authored by tcanabrava on Jul 4 2018, 12:02 PM.

Details

Summary

this is a WIP, I don't plan to merge this yet, there are many thigns to solve.

Drop the handmade TabWidget used inside of konsole in favor of Qt's QTabWidget
This drops tons of code we manually did, but it also drops a bit of functionality.

Everything besides detach should be working.
I'm now trying to finish detach.

a nice overvie:
15 files changed, 314 insertions(+), 1609 deletions(-)

Missing features:

  • Detach

and lots of testing.

Diff Detail

Repository
R319 Konsole
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
tcanabrava created this revision.Jul 4 2018, 12:02 PM
Restricted Application added a project: Konsole. · View Herald TranscriptJul 4 2018, 12:02 PM
Restricted Application added a subscriber: konsole-devel. · View Herald Transcript
tcanabrava requested review of this revision.Jul 4 2018, 12:02 PM
  • Restore TabBar visibility and position settings
  • Connect to KonsoleSettings directly
  • Remove Borders
  • Add the Close and New Tab Buttons
  • Re enable close tabs, simplify code
  • Focus widget in currentChanged
  • Re enable rename tab
  • Remove replay signal.
  • Fix Rename of tabs
  • Re-Enable dragging tabs
  • Re-Enable detaching actions
tcanabrava edited the summary of this revision. (Show Details)Jul 16 2018, 12:16 PM
tcanabrava updated this revision to Diff 37882.Jul 16 2018, 3:30 PM
  • Fix movement with keyboard
  • New tab to manage the drag out of the tabbar
  • Add currentTabIndex for easy menu access.
  • re-add detachTab

Awesome!

Here are the behavioral quirks that I found during testing:

  • When drag-re-ordering tabs, the dragged tab visually stays on the bar now and only moves horizontally (yay!) but if you release the mouse outside of the tab bar, the dragged tab now detaches with no warning*
  • Tab context menu lost its Close menu item
  • When "Show new tab and close tab buttons" becomes checked after being unchecked, the space reserved for the New Tab button doesn't actually have that button in it until you quit and restart Konsole
  • It would also be nice if there was some sort of visual feedback for detaching a tab, and also a 30 or so pixel "dead zone" above and below the bar where releasing the mouse wouldn't detach the tab yet--but this is probably material for another patch. :)

Awesome!

Here are the behavioral quirks that I found during testing:

  • When drag-re-ordering tabs, the dragged tab visually stays on the bar now and only moves horizontally (yay!) but if you release the mouse outside of the tab bar, the dragged tab now detaches with no warning*

Fixed.

  • Tab context menu lost its Close menu item

Fixed.

  • When "Show new tab and close tab buttons" becomes checked after being unchecked, the space reserved for the New Tab button doesn't actually have that button in it until you quit and restart Konsole

Fixed.

  • It would also be nice if there was some sort of visual feedback for detaching a tab, and also a 30 or so pixel "dead zone" above and below the bar where releasing the mouse wouldn't detach the tab yet--but this is probably material for another patch. :)

Fixed.

  • Restore TabBar visibility and position settings
  • Connect to KonsoleSettings directly
  • Remove Borders
  • Add the Close and New Tab Buttons
  • Re enable close tabs, simplify code
  • Focus widget in currentChanged
  • Re enable rename tab
  • Remove replay signal.
  • Fix Rename of tabs
  • Re-Enable dragging tabs
  • Re-Enable detaching actions
  • Fix movement with keyboard
  • New tab to manage the drag out of the tabbar
  • Add currentTabIndex for easy menu access.
  • re-add detachTab
  • Add tolerance for detach
  • Hint the cursor about a detach
  • Increase dead zone for detaching
  • Fix detach from menu
  • Readd closeAction
  • Show Quick Open button consistently

Well done. You fixed one of my biggest annoyances with Konsole: the high precision required to re-order tabs without detaching them. I feel like the size of the dead zone could be increased even a bit more, but this implementation is already radically better then what we had before. Kudos!

All the other issues I found before are fixed now. However I found a new one: When I first open Konsole, there's a gray line going down the window that contstrains the text:

The issue disappears if I resize the window at all or add a new tab.

tcanabrava updated this revision to Diff 37955.Jul 17 2018, 2:25 PM
  • Fix size of TerminalDisplay on show
ngraham accepted this revision.Jul 17 2018, 2:39 PM

Fantastic, that's fixed it! This is a terrific patch, IMHO. Let's land it in master rather than the just-created 18.08 branch, and let's also probably wait for @hindenburg's opinion, too. :) I don't feel comfortable doing a real code review for this kind of diff, so my review is for behavior only.

This revision is now accepted and ready to land.Jul 17 2018, 2:39 PM
This revision was automatically updated to reflect the committed changes.

shouldn't this has been postponed until after hindenburg had time to review it? it's pretty large, but I'll try to review it as well.

one first issue is that I get «QObject::connect: No such signal Konsole::TabbedViewContainer::destroyed(TabbedViewContainer*)» when launching.

in general a lot of regressions, and some things that should be cleaned up.

maybe best to open a new diff or whatever phabricator calls it to fix it, or revert and re-open since there's so many regressions.

and I also think hindenburg needs to take a look, there's some things I'm uncertain about.

and lastly, merge commits breaks a lot of stuff like bisecting (and it seems like it isn't common to do in konsole), so I'd suggest to rebase and then merge without a merge commit in the future.

src/Application.cpp
433

«konsole --hide-tabbar» and «konsole --show-tabbar» don't work anymore, I can't see them getting checked for anywhere else.

src/ViewContainer.cpp
63

I'm not sure if konsole follows the kdelibs/qt coding style guidelines, but in case it does iirc auto should not be used here.

75

remove dead code

87

maybe use qOverload instead of the ugly static_cast?

88

again, breaks selecting the new profile when opening a tab.

94

same with qt coding style and auto

117

same issues with auto wrt. qt coding style

122

the «put new tab after current tab» seems to also be broken.

152

extremely minor coding style nitpick, avoid abbreviations.

160

in case konsole is the opposite of the qt coding style wrt. auto this should also be auto, I guess.

206

outdated comment or is it broken?

285

for some reason phabricator doesn't seem to show the patch correctly, but looking at the diff in git a new newtabbutton was added.

but the new button isn't laid out properly (tiny and top-aligned).

src/ViewContainer.h
89

document these

125

this breaks selecting the profile when opening a new tab.

133

document these as well

145

document this

src/ViewManager.cpp
618

remove dead code? unless this breaks something and is a valid TODO

698

this breaks something in the kpart, I think?

897

why was this moved so far up?

1161

this breaks the «expand individual tab width to full window» option in settings -> configure konsole -> tabbar.

src/ViewManager.h
159

isn't this a semi-public header? in that case this breaks api and abi. it's a private library, but I'm not sure how much e. g. yakuake uses.

299

this breaks the dbus API (and the setting, as noted elsewhere).

394

it seems like you removed all use of this, so can probably just remove this as well?

sandsmark added inline comments.Jul 21 2018, 12:34 PM
src/ViewContainer.h
275

fwiw, I think this is what caused the warning to appear, in ViewSplitter::registerContainer().

be careful with fixing it, though, as the comment in registerContainer() warns about weird crashes.

I would have preferred that I could have looked the code over before this was committed. The new files don't have the license/copyright on them.

Tomaz will be at Akademy in a few weeks and will look at these issues.

Sorry for the mess people, I have a lack of time this week for work related
issues, if you guys want to revert I completely understand and I can open a
new review later without the regressions.

Em dom, 22 de jul de 2018 às 16:05, Kurt Hindenburg <
noreply@phabricator.kde.org> escreveu:

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

I would have preferred that I could have looked the code over before this
was committed. The new files don't have the license/copyright on them.

Tomaz will be at Akademy in a few weeks and will look at these issues.

*REPOSITORY*
R319 Konsole

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

*To: *tcanabrava, hindenburg, Konsole, ngraham
*Cc: *sandsmark, ngraham, konsole-devel, herrold, maximilianocuria,
hindenburg

Sorry for the mess people, I have a lack of time this week for work related
issues, if you guys want to revert I completely understand and I can open a
new review later without the regressions.

I'm open to revert if someone could tell me how using git; there's a merge here so that complicates it.

Sorry for the mess people, I have a lack of time this week for work related
issues, if you guys want to revert I completely understand and I can open a
new review later without the regressions.

I'm open to revert if someone could tell me how using git; there's a merge here so that complicates it.

I found some time today and I'm frenetically trying to fix some stuff here.

tcanabrava marked 11 inline comments as done.Jul 23 2018, 11:48 AM
tcanabrava added inline comments.
src/ViewContainer.cpp
63

We have some 'auto' in the code before this one so I used here. the kdelibs coding style was written before C++11, perhaps we should update it for future generations. I don't use auto where the type is ambiguous or when it's impossible for the human eye to know what's the type, but since here it's an list-of-actions, it's easy to see that auto will be a QAction*.

87

qOverload needs C++14, konsole is being compiled currently with C++11 - if you are ok with that I can enable C++ 14 for it.

206

outdated.

285

This is probably a issue with Qt as this method is from the QTabBar. Can you show a picture as in mine it looks correct so I can see what I can do?

src/ViewManager.cpp
618

invalid. forgot to remove.

698

I'v tested the kpart manyally and did not found bugs. I'll recheck.

897

it seemed logic when I did, now I'm unsure. if requested I can put it back.

1161

that's something I want to remove but I know some people use like that. I'm fixing it.

src/ViewManager.h
159

Yes, it breaks API and ABI. I request some help here as the idea of a searchBar() method *here* in the viewManager was when we had only *one* searchBar() that was reparented whenever we requested it. but now the TerminalDisplay has it's searchBar() internally and we don't share the instances. (this fixed around quite a few bugs actually), so in my perspective, this is inside the konsole private library and it actually should be removed as there's no searchBar() being managed by the viewManager anymore.

tcanabrava marked 2 inline comments as done.EditedJul 23 2018, 12:08 PM

Updates:
https://phabricator.kde.org/D14294
https://phabricator.kde.org/D14295
https://phabricator.kde.org/D14296
https://phabricator.kde.org/D14297
https://phabricator.kde.org/D14298

I think with those I fixed all of the issues raised (minus the nitpicks, I'll open another review for those)

hindenburg added a comment.EditedJul 24 2018, 3:02 AM

just realized this has been failing https://build.kde.org/job/Applications%20konsole%20kf5-qt5%20SUSEQt5.9/

Also note, Konsole currently only requires 5.6 - I'd really perfer to not change this unless really needed.

02:50:58 /home/jenkins/workspace/Applications konsole kf5-qt5 SUSEQt5.9/src/ViewContainer.cpp:316:52: error: ‘currentType’ is not a member of ‘QOperatingSystemVersion’
02:50:58 const auto isDarwin = QOperatingSystemVersion::currentType() == QOperatingSystemVersion::MacOS;

just realized this has been failing https://build.kde.org/job/Applications%20konsole%20kf5-qt5%20SUSEQt5.9/

Also note, Konsole currently only requires 5.6 - I'd really perfer to not change this unless really needed.

02:50:58 /home/jenkins/workspace/Applications konsole kf5-qt5 SUSEQt5.9/src/ViewContainer.cpp:316:52: error: ‘currentType’ is not a member of ‘QOperatingSystemVersion’
02:50:58 const auto isDarwin = QOperatingSystemVersion::currentType() == QOperatingSystemVersion::MacOS;

I fixed this by reverting back to using ENABLE_DETACHING

open items:

  1. QObject::connect: No such signal Konsole::TabbedViewContainer::destroyed(TabbedViewContainer*) in /home/kurthindenburg/Devel/KDE/src/kde/applications/konsole/src/ViewSplitter.cpp:96
  1. Can not move tabs between windows: detach a tab and try to reattach it
hwti added a subscriber: hwti.Mar 8 2019, 1:03 PM

This change breaks the previous behavior when closing a tab : now focus goes to the next tab (or to the last tab when closing the last tab).
Previously, it was going to the previously focused one.
https://bugs.kde.org/show_bug.cgi?id=405213

thats Easy to fix but I’d say that the current way is more “expected”. Nate?

Em sex, 8 de mar de 2019 às 14:03, Loïc Yhuel <noreply@phabricator.kde.org>
escreveu:

hwti added a comment. View Revision https://phabricator.kde.org/D13882

This change breaks the previous behavior when closing a tab : now focus
goes to the next tab (or to the last tab when closing the last tab).
Previously, it was going to the previously focused one.
https://bugs.kde.org/show_bug.cgi?id=405213

*REPOSITORY*
R319 Konsole

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

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

thats Easy to fix but I’d say that the current way is more “expected”. Nate?

Let's change it back for now and resolve that bug report. Behavioral changes should only be done intentionally following discussion. There may be merit to using this new style, but we should discuss it first and possibly make it a user-configurable setting IMO.