Close Dolphin if last tab closed
ClosedPublic

Authored by feverfew on Sep 4 2018, 8:36 PM.

Details

Summary

BUG: 397101
Allows closing of last tab via shortcut or menubar (File->Close Tab), which then results in closing dolphin.

Test Plan

[Manual] Check closing tab closes dolphin via shortcut and menubar.

Diff Detail

Repository
R318 Dolphin
Lint
Lint Skipped
Unit
Unit Tests Skipped
feverfew created this revision.Sep 4 2018, 8:36 PM
Restricted Application added a subscriber: kfm-devel. · View Herald TranscriptSep 4 2018, 8:36 PM
feverfew requested review of this revision.Sep 4 2018, 8:36 PM
feverfew added a reviewer: Dolphin.
feverfew added a subscriber: Dolphin.
ngraham added a subscriber: ngraham.Sep 4 2018, 8:45 PM

Thanks so much for the patch! I'll test it out later today. In the meantime, Can you give the title a human-friendly explanation, rather than "Fix BUG:397101", to follow commit message best practices? See https://community.kde.org/Infrastructure/Phabricator#Formatting_your_patch

Also, BUG: 397101 goes on its own line in the Description section.

src/dolphinmainwindow.cpp
1049

Since true is the default state, we don't need to explicitly set it to true, so this line can just be deleted now.

feverfew updated this revision to Diff 41010.Sep 4 2018, 9:44 PM
feverfew marked an inline comment as done.
feverfew retitled this revision from Fix BUG:397101 to Close Dolphin if last tab closed.
feverfew edited the summary of this revision. (Show Details)

Removed line due to it being redundant

ngraham accepted this revision.Sep 4 2018, 11:21 PM
ngraham added a subscriber: elvisangelaccio.

Works great. Tested in a number of ways and couldn't find any behavioral bugs or regressions. One thing to note is that the menu item "Close tab" that bears the keyboard shortcut will still be visible when there's only a single tab, but I don't really think this is a problem, and in fact may be a good thing as it will let people know that this feature exists.

I'm good with this now, but let's see what @elvisangelaccio and/or other Dolphin folks think before we land it. Thanks again for the patch!

This revision is now accepted and ready to land.Sep 4 2018, 11:21 PM

Hmm, I don't know. If the tabbar is not visible, there is no "last tab". Why should the "close tab" action be enabled then?

Browsers are different because they always have the tabbar visible (even if there is only one tab).

From a user perspective, it's less about the Close Tab action being enabled, and more about how Ctrl+W should quit Dolphin when if the tab tar isn't visible. The Close Tab action always enabled is simply a means to that end. If the request/bug report is valid, my personal opinion is that this patch isn't a bad way to implement it.

I think including the patch as is would only annoy the people who hold the close tab shortcut in attempt to close all tabs bar one, whoever that may be. We could also always show the tab bar, to make it more obvious that Dolphin uses tabs (as a Windows user I always used multiple windows). I think the best option would be to add closing Dolphin via closing the last tab as a setting. However, I originally tried to do this, the problem is that when I tried to add it as a setting it would only apply when the last tab was changed (DolphinMainWidget::tabCountChanged) and the change would never show on the menubar until Dolphin was restarted. The best solution for this patch is a setting that actually works, otherwise, if the features is not deemed too unobtrusive, this should go through, even if I may be a bit biased (seeming as it is my first patch for KDE) :)

Something this tiny isn't a good candidate for being turned into an option IMHO. Starting down that path makes settings windows intimidating and overwhelming over time. Since web browsers already have this behavior, I think people are generally already conditioned not to hold down Ctrl+w so personally I don't think that's a blocker.

Let's not make this an option, please :/

From a user perspective, it's less about the Close Tab action being enabled, and more about how Ctrl+W should quit Dolphin when if the tab tar isn't visible.

But why? Why not use Ctrl+Q instead?

Ctrl+W is the shortcut for the close button on the tab. If there is no such button around, its shortcut shouldn't be enabled imho.

Also, this would make dolphin behave differently than every other kde app.

ngraham added a reviewer: VDG.Oct 10 2018, 2:37 AM

If we don't want to do this, we should close the bug. If we don't want to do that, we should seriously evaluate whether or not to accept this patch.

I'm still in favor because I think it's a nice usability touch. In response to "this would make Dolphin behave differently than every other KDE app", I would turn it around and say that we ought to be willing to adopt nice features from web browsers. Perhaps all KDE apps with tabs should behave this way.

Let's see what VDG has to say about it. In theory, one of their functions is to offer informed guidance for this kind of situation.

abetts added a subscriber: abetts.Oct 10 2018, 3:39 AM

I am in favor of the patch. It helps understand that beyond the last tab there is no window to work with. This is sensible and other file managers do it. Seems pretty logical to me. I think we are mincing and dicing stuff that really doesn't happen. We can skip all those hypotheticals and go ahead with the patch.

+1

svenmauch added a subscriber: svenmauch.EditedOct 10 2018, 7:35 AM

I tried some programs on different operating systems and observed that Ctrl + W generally closes the window if it's the last tab and even closes the window on some programs that don't support tabs to begin with. Konsole also does this so it wouldn't behave differently than every other KDE app.

After holding Ctrl + W in Firefox it closed everything and instantly switched focus to my other instance of Firefox where it closed tabs I still needed. Out of curiosity I tested the same thing in Windows and it also happens there. I'm never holding Ctrl + W again...

From what I gather closing the window with the last tab if there's nothing left to work with is generally expected behavior and holding Ctrl + W is as risky as holding Alt + F4.

+1

Hmm, I don't know. If the tabbar is not visible, there is no "last tab". Why should the "close tab" action be enabled then?

Browsers are different because they always have the tabbar visible (even if there is only one tab).

I still think this should be addressed. Maybe we can change the text of the action from "Close Tab" to "Close Window" if there is only one tab ?

Maybe we can change the text of the action from "Close Tab" to "Close Window" if there is only one tab ?

I'm fine with that. @feverfew, could you make that change?

I have to say, I would love to see this behavior in Kate as well!

I still think this should be addressed. Maybe we can change the text of the action from "Close Tab" to "Close Window" if there is only one tab ?

I think that could confuse people into thinking Ctrl + W always means close the whole window.

feverfew added a comment.EditedOct 14 2018, 1:10 PM

In D15278#340898, @elvisangelaccio wrote:
I still think this should be addressed. Maybe we can change the text of the action from "Close Tab" to "Close Window" if there is only one tab ?

I assume you're talking about the action in the menubar? How about changing it to Close Tab (Quit)? It makes it clear as to what the feature does as Quit (ctrl-q) is an option available further down the menubar.

@ngraham Just checked on Firefox and if you right click the last tab it allows you to choose "Close Tab" even though that will also close the window. Hence, seeming as most people aren't bothered by this behaviour in Firefox, I think we can probably just land this as is?

ngraham accepted this revision.Oct 25 2018, 2:18 AM

Yeah, I'm okay with it. Anyway, looking again for guidance, the HIG says that we shouldn't dynamically change menu items' labels:

https://hig.kde.org/components/navigation/menubar.html#appearance
Do not change menu items’ labels dynamically.

If nobody formally objects in the next few days, I say we land this for 18.12.0. Alexander, could you provide your email address so we can add the correct authorship information?

@ngraham a.saoutkin@gmail.com Thanks!

elvisangelaccio accepted this revision.Oct 27 2018, 9:22 AM

Yeah, I'm okay with it. Anyway, looking again for guidance, the HIG says that we shouldn't dynamically change menu items' labels:

https://hig.kde.org/components/navigation/menubar.html#appearance
Do not change menu items’ labels dynamically.

Ah that's a good point.

Nevermind then. Also, the menubar is hidden by default, I can live with this.

@feverfew Are you going to patch all the other applications that use tabs? ;)

This revision was automatically updated to reflect the committed changes.