Differential D19311 Diff 66444 src/dolphinmainwindow.cpp

Changeset View

Standalone View

View Options

src/dolphinmainwindow.cpp

Show First 20 Lines • Show All 59 Lines • ▼ Show 20 Line(s)
60#include <KProtocolInfo> 60​#include <KProtocolInfo>
61#include <KProtocolManager> 61​#include <KProtocolManager>
62#include <KRun> 62​#include <KRun>
63#include <KShell> 63​#include <KShell>
64#include <KStandardAction> 64​#include <KStandardAction>
65#include <KStartupInfo> 65​#include <KStartupInfo>
66#include <KToggleAction> 66​#include <KToggleAction>
67#include <KToolBar> 67​#include <KToolBar>
68​#include <KToolBarPopupAction>
68#include <KToolInvocation> 69​#include <KToolInvocation>
69#include <KUrlComboBox> 70​#include <KUrlComboBox>
70#include <KUrlNavigator> 71​#include <KUrlNavigator>
71#include <KWindowSystem> 72​#include <KWindowSystem>
72 73​
73#include <QApplication> 74​#include <QApplication>
74#include <QClipboard> 75​#include <QClipboard>
75#include <QCloseEvent> 76​#include <QCloseEvent>
Show All 9 Lines
85#include <QTimer> 86​#include <QTimer>
86#include <QToolButton> 87​#include <QToolButton>
87#include <QWhatsThisClickedEvent> 88​#include <QWhatsThisClickedEvent>
88 89​
89namespace { 90​namespace {
90 // Used for GeneralSettings::version() to determine whether 91​ // Used for GeneralSettings::version() to determine whether
91 // an updated version of Dolphin is running. 92​ // an updated version of Dolphin is running.
92 const int CurrentDolphinVersion = 200; 93​ const int CurrentDolphinVersion = 200;
94​ // The maximum number of entries in the back/forward popup menu
95​ const int MaxNumberOfNavigationentries = 12;
hallasAuthorUnsubmitted
Not Done

Should this be user configurable? Is 10 a good number?

hallas: Should this be user configurable? Is 10 a good number?
ngrahamUnsubmitted
Not Done

Why does there have to be a maximum? If the user's done a lot of navigation, and they actually use the menu, they'll want to see everything.

ngraham: Why does there have to be a maximum? If the user's done a lot of navigation, and they actually…
96​ // Converts a QUrl to a QString suitable for showing in the back/forward popup menu
97​ QString GetPopupMenuLabel(const QUrl& url)
98​ {
99​ if (url.isLocalFile()) {
100​ return url.adjusted(QUrl::RemoveScheme).toString();
101​ }
102​ return url.toString();
103​ }
elvisangelaccioUnsubmitted
Done

The problem is that the menu can become huge and fill the whole screen:

Another problem from this screenshot: if an URL is already in the history, don't add it again.

elvisangelaccio: The problem is that the menu can become huge and fill the whole screen: {F7350920} Another…
hallasAuthorUnsubmitted
Done

Thanks for the review feedback :)

In the original patch I had a limit on the number of entries in the menu, but this was removed on request from review feedback, but would that be a way to solve it in combination with not adding duplicates? I just checked with my browser (Chrome) and that actually allows duplicates in the history, but I haven't tested how many entries it limits. @elvisangelaccio @ngraham what are your thoughts on this?

hallas: Thanks for the review feedback :) In the original patch I had a limit on the number of entries…
hallasAuthorUnsubmitted
Done

Actually, it seems like Chrome limits the navigation history to 12 entries, @ngraham would such a limit be an acceptable solution?

hallas: Actually, it seems like Chrome limits the navigation history to 12 entries, @ngraham would…
ngrahamUnsubmitted
Done

I'd be okay with that, sure.

ngraham: I'd be okay with that, sure.
elvisangelaccioUnsubmitted
Done

This whole function can be replaced with urlNavigator->locationUrl(i).toString(QUrl::PreferLocalFile) ;)

elvisangelaccio: This whole function can be replaced with `urlNavigator->locationUrl(i).toString(QUrl…
93} 104​}
94 105​
95DolphinMainWindow::DolphinMainWindow() : 106​DolphinMainWindow::DolphinMainWindow() :
96 KXmlGuiWindow(nullptr, Qt::WindowContextHelpButtonHint), 107​ KXmlGuiWindow(nullptr, Qt::WindowContextHelpButtonHint),
97 m_newFileMenu(nullptr), 108​ m_newFileMenu(nullptr),
98 m_helpMenu(nullptr), 109​ m_helpMenu(nullptr),
99 m_tabWidget(nullptr), 110​ m_tabWidget(nullptr),
100 m_activeViewContainer(nullptr), 111​ m_activeViewContainer(nullptr),
101 m_actionHandler(nullptr), 112​ m_actionHandler(nullptr),
102 m_remoteEncoding(nullptr), 113​ m_remoteEncoding(nullptr),
103 m_settingsDialog(), 114​ m_settingsDialog(),
104 m_bookmarkHandler(nullptr), 115​ m_bookmarkHandler(nullptr),
105 m_controlButton(nullptr), 116​ m_controlButton(nullptr),
106 m_updateToolBarTimer(nullptr), 117​ m_updateToolBarTimer(nullptr),
107 m_lastHandleUrlStatJob(nullptr), 118​ m_lastHandleUrlStatJob(nullptr),
108 m_terminalPanel(nullptr), 119​ m_terminalPanel(nullptr),
109 m_placesPanel(nullptr), 120​ m_placesPanel(nullptr),
110 m_tearDownFromPlacesRequested(false) 121​ m_tearDownFromPlacesRequested(false),
122​ m_backAction(nullptr),
123​ m_forwardAction(nullptr)
111{ 124​{
112 Q_INIT_RESOURCE(dolphin); 125​ Q_INIT_RESOURCE(dolphin);
113 setComponentName(QStringLiteral("dolphin"), QGuiApplication::applicationDisplayName()); 126​ setComponentName(QStringLiteral("dolphin"), QGuiApplication::applicationDisplayName());
114 setObjectName(QStringLiteral("Dolphin#")); 127​ setObjectName(QStringLiteral("Dolphin#"));
115 128​
116 connect(&DolphinNewFileMenuObserver::instance(), &DolphinNewFileMenuObserver::errorMessage, 129​ connect(&DolphinNewFileMenuObserver::instance(), &DolphinNewFileMenuObserver::errorMessage,
117 this, &DolphinMainWindow::showErrorMessage); 130​ this, &DolphinMainWindow::showErrorMessage);
118 131​
▲ Show 20 Lines • Show All 552 Lines • ▼ Show 20 Line(s)683 } else if (action == actionCollection()->action(KStandardAction::name(KStandardAction::Forward))) {
671 goForwardInNewTab(); 684​ goForwardInNewTab();
672 } else if (action == actionCollection()->action(QStringLiteral("go_up"))) { 685​ } else if (action == actionCollection()->action(QStringLiteral("go_up"))) {
673 goUpInNewTab(); 686​ goUpInNewTab();
674 } else if (action == actionCollection()->action(QStringLiteral("go_home"))) { 687​ } else if (action == actionCollection()->action(QStringLiteral("go_home"))) {
675 goHomeInNewTab(); 688​ goHomeInNewTab();
676 } 689​ }
677} 690​}
678 691​
692​void DolphinMainWindow::slotAboutToShowBackPopupMenu()
693​{
694​ KUrlNavigator* urlNavigator = m_activeViewContainer->urlNavigator();
elvisangelaccioUnsubmitted
Done

please call it urlNavigator

elvisangelaccio: please call it `urlNavigator`
695​ int entries = 0;
696​ m_backAction->menu()->clear();
697​ for (int i = urlNavigator->historyIndex() + 1; i < urlNavigator->historySize() && entries < MaxNumberOfNavigationentries; ++i, ++entries) {
698​ QAction* action = new QAction(GetPopupMenuLabel(urlNavigator->locationUrl(i)), m_backAction->menu());
hallasAuthorUnsubmitted
Not Done

What should we use as the action title? The full URL doesn't seem optimal :)

hallas: What should we use as the action title? The full URL doesn't seem optimal :)
699​ action->setData(i);
700​ m_backAction->menu()->addAction(action);
701​ }
702​}
703​
704​void DolphinMainWindow::slotGoBack(QAction* action)
705​{
706​ int gotoIndex = action->data().value<int>();
707​ KUrlNavigator* urlNavigator = m_activeViewContainer->urlNavigator();
elvisangelaccioUnsubmitted
Done

please call it urlNavigator

elvisangelaccio: please call it `urlNavigator`
708​ for (int numBack = gotoIndex - urlNavigator->historyIndex(); numBack; --numBack) {
elvisangelaccioUnsubmitted
Done

numBack > 0 would be more clear, probably? Btw I'd just call this variable i...

elvisangelaccio: `numBack > 0` would be more clear, probably? Btw I'd just call this variable `i`...
709​ goBack();
710​ }
711​}
712​
713​void DolphinMainWindow::slotBackForwardActionMiddleClicked(QAction* action)
714​{
715​ if (action) {
716​ KUrlNavigator* urlNavigator = activeViewContainer()->urlNavigator();
717​ openNewTabAfterCurrentTab(urlNavigator->locationUrl(action->data().value<int>()));
718​ }
719​}
hallasAuthorUnsubmitted
Not Done

What should we use as the action title? The full URL doesn't seem optimal :)

hallas: What should we use as the action title? The full URL doesn't seem optimal :)
720​
721​void DolphinMainWindow::slotAboutToShowForwardPopupMenu()
722​{
723​ KUrlNavigator* urlNavigator = m_activeViewContainer->urlNavigator();
elvisangelaccioUnsubmitted
Done

please call it urlNavigator

elvisangelaccio: please call it `urlNavigator`
724​ int entries = 0;
725​ m_forwardAction->menu()->clear();
726​ for (int i = urlNavigator->historyIndex() - 1; i >= 0 && entries < MaxNumberOfNavigationentries; --i, ++entries) {
727​ QAction* action = new QAction(GetPopupMenuLabel(urlNavigator->locationUrl(i)), m_forwardAction->menu());
728​ action->setData(i);
729​ m_forwardAction->menu()->addAction(action);
730​ }
731​}
732​
733​void DolphinMainWindow::slotGoForward(QAction* action)
734​{
735​ int gotoIndex = action->data().value<int>();
736​ KUrlNavigator* urlNavigator = m_activeViewContainer->urlNavigator();
737​ for (int numForward = urlNavigator->historyIndex() - gotoIndex; numForward; --numForward) {
elvisangelaccioUnsubmitted
Done

Same here.

elvisangelaccio: Same here.
738​ goForward();
739​ }
740​}
741​
679void DolphinMainWindow::selectAll() 742​void DolphinMainWindow::selectAll()
680{ 743​{
681 clearStatusBar(); 744​ clearStatusBar();
682 745​
683 // if the URL navigator is editable and focused, select the whole 746​ // if the URL navigator is editable and focused, select the whole
684 // URL instead of all items of the view 747​ // URL instead of all items of the view
685 748​
686 KUrlNavigator* urlNavigator = m_activeViewContainer->urlNavigator(); 749​ KUrlNavigator* urlNavigator = m_activeViewContainer->urlNavigator();
▲ Show 20 Lines • Show All 621 Lines • ▼ Show 20 Line(s)1218{
1308 // So you might want to be more verbose in your language to convey the meaning but it's up to you. 1371​ // So you might want to be more verbose in your language to convey the meaning but it's up to you.
1309 replaceLocation->setWhatsThis(xi18nc("@info:whatsthis", 1372​ replaceLocation->setWhatsThis(xi18nc("@info:whatsthis",
1310 "This switches to editing the location and selects it " 1373​ "This switches to editing the location and selects it "
1311 "so you can quickly enter a different location.")); 1374​ "so you can quickly enter a different location."));
1312 actionCollection()->setDefaultShortcut(replaceLocation, Qt::CTRL + Qt::Key_L); 1375​ actionCollection()->setDefaultShortcut(replaceLocation, Qt::CTRL + Qt::Key_L);
1313 connect(replaceLocation, &QAction::triggered, this, &DolphinMainWindow::replaceLocation); 1376​ connect(replaceLocation, &QAction::triggered, this, &DolphinMainWindow::replaceLocation);
1314 1377​
1315 // setup 'Go' menu 1378​ // setup 'Go' menu
1316 QAction* backAction = KStandardAction::back(this, &DolphinMainWindow::goBack, actionCollection()); 1379​ {
1317 auto backShortcuts = backAction->shortcuts(); 1380​ QScopedPointer<QAction> backAction(KStandardAction::back(nullptr, nullptr, nullptr));
hallasAuthorUnsubmitted
Not Done

I copied some of this stuff from the implementation of KStandardAction::back but that function actually changes the icon on Right To Left systems, so should I do the same here?

hallas: I copied some of this stuff from the implementation of KStandardAction::back but that function…
1381​ m_backAction = new KToolBarPopupAction(backAction->icon(), backAction->text(), actionCollection());
1382​ m_backAction->setObjectName(backAction->objectName());
1383​ }
1384​ m_backAction->setDelayed(true);
1385​ m_backAction->setStickyMenu(false);
1386​ connect(m_backAction, &QAction::triggered, this, &DolphinMainWindow::goBack);
1387​ connect(m_backAction->menu(), &QMenu::aboutToShow, this, &DolphinMainWindow::slotAboutToShowBackPopupMenu);
1388​ connect(m_backAction->menu(), &QMenu::triggered, this, &DolphinMainWindow::slotGoBack);
1389​ actionCollection()->addAction(m_backAction->objectName(), m_backAction);
1390​
1391​ auto backShortcuts = m_backAction->shortcuts();
1318 backShortcuts.append(QKeySequence(Qt::Key_Backspace)); 1392​ backShortcuts.append(QKeySequence(Qt::Key_Backspace));
1319 actionCollection()->setDefaultShortcuts(backAction, backShortcuts); 1393​ actionCollection()->setDefaultShortcuts(m_backAction, backShortcuts);
1320 1394​
1321 DolphinRecentTabsMenu* recentTabsMenu = new DolphinRecentTabsMenu(this); 1395​ DolphinRecentTabsMenu* recentTabsMenu = new DolphinRecentTabsMenu(this);
1322 actionCollection()->addAction(QStringLiteral("closed_tabs"), recentTabsMenu); 1396​ actionCollection()->addAction(QStringLiteral("closed_tabs"), recentTabsMenu);
1323 connect(m_tabWidget, &DolphinTabWidget::rememberClosedTab, 1397​ connect(m_tabWidget, &DolphinTabWidget::rememberClosedTab,
1324 recentTabsMenu, &DolphinRecentTabsMenu::rememberClosedTab); 1398​ recentTabsMenu, &DolphinRecentTabsMenu::rememberClosedTab);
1325 connect(recentTabsMenu, &DolphinRecentTabsMenu::restoreClosedTab, 1399​ connect(recentTabsMenu, &DolphinRecentTabsMenu::restoreClosedTab,
1326 m_tabWidget, &DolphinTabWidget::restoreClosedTab); 1400​ m_tabWidget, &DolphinTabWidget::restoreClosedTab);
1327 connect(recentTabsMenu, &DolphinRecentTabsMenu::closedTabsCountChanged, 1401​ connect(recentTabsMenu, &DolphinRecentTabsMenu::closedTabsCountChanged,
Show All 12 Lines
1340 undoAction->setWhatsThis(xi18nc("@info:whatsthis", "This undoes " 1414​ undoAction->setWhatsThis(xi18nc("@info:whatsthis", "This undoes "
1341 "the last change you made to files or folders.<nl/>" 1415​ "the last change you made to files or folders.<nl/>"
1342 "Such changes include <interface>creating, renaming</interface> " 1416​ "Such changes include <interface>creating, renaming</interface> "
1343 "and <interface>moving</interface> them to a different location " 1417​ "and <interface>moving</interface> them to a different location "
1344 "or to the <filename>Trash</filename>. <nl/>Changes that can't " 1418​ "or to the <filename>Trash</filename>. <nl/>Changes that can't "
1345 "be undone will ask for your confirmation.")); 1419​ "be undone will ask for your confirmation."));
1346 undoAction->setEnabled(false); // undo should be disabled by default 1420​ undoAction->setEnabled(false); // undo should be disabled by default
1347 1421​
1348 KStandardAction::forward(this, &DolphinMainWindow::goForward, actionCollection()); 1422​ {
1423​ QScopedPointer<QAction> forwardAction(KStandardAction::forward(nullptr, nullptr, nullptr));
hallasAuthorUnsubmitted
Not Done

I copied some of this stuff from the implementation of KStandardAction::forward but that function actually changes the icon on Right To Left systems, so should I do the same here?

hallas: I copied some of this stuff from the implementation of KStandardAction::forward but that…
1424​ m_forwardAction = new KToolBarPopupAction(forwardAction->icon(), forwardAction->text(), actionCollection());
1425​ m_forwardAction->setObjectName(forwardAction->objectName());
1426​ }
1427​ m_forwardAction->setDelayed(true);
1428​ m_forwardAction->setStickyMenu(false);
1429​ connect(m_forwardAction, &QAction::triggered, this, &DolphinMainWindow::goForward);
1430​ connect(m_forwardAction->menu(), &QMenu::aboutToShow, this, &DolphinMainWindow::slotAboutToShowForwardPopupMenu);
1431​ connect(m_forwardAction->menu(), &QMenu::triggered, this, &DolphinMainWindow::slotGoForward);
1432​ actionCollection()->addAction(m_forwardAction->objectName(), m_forwardAction);
1433​ // enable middle-click to open in a new tab
1434​ auto *middleClickEventFilter = new MiddleClickActionEventFilter(this);
1435​ connect(middleClickEventFilter, &MiddleClickActionEventFilter::actionMiddleClicked, this, &DolphinMainWindow::slotBackForwardActionMiddleClicked);
1436​ m_backAction->menu()->installEventFilter(middleClickEventFilter);
1437​ m_forwardAction->menu()->installEventFilter(middleClickEventFilter);
1349 KStandardAction::up(this, &DolphinMainWindow::goUp, actionCollection()); 1438​ KStandardAction::up(this, &DolphinMainWindow::goUp, actionCollection());
1350 QAction* homeAction = KStandardAction::home(this, &DolphinMainWindow::goHome, actionCollection()); 1439​ QAction* homeAction = KStandardAction::home(this, &DolphinMainWindow::goHome, actionCollection());
1351 homeAction->setWhatsThis(xi18nc("@info:whatsthis", "Go to your " 1440​ homeAction->setWhatsThis(xi18nc("@info:whatsthis", "Go to your "
1352 "<filename>Home</filename> folder.<nl/>Every user account " 1441​ "<filename>Home</filename> folder.<nl/>Every user account "
1353 "has their own <filename>Home</filename> that contains their data " 1442​ "has their own <filename>Home</filename> that contains their data "
1354 "including folders that contain personal application data.")); 1443​ "including folders that contain personal application data."));
1355 1444​
1356 // setup 'Tools' menu 1445​ // setup 'Tools' menu
▲ Show 20 Lines • Show All 766 Lines • ▼ Show 20 Line(s)2211{
2123 DolphinMainWindow* mainWin= qobject_cast<DolphinMainWindow *>(parentWidget()); 2212​ DolphinMainWindow* mainWin= qobject_cast<DolphinMainWindow *>(parentWidget());
2124 if (mainWin) { 2213​ if (mainWin) {
2125 DolphinViewContainer* container = mainWin->activeViewContainer(); 2214​ DolphinViewContainer* container = mainWin->activeViewContainer();
2126 container->showMessage(job->errorString(), DolphinViewContainer::Error); 2215​ container->showMessage(job->errorString(), DolphinViewContainer::Error);
2127 } else { 2216​ } else {
2128 KIO::FileUndoManager::UiInterface::jobError(job); 2217​ KIO::FileUndoManager::UiInterface::jobError(job);
2129 } 2218​ }
2130} 2219​}
2131 2220​
2132bool DolphinMainWindow::isUrlOpen(const QString& url) 2221​bool DolphinMainWindow::isUrlOpen(const QString& url)
2133{ 2222​{
2134 return m_tabWidget->isUrlOpen(QUrl::fromUserInput((url))); 2223​ return m_tabWidget->isUrlOpen(QUrl::fromUserInput((url)));
2135} 2224​}
2136 2225​
elvisangelaccioUnsubmitted
Not Done

Unrelated whitespace change

elvisangelaccio: Unrelated whitespace change