D10960: Ask for confirmation when Closing Dolphin windows with a terminal panel running a program

[Nathaniel Graham]

ngraham added a comment.


  Getting there! Few more comments:

INLINE COMMENTS

> dolphinmainwindow.cpp:422
> +    if (m_terminalPanel->isAnyProgramRunning() && GeneralSettings::confirmClosingTerminalRunningProgram() && closedByUser) {
> +        // Ask the user if he really wants to quit and close terminal panel running a program.
> +        // Open a confirmation dialog with 3 buttons:

In general we try to avoid gendered language where possible, even in a code comment. How about:

"Ask if the user really wants to..."

> dolphinmainwindow.cpp:432
> +        KGuiItem::assign(buttons->button(QDialogButtonBox::Yes), KStandardGuiItem::quit());
> +        KGuiItem::assign(buttons->button(QDialogButtonBox::No), KGuiItem(i18n("Show &Terminal Panel"), QIcon::fromTheme(QStringLiteral("utilities-terminal"))));
> +        KGuiItem::assign(buttons->button(QDialogButtonBox::Cancel), KStandardGuiItem::cancel());

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.

> dolphinmainwindow.cpp:434
> +        KGuiItem::assign(buttons->button(QDialogButtonBox::Cancel), KStandardGuiItem::cancel());
> +        buttons->button(QDialogButtonBox::Yes)->setDefault(true);
> +

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.

> dolphinmainwindow.cpp:442
> +                QMessageBox::Warning,
> +                i18n("A program '%1' is still running in the Terminal panel. Are you sure you want to quit?", m_terminalPanel->runningProgramName()),
> +                QStringList(),

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

REPOSITORY
  R318 Dolphin

REVISION DETAIL
  https://phabricator.kde.org/D10960

To: rominf, #dolphin, ngraham, markg
Cc: markg, ngraham, rkflx, broulik, #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20180303/a602c816/attachment.htm>