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

Authored by ngraham on Mar 2 2018, 7:08 AM.

Details

Summary

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

FEATURE: 304816
FIXED-IN: 19.04.0

Test Plan
  1. Open terminal panel
  2. Run watch ls
  3. Close Dolphin
  4. Observe confirmation
  5. Disable confirmation
  6. Repeat, observe no confirmation
  7. Enable confirmation in the settings
  8. Repeat, observe a confirmation

Diff Detail

Repository
R318 Dolphin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
rominf updated this revision to Diff 28455.Mar 3 2018, 6:32 AM
  • Fix crash on Dolphin closing
rkflx added a comment.Mar 3 2018, 7:38 AM
  • Fix crash on Dolphin closing

πŸ‘

src/panels/terminal/terminalpanel.cpp
112–115

Add {} like it's done in most places in the codebase.

rominf updated this revision to Diff 28459.Mar 3 2018, 8:18 AM

Rebase on master

rominf marked an inline comment as done.Mar 3 2018, 8:18 AM
rkflx added inline comments.Mar 3 2018, 8:40 AM
src/panels/terminal/terminalpanel.cpp
112–115

Better, but indentation seem off.

rominf updated this revision to Diff 28463.Mar 3 2018, 8:51 AM
  • Fix indentation
rominf marked an inline comment as done.Mar 3 2018, 8:54 AM
rominf added inline comments.Mar 3 2018, 11:30 AM
src/dolphinmainwindow.cpp
406

@ngraham
How about this? Closing a window with all opened tabs is destructive in my opinion. My vote is for Cancel.

elvisangelaccio requested changes to this revision.Mar 3 2018, 1:58 PM
elvisangelaccio added a subscriber: elvisangelaccio.
elvisangelaccio added inline comments.
src/dolphinmainwindow.cpp
454

Please use const auto result, since this function returns a QDialogButtonBox::StandardButton.

473–476

Missing braces

src/panels/terminal/terminalpanel.cpp
68

Unrelated change, should go in its own commit.

114

else is not really need after a return.

src/panels/terminal/terminalpanel.h
103

Unrelated whitespace change

This revision now requires changes to proceed.Mar 3 2018, 1:58 PM
rominf marked 4 inline comments as done.Mar 3 2018, 2:16 PM
ngraham added inline comments.Mar 3 2018, 2:21 PM
src/dolphinmainwindow.cpp
406

Yes, I think it's appropriate to have the cancel button be the default button for this dialog.

448

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.

markg resigned from this revision.Mar 3 2018, 2:26 PM
markg added inline comments.
src/dolphinmainwindow.cpp
468–482

Right, but this also hurts ;)
Oke, it's a tradeoff i suppose. Readability over duplication. Readability (in this case) wins imho so feel free to ignore my comment on this.

rominf updated this revision to Diff 28666.Mar 5 2018, 7:27 AM

Rebase on master

rominf updated this revision to Diff 28669.Mar 5 2018, 7:40 AM

Rebase on master

rominf updated this revision to Diff 28910.Mar 7 2018, 11:03 AM
  • Show "Show Terminal Panel" button only if terminal panel is hidden

Err, doesn't work:

rominf updated this revision to Diff 29142.Mar 10 2018, 10:12 AM
  • Fix show "Show Terminal Panel" button when terminal panel is hidden

Close! Now the dialog box has the right buttons depending on whether or not the Terminal panel is open, but the Cancel button still isn't the default.

rominf marked 3 inline comments as done.Mar 12 2018, 5:17 AM
rominf added inline comments.
src/dolphinmainwindow.cpp
458

but the Cancel button still isn't the default

But it is a default. Why isn't this working?

ngraham added inline comments.Mar 12 2018, 5:02 PM
src/dolphinmainwindow.cpp
458

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!

rominf updated this revision to Diff 29418.Mar 13 2018, 4:10 PM
  • Set Cancel active by default when closing Dolphin with multiple tabs or running

program in terminal panel

rkflx added inline comments.Mar 13 2018, 4:19 PM
src/dolphinmainwindow.cpp
417

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…)

rominf added inline comments.Mar 13 2018, 5:35 PM
src/dolphinmainwindow.cpp
417

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?

ngraham added inline comments.Mar 13 2018, 5:37 PM
src/dolphinmainwindow.cpp
417

One-line patches are perfectly fine. In general, keep to one change per patch, and don't try to sneak unrelated changes into patches.

rkflx added inline comments.Mar 13 2018, 5:49 PM
src/dolphinmainwindow.cpp
417

Do you agree with the change by itself?

That's for Dolphin to decide. Personally I think this should be a coordinated effort, see T7022 for a longer discussion. I did not think this through yet, but for the moment I'm not too fond of making Cancel default.

rominf updated this revision to Diff 29423.Mar 13 2018, 5:50 PM
  • Remove unrelated changes
src/dolphinmainwindow.cpp
417

OK. See D11293.

ngraham added inline comments.Mar 13 2018, 5:52 PM
src/dolphinmainwindow.cpp
417

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.

rkflx added inline comments.Mar 13 2018, 5:58 PM
src/dolphinmainwindow.cpp
417

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 added inline comments.Mar 13 2018, 6:21 PM
src/dolphinmainwindow.cpp
417

Okay, looking at the new Okular dialog, I'm pretty sure it should work like this:

  • "Are you sure"-type dialog: Action is default.
  • "Discard changes"-type dialog: Cancel is default.

IOW, window state is less important (but still worth a confirmation) than unsaved user files (which are worth a warning).

ngraham added inline comments.Mar 13 2018, 6:25 PM
src/dolphinmainwindow.cpp
417

Hmm, you make some good points. I'll have a think.

rkflx added inline comments.Mar 13 2018, 6:29 PM
src/dolphinmainwindow.cpp
417

Cannot edit inline comments, therefore let me clarify:

"Discard changes"-type dialog: Cancel is default.

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.

src/dolphinmainwindow.cpp
417

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.

+1

ngraham accepted this revision as: ngraham.Mar 14 2018, 2:33 AM

All right, this looks good to me now!

For the confirmation dialog added here, I'm unsure whether we should follow the advice of our inline conversation and make the Quit button be default. Depending on what's running in the Terminal panel, killing it could be quite destructive. But I won't block on that, and will accept whatever Dolphin maintainers decide.

src/dolphinmainwindow.cpp
417

OK, you guys have convinced me! Good arguments.

For the confirmation dialog added here, I'm unsure whether we should follow the advice of our inline conversation and make the Quit button be default. Depending on what's running in the Terminal panel, killing it could be quite destructive. But I won't block on that, and will accept whatever Dolphin maintainers decide.

I agree with @ngraham (I also for Cancel as a default). @elvisangelaccio what do you think?

rkflx added a comment.EditedMar 14 2018, 9:04 AM

For the confirmation dialog added here, I'm unsure whether we should follow the advice of our inline conversation and make the Quit button be default. Depending on what's running in the Terminal panel, killing it could be quite destructive. But I won't block on that, and will accept whatever Dolphin maintainers decide.

Huh? The inline conversation started with the unrelated change to the tab handling, where Cancel is not the right default. Applying what we learned there to the terminal panel handling (i.e. you could have started Kate, which would be killed without a chance to save), defaulting to Cancel in this Diff makes sense to me.

Edit: In case the panel is hidden, a good default would be the button to show the panel.

+1, cancel is fine. You guys were right.

Today's the feature and string freeze for 18.04. @elvisangelaccio, do you have any remaining objections?

rominf updated this revision to Diff 30244.Mar 22 2018, 6:48 PM
  • Revert setDefault to the Yes button when closing multiple tabs
elvisangelaccio requested changes to this revision.Mar 29 2018, 9:13 PM
elvisangelaccio added inline comments.
src/dolphinmainwindow.cpp
74

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

77

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

452

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

481–482

Please use Q_FALLTHROUGH instead.

src/panels/terminal/terminalpanel.h
59

How about calling it hasProgramRunning() ?

src/settings/general/confirmationssettingspage.cpp
68

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

69

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

This revision now requires changes to proceed.Mar 29 2018, 9:13 PM
Restricted Application added a subscriber: kfm-devel. Β· View Herald TranscriptMay 9 2018, 1:16 PM
ngraham commandeered this revision.Jan 13 2019, 1:16 AM
ngraham edited reviewers, added: rominf; removed: ngraham.
ngraham updated this revision to Diff 49368.Jan 13 2019, 1:30 AM

Commandeer revision so I can finish it up, rebase on master, and fix merge conflicts

ngraham updated this revision to Diff 49371.Jan 13 2019, 2:27 AM
ngraham marked 27 inline comments as done.

Address all remaining review comments

Didnt you rebase it?

ngraham updated this revision to Diff 49372.Jan 13 2019, 2:30 AM

For some reason Phabricator created a second commit for this patch and now the diff shown is only the contents of the second one; replace it with the full diff that spans both commits

src/dolphinmainwindow.cpp
452

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 edited the summary of this revision. (Show Details)Jan 13 2019, 2:38 AM

Friendly ping.

shubham removed a subscriber: shubham.Jan 19 2019, 4:39 AM
This revision is now accepted and ready to land.Jan 19 2019, 11:35 AM
This revision was automatically updated to reflect the committed changes.