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

[Elvis Angelaccio]

elvisangelaccio requested changes to this revision.
elvisangelaccio added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> dolphinmainwindow.cpp:75
>  #include <QFileInfo>
> +#include <QDialogButtonBox>
>  #include <QLineEdit>

Not needed? (or at least, I can build without it)

> dolphinmainwindow.cpp:79
>  #include <QMenuBar>
> +#include <QMessageBox>
>  #include <QPushButton>

Not needed? (or at least, I can build without it)

> dolphinmainwindow.cpp:456
> +        KGuiItem::assign(buttons->button(QDialogButtonBox::Yes), KStandardGuiItem::quit());
> +        if (!m_terminalPanel->isVisible()) {
> +            KGuiItem::assign(

Why another `if (!m_terminalPanel->isVisible())` block? Can't we merge this with the one above?

> dolphinmainwindow.cpp:485-486
> +                actionCollection()->action("show_terminal_panel")->trigger();
> +                // Do not quit, ignore quit event
> +                // fall through
> +            default:

Please use `Q_FALLTHROUGH` instead.

> terminalpanel.h:59
> +    bool isHiddenInVisibleWindow() const;
> +    bool isAnyProgramRunning() const;
> +    QString runningProgramName() const;

How about calling it `hasProgramRunning()` ?

> confirmationssettingspage.cpp:64
>                                                         "Closing Dolphin windows with multiple tabs"), this);
> +    m_confirmClosingTerminalRunningProgram = new QCheckBox(i18nc("@option:check Ask for confirmation when",
> +                                                       "Closing Dolphin windows with a program that is still running in the Terminal panel."), this);

I see a problem with this setting/checkbox: it will be visible also on Windows, even though the terminal panel is not available there.

> confirmationssettingspage.cpp:65
> +    m_confirmClosingTerminalRunningProgram = new QCheckBox(i18nc("@option:check Ask for confirmation when",
> +                                                       "Closing Dolphin windows with a program that is still running in the Terminal panel."), this);
> +

Alternative which is one word shorter: "Closing Dolphin windows while a program is still running in the Terminal panel."

REPOSITORY
  R318 Dolphin

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

To: rominf, #dolphin, ngraham, elvisangelaccio
Cc: elvisangelaccio, 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/20180329/e4b73aa2/attachment.htm>