Merge pull request #1689 from contour-terminal/improvement/tab-switching

[contour] Improve tab switch handling and add `SwitchToPreviousTab`

Author: christianparpart

metainfo.xml

@@ -113,6 +113,8 @@
           <li>Fixes startup crash when window is not yet fully initialized</li>
           <li>Fixes backtab (Shift+Tab) handling (#1685)</li>
           <li>Fixes various spelling typos across the codebase (#1688)</li>
+          <li>Improve tab close handling to better select previously focused tab</li>
+          <li>Add action `SwitchToPreviousTab` to switch to the previously focused tab</li>
         </ul>
       </description>
     </release>

src/contour/Actions.cpp

@@ -82,6 +82,7 @@ optional<Action> fromString(string const& name)
         mapAction<actions::CreateNewTab>("CreateNewTab"),
         mapAction<actions::CloseTab>("CloseTab"),
         mapAction<actions::SwitchToTab>("SwitchToTab"),
+        mapAction<actions::SwitchToPreviousTab>("SwitchToPreviousTab"),
         mapAction<actions::SwitchToTabLeft>("SwitchToTabLeft"),
         mapAction<actions::SwitchToTabRight>("SwitchToTabRight"),
     };

src/contour/Actions.h

@@ -82,6 +82,7 @@ struct WriteScreen{ std::string chars; }; // "\033[2J\033[3J"
 struct CreateNewTab{};
 struct CloseTab{};
 struct SwitchToTab{ int position; };
+struct SwitchToPreviousTab{};
 struct SwitchToTabLeft{};
 struct SwitchToTabRight{};
 // clang-format on
@@ -140,6 +141,7 @@ using Action = std::variant<CancelSelection,
                             CreateNewTab,
                             CloseTab,
                             SwitchToTab,
+                            SwitchToPreviousTab,
                             SwitchToTabLeft,
                             SwitchToTabRight>;
 
@@ -260,6 +262,7 @@ namespace documentation
     constexpr inline std::string_view SwitchToTab {
         "Switch to absolute tab position (starting at number 1)"
     };
+    constexpr inline std::string_view SwitchToPreviousTab { "Switch to the previously focused tab" };
     constexpr inline std::string_view SwitchToTabLeft { "Switch to tab to the left" };
     constexpr inline std::string_view SwitchToTabRight { "Switch to tab to the right" };
 } // namespace documentation
@@ -321,6 +324,7 @@ inline auto getDocumentation()
         std::tuple { Action { CreateNewTab {} }, documentation::CreateNewTab },
         std::tuple { Action { CloseTab {} }, documentation::CloseTab },
         std::tuple { Action { SwitchToTab {} }, documentation::SwitchToTab },
+        std::tuple { Action { SwitchToPreviousTab {} }, documentation::SwitchToPreviousTab },
         std::tuple { Action { SwitchToTabLeft {} }, documentation::SwitchToTabLeft },
         std::tuple { Action { SwitchToTabRight {} }, documentation::SwitchToTabRight },
     };
@@ -393,6 +397,7 @@ DECLARE_ACTION_FMT(ViNormalMode)
 DECLARE_ACTION_FMT(WriteScreen)
 DECLARE_ACTION_FMT(CreateNewTab)
 DECLARE_ACTION_FMT(CloseTab)
+DECLARE_ACTION_FMT(SwitchToPreviousTab)
 DECLARE_ACTION_FMT(SwitchToTabLeft)
 DECLARE_ACTION_FMT(SwitchToTabRight)
 // }}}
@@ -471,6 +476,7 @@ struct std::formatter<contour::actions::Action>: std::formatter<std::string>
         HANDLE_ACTION(ViNormalMode);
         HANDLE_ACTION(CreateNewTab);
         HANDLE_ACTION(CloseTab);
+        HANDLE_ACTION(SwitchToPreviousTab);
         HANDLE_ACTION(SwitchToTabLeft);
         HANDLE_ACTION(SwitchToTabRight);
         if (std::holds_alternative<contour::actions::SwitchToTab>(_action))

src/contour/TerminalSession.cpp

@@ -1446,6 +1446,12 @@ bool TerminalSession::operator()(actions::SwitchToTab const& event)
     return true;
 }
 
+bool TerminalSession::operator()(actions::SwitchToPreviousTab)
+{
+    emit switchToPreviousTab();
+    return true;
+}
+
 bool TerminalSession::operator()(actions::SwitchToTabLeft)
 {
     emit switchToTabLeft();

src/contour/TerminalSession.h

@@ -357,6 +357,7 @@ class TerminalSession: public QAbstractItemModel, public vtbackend::Terminal::Ev
     bool operator()(actions::CreateNewTab);
     bool operator()(actions::CloseTab);
     bool operator()(actions::SwitchToTab const& event);
+    bool operator()(actions::SwitchToPreviousTab);
     bool operator()(actions::SwitchToTabLeft);
     bool operator()(actions::SwitchToTabRight);
 
@@ -406,6 +407,7 @@ class TerminalSession: public QAbstractItemModel, public vtbackend::Terminal::Ev
     // Tab handling signals
     void createNewTab();
     void closeTab();
+    void switchToPreviousTab();
     void switchToTabLeft();
     void switchToTabRight();
     void switchToTab(int position);

src/contour/TerminalSessionManager.cpp

@@ -14,7 +14,6 @@
 
 #include <algorithm>
 #include <filesystem>
-#include <mutex>
 #include <string>
 
 using namespace std::string_literals;
@@ -44,10 +43,16 @@ std::unique_ptr<vtpty::Pty> TerminalSessionManager::createPty(std::optional<std:
 }
 
 TerminalSession* TerminalSessionManager::createSession()
+{
+    return activateSession(createSessionInBackground());
+}
+
+TerminalSession* TerminalSessionManager::createSessionInBackground()
 {
     // TODO: Remove dependency on app-knowledge and pass shell / terminal-size instead.
     // The GuiApp *or* (Global)Config could be made a global to be accessable from within QML.
-    //
+
+    _previousActiveSession = _activeSession;
 
 #if !defined(_WIN32)
     auto ptyPath = [this]() -> std::optional<std::string> {
@@ -72,7 +77,7 @@ TerminalSession* TerminalSessionManager::createSession()
 #endif
 
     auto* session = new TerminalSession(createPty(ptyPath), _app);
-    managerLog()("CREATE SESSION, new session: {}", (void*) session);
+    managerLog()("Create new session with ID {} at index {}", session->id(), _sessions.size());
 
     _sessions.push_back(session);
 
@@ -84,55 +89,81 @@ TerminalSession* TerminalSessionManager::createSession()
     // sessions. This will work around it, by explicitly claiming ownership of the object.
     QQmlEngine::setObjectOwnership(session, QQmlEngine::CppOwnership);
 
-    // we can close application right after session has been created
-    _lastTabChange = std::chrono::steady_clock::now() - std::chrono::seconds(1);
-    _activeSession = session;
     return session;
 }
 
 void TerminalSessionManager::setSession(size_t index)
 {
     Require(index <= _sessions.size());
     managerLog()(std::format("SET SESSION: index: {}, _sessions.size(): {}", index, _sessions.size()));
+
     if (!isAllowedToChangeTabs())
         return;
 
-    Require(display != nullptr);
-    auto const pixels = display->pixelSize();
-    auto const totalPageSize = display->calculatePageSize() + _activeSession->terminal().statusLineHeight();
+    if (index < _sessions.size())
+        activateSession(_sessions[index]);
+    else
+        activateSession(createSessionInBackground());
+}
 
-    auto* oldSession = _activeSession;
+TerminalSession* TerminalSessionManager::activateSession(TerminalSession* session, bool isNewSession)
+{
+    managerLog()(
+        "Activating session ID {} at index {}", session->id(), getSessionIndexOf(session).value_or(-1));
 
-    if (index < _sessions.size())
+    if (_activeSession == session)
     {
-        _activeSession = _sessions[index];
-        // Ensure that the existing session is resized to the display's size.
-        _activeSession->terminal().resizeScreen(totalPageSize, pixels);
+        managerLog()("Session is already active. (index {}, ID {})", getCurrentSessionIndex(), session->id());
+        return session;
     }
-    else
-        createSession();
-
-    if (oldSession == _activeSession)
-        return;
 
-    display->setSession(_activeSession);
-    // Resize active session after display is attached to it
-    // to return a lost line
-    _activeSession->terminal().resizeScreen(totalPageSize, pixels);
+    _previousActiveSession = _activeSession;
+    _activeSession = session;
+    _lastTabChange = std::chrono::steady_clock::now();
     updateStatusLine();
 
-    _lastTabChange = std::chrono::steady_clock::now();
+    if (display)
+    {
+        managerLog()("Attaching display to session.");
+        auto const pixels = display->pixelSize();
+        auto const totalPageSize =
+            display->calculatePageSize() + _previousActiveSession->terminal().statusLineHeight();
+
+        // Ensure that the existing session is resized to the display's size.
+        if (!isNewSession)
+            _activeSession->terminal().resizeScreen(totalPageSize, pixels);
+
+        display->setSession(_activeSession);
+
+        // Resize active session after display is attached to it
+        // to return a lost line
+        _activeSession->terminal().resizeScreen(totalPageSize, pixels);
+    }
+
+    return session;
 }
 
 void TerminalSessionManager::addSession()
 {
-    setSession(_sessions.size());
+    activateSession(createSessionInBackground(), true /*force resize on before display-attach*/);
+}
+
+void TerminalSessionManager::switchToPreviousTab()
+{
+    managerLog()("switch to previous tab (current: {}, previous: {})",
+                 getSessionIndexOf(_activeSession).value_or(-1),
+                 getSessionIndexOf(_previousActiveSession).value_or(-1));
+
+    if (!isAllowedToChangeTabs())
+        return;
+
+    activateSession(_previousActiveSession);
 }
 
 void TerminalSessionManager::switchToTabLeft()
 {
     const auto currentSessionIndex = getCurrentSessionIndex();
-    managerLog()(std::format("PREVIOUS TAB: currentSessionIndex: {}, _sessions.size(): {}",
+    managerLog()(std::format("previous tab: currentSessionIndex: {}, _sessions.size(): {}",
                              currentSessionIndex,
                              _sessions.size()));
 
@@ -164,73 +195,59 @@ void TerminalSessionManager::switchToTabRight()
 
 void TerminalSessionManager::switchToTab(int position)
 {
-    managerLog()(std::format(
-        "switchToTab from {} to {} (out of {})", getCurrentSessionIndex(), position - 1, _sessions.size()));
+    managerLog()("switchToTab from index {} to {} (out of {})",
+                 getSessionIndexOf(_activeSession).value_or(-1),
+                 position - 1,
+                 _sessions.size());
+
+    if (!isAllowedToChangeTabs())
+        return;
 
     if (1 <= position && position <= static_cast<int>(_sessions.size()))
-    {
-        setSession(position - 1);
-    }
+        activateSession(_sessions[position - 1]);
 }
 
 void TerminalSessionManager::closeTab()
 {
-    const auto currentSessionIndex = getCurrentSessionIndex();
-    managerLog()(std::format(
-        "CLOSE TAB: currentSessionIndex: {}, _sessions.size(): {}", currentSessionIndex, _sessions.size()));
-
-    // Session was removed outside of terminal session manager, we need to switch to another tab.
-    if (currentSessionIndex == -1 && !_sessions.empty())
-    {
-        // We need to switch to another tab, so we permit consequent tab changes.
-        // TODO: This is a bit hacky.
-        _lastTabChange = std::chrono::steady_clock::now() - std::chrono::seconds(1);
-        setSession(0);
-        return;
-    }
-
-    if (_sessions.size() > 1)
-    {
+    managerLog()("Close tab: current session ID {}, index {}",
+                 getSessionIndexOf(_activeSession).value_or(-1),
+                 _activeSession->id());
 
-        if (!isAllowedToChangeTabs())
-            return;
-
-        removeSession(*_activeSession);
-
-        // We need to switch to another tab, so we permit consequent tab changes.
-        // TODO: This is a bit hacky.
-        _lastTabChange = std::chrono::steady_clock::now() - std::chrono::seconds(1);
-
-        if (std::cmp_less_equal(currentSessionIndex, _sessions.size() - 1))
-        {
-            setSession(currentSessionIndex + 1);
-        }
-        else
-        {
-            setSession(currentSessionIndex - 1);
-        }
-    }
+    removeSession(*_activeSession);
 }
 
 void TerminalSessionManager::removeSession(TerminalSession& thatSession)
 {
-    managerLog()(std::format(
-        "REMOVE SESSION: session: {}, _sessions.size(): {}", (void*) &thatSession, _sessions.size()));
+    managerLog()("REMOVE SESSION: session: {}, _sessions.size(): {}", (void*) &thatSession, _sessions.size());
 
     if (!isAllowedToChangeTabs())
         return;
 
-    _app.onExit(thatSession); // TODO: the logic behind that impl could probably be moved here.
+    if (&thatSession == _activeSession && _previousActiveSession)
+        activateSession(_previousActiveSession);
 
-    auto i = std::ranges::find_if(_sessions, [&](auto p) { return p == &thatSession; });
-    if (i != _sessions.end())
+    auto i = std::ranges::find(_sessions, &thatSession);
+    if (i == _sessions.end())
     {
-        _sessions.erase(i);
+        managerLog()("Session not found in session list.");
+        return;
     }
+    _sessions.erase(i);
+    _app.onExit(thatSession); // TODO: the logic behind that impl could probably be moved here.
+
+    _previousActiveSession = [&]() -> TerminalSession* {
+        auto const currentIndex = getSessionIndexOf(_activeSession).value_or(0);
+        if (currentIndex + 1 < _sessions.size())
+            return _sessions[currentIndex + 1];
+        else if (currentIndex > 0)
+            return _sessions[currentIndex - 1];
+        else
+            return nullptr;
+    }();
+    managerLog()("Calculated next \"previous\" session index {}",
+                 getSessionIndexOf(_previousActiveSession).value_or(-1));
 
     updateStatusLine();
-    _lastTabChange = std::chrono::steady_clock::now();
-    // Notify app if all sessions have been killed to trigger app termination.
 }
 
 void TerminalSessionManager::updateColorPreference(vtbackend::ColorPreference const& preference)