D13882: Use QTabBar: drop tons of code

[Tomaz Canabrava]

tcanabrava marked 11 inline comments as done.
tcanabrava added inline comments.

INLINE COMMENTS

> sandsmark wrote in 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.

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*.

> sandsmark wrote in ViewContainer.cpp:87
> maybe use qOverload instead of the ugly static_cast?

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.

> sandsmark wrote in ViewContainer.cpp:206
> outdated comment or is it broken?

outdated.

> sandsmark wrote in ViewContainer.cpp: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).

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?

> sandsmark wrote in ViewManager.cpp:618
> remove  dead code? unless this breaks something and is a valid TODO

invalid. forgot to remove.

> sandsmark wrote in ViewManager.cpp:698
> this breaks something in the kpart, I think?

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

> sandsmark wrote in ViewManager.cpp:897
> why was this moved so far up?

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

> sandsmark wrote in ViewManager.cpp:1161
> this breaks the «expand individual tab width to full window» option in settings -> configure konsole -> tabbar.

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

> sandsmark wrote in 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.

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.

REPOSITORY
  R319 Konsole

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

To: tcanabrava, hindenburg, #konsole, ngraham
Cc: sandsmark, ngraham, konsole-devel, herrold, maximilianocuria, hindenburg
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/konsole-devel/attachments/20180723/f7796a69/attachment.html>