| hindenburg | |
| ngraham |
| Konsole |
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:
and lots of testing.
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
Awesome!
Here are the behavioral quirks that I found during testing:
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.
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.
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.
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? | |
| 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/D13882I 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
I'm open to revert if someone could tell me how using git; there's a merge here so that complicates it.
| 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. | |
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)
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;
open items:
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
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.