Changeset View
Standalone View
src/dolphinmainwindow.cpp
| Show First 20 Lines • Show All 63 Lines • ▼ Show 20 Line(s) | |||||
| 64 | #include <KToolInvocation> | 64 | #include <KToolInvocation> | ||
| 65 | #include <KUrlComboBox> | 65 | #include <KUrlComboBox> | ||
| 66 | #include <KUrlNavigator> | 66 | #include <KUrlNavigator> | ||
| 67 | 67 | | |||
| 68 | #include <QApplication> | 68 | #include <QApplication> | ||
| 69 | #include <QClipboard> | 69 | #include <QClipboard> | ||
| 70 | #include <QCloseEvent> | 70 | #include <QCloseEvent> | ||
| 71 | #include <QDialog> | 71 | #include <QDialog> | ||
| 72 | #include <QLineEdit> | 72 | #include <QLineEdit> | ||
elvisangelaccio: Not needed? (or at least, I can build without it) | |||||
| 73 | #include <QMenu> | 73 | #include <QMenu> | ||
| 74 | #include <QMenuBar> | 74 | #include <QMenuBar> | ||
| 75 | #include <QPushButton> | 75 | #include <QPushButton> | ||
elvisangelaccio: Not needed? (or at least, I can build without it) | |||||
| 76 | #include <QShowEvent> | 76 | #include <QShowEvent> | ||
| 77 | #include <QStandardPaths> | 77 | #include <QStandardPaths> | ||
| 78 | #include <QTimer> | 78 | #include <QTimer> | ||
| 79 | #include <QToolButton> | 79 | #include <QToolButton> | ||
| 80 | #include <kdualaction.h> | 80 | #include <kdualaction.h> | ||
| 81 | 81 | | |||
| 82 | namespace { | 82 | namespace { | ||
| 83 | // Used for GeneralSettings::version() to determine whether | 83 | // Used for GeneralSettings::version() to determine whether | ||
| ▲ Show 20 Lines • Show All 297 Lines • ▼ Show 20 Line(s) | 376 | if (m_tabWidget->count() > 1 && GeneralSettings::confirmClosingMultipleTabs() && closedByUser) { | |||
| 381 | // QDialogButtonBox::Cancel -> do nothing | 381 | // QDialogButtonBox::Cancel -> do nothing | ||
| 382 | QDialog *dialog = new QDialog(this, Qt::Dialog); | 382 | QDialog *dialog = new QDialog(this, Qt::Dialog); | ||
| 383 | dialog->setWindowTitle(i18nc("@title:window", "Confirmation")); | 383 | dialog->setWindowTitle(i18nc("@title:window", "Confirmation")); | ||
| 384 | dialog->setModal(true); | 384 | dialog->setModal(true); | ||
| 385 | QDialogButtonBox* buttons = new QDialogButtonBox(QDialogButtonBox::Yes | QDialogButtonBox::No | QDialogButtonBox::Cancel); | 385 | QDialogButtonBox* buttons = new QDialogButtonBox(QDialogButtonBox::Yes | QDialogButtonBox::No | QDialogButtonBox::Cancel); | ||
| 386 | KGuiItem::assign(buttons->button(QDialogButtonBox::Yes), KStandardGuiItem::quit()); | 386 | KGuiItem::assign(buttons->button(QDialogButtonBox::Yes), KStandardGuiItem::quit()); | ||
| 387 | KGuiItem::assign(buttons->button(QDialogButtonBox::No), KGuiItem(i18n("C&lose Current Tab"), QIcon::fromTheme(QStringLiteral("tab-close")))); | 387 | KGuiItem::assign(buttons->button(QDialogButtonBox::No), KGuiItem(i18n("C&lose Current Tab"), QIcon::fromTheme(QStringLiteral("tab-close")))); | ||
| 388 | KGuiItem::assign(buttons->button(QDialogButtonBox::Cancel), KStandardGuiItem::cancel()); | 388 | KGuiItem::assign(buttons->button(QDialogButtonBox::Cancel), KStandardGuiItem::cancel()); | ||
| 389 | buttons->button(QDialogButtonBox::Yes)->setDefault(true); | 389 | buttons->button(QDialogButtonBox::Cancel)->setDefault(true); | ||
@ngraham rominf: @ngraham
How about this? Closing a window with all opened tabs is destructive in my opinion. My… | |||||
Yes, I think it's appropriate to have the cancel button be the default button for this dialog. ngraham: Yes, I think it's appropriate to have the cancel button be the default button for this dialog. | |||||
| 390 | 390 | | |||
| 391 | bool doNotAskAgainCheckboxResult = false; | 391 | bool doNotAskAgainCheckboxResult = false; | ||
| 392 | 392 | | |||
| 393 | const int result = KMessageBox::createKMessageBox(dialog, | 393 | const auto result = KMessageBox::createKMessageBox(dialog, | ||
| 394 | buttons, | 394 | buttons, | ||
| 395 | QMessageBox::Warning, | 395 | QMessageBox::Warning, | ||
| 396 | i18n("You have multiple tabs open in this window, are you sure you want to quit?"), | 396 | i18n("You have multiple tabs open in this window, are you sure you want to quit?"), | ||
| 397 | QStringList(), | 397 | QStringList(), | ||
| 398 | i18n("Do not ask again"), | 398 | i18n("Do not ask again"), | ||
| 399 | &doNotAskAgainCheckboxResult, | 399 | &doNotAskAgainCheckboxResult, | ||
| 400 | KMessageBox::Notify); | 400 | KMessageBox::Notify); | ||
Unrelated change? (At least in Okular there were huge discussions on the type of icon, and I think it's bad form if you just sneak in such changes in this patch…) rkflx: Unrelated change?
(At least in Okular there were huge discussions on the type of icon, and I… | |||||
I thought that making the separate patch because of one line of code is too much. Sorry. Do you agree with the change by itself? Should I do it in the separate patch? rominf: I thought that making the separate patch because of one line of code is too much. Sorry. Do you… | |||||
One-line patches are perfectly fine. In general, keep to one change per patch, and don't try to sneak unrelated changes into patches. ngraham: One-line patches are perfectly fine. In general, keep to one change per patch, and don't try to… | |||||
OK. See D11293. rominf: OK. See D11293. | |||||
rkflx: > Do you agree with the change by itself?
That's for #dolphin to decide. Personally I think… | |||||
Usability-wise, Cancel is the appropriate default. The default button should never cause you to lose work or state. If tab state is considered unimportant, then we shouldn't have a "close multiple tabs" confirmation in the first place. But if we do have one, then the safe/non-destructive choice must be default. ngraham: Usability-wise, {nav Cancel} is the appropriate default. The default button should never cause… | |||||
What you say applies for regular dialogs. However, here we are in a confirmation dialog, where we already saved the user from a destructive action. Now we should present the sensible choice by default, which IMO is quitting. Or do you want another safety net? Anyway, do what you want and talk to Dolphin's maintainers, I don't have time to discuss this right now. Maybe we'll change it back later when this is unified across all apps. rkflx: What you say applies for regular dialogs. However, here we are in a //confirmation// dialog… | |||||
Okay, looking at the new Okular dialog, I'm pretty sure it should work like this:
IOW, window state is less important (but still worth a confirmation) than unsaved user files (which are worth a warning). rkflx: Okay, looking at the new Okular dialog, I'm pretty sure it should work like this:
- "Are you… | |||||
ngraham: Hmm, you make some good points. I'll have a think. | |||||
Cannot edit inline comments, therefore let me clarify:
For unsaved changes, the "Save" action should be default, of course. What I meant here was when you can only choose between losing all work and cancelling. rkflx: Cannot edit inline comments, therefore let me clarify:
> "Discard changes"-type dialog: Cancel… | |||||
+1 elvisangelaccio: > What you say applies for regular dialogs. However, here we are in a confirmation dialog… | |||||
ngraham: OK, you guys have convinced me! Good arguments. | |||||
| 401 | 401 | | |||
| 402 | if (doNotAskAgainCheckboxResult) { | 402 | if (doNotAskAgainCheckboxResult) { | ||
| 403 | GeneralSettings::setConfirmClosingMultipleTabs(false); | 403 | GeneralSettings::setConfirmClosingMultipleTabs(false); | ||
| 404 | } | 404 | } | ||
| 405 | 405 | | |||
| 406 | switch (result) { | 406 | switch (result) { | ||
| 407 | case QDialogButtonBox::Yes: | 407 | case QDialogButtonBox::Yes: | ||
| 408 | // Quit | 408 | // Quit | ||
| 409 | break; | 409 | break; | ||
| 410 | case QDialogButtonBox::No: | 410 | case QDialogButtonBox::No: | ||
| 411 | // Close only the current tab | 411 | // Close only the current tab | ||
| 412 | m_tabWidget->closeTab(); | 412 | m_tabWidget->closeTab(); | ||
| 413 | // Do not quit, ignore quit event | 413 | // Do not quit, ignore quit event | ||
| 414 | // fall through | 414 | // fall through | ||
| 415 | default: | 415 | default: | ||
| 416 | event->ignore(); | 416 | event->ignore(); | ||
| 417 | return; | 417 | return; | ||
| 418 | } | 418 | } | ||
| 419 | } | 419 | } | ||
| 420 | 420 | | |||
| 421 | if (m_terminalPanel->isAnyProgramRunning() && GeneralSettings::confirmClosingTerminalRunningProgram() && closedByUser) { | ||||
In general we try to avoid gendered language where possible, even in a code comment. How about: "Ask if the user really wants to..." ngraham: In general we try to avoid gendered language where possible, even in a code comment. How about… | |||||
| 422 | // Ask if the user really wants to quit Dolphin with a program that is still running in the Terminal panel | ||||
| 423 | // Open a confirmation dialog with 3 buttons: | ||||
| 424 | // QDialogButtonBox::Yes -> Quit | ||||
| 425 | // QDialogButtonBox::No -> Show Terminal Panel | ||||
| 426 | // QDialogButtonBox::Cancel -> do nothing | ||||
Can't you use any of KMessageBox's built-in dialogs rather than creating the entire QDialog yourself? broulik: Can't you use any of `KMessageBox`'s built-in dialogs rather than creating the entire `QDialog`… | |||||
I just copy pasted ConfirmClosingMultipleTabs code because that's my first written dialog in KDE and I don't know how the things are done. If you think that this should be changed, then we have to change ConfirmClosingMultipleTabs too. rominf: I just copy pasted `ConfirmClosingMultipleTabs` code because that's my first written dialog in… | |||||
See also T7022: Improve confirmation dialog when closing multiple tabs. TL;DR: Everybody does it differently, in the far future it would be nice to make it consistent across apps. Perhaps I'd keep Dolphin's style here for now, but that's for Dolphin to decide. rkflx: See also {T7022}.
TL;DR: Everybody does it differently, in the far future it would be nice to… | |||||
| 427 | QDialog *dialog = new QDialog(this, Qt::Dialog); | ||||
| 428 | dialog->setWindowTitle(i18nc("@title:window", "Confirmation")); | ||||
| 429 | dialog->setModal(true); | ||||
| 430 | auto standardButtons = QDialogButtonBox::Yes | QDialogButtonBox::Cancel; | ||||
| 431 | if (m_terminalPanel->isHidden()) { | ||||
The Show Terminal Panel button should probably only be displayed if the Terminal panel is hidden. Otherwise it seems odd that it's there, and when you click on it, nothing different appears to happen compared to just hitting the cancel button. ngraham: The {nav Show Terminal Panel} button should probably only be displayed if the Terminal panel is… | |||||
Terminal panel gets focus. I think it's reasonable to keep the button. Your opinion? rominf: > Otherwise it seems odd that it's there, and when you click on it, nothing different appears… | |||||
Regardless, some kind of change is needed, because the button says "Show Terminal Panel", not "Focus Terminal Panel". A button should always do what it says it'll do, or else users learn not to truth the software. If we want to do make it simply focus the Terminal panel when the Terminal panel is open, then the text should change accordingly. ngraham: Regardless, some kind of change is needed, because the button says "Show Terminal Panel", not… | |||||
| 432 | standardButtons |= QDialogButtonBox::No; | ||||
| 433 | } | ||||
A dialog box's default button should never invoke a destructive action. In this context, killing the process is akin to quitting with unsaved changes and should be considered destructive. The button's red icon also provides a hint. :) The Cancel button should be the default button here. ngraham: A dialog box's default button should //never// invoke a destructive action. In this context… | |||||
| 434 | QDialogButtonBox *buttons = new QDialogButtonBox(standardButtons); | ||||
| 435 | KGuiItem::assign(buttons->button(QDialogButtonBox::Yes), KStandardGuiItem::quit()); | ||||
| 436 | if (m_terminalPanel->isHidden()) { | ||||
Why another if (!m_terminalPanel->isVisible()) block? Can't we merge this with the one above? elvisangelaccio: Why another `if (!m_terminalPanel->isVisible())` block? Can't we merge this with the one above? | |||||
We actually can't--at least not cleanly. The top block modifies standardButtons, which is consumed later, the product of which (buttons) is needed in the second block. ngraham: We actually can't--at least not cleanly. The top block modifies `standardButtons`, which is… | |||||
| 437 | KGuiItem::assign( | ||||
Please use const auto result, since this function returns a QDialogButtonBox::StandardButton. elvisangelaccio: Please use `const auto result`, since this function returns a `QDialogButtonBox… | |||||
| 438 | buttons->button(QDialogButtonBox::No), | ||||
| 439 | KGuiItem(i18n("Show &Terminal Panel"), QIcon::fromTheme(QStringLiteral("utilities-terminal")))); | ||||
Let's phrase this text in a more standard, grammatically correct manner: "A program is still running in the Terminal panel. Are you sure you want to quit?" Bonus points if we can get the actual running command and display it in the text here somehow. Also +1 for @rkflx's suggestion for a button that will show the terminal panel if it's hidden. ngraham: Let's phrase this text in a more standard, grammatically correct manner:
"A program is still… | |||||
Done your task for bonus points and @rkflx's suggestion. rominf: Done your task for bonus points and @rkflx's suggestion. | |||||
| 440 | } | ||||
| 441 | KGuiItem::assign(buttons->button(QDialogButtonBox::Cancel), KStandardGuiItem::cancel()); | ||||
This is the name of the key that will be stored in settings to remember this. The text in the user interface is always "Do not ask again" broulik: This is the name of the key that will be stored in settings to remember this. The text in the… | |||||
With the actual program name in the string (thumbs up for that), we need a small change to make it grammatically correct: A program -> The program ngraham: With the actual program name in the string (thumbs up for that), we need a small change to make… | |||||
| 442 | buttons->button(QDialogButtonBox::Cancel)->setDefault(true); | ||||
But it is a default. Why isn't this working? rominf: > but the Cancel button still isn't the default
But it is a default. Why isn't this working? | |||||
Does it work for you? If not, you need to test the changes you're making. As for why, I don't know; it's your patch. :) One of the fun parts of producing a patch is that you often go down the rabbit hole of finding other bugs in the code you're touching! ngraham: Does it work for you? If not, you need to test the changes you're making. As for why, I don't… | |||||
| 443 | | ||||
No word puzzles, please. Let's add the program within the i18n function call using a format string, rather than string concatenation. ngraham: No word puzzles, please. Let's add the program within the `i18n` function call using a format… | |||||
rominf: Sorry, I didn't know that `i18n` works that way. Fixed it. | |||||
| 444 | bool doNotAskAgainCheckboxResult = false; | ||||
| 445 | | ||||
| 446 | const auto result = KMessageBox::createKMessageBox( | ||||
| 447 | dialog, | ||||
| 448 | buttons, | ||||
| 449 | QMessageBox::Warning, | ||||
| 450 | i18n("The program '%1' is still running in the Terminal panel. Are you sure you want to quit?", m_terminalPanel->runningProgramName()), | ||||
| 451 | QStringList(), | ||||
| 452 | i18n("Do not ask again"), | ||||
| 453 | &doNotAskAgainCheckboxResult, | ||||
| 454 | KMessageBox::Notify); | ||||
| 455 | | ||||
| 456 | if (doNotAskAgainCheckboxResult) { | ||||
| 457 | GeneralSettings::setConfirmClosingTerminalRunningProgram(false); | ||||
| 458 | } | ||||
| 459 | | ||||
elvisangelaccio: Missing braces | |||||
| 460 | switch (result) { | ||||
| 461 | case QDialogButtonBox::Yes: | ||||
| 462 | // Quit | ||||
| 463 | break; | ||||
| 464 | case QDialogButtonBox::No: | ||||
| 465 | actionCollection()->action("show_terminal_panel")->trigger(); | ||||
This switch now exists twice in the same function. There must be a smarter way of doing that, right? markg: This switch now exists twice in the same function. There must be a smarter way of doing that… | |||||
That's 2 different switches. Look at case QDialogButtonBox::No: branch. I'm sure it's possible to merge them, but that would hurt readability. rominf: That's 2 different switches. Look at `case QDialogButtonBox::No:` branch. I'm sure it's… | |||||
Right, but this also hurts ;) markg: Right, but this also hurts ;)
Oke, it's a tradeoff i suppose. Readability over duplication. | |||||
| 466 | // Do not quit, ignore quit event | ||||
elvisangelaccio: Please use `Q_FALLTHROUGH` instead. | |||||
| 467 | // fall through | ||||
| 468 | default: | ||||
| 469 | event->ignore(); | ||||
| 470 | return; | ||||
| 471 | } | ||||
| 472 | } | ||||
| 473 | | ||||
| 421 | GeneralSettings::setVersion(CurrentDolphinVersion); | 474 | GeneralSettings::setVersion(CurrentDolphinVersion); | ||
| 422 | GeneralSettings::self()->save(); | 475 | GeneralSettings::self()->save(); | ||
| 423 | 476 | | |||
| 424 | KXmlGuiWindow::closeEvent(event); | 477 | KXmlGuiWindow::closeEvent(event); | ||
| 425 | } | 478 | } | ||
| 426 | 479 | | |||
| 427 | void DolphinMainWindow::saveProperties(KConfigGroup& group) | 480 | void DolphinMainWindow::saveProperties(KConfigGroup& group) | ||
| 428 | { | 481 | { | ||
| ▲ Show 20 Lines • Show All 1175 Lines • Show Last 20 Lines | |||||
Not needed? (or at least, I can build without it)