Add "Undo close tab" feature by etude11 · Pull Request #1342 · kiwix/kiwix-desktop

etude11 · 2025-02-11T18:08:03Z

etude11 · 2025-03-15T17:37:30Z

@kelson42 I have implemented the feature of undo Close Tab. Kindly review.

kelson42 · 2025-10-09T07:46:45Z

@etude11 Sorry, somehow this PR got forgotten. I will make the review.

Copilot

Pull request overview

This PR implements an "undo close tab" feature (#1154) for the Kiwix desktop application. When users close tabs, the tab information (URL and title) is saved to a stack, allowing them to reopen recently closed tabs via a keyboard shortcut (Ctrl+Shift+T) or context menu. The implementation uses a QStack to maintain the history of closed tabs and enables the reopen action when tabs are available.

Key Changes

Adds a stack-based history of closed tabs with URL and title information
Enables the previously hidden "Reopen Closed Tab" action and connects it to functionality
Implements a context menu on the tab bar's empty area to access the reopen action
Saves tab information before closing ZimView tabs

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

File	Description
src/tabbar.h	Adds contextMenuEvent override declaration to handle right-click context menu
src/tabbar.cpp	Implements tab info saving before closure and context menu for reopening; simplifies comment
src/kiwixapp.h	Adds ClosedTabInfo struct, m_closedTabs stack, and public methods for managing closed tabs
src/kiwixapp.cpp	Enables ReopenClosedTabAction, implements pushClosedTab/reopenLastClosedTab methods

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-09T06:53:20Z

+{
+    int tabIndex = tabAt(event->pos());
+    if (tabIndex == -1) {  // Clicked outside tabs
+        QMenu menu;


[nitpick] The QMenu menu is created on the stack and will be properly destroyed when it goes out of scope after menu.exec() returns. However, consider using a parent pointer (e.g., QMenu menu(this)) to ensure proper cleanup even if exceptions occur, following Qt best practices for object lifetime management.

Suggested change

QMenu menu;

QMenu menu(this);

Copilot · 2025-12-09T06:53:20Z

 {
-    // The first and last tabs (i.e. the library tab and the + (new tab) button)
-    // cannot be closed
+    // First and last tabs cannot be closed


[nitpick] The comment was simplified but lost helpful context. The original comment explained what the first and last tabs are (library tab and new tab button), which aids code comprehension. Consider restoring the original comment or keeping at least: // First and last tabs (library tab and + button) cannot be closed

Suggested change

// First and last tabs cannot be closed

// First and last tabs (library tab and + button) cannot be closed

Copilot · 2025-12-09T06:53:21Z

+    struct ClosedTabInfo {
+        QString url;
+        QString title;
+    };
+    QStack<ClosedTabInfo> m_closedTabs;
+
+public:
+    void pushClosedTab(const QString& url, const QString& title);
+    bool hasClosedTabs() const { return !m_closedTabs.isEmpty(); }
+    void reopenLastClosedTab();


Avoid adding a new public: access specifier in the middle of the private: section. This breaks the class organization pattern. Move the struct definition and member variable to stay within the private: section, and relocate the three public methods to the existing public: section (before line 116). Following the codebase pattern seen in other files (e.g., contentmanager.h, downloadmanagement.h), you could organize the private section with comments like // types and // data for better clarity.

Copilot · 2025-12-09T06:53:21Z

 }
+
+void KiwixApp::pushClosedTab(const QString& url, const QString& title) {
+    if (url.isEmpty() || title.isEmpty())


The check title.isEmpty() may reject valid tabs with empty titles. Consider saving tabs even with empty titles, as some pages may legitimately have no title yet, or remove only the URL check if that's the critical validation. This could prevent users from reopening legitimately closed tabs.

Suggested change

if (url.isEmpty() || title.isEmpty())

if (url.isEmpty())

Copilot · 2025-12-09T06:53:21Z

+void KiwixApp::pushClosedTab(const QString& url, const QString& title) {
+    if (url.isEmpty() || title.isEmpty())
+        return;
+    m_closedTabs.push({url, title});


The m_closedTabs stack has no size limit, which could lead to unbounded memory growth if users close many tabs. Consider implementing a maximum size (e.g., 10-20 tabs) and removing the oldest entries when the limit is reached to prevent memory issues during long sessions.

Copilot · 2025-12-09T06:53:21Z

+    if (tabIndex == -1) {  // Clicked outside tabs
+        QMenu menu;
+        menu.addAction(KiwixApp::instance()->getAction(KiwixApp::ReopenClosedTabAction));
+        menu.exec(event->globalPos());


[nitpick] The context menu is only shown when clicking outside tabs (tabIndex == -1), but no context menu is shown when clicking on a tab. Consider adding a context menu for tabs themselves (when tabIndex >= 0) with options like "Close Tab", "Close Other Tabs", etc., which would provide better UX consistency with other tabbed applications.

Suggested change

menu.exec(event->globalPos());

menu.exec(event->globalPos());

} else { // Clicked on a tab

QMenu menu;

QAction* closeTabAction = menu.addAction(tr("Close Tab"));

QAction* closeOtherTabsAction = menu.addAction(tr("Close Other Tabs"));

QAction* closeTabsToRightAction = menu.addAction(tr("Close Tabs to the Right"));

QAction* selectedAction = menu.exec(event->globalPos());

if (selectedAction == closeTabAction) {

// Close the clicked tab

emit tabCloseRequested(tabIndex);

} else if (selectedAction == closeOtherTabsAction) {

// Close all tabs except the clicked one

for (int i = count() - 1; i >= 0; --i) {

if (i != tabIndex) {

emit tabCloseRequested(i);

}

}

} else if (selectedAction == closeTabsToRightAction) {

// Close all tabs to the right of the clicked one

for (int i = count() - 1; i > tabIndex; --i) {

emit tabCloseRequested(i);

}

}

kelson42

@etude11 Thank you very much for your PR and sorry again for the very long time before review. To me, the basic feature, it works fine and as requested in the issue.
@veloman-yunkan Can you please do the code review?

veloman-yunkan

From what I get from reviewing the code

The tab is reopened without restoring its scroll state,
The reopened tab appears in the end of the tab list rather than at its old position.

Also Copilot has made quite reasonable comments.

Other than that (and an unused public method introduced by the code change) the PR looks good to me.

veloman-yunkan · 2025-12-11T13:13:40Z

+
+public:
+    void pushClosedTab(const QString& url, const QString& title);
+    bool hasClosedTabs() const { return !m_closedTabs.isEmpty(); }


Unused function

kelson42 · 2025-12-12T07:36:39Z

@etude11 Hi, do you plan to look at this PR again?

kelson42 · 2025-12-26T14:05:57Z

@etude11 Any feedback? Do you plan to improve this PR later?

etude11 force-pushed the fix-issue-1154 branch from ea4bacf to b5849ef Compare February 11, 2025 18:38

etude11 force-pushed the fix-issue-1154 branch from b5849ef to a4064f4 Compare March 15, 2025 17:45

kelson42 force-pushed the fix-issue-1154 branch from a4064f4 to dfcc19b Compare October 9, 2025 07:45

kelson42 requested review from kelson42 and veloman-yunkan October 9, 2025 07:46

kelson42 force-pushed the fix-issue-1154 branch from dfcc19b to 9f32e23 Compare November 7, 2025 19:24

feat: (kiwix#1154) Undo Close tab

5b00ea8

kelson42 force-pushed the fix-issue-1154 branch from 9f32e23 to 5b00ea8 Compare December 9, 2025 06:48

Copilot AI review requested due to automatic review settings December 9, 2025 06:48

Copilot started reviewing on behalf of kelson42 December 9, 2025 06:48 View session

kelson42 changed the title ~~feat: (#1154) undo close tab~~ Add "Undo close tab" feature Dec 9, 2025

Copilot AI reviewed Dec 9, 2025

View reviewed changes

kelson42 approved these changes Dec 10, 2025

View reviewed changes

veloman-yunkan reviewed Dec 11, 2025

View reviewed changes

	// First and last tabs cannot be closed
	// First and last tabs (library tab and + button) cannot be closed

-        menu.exec(event->globalPos());
+        menu.exec(event->globalPos());
+    } else { // Clicked on a tab
+        QMenu menu;
+        QAction* closeTabAction = menu.addAction(tr("Close Tab"));
+        QAction* closeOtherTabsAction = menu.addAction(tr("Close Other Tabs"));
+        QAction* closeTabsToRightAction = menu.addAction(tr("Close Tabs to the Right"));
+        QAction* selectedAction = menu.exec(event->globalPos());
+        if (selectedAction == closeTabAction) {
+            // Close the clicked tab
+            emit tabCloseRequested(tabIndex);
+        } else if (selectedAction == closeOtherTabsAction) {
+            // Close all tabs except the clicked one
+            for (int i = count() - 1; i >= 0; --i) {
+                if (i != tabIndex) {
+                    emit tabCloseRequested(i);
+                }
+            }
+        } else if (selectedAction == closeTabsToRightAction) {
+            // Close all tabs to the right of the clicked one
+            for (int i = count() - 1; i > tabIndex; --i) {
+                emit tabCloseRequested(i);
+            }
+        }

Uh oh!

Conversation

etude11 commented Feb 11, 2025 • edited by kelson42 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

etude11 commented Mar 15, 2025

Uh oh!

kelson42 commented Oct 9, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

kelson42 left a comment

Choose a reason for hiding this comment

Uh oh!

veloman-yunkan left a comment

Choose a reason for hiding this comment

Uh oh!

veloman-yunkan Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

kelson42 commented Dec 12, 2025

Uh oh!

kelson42 commented Dec 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

etude11 commented Feb 11, 2025 •

edited by kelson42

Loading