D15278: Fix BUG:397101

[Alexander Saoutkin]

feverfew created this revision.
feverfew added a project: Dolphin.
Herald added a subscriber: kfm-devel.
feverfew requested review of this revision.

REVISION SUMMARY
Allows closing of last tab via shortcut or menubar (File->Close Tab), which then results in closing dolphin.
Consult https://bugs.kde.org/show_bug.cgi?id=397101 for discussion

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

REPOSITORY
R318 Dolphin

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

AFFECTED FILES
src/dolphinmainwindow.cpp
src/dolphintabwidget.cpp

To: feverfew
Cc: kfm-devel, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Alexander Saoutkin]

feverfew added a reviewer: Dolphin.
feverfew added a subscriber: Dolphin.

REPOSITORY
R318 Dolphin

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

To: feverfew, #dolphin
Cc: #dolphin, kfm-devel, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham added a comment.


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.

INLINE COMMENTS

dolphinmainwindow.cpp:1049
- closeTab->setEnabled(false);

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

REPOSITORY
R318 Dolphin

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

To: feverfew, #dolphin
Cc: ngraham, #dolphin, kfm-devel, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Alexander Saoutkin]

feverfew updated this revision to Diff 41010.
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.
feverfew added a comment.


Removed line due to it being redundant

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D15278?vs=41006&id=41010

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

AFFECTED FILES
src/dolphinmainwindow.cpp
src/dolphintabwidget.cpp

To: feverfew, #dolphin
Cc: ngraham, #dolphin, kfm-devel, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a subscriber: elvisangelaccio.
ngraham added a comment.
This revision is now accepted and ready to land.


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 <https://phabricator.kde.org/tag/dolphin/> folks think before we land it. Thanks again for the patch!

REPOSITORY
R318 Dolphin

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

To: feverfew, #dolphin, ngraham
Cc: elvisangelaccio, ngraham, #dolphin, kfm-devel, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Elvis Angelaccio]

elvisangelaccio added a comment.


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

REPOSITORY
R318 Dolphin

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

To: feverfew, #dolphin, ngraham
Cc: elvisangelaccio, ngraham, #dolphin, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham added a comment.


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.

REPOSITORY
R318 Dolphin

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

To: feverfew, #dolphin, ngraham
Cc: elvisangelaccio, ngraham, #dolphin, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Alexander Saoutkin]

feverfew added a comment.


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

REPOSITORY
R318 Dolphin

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

To: feverfew, #dolphin, ngraham
Cc: elvisangelaccio, ngraham, #dolphin, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham added a comment.


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.

REPOSITORY
R318 Dolphin

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

To: feverfew, #dolphin, ngraham
Cc: elvisangelaccio, ngraham, #dolphin, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Elvis Angelaccio]

elvisangelaccio added a comment.


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

Post by Nathaniel Graham
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.

REPOSITORY
R318 Dolphin

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

To: feverfew, #dolphin, ngraham
Cc: elvisangelaccio, ngraham, #dolphin, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham added a reviewer: VDG.
ngraham added a comment.


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 <https://phabricator.kde.org/tag/vdg/> has to say about it. In theory, one of their functions is to offer informed guidance for this kind of situation.

REPOSITORY
R318 Dolphin

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

To: feverfew, #dolphin, ngraham, #vdg
Cc: elvisangelaccio, ngraham, #dolphin, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Andres Betts]

abetts added a comment.


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

REPOSITORY
R318 Dolphin

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

To: feverfew, #dolphin, ngraham, #vdg
Cc: abetts, elvisangelaccio, ngraham, #dolphin, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Sven Mauch]

svenmauch added a comment.


I tried some programs on different operating systems and observed that Strg + 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 Strg + 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 Strg + 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 Strg + W is as risky as holding Alt + F4 <https://phabricator.kde.org/F4>.

+1

REPOSITORY
R318 Dolphin

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

To: feverfew, #dolphin, ngraham, #vdg
Cc: svenmauch, abetts, elvisangelaccio, ngraham, #dolphin, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Elvis Angelaccio]

elvisangelaccio added a comment.

Post by Elvis Angelaccio
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 ?

REPOSITORY
R318 Dolphin

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

To: feverfew, #dolphin, ngraham, #vdg
Cc: svenmauch, abetts, elvisangelaccio, ngraham, #dolphin, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham added a comment.

Post by Elvis Angelaccio
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?

REPOSITORY
R318 Dolphin

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

To: feverfew, #dolphin, ngraham, #vdg
Cc: svenmauch, abetts, elvisangelaccio, ngraham, #dolphin, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Thomas Surrel]

thsurrel added a comment.


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

REPOSITORY
R318 Dolphin

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

To: feverfew, #dolphin, ngraham, #vdg
Cc: thsurrel, svenmauch, abetts, elvisangelaccio, ngraham, #dolphin, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Sven Mauch]

svenmauch added a comment.

Post by Elvis Angelaccio
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.

REPOSITORY
R318 Dolphin

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

To: feverfew, #dolphin, ngraham, #vdg
Cc: thsurrel, svenmauch, abetts, elvisangelaccio, ngraham, #dolphin, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Alexander Saoutkin]

feverfew added a comment.

Post by Elvis Angelaccio

Post by Elvis Angelaccio
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 ?

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.

REPOSITORY
R318 Dolphin

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

To: feverfew, #dolphin, ngraham, #vdg
Cc: thsurrel, svenmauch, abetts, elvisangelaccio, ngraham, #dolphin, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Alexander Saoutkin]

feverfew added a comment.


@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?

REPOSITORY
R318 Dolphin

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

To: feverfew, #dolphin, ngraham, #vdg
Cc: thsurrel, svenmauch, abetts, elvisangelaccio, ngraham, #dolphin, kfm-devel, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.

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?

REPOSITORY
R318 Dolphin

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

To: feverfew, #dolphin, ngraham, #vdg
Cc: thsurrel, svenmauch, abetts, elvisangelaccio, ngraham, #dolphin, kfm-devel, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Alexander Saoutkin]

feverfew added a comment.


@ngraham ***@gmail.com Thanks!

REPOSITORY
R318 Dolphin

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

To: feverfew, #dolphin, ngraham, #vdg
Cc: thsurrel, svenmauch, abetts, elvisangelaccio, ngraham, #dolphin, kfm-devel, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Elvis Angelaccio]

elvisangelaccio accepted this revision.
elvisangelaccio added a comment.

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? ;)

REPOSITORY
R318 Dolphin

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

To: feverfew, #dolphin, ngraham, #vdg, elvisangelaccio
Cc: thsurrel, svenmauch, abetts, elvisangelaccio, ngraham, #dolphin, kfm-devel, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Elvis Angelaccio]

This revision was automatically updated to reflect the committed changes.
Closed by commit R318:ac118ae1c236: Close Dolphin if last tab closed (authored by feverfew, committed by elvisangelaccio).

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D15278?vs=41010&id=44296

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

AFFECTED FILES
src/dolphinmainwindow.cpp
src/dolphintabwidget.cpp

To: feverfew, #dolphin, ngraham, #vdg, elvisangelaccio
Cc: thsurrel, svenmauch, abetts, elvisangelaccio, ngraham, #dolphin, kfm-devel, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp