D13882: Use QTabBar: drop tons of code

[Martin Tobias Holmedahl Sandsmark]

sandsmark added a comment.


  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.

INLINE COMMENTS

> Application.cpp:433
> -        }
> -        if (m_parser->isSet(QStringLiteral("hide-tabbar"))) {
> -            // never show

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

> ViewContainer.cpp:63
>          // Remove the read-only action when the popup closes
>          for (auto &action : _contextPopupMenu->actions()) {
>              if (action->objectName() == QStringLiteral("view-readonly")) {

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.

> ViewContainer.cpp:75
> +//                                 SLOT(tabContextMenuDetachTab()));
>  #endif
>  

remove dead code

> ViewContainer.cpp:87
>      connect(profileList, &Konsole::ProfileList::profileSelected, this,
> -            static_cast<void (TabbedViewContainer::*)(Profile::Ptr)>(&Konsole::TabbedViewContainer::
> -                                                                     newViewRequest));
> -    setNewViewMenu(profileMenu);
> -
> -    _closeTabButton = new QToolButton(_containerWidget);
> -    _closeTabButton->setFocusPolicy(Qt::NoFocus);
> -    _closeTabButton->setIcon(QIcon::fromTheme(QStringLiteral("tab-close")));
> -    _closeTabButton->setToolTip(i18nc("@info:tooltip", "Close tab"));
> -    _closeTabButton->setWhatsThis(i18nc("@info:whatsthis", "Close the active tab"));
> -    _closeTabButton->adjustSize();
> -
> -    // 'new tab' button is initially hidden. It will be shown when setFeatures()
> -    // is called with the QuickNewView flag enabled. The 'close tab' is the same.
> -    _newTabButton->setHidden(true);
> -    _closeTabButton->setHidden(true);
> -
> -    connect(_newTabButton, &QToolButton::clicked, this,
> -            static_cast<void (TabbedViewContainer::*)()>(&Konsole::TabbedViewContainer::newViewRequest));
> -    connect(_closeTabButton, &QToolButton::clicked, this,
> -            &Konsole::TabbedViewContainer::closeCurrentTab);
> -
> -    // Combine tab bar and 'new/close tab' buttons
> -    _tabBarLayout = new QHBoxLayout;
> -    _tabBarLayout->setSpacing(0);
> -    _tabBarLayout->setContentsMargins(0, 0, 0, 0);
> -    _tabBarLayout->addWidget(_newTabButton);
> -    _tabBarLayout->addWidget(_tabBar);
> -    _tabBarLayout->addWidget(_closeTabButton);
> -
> -    // The search bar
> -    searchBar()->setParent(_containerWidget);
> -
> -    // The overall layout
> -    _layout = new QVBoxLayout;
> -    _layout->setSpacing(0);
> -    _layout->setContentsMargins(0, 0, 0, 0);
> -
> -    setNavigationPosition(position);
> -
> -    _containerWidget->setLayout(_layout);
> +            static_cast<void (TabbedViewContainer::*)(Profile::Ptr)>(&Konsole::TabbedViewContainer::newViewRequest));
> +  //  setNewViewMenu(profileMenu);

maybe use qOverload instead of the ugly static_cast?

> ViewContainer.cpp:88
> +            static_cast<void (TabbedViewContainer::*)(Profile::Ptr)>(&Konsole::TabbedViewContainer::newViewRequest));
> +  //  setNewViewMenu(profileMenu);
>  }

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

> ViewContainer.cpp:94
> +    for(int i = 0, end = count(); i < end; i++) {
> +        auto view = widget(i);
> +        disconnect(view, &QWidget::destroyed, this, &Konsole::TabbedViewContainer::viewDestroyed);

same with qt coding style and auto

> ViewContainer.cpp:117
> +    auto swappedWidget = widget(newIndex);
> +    auto currentWidget = widget(currentIndex);
> +    removeTab(newIndex);

same issues with auto wrt. qt coding style

> ViewContainer.cpp:122
>  
> -void TabbedViewContainer::navigationPositionChanged(NavigationPosition position)
> +void TabbedViewContainer::addView(QWidget *view, ViewProperties *item, int index)
>  {

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

> ViewContainer.cpp:152
>  {
> -    if (_tabBar->count() > 1 && _tabBar->isHidden()) {
> -        setTabBarVisible(true);
> -    }
> -
> -    if (_tabBar->count() < 2 && !_tabBar->isHidden()) {
> -        setTabBarVisible(false);
> -    }
> +    const int idx = indexOf(view);
> +    disconnect(view, &QWidget::destroyed, this, &Konsole::TabbedViewContainer::viewDestroyed);

extremely minor coding style nitpick, avoid abbreviations.

> ViewContainer.cpp:160
>  {
> -    _tabBar->setStyleSheet(styleSheet);
> +    QWidget *active = currentWidget();
> +    int index = indexOf(active);

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

> ViewContainer.cpp:206
>  {
> -    viewProperties(views()[index])->rename();
> +    // TODO: Fix rename.
> +    // _navigation[index]->rename();

outdated comment or is it broken?

> ViewContainer.cpp:285
> -    _tabBarLayout(nullptr),
> -    _newTabButton(nullptr),
> -    _closeTabButton(nullptr),

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

> ViewContainer.h:89
> +    void updateIcon(ViewProperties *item);
> +    void updateActivity(ViewProperties *item);
>  

document these

> ViewContainer.h:125
> +    // TODO: Reenable this later.
> +    //    void setNewViewMenu(QMenu *menu);
> +    void renameTab(int index);

this breaks selecting the profile when opening a new tab.

> ViewContainer.h:133
> +    void tabDoubleClicked(int index);
> +    void openTabContextMenu(const QPoint &point);
>  

document these as well

> ViewContainer.h:145
>  
> +    void closeTab(TabbedViewContainer *tabbedViewContainer, QWidget *index);
> +

document this

> ViewManager.cpp:618
> +    //TODO: Fix Detaching.
> +    //    connect(container, &TabbedViewContainer::detachTab, this, &ViewManager::detachView);
> +    connect(container, &TabbedViewContainer::closeTab, this, &ViewManager::closeTabFromContainer);

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

> ViewManager.cpp:698
> -    if (_navigationMethod != TabbedNavigation) {
> -        container->setTabBarVisible(false);
> -    }

this breaks something in the kpart, I think?

> ViewManager.cpp:897
>      QSet<Session *> unique;
> +    int tab = 1;
> +

why was this moved so far up?

> ViewManager.cpp:1161
> -    Q_ASSERT(container);
> -    container->setNavigationTextMode(useTextWidth);
> -}

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

> ViewManager.h:159
> -     */
> -    IncrementalSearchBar *searchBar() const;
> -

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.

> ViewManager.h:299
> -    /** DBus slot that sets ALL tabs' width to match their text */
> -    Q_SCRIPTABLE void setTabWidthToText(bool);
> -

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

> ViewManager.h:394
>  
>      NavigationMethod _navigationMethod;
>      QString _navigationStyleSheet;

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

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/20180721/9c809f73/attachment-0001.html>