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.
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.
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.
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.
Does this break the context menu on right click? Maybe the enabled check should be below the primary button check?
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.
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