Differential D10990 Diff 29067 src/dolphinmainwindow.cpp

Changeset View

Standalone View

View Options

src/dolphinmainwindow.cpp

Show First 20 Lines • Show All 45 Lines • ▼ Show 20 Line(s)
46#include <KActionCollection> 46​#include <KActionCollection>
47#include <KActionMenu> 47​#include <KActionMenu>
48#include <KAuthorized> 48​#include <KAuthorized>
49#include <KConfig> 49​#include <KConfig>
50#include <KFileItemListProperties> 50​#include <KFileItemListProperties>
51#include <KFilePlacesModel> 51​#include <KFilePlacesModel>
52#include <KHelpMenu> 52​#include <KHelpMenu>
53#include <KIO/JobUiDelegate> 53​#include <KIO/JobUiDelegate>
54​#include <KIO/OpenFileManagerWindowJob>
54#include <KJobWidgets> 55​#include <KJobWidgets>
55#include <KLocalizedString> 56​#include <KLocalizedString>
56#include <KMessageBox> 57​#include <KMessageBox>
57#include <KProtocolInfo> 58​#include <KProtocolInfo>
58#include <KProtocolManager> 59​#include <KProtocolManager>
59#include <KRun> 60​#include <KRun>
60#include <KShell> 61​#include <KShell>
61#include <KStandardAction> 62​#include <KStandardAction>
62#include <KToggleAction> 63​#include <KToggleAction>
63#include <KToolBar> 64​#include <KToolBar>
64#include <KToolInvocation> 65​#include <KToolInvocation>
65#include <KUrlComboBox> 66​#include <KUrlComboBox>
66#include <KUrlNavigator> 67​#include <KUrlNavigator>
67 68​
68#include <QApplication> 69​#include <QApplication>
69#include <QClipboard> 70​#include <QClipboard>
70#include <QCloseEvent> 71​#include <QCloseEvent>
71#include <QDialog> 72​#include <QDialog>
73​#include <QFileInfo>
72#include <QLineEdit> 74​#include <QLineEdit>
73#include <QMenu> 75​#include <QMenu>
74#include <QMenuBar> 76​#include <QMenuBar>
75#include <QPushButton> 77​#include <QPushButton>
76#include <QShowEvent> 78​#include <QShowEvent>
77#include <QStandardPaths> 79​#include <QStandardPaths>
78#include <QTimer> 80​#include <QTimer>
79#include <QToolButton> 81​#include <QToolButton>
▲ Show 20 Lines • Show All 282 Lines • ▼ Show 20 Line(s)361{
362 if (!event->spontaneous()) { 364​ if (!event->spontaneous()) {
363 m_activeViewContainer->view()->setFocus(); 365​ m_activeViewContainer->view()->setFocus();
364 } 366​ }
365} 367​}
366 368​
367void DolphinMainWindow::closeEvent(QCloseEvent* event) 369​void DolphinMainWindow::closeEvent(QCloseEvent* event)
368{ 370​{
369 // Find out if Dolphin is closed directly by the user or 371​ // Find out if Dolphin is closed directly by the user or
370 // by the session manager because the session is closed 372​ // by the session manager because the session is closed
elvisangelaccioUnsubmitted
Not Done

One last thing: this should be

i18nc("@info", "Could not access <filename>%1</filename>.", linkDestination);

See https://api.kde.org/frameworks/ki18n/html/prg_guide.html#kuit_markup

elvisangelaccio: One last thing: this should be ``` i18nc("@info", "Could not access <filename>%1</filename>."…
371 bool closedByUser = true; 373​ bool closedByUser = true;
372 if (qApp->isSavingSession()) { 374​ if (qApp->isSavingSession()) {
373 closedByUser = false; 375​ closedByUser = false;
374 } 376​ }
375 377​
376 if (m_tabWidget->count() > 1 && GeneralSettings::confirmClosingMultipleTabs() && closedByUser) { 378​ if (m_tabWidget->count() > 1 && GeneralSettings::confirmClosingMultipleTabs() && closedByUser) {
377 // Ask the user if he really wants to quit and close all tabs. 379​ // Ask the user if he really wants to quit and close all tabs.
378 // Open a confirmation dialog with 3 buttons: 380​ // Open a confirmation dialog with 3 buttons:
▲ Show 20 Lines • Show All 828 Lines • ▼ Show 20 Line(s)1187#endif
1207 QAction* activatePrevTab = actionCollection()->addAction(QStringLiteral("activate_prev_tab")); 1209​ QAction* activatePrevTab = actionCollection()->addAction(QStringLiteral("activate_prev_tab"));
1208 activatePrevTab->setIconText(i18nc("@action:inmenu", "Previous Tab")); 1210​ activatePrevTab->setIconText(i18nc("@action:inmenu", "Previous Tab"));
1209 activatePrevTab->setText(i18nc("@action:inmenu", "Activate Previous Tab")); 1211​ activatePrevTab->setText(i18nc("@action:inmenu", "Activate Previous Tab"));
1210 activatePrevTab->setEnabled(false); 1212​ activatePrevTab->setEnabled(false);
1211 connect(activatePrevTab, &QAction::triggered, m_tabWidget, &DolphinTabWidget::activatePrevTab); 1213​ connect(activatePrevTab, &QAction::triggered, m_tabWidget, &DolphinTabWidget::activatePrevTab);
1212 actionCollection()->setDefaultShortcuts(activatePrevTab, prevTabKeys); 1214​ actionCollection()->setDefaultShortcuts(activatePrevTab, prevTabKeys);
1213 1215​
1214 // for context menu 1216​ // for context menu
1217​ QAction* openSymlinkDestinationFolder = actionCollection()->addAction(QStringLiteral("open_symlink_destination_folder"));
1218​ openSymlinkDestinationFolder->setText(i18nc("@action:inmenu", "Show Original"));
elvisangelaccioUnsubmitted
Done

Why "Show Original"? What does original mean?

I'd have expected either "Show Destination" or "Show Target".

elvisangelaccio: Why "Show Original"? What does original mean? I'd have expected either "Show Destination" or…
rominfAuthorUnsubmitted
Done

I'm for "Show Target" ("symlink target" is 5x times more popular than "symlink destination" in Google).
BTW, the author of "Show Original" was @ngraham.

rominf: I'm for "Show Target" ("symlink target" is 5x times more popular than "symlink destination" in…
1219​ openSymlinkDestinationFolder->setIcon(QIcon::fromTheme(QStringLiteral("document-open-folder")));
rkflxUnsubmitted
Done

In Gwenview and Spectacle we recently used document-open-folder for the same feature. That icon basically looks the same, but does not "colourize" on HiDPI (and still has non-symbolic line-width).

(Hmh, we should really make this a standard action…)

rkflx: In Gwenview and Spectacle we recently used `document-open-folder` for the same feature. That…
1220​ connect(openSymlinkDestinationFolder, &QAction::triggered, [this]() {
1221​ const auto link = m_activeViewContainer->view()->selectedItems().at(0);
elvisangelaccioUnsubmitted
Done

Missing this as receiver parameter.

elvisangelaccio: Missing `this` as receiver parameter.
1222​ const auto linkLocationDir = QFileInfo(link.localPath()).absoluteDir();
1223​ auto linkDestination = link.linkDest();
1224​ if (QFileInfo(linkDestination).isRelative())
1225​ linkDestination = linkLocationDir.filePath(linkDestination);
1226​ if (QFileInfo(linkDestination).exists()) {
elvisangelaccioUnsubmitted
Done

Missing braces

elvisangelaccio: Missing braces
1227​ KIO::highlightInFileManager({QUrl::fromLocalFile(linkDestination).adjusted(QUrl::StripTrailingSlash)});
elvisangelaccioUnsubmitted
Done

Use the static QFileInfo::exists() which is faster.

elvisangelaccio: Use the static `QFileInfo::exists()` which is faster.
1228​ } else {
1229​ m_activeViewContainer->showMessage(
1230​ i18n("Could not access \"%1\".").arg(linkDestination), DolphinViewContainer::Warning);
1231​ }
1232​ });
1233​
elvisangelaccioUnsubmitted
Done

This should go in a dedicated function. Lambdas should not be bigger than 2/3 lines of code.

elvisangelaccio: This should go in a dedicated function. Lambdas should not be bigger than 2/3 lines of code.
1215 QAction* openInNewTab = actionCollection()->addAction(QStringLiteral("open_in_new_tab")); 1234​ QAction* openInNewTab = actionCollection()->addAction(QStringLiteral("open_in_new_tab"));
1216 openInNewTab->setText(i18nc("@action:inmenu", "Open in New Tab")); 1235​ openInNewTab->setText(i18nc("@action:inmenu", "Open in New Tab"));
1217 openInNewTab->setIcon(QIcon::fromTheme(QStringLiteral("tab-new"))); 1236​ openInNewTab->setIcon(QIcon::fromTheme(QStringLiteral("tab-new")));
1218 connect(openInNewTab, &QAction::triggered, this, &DolphinMainWindow::openInNewTab); 1237​ connect(openInNewTab, &QAction::triggered, this, &DolphinMainWindow::openInNewTab);
1219 1238​
1220 QAction* openInNewTabs = actionCollection()->addAction(QStringLiteral("open_in_new_tabs")); 1239​ QAction* openInNewTabs = actionCollection()->addAction(QStringLiteral("open_in_new_tabs"));
1221 openInNewTabs->setText(i18nc("@action:inmenu", "Open in New Tabs")); 1240​ openInNewTabs->setText(i18nc("@action:inmenu", "Open in New Tabs"));
1222 openInNewTabs->setIcon(QIcon::fromTheme(QStringLiteral("tab-new"))); 1241​ openInNewTabs->setIcon(QIcon::fromTheme(QStringLiteral("tab-new")));
▲ Show 20 Lines • Show All 147 Lines • ▼ Show 20 Line(s)1388 if (list.isEmpty()) {
1370 stateChanged(QStringLiteral("has_no_selection")); 1389​ stateChanged(QStringLiteral("has_no_selection"));
1371 } else { 1390​ } else {
1372 stateChanged(QStringLiteral("has_selection")); 1391​ stateChanged(QStringLiteral("has_selection"));
1373 1392​
1374 KActionCollection* col = actionCollection(); 1393​ KActionCollection* col = actionCollection();
1375 QAction* renameAction = col->action(KStandardAction::name(KStandardAction::RenameFile)); 1394​ QAction* renameAction = col->action(KStandardAction::name(KStandardAction::RenameFile));
1376 QAction* moveToTrashAction = col->action(KStandardAction::name(KStandardAction::MoveToTrash)); 1395​ QAction* moveToTrashAction = col->action(KStandardAction::name(KStandardAction::MoveToTrash));
1377 QAction* deleteAction = col->action(KStandardAction::name(KStandardAction::DeleteFile)); 1396​ QAction* deleteAction = col->action(KStandardAction::name(KStandardAction::DeleteFile));
1378 QAction* cutAction = col->action(KStandardAction::name(KStandardAction::Cut)); 1397​ QAction* cutAction = col->action(KStandardAction::name(KStandardAction::Cut));
1379 QAction* deleteWithTrashShortcut = col->action(QStringLiteral("delete_shortcut")); // see DolphinViewActionHandler 1398​ QAction* deleteWithTrashShortcut = col->action(QStringLiteral("delete_shortcut")); // see DolphinViewActionHandler
elvisangelaccioUnsubmitted
Done

Unrelated code style change. Please revert.

elvisangelaccio: Unrelated code style change. Please revert.
rominfAuthorUnsubmitted
Done

Note = after showOriginal. It's aligned to the = after deleteWithTrashShortcut. Other actions should be formatted the same way. It's previous commiter fault that he didn't format the things well.
What is the purpose to revert formatting if it's already in the repository?
OK, now I understand that I don't have to commit formatting changes alongside the main code. Should I do separate patch like "And now format the code I pushed before"?

rominf: Note `=` after `showOriginal`. It's aligned to the `=` after `deleteWithTrashShortcut`. Other…
elvisangelaccioUnsubmitted
Done

Fine, let's forget about this. But next time remember that commits should be atomic (= no unrelated stuff).

elvisangelaccio: Fine, let's forget about this. But next time remember that commits should be atomic (= no…
1380 1399​
1381 KFileItemListProperties capabilities(list); 1400​ KFileItemListProperties capabilities(list);
1382 const bool enableMoveToTrash = capabilities.isLocal() && capabilities.supportsMoving(); 1401​ const bool enableMoveToTrash = capabilities.isLocal() && capabilities.supportsMoving();
1383 1402​
1384 renameAction->setEnabled(capabilities.supportsMoving()); 1403​ renameAction->setEnabled(capabilities.supportsMoving());
1385 moveToTrashAction->setEnabled(enableMoveToTrash); 1404​ moveToTrashAction->setEnabled(enableMoveToTrash);
1386 deleteAction->setEnabled(capabilities.supportsDeleting()); 1405​ deleteAction->setEnabled(capabilities.supportsDeleting());
1387 deleteWithTrashShortcut->setEnabled(capabilities.supportsDeleting() && !enableMoveToTrash); 1406​ deleteWithTrashShortcut->setEnabled(capabilities.supportsDeleting() && !enableMoveToTrash);
▲ Show 20 Lines • Show All 214 Lines • Show Last 20 Lines