Select correct notebook tab after closing while filtered (#4770)

CHANGELOG.md

@@ -6,6 +6,7 @@
 - Bugfix: Fixed a performance issue when displaying replies to certain messages. (#4807)
 - Bugfix: Fixed a data race when disconnecting from Twitch PubSub. (#4771)
 - Bugfix: Fixed `/shoutout` command not working with usernames starting with @'s (e.g. `/shoutout @forsen`). (#4800)
+- Bugfix: Fixed selection of tabs after closing a tab when using "Live Tabs Only". (#4770)
 - Dev: Fixed UTF16 encoding of `modes` file for the installer. (#4791)
 - Dev: Temporarily disable High DPI scaling on Qt6 builds on Windows. (#4767)
 - Dev: Tests now run on Ubuntu 22.04 instead of 20.04 to loosen C++ restrictions in tests. (#4774)

src/widgets/Notebook.cpp

@@ -101,35 +101,47 @@ void Notebook::removePage(QWidget *page)
     // Queue up save because: Tab removed
     getApp()->windows->queueSave();
 
-    for (int i = 0; i < this->items_.count(); i++)
+    int removingIndex = this->indexOf(page);
+    assert(removingIndex != -1);
+
+    if (this->selectedPage_ == page)
     {
-        if (this->items_[i].page == page)
+        // The page that we are removing is currently selected. We need to determine
+        // the best tab to select before we remove this one. We follow a strategy used
+        // by many web browsers: select the next tab. If there is no next tab, select
+        // the previous tab.
+        int countVisible = this->getVisibleTabCount();
+        int visibleIndex = this->visibleIndexOf(page);
+        assert(visibleIndex != -1);  // A selected page should always be visible
+
+        if (this->items_.count() == 1)
         {
-            if (this->items_.count() == 1)
-            {
-                this->select(nullptr);
-            }
-            else if (i == this->items_.count() - 1)
-            {
-                this->select(this->items_[i - 1].page);
-            }
-            else
-            {
-                this->select(this->items_[i + 1].page);
-            }
-
-            this->items_[i].page->deleteLater();
-            this->items_[i].tab->deleteLater();
-
-            //    if (this->items.empty()) {
-            //        this->addNewPage();
-            //    }
-
-            this->items_.removeAt(i);
-            break;
+            // Deleting only tab, select nothing
+            this->select(nullptr);
+        }
+        else if (countVisible == 1)
+        {
+            // Closing the only visible tab, try to select any tab (even if not visible)
+            int nextIndex = (removingIndex + 1) % this->items_.count();
+            this->select(this->items_[nextIndex].page);
+        }
+        else if (visibleIndex == countVisible - 1)
+        {
+            // Closing last visible tab, select the previous visible tab
+            this->selectPreviousTab();
+        }
+        else
+        {
+            // Otherwise, select the next visible tab
+            this->selectNextTab();
         }
     }
 
+    // Remove page and delete resources
+    this->items_[removingIndex].page->deleteLater();
+    this->items_[removingIndex].tab->deleteLater();
+    this->items_.removeAt(removingIndex);
+
     this->performLayout(true);
 }
 
@@ -154,6 +166,48 @@ int Notebook::indexOf(QWidget *page) const
     return -1;
 }
 
+int Notebook::visibleIndexOf(QWidget *page) const
+{
+    if (!this->tabVisibilityFilter_)
+    {
+        return this->indexOf(page);
+    }
+
+    int i = 0;
+    for (const auto &item : this->items_)
+    {
+        if (item.page == page)
+        {
+            assert(this->tabVisibilityFilter_(item.tab));
+            return i;
+        }
+        if (this->tabVisibilityFilter_(item.tab))
+        {
+            ++i;
+        }
+    }
+
+    return -1;
+}
+
+int Notebook::getVisibleTabCount() const
+{
+    if (!this->tabVisibilityFilter_)
+    {
+        return this->items_.count();
+    }
+
+    int i = 0;
+    for (const auto &item : this->items_)
+    {
+        if (this->tabVisibilityFilter_(item.tab))
+        {
+            ++i;
+        }
+    }
+    return i;
+}
+
 void Notebook::select(QWidget *page, bool focusPage)
 {
     if (page == this->selectedPage_)
@@ -1162,6 +1216,15 @@ size_t Notebook::visibleButtonCount() const
 
 void Notebook::setTabVisibilityFilter(TabVisibilityFilter filter)
 {
+    if (filter)
+    {
+        // Wrap tab filter to always accept selected tabs. This prevents confusion
+        // when jumping to hidden tabs with the quick switcher, for example.
+        filter = [originalFilter = std::move(filter)](const NotebookTab *tab) {
+            return tab->isSelected() || originalFilter(tab);
+        };
+    }
+
     this->tabVisibilityFilter_ = std::move(filter);
     this->performLayout();
     this->updateTabVisibility();
@@ -1200,11 +1263,15 @@ SplitNotebook::SplitNotebook(Window *parent)
     getSettings()->tabVisibility.connect(
         [this](int val, auto) {
             auto visibility = NotebookTabVisibility(val);
+            // Set the correct TabVisibilityFilter for the given visiblity setting.
+            // Note that selected tabs are always shown regardless of what the tab
+            // filter returns, so no need to include `tab->isSelected()` in the
+            // predicate. See Notebook::setTabVisibilityFilter.
             switch (visibility)
             {
                 case NotebookTabVisibility::LiveOnly:
                     this->setTabVisibilityFilter([](const NotebookTab *tab) {
-                        return tab->isLive() || tab->isSelected();
+                        return tab->isLive();
                     });
                     break;
                 case NotebookTabVisibility::AllTabs:

src/widgets/Notebook.hpp

@@ -45,12 +45,55 @@ public:
     void removePage(QWidget *page);
     void removeCurrentPage();
 
+    /**
+     * @brief Returns index of page in Notebook, or -1 if not found.
+     **/
     int indexOf(QWidget *page) const;
+
+    /**
+     * @brief Returns the visible index of page in Notebook, or -1 if not found.
+     * Given page should be visible according to the set TabVisibilityFilter.
+     **/
+    int visibleIndexOf(QWidget *page) const;
+
+    /**
+     * @brief Returns the number of visible tabs in Notebook. 
+     **/
+    int getVisibleTabCount() const;
+
+    /**
+     * @brief Selects the Notebook tab containing the given page.
+     **/
     virtual void select(QWidget *page, bool focusPage = true);
+
+    /**
+     * @brief Selects the Notebook tab at the given index. Ignores whether tabs
+     * are visible or not. 
+     **/
     void selectIndex(int index, bool focusPage = true);
+
+    /**
+     * @brief Selects the index'th visible tab in the Notebook.
+     * 
+     * For example, selecting the 0th visible tab selects the first tab in this 
+     * Notebook that is visible according to the TabVisibilityFilter. If no filter
+     * is set, equivalent to Notebook::selectIndex.
+     **/
     void selectVisibleIndex(int index, bool focusPage = true);
+
+    /**
+     * @brief Selects the next visible tab. Wraps to the start if required. 
+     **/
     void selectNextTab(bool focusPage = true);
+
+    /**
+     * @brief Selects the previous visible tab. Wraps to the end if required. 
+     **/
     void selectPreviousTab(bool focusPage = true);
+
+    /**
+     * @brief Selects the last visible tab. 
+     **/
     void selectLastTab(bool focusPage = true);
 
     int getPageCount() const;