-
Notifications
You must be signed in to change notification settings - Fork 117
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
Consider middle click config in window mousedown listener #366
Conversation
@@ -447,7 +447,19 @@ function startUpMainMenu(extensionManager: ExtensionManager, keybindingsManager: | |||
}); | |||
} | |||
|
|||
function startUpWindowEvents(mainWebUi: MainWebUi): void { | |||
function mapEventToMouseButtonActionKey(ev: MouseEvent): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have noticed this function to be already copy-pasted in Terminal.ts and TerminalAceViewer.ts.
I wish I could extract it's definition to one place and reuse if only you suggested the best place for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is good practice, but this code base has a very limited lifespan.
@@ -457,7 +469,12 @@ function startUpWindowEvents(mainWebUi: MainWebUi): void { | |||
|
|||
window.document.addEventListener('mousedown', (ev: MouseEvent) => { | |||
if (ev.which === 2) { | |||
WebIpc.clipboardReadRequest(); | |||
const key = mapEventToMouseButtonActionKey(ev); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not need to check for null
because it is guaranteed by the if
above that exactly middle button is pressed
const key = mapEventToMouseButtonActionKey(ev); | ||
const generalConfig = configDatabase.getGeneralConfig(); | ||
const action = <MouseButtonAction> generalConfig[key]; | ||
if (action === 'paste') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall I also consider paste_selection
here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The middle mouse button bug only seems to affect Windows. paste_selection
is only available on Linux.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was able to reproduce it on Linux (Ubuntu 20.04)
Thanks for digging into this. I've been very busy over on the I'll accept this and try out the nightly build. If that is ok then I can do another bug fix release on 0.59. |
Co-authored-by: Dmitriy Baranov <[email protected]>
Fixes #363
The idea is to take into account mouse button click config in the way it is done in Terminal.ts