Skip to content

Add "Undo close tab" feature #1342

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
etude11 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add "Undo close tab" feature #1342

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion src/kiwixapp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,7 +420,9 @@ void KiwixApp::createActions()
CREATE_ACTION_SHORTCUTS(CloseCurrentTabAction, gt("close-tab"), QList<QKeySequence>({QKeySequence(Qt::CTRL | Qt::Key_F4), QKeySequence(Qt::CTRL | Qt::Key_W)}));

CREATE_ACTION_SHORTCUT(ReopenClosedTabAction, gt("reopen-closed-tab"), QKeySequence(Qt::CTRL | Qt::SHIFT | Qt::Key_T));
HIDE_ACTION(ReopenClosedTabAction);
mpa_actions[ReopenClosedTabAction]->setEnabled(false);
connect(mpa_actions[ReopenClosedTabAction], &QAction::triggered,
this, &KiwixApp::reopenLastClosedTab);

CREATE_ACTION_SHORTCUT(BrowseLibraryAction, gt("browse-library"), QKeySequence(Qt::CTRL | Qt::Key_E));
HIDE_ACTION(BrowseLibraryAction);
Expand Down Expand Up @@ -623,3 +625,20 @@ QString KiwixApp::getPrevSaveDir() const
QDir dir(prevSaveDir);
return dir.exists() ? prevSaveDir : DEFAULT_SAVE_DIR;
}

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

Copilot AI Dec 9, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
return;
m_closedTabs.push({url, title});

Copilot AI Dec 9, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
mpa_actions[ReopenClosedTabAction]->setEnabled(true);
}

void KiwixApp::reopenLastClosedTab() {
if (m_closedTabs.isEmpty())
return;

auto tab = m_closedTabs.pop();
openUrl(tab.url, true);
if (m_closedTabs.isEmpty())
mpa_actions[ReopenClosedTabAction]->setEnabled(false);
}
12 changes: 12 additions & 0 deletions src/kiwixapp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

#include <mutex>
#include <iostream>
#include <QStack>

typedef TabBar::TabType TabType;

Expand Down Expand Up @@ -144,6 +145,17 @@ public slots:

QString findLibraryDirectory();
void loadAndInstallTranslations(QTranslator& translator, const QString& filename, const QString& directory);

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(); }

@veloman-yunkan veloman-yunkan Dec 11, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused function

void reopenLastClosedTab();
Comment on lines +149 to +158

Copilot AI Dec 9, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
};

QString getFileContent(QString filePath);
Expand Down
24 changes: 22 additions & 2 deletions src/tabbar.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,11 +310,21 @@ QStringList TabBar::getTabZimIds() const

void TabBar::closeTab(int index)
{
// 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

Copilot AI Dec 9, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 uses AI. Check for mistakes.
if (index <= 0 || index >= this->realTabCount())
return;

// Save tab info before closing
if (ZimView* zv = qobject_cast<ZimView*>(mp_stackedWidget->widget(index))) {
auto webView = zv->getWebView();
if (webView) {
KiwixApp::instance()->pushClosedTab(
webView->url().toString(),
webView->title()
);
}
}

if ( index == currentIndex() ) {
setCurrentIndex(index + 1 == realTabCount() ? index - 1 : index + 1);
}
Expand Down Expand Up @@ -549,3 +559,13 @@ void TabBar::onTabMoved(int from, int to)

KiwixApp::instance()->saveListOfOpenTabs();
}

void TabBar::contextMenuEvent(QContextMenuEvent *event)
{
int tabIndex = tabAt(event->pos());
if (tabIndex == -1) { // Clicked outside tabs
QMenu menu;

Copilot AI Dec 9, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 uses AI. Check for mistakes.
menu.addAction(KiwixApp::instance()->getAction(KiwixApp::ReopenClosedTabAction));
menu.exec(event->globalPos());

Copilot AI Dec 9, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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);
}
}

Copilot uses AI. Check for mistakes.
}
}
1 change: 1 addition & 0 deletions src/tabbar.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ class TabBar : public QTabBar
void tabRemoved(int index) override;
void tabInserted(int index) override;
void resizeEvent(QResizeEvent *) override;
void contextMenuEvent(QContextMenuEvent *event) override;

signals:
void webActionEnabledChanged(QWebEnginePage::WebAction action, bool enabled);
Expand Down