Ask for confirmation when Closing Dolphin windows with a terminal panel running a program.
FEATURE: 304816
FIXED-IN: 19.04.0
elvisangelaccio | |
rominf | |
markg |
Dolphin |
Ask for confirmation when Closing Dolphin windows with a terminal panel running a program.
FEATURE: 304816
FIXED-IN: 19.04.0
Automatic diff as part of commit; lint not applicable. |
Automatic diff as part of commit; unit tests not applicable. |
👍
src/panels/terminal/terminalpanel.cpp | ||
---|---|---|
112–115 | Add {} like it's done in most places in the codebase. |
src/panels/terminal/terminalpanel.cpp | ||
---|---|---|
112–115 | Better, but indentation seem off. |
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 |
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. |
src/dolphinmainwindow.cpp | ||
---|---|---|
468–482 | Right, but this also hurts ;) |
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.
src/dolphinmainwindow.cpp | ||
---|---|---|
458 |
But it is a default. Why isn't this working? |
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! |
program in terminal panel
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…) |
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? |
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. |
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. |
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. |
src/dolphinmainwindow.cpp | ||
---|---|---|
417 | 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). |
src/dolphinmainwindow.cpp | ||
---|---|---|
417 | Hmm, you make some good points. I'll have a think. |
src/dolphinmainwindow.cpp | ||
---|---|---|
417 | 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. |
src/dolphinmainwindow.cpp | ||
---|---|---|
417 |
+1 |
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. |
I agree with @ngraham (I also for Cancel as a default). @elvisangelaccio what do you think?
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?
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." |
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. |