[effects/presentwindows] Allow closing windows on middle-click
ClosedPublic

Authored by ngraham on May 8 2019, 3:02 PM.

Details

Summary

Plasma's Task manager exposes an optional feature whereby the user
can middle-click on a window to close it, but the Present Windows effect
does not do the same.

The presence of a close button you can left-click does not replace the desirable
feature to be able to middle-click on a window to close it, because then the
whole window becomes a click target, so it can be much much faster than
having to aim for the little close button. Also it's off by default, so a user
who goes out of their way to turn it on is signaling that they want to accept the
risk of accidentally closing a window by accident.

Finally, the feature is not allowed for left-click, so people can never accidentally
wreck Present Windows for themselves by assigning it to left-click by accident
and then mistakenly closing their windows.

This reverts commit 55585514f926d1251148e876bfe9ce3504432997.

FEATURE: 321190
FIXED-IN: 5.17.0

Test Plan

Set "Close window" in the Present windows effect, trigger effect, and middle-click on window

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ngraham created this revision.May 8 2019, 3:02 PM
Restricted Application added a project: KWin. · View Herald TranscriptMay 8 2019, 3:02 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
ngraham requested review of this revision.May 8 2019, 3:02 PM
ngraham edited the test plan for this revision. (Show Details)May 8 2019, 3:06 PM

Please do not add this back. This is a very hot topic to many users, but let me explain. I was the one adding the feature in the first place and I removed it again. Personally I think adding the mouse actions functionality was a big mistake. I did this in 2008 when I joined KWin development. My work on Present Windows back then was a failure. I - and Lucas - added several features which turned the code base into an unmaintainable mess and significantly affected the stability of overall KWin. Several of the features added back then were reverted and replaced by better functionality. What I wanted to achieve back then was having the functionality of closing windows. My big mistake was to make this configurable. Instead of just adding this feature we made it completely configurable and turned it into a space ship control of window management. But nothing else in Present Windows is a window management feature. As the name says, it's intended to present all windows, not to manage them. Furthermore this introduced a big usability issue by reusing Present Windows in desktop grid to present the windows but not having the user interaction feature.

A few years later we found a better way to support closing windows in Present Windows: the close button. The only logical result was to remove this feature again. This is the correct decision. If I had not been so stupid before we would have been able to remove the complete mouse interaction feature but the over configurability made this impossible. Nowadays with even more experience I would remove it as it was a failure from user interaction, discoverability and maintenance. It's one of the areas in KWin where I am really ashamed of what I have done to the code.

I admitted defeat years ago and called out to rewrite Present Windows and Desktop Grid as a KWin QML script. Unfortunately nobody stepped up to do this task and so Present Windows and Desktop Grid stayed in the bitrotten state where we have huge user interaction problems and an unmaintainable code base.

I know that "many" users scream for this feature, but in fact it's just a handful. You can really count them. Before merging this feature I urge you to look through the support information attached to our bug reports and check how many users actually change anything in the effects. I would continue this comment, but my baby just woke up.

ngraham added a comment.EditedMay 8 2019, 6:30 PM

Thanks for joining the conversation, @graesslin.

I disagree with all of your arguments, and I believe the text I wrote in the Description section pre-refuted them except for two: the assertions of complex UI and code fragility.

  • On the subject of the UI, this patch adds one entry to one combobox that already exists. It adds no new complexity to the vast majority of people who never click on the combobox. Your complaint about the general complexity of the UI is legitimate, but unrelated to this patch and this patch does not make the existing UI any more complex than it already is, so that cannot be a valid reason to reject it.
  • On the subject of code fragility, if you look at the source changes, only a very very small number of lines of code are being added, and there is no complex logic at all. In fact the feature itself is already implemented; this patch only exposes it in the user interface. I don't believe it is possible that landing this patch would make the codebase more fragile. If your fear is really that any changes at all should be rejected as introducing risk, that would be an implicit insult to the current maintainers' ability to keep up KWin's code quality, and amounts to an argument for never changing anything ever again. This is incompatible with the nature of open-source software that is developed publicly, and cannot be the correct position to take.

It is also not fair, nice, or feasible to freeze or remove features from a piece of code that is in production and relied upon by users just because you consider it unmaintainable and want to replace it. Once an adequate replacement is released, sure, then you freeze and deprecate the old thing. Doing that before the replacement is finished, or indeed before it's even been started (as in the case of the proposed rewritten effect), only has the effect of damaging the user experience and generating user anger towards you. If you want to rewrite this effect, then do it, but please don't throw cold water on people trying to improve the existing one.

Good luck with the baby!

davidedmundson added inline comments.May 9 2019, 3:12 PM
effects/presentwindows/presentwindows_config.ui
229

What's the rationale for changing middle click, but not right click?

A few years later we found a better way to support closing windows in Present Windows: the close button. The only logical result was to remove this feature again.

I don't see how introducing an action on a small hit area on a common mouse button, negates it being a shortcut to have the same action on a larger hit area.

One relevant development on this topic that does make it worth re-evaluating is that the task manager gained middle click close as an option. I would like our overall desktop to be consistent.

The interaction pattern Close-on-MMB can be also seen with tabs, like Firefox, Chromium, KDevelop, and surely more tabbed view shells. So the muscle memory of those used to this really calls to try this pattern here as well, given this is an overview list of window/view representing items. And this not working is rather experienced inconsistency.

So ideally this would be rather a global platform flag (support close-on-mmb-where-paste-not-expected) :)

ngraham added inline comments.May 9 2019, 8:36 PM
effects/presentwindows/presentwindows_config.ui
229

I was thinking I'd limit it only to middle-click because that's the feature people want, but I can re-add it to right-click too. Now allowing it for left-click was deliberate since that seems like a way for people to accidentally mess themselves up more so than any kind of real productivity improvement.

ngraham updated this revision to Diff 57821.May 9 2019, 8:38 PM

Allow the Close Window action to be bound to right-click too

ngraham marked 2 inline comments as done.May 9 2019, 8:38 PM
ngraham edited the summary of this revision. (Show Details)

Having some more time to write. I really do not want to see this option come back. It was the right decision to remove it, I stand to it. I had a shitload of of negative comments due to it and it would have been easy to just revert the change, but from a product development perspective it's wrong to do so. I really need to point out that this is a feature only required by a handful of very noisy and angry users. We don't have to cater to those who scream load but we need to develop a good working product with a streamlined UI and good interaction. While it's true that there are other applications implementing close on middle click, I think it's wrong to offer it on X11. Middle click has a very defined meaning as a fast way to paste. Users could expect that middle click performs a paste action and not close windows.

Present Windows as explained is in a very bad state. I blogged about this in 2013 https://blog.martin-graesslin.com/blog/2013/04/hitting-walls-a-story-of-present-windows-2/ - I recommend to read it. This was basically my answer to the feedback on the change: we hit walls and we need to do something about it. You are very much for improving the UI: please do so. Please address the issue by rewriting Present Windows and Desktop Grid through QML, but don't make Present Windows a feature and maintenance mess offering multiple ways to achieve the same.

I personally consider Present Windows as feature frozen and don't touch code since that blog post. The code base is fragile - desktop grid is even worse. Let's fix it properly by rewriting. Let's fix it by making it a script, so that users can provide variants on store.kde.org where they have their special features without having to maintain all of that in KWin. Without having space ship control UI for something as Present Windows. Present Windows doesn't need options, but we currently offer 40 options not counting the additional options to activate it. This is ridiculous. Let's not make it worse. Please take this as my feedback as domain expert having maintained the code base for years: we took it too far, we destroyed it by the configurability. And at same time we know that users don't use these settings. Nobody knows these options exist, nobody changes them. We have seen options being broken for years without anybody noticing. It's quite common in the effect system.

Last but not least a personal comment: I think it's very bad from a community perspective if changes of former maintainers get reverted once they are no longer active. Especially if these were changes where the maintainer took a hard decision and an unpopular decision. I always considered Lubos's hard decisions (e.g. usability of window decorations) as very important and did not just screw these decisions over although it caused issues with other team members, especially the Oxygen team. I think it would have been unfair to their work and passion.

Quick reminder: the constantly brought argument for this was that it would allow mass closing of windows, what's outright silly, because the window layout would change w/ every closed window - not necessarily in a predictable way (not only because of the re-layout but attempting to close a window might instead add a dialog - in worst case not a transient one)

I would also like to notice that dismissing all worries about pot. havoc but then using that very argument to rule out an LMB option is less than consistent.

I used to make the case that iff PW needed a mass closing feature, a dual step (1. select a bunch of windows, 2. click a delete icon) would be a more robust UX.
If nothing else, the re-layout has to become more predictable, ie. eg. freeze the layout (inc. not adding any new windows) once this action started.

Having some more time to write. I really do not want to see this option come back. It was the right decision to remove it, I stand to it. I had a shitload of of negative comments due to it and it would have been easy to just revert the change, but from a product development perspective it's wrong to do so. I really need to point out that this is a feature only required by a handful of very noisy and angry users. We don't have to cater to those who scream load but we need to develop a good working product with a streamlined UI and good interaction. While it's true that there are other applications implementing close on middle click, I think it's wrong to offer it on X11. Middle click has a very defined meaning as a fast way to paste. Users could expect that middle click performs a paste action and not close windows.

You can fight the rest of the world, but you'll lose. Middle-click-to-close has appeared as an interaction method and won't go away just because it's not in the X11 spec or you don't like it. Ignoring people's changing expectations over time is a sure way for both you and your software to become obsolete and irrelevant.

I personally consider Present Windows as feature frozen and don't touch code since that blog post. The code base is fragile - desktop grid is even worse. Let's fix it properly by rewriting. Let's fix it by making it a script, so that users can provide variants on store.kde.org where they have their special features without having to maintain all of that in KWin. Without having space ship control UI for something as Present Windows. Present Windows doesn't need options, but we currently offer 40 options not counting the additional options to activate it. This is ridiculous. Let's not make it worse. Please take this as my feedback as domain expert having maintained the code base for years: we took it too far, we destroyed it by the configurability. And at same time we know that users don't use these settings. Nobody knows these options exist, nobody changes them.

Except for the people who want this option to come back. You're ignoring them on purpose, which is why they're angry. If you want to rewrite the effect, then do it. But this is production code with real users, even though you consider it fragile. As such, it deserves to be maintained, upgraded, and polished, until the time when it's replaced. But you need to choose one or the other: rewrite, or maintain and upgrade. You can't choose neither. That's just irresponsible. I'm not technically capable of rewriting it, so I'm doing what I can for the existing version. I see you doing nothing but trying to block me. If you want to help, rewrite the effect like you've been saying you want to do for the past six years. If you're not going to do it, then stop trying to make it difficult for other people to step up and maintain the existing one.

Last but not least a personal comment: I think it's very bad from a community perspective if changes of former maintainers get reverted once they are no longer active. Especially if these were changes where the maintainer took a hard decision and an unpopular decision. I always considered Lubos's hard decisions (e.g. usability of window decorations) as very important and did not just screw these decisions over although it caused issues with other team members, especially the Oxygen team. I think it would have been unfair to their work and passion.

Sometimes unpopular decisions are unpopular because they're wrong. By way of illustration, you fought me tooth and nail over being able to run Dolphin and Kate as the root user (not as sudo, but as the actual root user). I won that fight over your objections, and nothing bad happened. Users are happier and no security was compromised (running in a root session is already insecure; being unable to run your file manager or text editor does not improve the situation). You were simply wrong about that. And that's fine! We all make mistakes. It doesn't mean you're a bad person or you have bad judgment. But all decisions should be subject to being re-visited from time to time, and we should have the humility to admit when we were wrong and change course.

This idea of a decision that must be set in stone even after the person who made it is gone runs totally counter to the ethos of free software. It's designed to be a collaborative project that evolves over time, not a marble obelisk carved by a sculptor that must be protected from vandals for all eternity. If that's the development model you want, it's all over the closed-source world. Half the people in my last company were still on RHEL6, and I imagine there must be some team at Red Hat whose job it is to maintain that ancient software and keep it safe from the world changing all around it. I spent 10 years in that sort of environment only to realize that it wasn't the career I wanted, and I find the openness to change in FOSS communities refreshing. KWin is an unfortunate exception from my perspective, and I think it's something that needs to change for the overall health of the project. Freezing production code with no replacement and pushing away contributors who don't agree 100% with your opinions are great ways to make a project slowly die.


We can argue forever--and I really do expect that we can; both of us seem to have very high levels of endurance for this sort of discussion. I expect that neither one of us will agree with the other on this, but neither of us is the KWin maintainer so ultimately it's up to KWin's current pseudo-maintainer (@zzag) or active developers (@davidedmundson, @apol) to decide whether they want this or not.

I won that fight over your objections
You were simply wrong about that. And that's fine!

Nate. Don't deliberately antagonise people. It's very counter productive.

Lets try and keep this vaguely only about UX.

I'm not trying to deliberately antagonize, and I apologize if I came off too strong. But I'm tired of Martin acting as though every decision he's ever made was 100% correct and can never be altered without grave consequences. This attitude makes it impossible to have a real discussion, so I thought it deserved to be challenged. I brought up a specific example where reversing course had no negative consequences, improved the user experience, and made people happy, as a way of demonstrating that revisiting past decisions with a new point of view can sometimes be a good thing and we should be open to it.

I'm more than happy to have a UX discussion regarding this feature. But I don't appreciate the attempt to shut it down before it begins by asserting that decisions made years ago cannot be revisited, that this code should be frozen and allowed to bit-rot before a replacement is even on the drawing board (let alone in production), and that we should ignore users who are angry and upset just because they didn't express themselves with perfect tact and grace.

abetts added a subscriber: abetts.May 10 2019, 5:18 PM

I agree with this on principle. A feature that is perceived as perfect and immutable is a feature that becomes part of history. If we don't have the attitude of wanting to reiterate on our past work, then we are as good as dead as a developer. Nate has provided clear examples of good outcomes in the face of change. Martin has decided to shut down the idea outright with maybes and what ifs. I feel we should talk about UX in this regard and weed out potential risks, but never stop innovating.

I'll try to reason that out, because I think there's some confusion about the development of the topic.

After the feature was removed but before the actual release there was a decision to feature freeze KDE 4.x which tied everybodies hands and sparked the "intense" debate. Afaik this decision was not influenced by Martin and it was certainly unknown to me - I would never have signed off on the patch (and some others) if I had known that this would be a terminal change - even w/o the prospect that it would take quite some time before KDE 5 was released (and by this I don't mean "usable"). It's been 6 years and I'm *still* quite pissed about this unannounced last minute global freeze decision.

Martins decision to "freeze" the PW (and iirc DG) code is unrelated from that and based on the *fact* that the code (back then, I've not looked at it since 2014/15 - everything that follows assumes that the PW code did not fundamentally change) was a convoluted mess. This isn't tied to an entry in a combobox, but the mood was that instead of tampering w/ the existing code in a hopeless effort to fix it, this desperately needed a restart. If the code hasn't been rewritten in the meantime, this is still true today (not sure whether I still have them, but I lost some hours in patches, trying to deal w/ the multiple problems, to this conclusion - yet I agree, in part because I lost some hours trying to deal w/ the multiple problems …)

Re-adding the feature won't make things much worse, but you're in a way giving up and simply arrange w/ a barely maintainable source of problems which you will encounter if you try to adjust the behavior to deal w/ the suggested UX questions.
Simply re-adding it to deal with the situation might therefore be the popular, but not the right choice. And that's not the same.

Having worked on the code myself, I agree that it needs a rewrite. So let's rewrite it. But until that effort has been completed, it is not kind or fair to KWin's users or contributors to remove features and prevent maintenance and polish for the existing code. These total rewrites often take far longer than anyone anticipates. For the PW effect, it has been six years and no rewrite has even been started, and during that time, bugs and feature requests for the existing one pile up and go un-fixed because we want to rewrite it.

So until the rewritten version has been released, I would like for us to continue to maintain the existing one, and evaluate patches on a case-by-case basis to see if they would introduce technical risk or exacerbate existing UI issues. I don't believe this patch does either of those things, so I request a review on its merits, rather than continuing to debate the future of the effect itself or the codebase's general fragility.

Ping? Can we get this done?

zzag requested changes to this revision.May 15 2019, 1:44 PM

So let's rewrite it

It's easier said than done. There are lots things that have to be re-written or refactored.

I'm not against destructive middle click, but I think it should be a feature of the next iteration of present windows effect. We still maintain the present windows effect, we still fix bugs, but not extend it.

On the other note, I don't like that the new option is off by default. It will definitely have discoverability problems!

This revision now requires changes to proceed.May 15 2019, 1:44 PM
zzag added a comment.May 15 2019, 1:47 PM

Before merging this feature I urge you to look through the support information attached to our bug reports and check how many users actually change anything in the effects.

Agreed, most users don't change the defaults (except maybe enabling/disabling some effects).

On the other note, I don't like that the new option is off by default. It will definitely have discoverability problems!

Being off by default matches taskmanager.
The only thing I care about is consistency across Plasma.

In D21083#465601, @zzag wrote:

We still maintain the present windows effect, we still fix bugs, but not extend it.

Why not?

So far I have heard a lot of vague reasons ("code is fragile" "we want to rewrite it") but no specific reasons. I understand those general concerns but I don't think they should be de facto blockers. It's very frustrating to be blocked by generalities and not have this patch evaluated on its merits. This is a trivial patch that does nothing more than add back code which was previously working just fine.

On the other note, I don't like that the new option is off by default. It will definitely have discoverability problems!

That's because it's basically a hidden feature for experts. Those are the people who want this.

Ping. I believe this patch improves the consistency of available options and functionality across KWin and Plasma and does not add technical risk. I would appreciate a review of this patch on its merits.

ngraham edited the summary of this revision. (Show Details)Jun 18 2019, 5:48 PM
ngraham edited the summary of this revision. (Show Details)Jun 19 2019, 7:58 AM
ngraham added reviewers: Plasma, hein.
mart accepted this revision.Jun 21 2019, 4:01 PM
mart added a subscriber: mart.

From a Plasma UI point of view, thins patch is needed as for the end user of plasma, present windows is just another task manager, and the others (as anel applets) have this configurable behavior, so from an UX point of view consistency is needed (default off as on the panel task manager)

This revision was not accepted when it landed; it landed in state Needs Revision.Jun 21 2019, 4:28 PM
This revision was automatically updated to reflect the committed changes.
hein added a comment.Jun 21 2019, 4:49 PM

As the Task Manager maintainer I agree with @mart's take -- it's strange to have this (and have this be popular) as an option in the Task Manager but not a UI that serves a similar purpose, and also gets invoked from the TM.