[WIP]Close tab on middle mouse button event
AbandonedPublic

Authored by shubham on Jan 29 2019, 7:11 PM.

Details

Reviewers
hindenburg
ngraham
Group Reviewers
Konsole
Summary

BUG: 361756

Diff Detail

Repository
R319 Konsole
Branch
tab
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 7743
Build 7761: arc lint + arc unit
shubham created this revision.Jan 29 2019, 7:11 PM
Restricted Application added a project: Konsole. · View Herald TranscriptJan 29 2019, 7:11 PM
Restricted Application added a subscriber: konsole-devel. · View Herald Transcript
shubham requested review of this revision.Jan 29 2019, 7:11 PM
shubham removed a reviewer: tcanabrava.
shubham added a subscriber: Konsole.

I'd like this, but I know @hindenburg doesn't (and he's the maintainer, so his opinion is the one that counts), so I suspect this will be rejected. But maybe it could be acceptable if it's an off-by-default option?

shubham added a comment.EditedJan 29 2019, 8:08 PM

This patch will bring consistency among other KDE apps like dolphin, kate, yakuake etc. @ngraham Do you know why he may not like this feature?

We'll need to wait for him. :)

I've been against this in the past - it is too easy to accidentally close a tab. Unlike other KDE apps, accidentally closing a Konsole tab would be really bad IMHO.

Also this conflicts w/ the huge tab split patch which changes a lot of code.

@hindenburg Here are my points for endorsing it

  1. The very same thing is present in yakuake(clone of konsole I believe).
  2. It is seldom that middle mouse button is pressed unintentionally.

But at the end, the decision is yours.

Is it really that easy to accidentally middle-click on a tab? On most mice, the mouse wheel is really small. I think the likeliest way to accidentally middle-click on a tab when trying to left-click on it is when using a buttonless touchpad with the Libinput driver and the default "Areas" click method, which creates a virtual, invisible middle-click button in the center of the touchpad's bottom edge. This is a really awful UX for other reasons though, not just here. :/

I think there's something to be said for consistency. Konsole already displays a warning when you try to close a tab with an active process, and that seems like probably enough safety. Konsole is an expert's tool, after all. There's not as much of a need to protect users from themselves when they're using experts-only tools.

shubham updated this revision to Diff 50633.Feb 1 2019, 8:35 AM

Inside Qt::MiddleButton release event

shubham updated this revision to Diff 50634.Feb 1 2019, 8:37 AM

Remove unncessary include

shubham retitled this revision from Close tab on middle mouse button event to [RFC]Close tab on middle mouse button event.Feb 5 2019, 2:36 AM
shubham added a comment.EditedFeb 6 2019, 3:06 PM

@hindenburg Are you okay with this after me and nate have explained it's pros? If not, I will abandon this.

@hindenburg Are you okay with this after me and nate have explained it's pros? If not, I will abandon this.

Let me test this a bit more

I can't get this code to work - middle click doesn't do anything - however, https://phabricator.kde.org/D15742 does work - I assume you didn't see that as it duplicate the new/close middle button.

I'm still testing - do you see any issue w/ D15742?

pino added a subscriber: pino.Feb 8 2019, 8:30 AM

I've been against this in the past - it is too easy to accidentally close a tab. Unlike other KDE apps, accidentally closing a Konsole tab would be really bad IMHO.

Exactly, +1.

Is it really that easy to accidentally middle-click on a tab?

Yes, it is. I lost the count I closed tabs when trying to paste text in yakuake/konsole, and the finger/mouse accidentally slipped on the tab just before the click.
Also, this has been requested as opt-out feature in yakuake, see D14860: Make middle click closes tab optional.

On most mice, the mouse wheel is really small.

There is no wheel when using a touchpad/trackpad...

In D18606#407602, @pino wrote:

On most mice, the mouse wheel is really small.

There is no wheel when using a touchpad/trackpad...

I think you missed the next sentence of my comment which specifically discussed touchpads. :)

+1 for making it optional due to the possibility for destructiveness.

How about a normal confirmation dialog (with the normal "don't ask again")?

There is already a confirmation dialog when you close a tab with a running process, so unless we cleverly account for that, this would result in two dialogs.

I don't think we should present a dialog box because previously I have heard that dialog boxes are annoying to most users. @ngraham will know it.

There is already a confirmation dialog when you close a tab with a running process, so unless we cleverly account for that, this would result in two dialogs.

Makes sense, that was actually why I thought about it, but two dialogs is a bit too much indeed.

Every config options add a ton of extra code, though, but it's probably better to just refactor that in general.

I don't think we should present a dialog box because previously I have heard that dialog boxes are annoying to most users. @ngraham will know it.

Yeah, dialogs are bad if they happen too often, but config options aren't always good either (discoverability etc.).

mglb added a subscriber: mglb.Feb 8 2019, 4:55 PM

What about hidding the tab, showing kmessagewidget "Tab xxx closed. undo" + timeout, and really closing the tab after the timeout? Works well on web/mobiles.

I would love that. If we did that, we could even remove the current confirmation dialog box IMO.

What about hidding the tab, showing kmessagewidget "Tab xxx closed. undo" + timeout, and really closing the tab after the timeout? Works well on web/mobiles.

I can't stand it tbh., either the notification covers up stuff, or I don't manage to hit the undo button in time. Just hitting enter or escape to confirm or cancel is easier for something that's exceptional (i. e. doesn't usually happen).

If there was a confirmation dialog every time you closed a tab it would be different, but that's why we track the running process.

shubham retitled this revision from [RFC]Close tab on middle mouse button event to [WIP]Close tab on middle mouse button event.Feb 9 2019, 3:11 PM

What should I do to this revision? continue or abandon in favor of the other?

I'd rather use the other one D15742 - we still need a GUI option added

shubham abandoned this revision.Feb 12 2019, 4:47 PM