Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow closing tabs with middle mouse button when close button is hidden #15924

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kasper93
Copy link

@kasper93 kasper93 commented Sep 2, 2023

Summary of the Pull Request

This commit fixes the middle mouse button handler. The PointerReleased callback is registered, but it is not operational because, on the Release event, the mouse button is no longer pressed. We need to track its state and act accordingly.

Issue was introduced by commit 05e7ea1, which changed the event handler from PointerPressed to PointerReleased, rendering it inoperative. Instead, the default handler is used. The main issue is that when the close button is hidden with the showCloseButton option, the default handler no longer closes the tab on middle mouse clicks.

Also made it consistent with the Settings tab, which was never converted to PointerReleased and is still handled with a custom handler.

References and Relevant Issues

Related commit 05e7ea1

Validation Steps Performed

I've been using this commit locally for quite some time, figured out I might as well share it.

PR Checklist

  • Closes #xxx
  • Tests added/passed
  • Documentation updated
    • If checked, please file a pull request on our docs repo and link it here: #xxx
  • Schema updated (if necessary)

src/cascadia/TerminalApp/TabManagement.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TabManagement.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TabManagement.cpp Outdated Show resolved Hide resolved
{
if (_pointerMiddleButtonPressed && !eventArgs.GetCurrentPoint(*this).Properties().IsMiddleButtonPressed())
{
_pointerMiddleButtonPressed = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_pointerMiddleButtonExited must be reset to false as well, doesn't it?

Copy link
Author

@kasper93 kasper93 Oct 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_pointerMiddleButtonExited is reset on each _OnTabPointerPressed and updated on each enter/exit event. Not strictly needed to clear it here.

@kasper93
Copy link
Author

kasper93 commented Oct 7, 2023

Updated with review changes.

@kasper93 kasper93 requested a review from lhecker October 26, 2023 00:28
@DHowett
Copy link
Member

DHowett commented Dec 15, 2023

I'm sorry we left this one on read for so long! We're quieting down for the holidays, but I will make sure that we get a look at it as soon as is possible after that. Thank you for fixing this bug.

@kasper93
Copy link
Author

Any news about this? Thanks.

@lhecker lhecker added the Needs-Second It's a PR that needs another sign-off label Mar 18, 2024
This commit fixes the middle mouse button handler. The `PointerReleased`
callback is registered, but it is not operational because, on the
Release event, the mouse button is no longer pressed. We need to track
its state and act accordingly.

Issue was introduced by commit 05e7ea1,
which changed the event handler from `PointerPressed` to
`PointerReleased`, rendering it inoperative. Instead, the default
handler is used. The main issue is that when the close button is hidden
with the `showCloseButton` option, the default handler no longer closes
the tab on middle mouse clicks.

Also made it consistent with the Settings tab, which was never converted
to `PointerReleased` and is still handled with a custom handler.
@kasper93
Copy link
Author

Rebased to resolve merge conflict.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Second It's a PR that needs another sign-off
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants