Enable closing a tab by middle click
AbandonedPublic

Authored by bdbai on Nov 23 2019, 8:29 AM.

Details

Reviewers
None
Group Reviewers
VDG
Summary

In some applications like Chromium or Dolphin, it is possible to use middle click to close a tab. This patch brings the similar user experience to Okular.

Diff Detail

Repository
R223 Okular
Lint
Lint Skipped
Unit
Unit Tests Skipped
bdbai created this revision.Nov 23 2019, 8:29 AM
Restricted Application added a project: Okular. · View Herald TranscriptNov 23 2019, 8:29 AM
Restricted Application added a subscriber: okular-devel. · View Herald Transcript
bdbai requested review of this revision.Nov 23 2019, 8:29 AM
pino added a subscriber: pino.Nov 23 2019, 9:55 AM

Please make it configurable, and off by default (otherwise it is too easy to mis-click and close a document).

ndavis added a subscriber: ndavis.EditedNov 23 2019, 10:25 AM
In D25484#566470, @pino wrote:

Please make it configurable, and off by default (otherwise it is too easy to mis-click and close a document).

Do people frequently misclick with the middle mouse button? Most applications with non-permanent tabs allow tabs to be closed with MMB without an option to disable the feature. If it's not a problem for Firefox or Chromium, I don't see it being a problem for Okular.

pino added a comment.Nov 23 2019, 10:33 AM

Do people frequently misclick with the middle mouse button?

It can happen, and more often with a touchpad.

Most applications with non-permanent tabs allow tabs to be closed with MMB without an option to disable the feature. If it's not a problem for Firefox or Chromium, I don't see it being a problem for Okular.

I stopped counting the tabs I accidentally closed in browsers and yakuake (hello D14860: Make middle click closes tab optional...).

In D25484#566477, @pino wrote:

Do people frequently misclick with the middle mouse button?

It can happen, and more often with a touchpad.

I hadn't considered that. By default, you have to tap with 3 fingers to middle click. What are you doing with the touchpad when this happens? Does this happen when your hands are resting or when you try to scroll?

romangg added a subscriber: romangg.EditedNov 23 2019, 10:59 AM

The solution to accidentally clicks is not disabling it by default and making it configurable, but providing the functionality to "restore" a tab after closing it accidentally. Dolphin and Chrome allow me to restore the last closed tab with Ctrl+Shift+T. Does Okular the same?

ndavis added a comment.EditedNov 23 2019, 11:02 AM

The solution to accidentally clicks is not disabling it by default and making it configurable, but providing the functionality to "restore" a tab after closing it accidentally. Dolphin and Chrome allow me to restore the last closed tab with Ctrl+Shift+T. Does Okular the same?

It has recent documents, but not that shortcut.

pino added a comment.Nov 23 2019, 11:08 AM
In D25484#566477, @pino wrote:

Do people frequently misclick with the middle mouse button?

It can happen, and more often with a touchpad.

I hadn't considered that. By default, you have to tap with 3 fingers to middle click. What are you doing with the touchpad when this happens? [...]

One use case is clickpads. However, it can happen also with classic mice, especially the small ones where the buttons are close.

The solution to accidentally clicks is not disabling it by default and making it configurable, but providing the functionality to "restore" a tab after closing it accidentally. Dolphin and Chrome allow me to restore the last closed tab with Ctrl+Shift+T.

And this "solution" does not always work, for example when a page was the result of a form submission, redirection, or other kind of interactivity.

ndavis added a comment.EditedNov 23 2019, 11:16 AM
In D25484#566490, @pino wrote:

I hadn't considered that. By default, you have to tap with 3 fingers to middle click. What are you doing with the touchpad when this happens? [...]

One use case is clickpads. However, it can happen also with classic mice, especially the small ones where the buttons are close.

I'm aware, but I wanted to know how the misclick happens. It might point to another problem that needs to be fixed.

In D25484#566477, @pino wrote:

Do people frequently misclick with the middle mouse button?

It can happen, and more often with a touchpad.

I assume you're talking about Libinput's virtual middle mouse button for buttonless touchpads when using the default "areas" click mode. IMO this is a library problem that we should not work around in apps. You can tell Libinput to use "fingers" rather than "areas" mode and then middle-clicking requires three fingers which is much more difficult to do accidentally. Our touchpad KCM allows configuring this, though we don't make it the default because we respect upstream defaults. We could consider overriding that and using the "fingers" click method by default the way we currently override upstream defaults to force better font hinting in the Fonts KCM.

The solution to accidentally clicks is not disabling it by default and making it configurable, but providing the functionality to "restore" a tab after closing it accidentally. Dolphin and Chrome allow me to restore the last closed tab with Ctrl+Shift+T. Does Okular the same?

Exactly. If tab closing is undoable, then closing accidentally is no big deal at all.

davidhurka added a subscriber: davidhurka.EditedNov 23 2019, 7:16 PM

To prevent some accidental middle-mouse-clicks on the tab bar, we could lock the cursor while middle-mouse-button-zoom.

D21759 would do that. :)

@bdbai, to move this forward, do you think you could also make tab closing undoable?

bdbai added a comment.Nov 24 2019, 6:03 PM

@bdbai, to move this forward, do you think you could also make tab closing undoable?

It is possible. However, before getting down to implement it, I am not sure if anyone else agree with this solution.

By the way, what do you think should be the best way to trigger an undo, such as a keyboard shortcut or a menu entry? Do we need some visual elements to tell the user that the tab closing is undoable?

Undoable tab closing seems totally uncontroversial to me. Web browsers have it, Dolphin has it... It's really just a matter of consistency. On that subject, here's how Dolphin does it:

So it would have a menu item + the standard keyboard shortcut. Seems like a pretty decent UI to me.

aacid added a subscriber: aacid.Nov 24 2019, 9:50 PM

Remember any UI change as this should have it's UI autotest to make sure things don't break in the future.

Also please remember to use invent.kde.org and not phabricator for okular merge requests

aacid added a comment.Nov 24 2019, 9:50 PM

As a sidenote i don't think this makes sense at all, but i don't think allowing tabs in okular makes sense either and i let them in so i'll just say it and then go back to my cave

bdbai added a comment.Nov 25 2019, 1:31 AM

...
Also please remember to use invent.kde.org and not phabricator for okular merge requests

May I know what the difference is between posting a diff patch here and making a MR at invent.kde.org? I was brought here from Get Involved/development where invent.kde.org was never mentioned.

The KDE Community is transitioning away from Phabricator and towards GitLab. The transition is not yet complete, which is why the documentation still points you towards Phab. Individual apps--such as Okular--have already made the jump as "early adopters", so to speak.

Probably it's fine to keep this here since there's already been some discussion that it would be good not to lose, but please use https://invent.kde.org/kde/okular/merge_requests for future Okular contributions. Thanks!

bdbai added a comment.EditedNov 25 2019, 8:16 AM
In D25484#566490, @pino wrote:

...

The solution to accidentally clicks is not disabling it by default and making it configurable, but providing the functionality to "restore" a tab after closing it accidentally. Dolphin and Chrome allow me to restore the last closed tab with Ctrl+Shift+T.

And this "solution" does not always work, for example when a page was the result of a form submission, redirection, or other kind of interactivity.

@ngraham In Okular, when a tab with unsaved changes is about to close, the user will be prompted first. Does this help?

In D25484#566490, @pino wrote:

...

The solution to accidentally clicks is not disabling it by default and making it configurable, but providing the functionality to "restore" a tab after closing it accidentally. Dolphin and Chrome allow me to restore the last closed tab with Ctrl+Shift+T.

And this "solution" does not always work, for example when a page was the result of a form submission, redirection, or other kind of interactivity.

@ngraham In Okular, when a tab with unsaved changes is about to close, the user will be prompted first. Does this help?

Yes this helps the case that @pino brings up, but it handles a different use case. Most of the time a tab won't have unsaved changes because Okular is primarily a reading app. An "Undo last closed tab" feature is for these other cases.

bdbai added a comment.Nov 25 2019, 4:24 PM

Yes this helps the case that @pino brings up, but it handles a different use case. Most of the time a tab won't have unsaved changes because Okular is primarily a reading app. An "Undo last closed tab" feature is for these other cases.

Based on this feature, can we assume a closed tab has no unsaved changes? If that is the case, we might no need any associated data (except url) to serialize for reopening this tab.
Another thing is, how many closed tabs should be saved in memory?

I think we can assume that, but check in the code. I don't think we necessarily need to save anything in memory except the URL for the document shown in the closed tab.

bdbai updated this revision to Diff 70349.Nov 26 2019, 2:05 PM
  • Added "undo tab close"
  • Added a test

Thanks, this looks great. It works just fine and the UI seems sane to me. I have some code comments:

shell/shell.cpp
660

prefer append()

778

coding style: always use braces

778

m_closedTabUrls.isEmpty()

783

ditto

shell/shell.h
175

A regular old QList of QUrls would seem to do just fine as it doesn't seem that you're using any functionality present in QLinkedList not in QList.

shell/shell.rc
8

Need to bump the version number at the top of this file anything you change anything

bdbai updated this revision to Diff 70387.Nov 27 2019, 12:26 AM

Fixed code styles

bdbai marked 6 inline comments as done.Dec 3 2019, 2:01 AM

An MR is drafted at invent, see https://invent.kde.org/kde/okular/merge_requests/69 .

Since you've moved it over there, you can Abandon this from the Add Action... menu.