Skip to content

Conversation

@awmottaz
Copy link
Contributor

Adds a new Boolean option altClickMovesCursor which enables/disables the alt+click behavior that moves the prompt cursor to the location of the mouse.

Closes #3145

@awmottaz
Copy link
Contributor Author

Just a note: I am working on adding a couple of tests:

  • alt+click moves the cursor when option is enabled
  • alt+click does not move cursor when option is disabled
  • option is enabled by default

But I thought I would push up the PR with the implementation right away to get feedback on that.

@awmottaz
Copy link
Contributor Author

I am struggling with writing tests for this. It appears that interaction with the DOM is explicitly disabled in the TestSelectionService. Adding it back in causes lots of issues. Any advice for how to write these tests? Or would you consider this contribution without tests?

@matthias-ccri
Copy link

Just wondering if we want to make this a breaking change. Before, the alt-click behavior was enabled by default. With this change it's disabled by default and must be enabled.

It may be that we should disable it; I don't know, but I thought I'd ask.

@awmottaz
Copy link
Contributor Author

Just wondering if we want to make this a breaking change. Before, the alt-click behavior was enabled by default. With this change it's disabled by default and must be enabled.

I meant for this not to be a breaking change. I set the default value to true. Is there more I need to do to ensure it's enabled by default?

https://github.com/xtermjs/xterm.js/pull/3181/files#diff-d156a9a2b47b63f41faf54f727fc23ffd0ee8fa672cbfe8b77ce20dffa846082R53

Copy link
Contributor

@mofux mofux left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Looks good so far, IMO no additional tests for this feature are required. There are still some missing bits though:

Please add the new option here:

public getOption(key: 'allowTransparency' | 'cancelEvents' | 'convertEol' | 'cursorBlink' | 'disableStdin' | 'macOptionIsMeta' | 'rightClickSelectsWord' | 'popOnBell' | 'visualBell'): boolean;

and here:

public setOption(key: 'allowTransparency' | 'cancelEvents' | 'convertEol' | 'cursorBlink' | 'disableStdin' | 'macOptionIsMeta' | 'rightClickSelectsWord' | 'popOnBell' | 'visualBell', value: boolean): void;

and here:

export interface IPartialTerminalOptions {

false
);
if (coordinates && coordinates[0] !== undefined && coordinates[1] !== undefined) {
if (this._optionsService.getOption('altClickMovesCursor') && coordinates && coordinates[0] !== undefined && coordinates[1] !== undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the check for this option should be moved up to the parent if statement:

if (this.selectionText.length <= 1 && timeElapsed < ALT_CLICK_MOVE_CURSOR_TIME && event.altKey) {

@awmottaz
Copy link
Contributor Author

Thanks @mofux, I made all of your requested changes

@matthias-ccri
Copy link

@awmottaz I see, my mistake!
Thanks for the contribution.

@Tyriar
Copy link
Member

Tyriar commented Nov 24, 2020

Thanks @awmottaz 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Option to disable alt-click move cursor to position behavior

4 participants