-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Alt key should force selection #816
Conversation
@Tyriar This is not yet done, still a WIP. I've changed what I'll still need to address these questions:
As for an option, I'd say no. I couldn't find one for iTerm. |
src/xterm.js
Outdated
@@ -517,7 +517,7 @@ Terminal.prototype.initGlobal = function() { | |||
on(this.element, 'copy', event => { | |||
// If mouse events are active it means the selection manager is disabled and | |||
// copy should be handled by the host program. | |||
if (this.mouseEvents) { | |||
if (!this.selectionManager.hasSelection) { |
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 think you should either guard this call or use this.hasSelection()
because this.selectionManager
might not yet be available at this stage (it is only available after term.open
was called).
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.
Now that you say this, Terminal.hasSelection
should probably have a null check in it as consumers could call the API before Terminal.open
.
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.
It has since #797 😉
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.
Opps, was looking at the code from GitHub and I guess it wasn't master 😕
Fixed the NPE. Answering the questions:
In the Gnome terminal, the Shift key is used to escape this Mouse mode. We should make this platform dependent since in many Linux systems it seems Alt moves the window.
In the Gnome terminal, nothing really happens, the selection continues to be modified as the mouse moves. Seems like we are OK there.
In the Gnome terminal, in such a mode, Shift no longer allows to extend a selection; every selection is a new one. This is fine by me and I propose to implement this.
I wouldn't make it an option. |
Also, sorry about the auto formatting. Let me know if you prefer a clean commit which doesn't format untouched code. |
Your assumptions sound reasonable. My only slight concern is if we implement an 'altIsMeta' option in the future, this might conflict with the alt key being used for selection... but since it's only mouse interaction that shouldn't really be a problem... |
Alright guys. I've pushed more changes. Tests should go green soon. Feel free to properly review and/or merge. 👍 |
src/SelectionManager.ts
Outdated
} else { | ||
|
||
// Don't send the mouse down event to the current process | ||
event.stopPropagation(); |
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.
Does this break the context menu on right click? Maybe the enabled check should be below the primary button check?
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.
Great catch!
src/SelectionManager.ts
Outdated
@@ -314,6 +317,17 @@ export class SelectionManager extends EventEmitter { | |||
* @param event The mousedown event. | |||
*/ | |||
private _onMouseDown(event: MouseEvent) { | |||
// Ignore this event when selection is disabled | |||
if (!this._enabled) { | |||
if (!(Browser.isLinux ? event.shiftKey : event.altKey)) { |
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.
Shouldn't this be isMac? Also the inverted ternary is pretty hard to read
Refactored a bit to accommodate for the fact that we always want the context menu to show up when there is a selection in the editor. |
1 similar comment
Pushed! |
Looks good, I made a minor change to disable it on Linux/Windows as it will probably lead to weird behavior being on shift a that performs an incremental not a single click. |
@Tyriar I started selfhosting on Linux last month. Without that change, I still won't be able to select text in the terminal. 😢 Note that the shift key hijacking would only happen in a program which enabled mouse mode, thus disabling selection. |
After checking now this is how it works on gnome-terminal, we should bring it back with shift for the next version. I don't think I had a Linux box with me when I was merging, sorry 🙁. Tracking in #869 |
From microsoft/vscode#28863
Fixes #825