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

Consider adding smarter double click to select word logic #682

Closed
Tyriar opened this issue Jun 7, 2017 · 9 comments · Fixed by #3230
Closed

Consider adding smarter double click to select word logic #682

Tyriar opened this issue Jun 7, 2017 · 9 comments · Fixed by #3230
Labels
good first issue help wanted type/enhancement Features or improvements to existing features
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Jun 7, 2017

#670 brings in new double click to select word that allows you to select whole paths/URLs instead of just small fragments of them. This could be improved though for a number of cases, for example:

  • Not selecting trailing : in ls -lR output
  • Not selecting (, ), " surrounding paths, etc.
@Tyriar Tyriar added the feature label Jun 7, 2017
@Tyriar Tyriar added help wanted type/enhancement Features or improvements to existing features and removed type/feature labels Apr 4, 2018
@marvinthepa
Copy link
Contributor

* Not selecting trailing `:` in `ls -lR` output

This can be achieved adding : to wordSeparator (defaults to `` ()[]{}',"```).
Do you want to change the defaults?

  • Not selecting (, ), " surrounding paths, etc.

Already works with the default settings.

I guess this issue can be closed?

@Tyriar
Copy link
Member Author

Tyriar commented Jan 19, 2021

This can be achieved adding : to wordSeparator (defaults to `` ()[]{}',"```).

@marvinthepa adding : there will break http links unfortunately, I think this needs special logic to only exclude trailing colons.

Already works with the default settings.

Here is a case I had in mind here:

image

Ideally double clicking would select the same as the link (which is smarter as it's a link provider in VS Code based on monaco's link detection). I won't if we could solve most of these issues by just deferring to the relatively new link providers to select the link if it's a link, otherwise revert to the existing behavior?

@marvinthepa
Copy link
Contributor

@marvinthepa adding : there will break http links unfortunately, I think this needs special logic to only exclude trailing colons.

I know. This is the default behavior in iterm2 though (quite annoying), that is why I was asking.
If you use the web-links addon it does not make a difference though, the first click of a double click will open the link anyway, and it can be copied using the context-menu. So in this case it might make sense to include it in wordSeparator (not as default).

Ideally double clicking would select the same as the link (which is smarter as it's a link provider in VS Code based on monaco's link detection). I won't if we could solve most of these issues by just deferring to the relatively new link providers to select the link if it's a link, otherwise revert to the existing behavior?

Is there a way to detect if this link provider is present and use it if it is? Sorry for the question, I am unfamiliar with the topic.

Maybe I should have a look at how the web-links addon is detecting links and re-use this. It seems to handle your example just fine, and the code is always present in xterm.js..

@Tyriar
Copy link
Member Author

Tyriar commented Jan 19, 2021

If you use the web-links addon it does not make a difference though, the first click of a double click will open the link anyway

I think you can customize it to require a modifier, just like it works in VS Code.

Is there a way to detect if this link provider is present and use it if it is?

The double click behavior is built into the core, so it could theoretically depend on files that own the link providers. The only concern here is that link providers provide asynchronously, hopefully that won't affect the responsiveness much, even when providers communicate across processes (it's probably fine).

@marvinthepa
Copy link
Contributor

Do you want this to try both registered linkmatchers (sync) and registered linkproviders (async)?

Linkproviders are not even documented on https://xtermjs.org/docs/api/terminal/classes/terminal/, are they considered a supported API, or just used internally for vs-code? Or are linkmatchers deprecated in favor of linkproviders (I did not find any information about this)?

@Tyriar
Copy link
Member Author

Tyriar commented Jan 20, 2021

@marvinthepa oh wow, our website seems to be locked on v4.4 API 😱

image

We'll make sure we fix that this upcoming release.

For checking the API I normally go straight to the .d.ts file in the repo as it's up to date with master (we haven't had a release in ~3 months).

xterm.js/typings/xterm.d.ts

Lines 772 to 778 in 74ea558

/**
* (EXPERIMENTAL) Registers a link provider, allowing a custom parser to
* be used to match and handle links. Multiple link providers can be used,
* they will be asked in the order in which they are registered.
* @param linkProvider The link provider to use to detect links.
*/
registerLinkProvider(linkProvider: ILinkProvider): IDisposable;

Or are linkmatchers deprecated in favor of linkproviders (I did not find any information about this)?

Link matchers are deprecated in favor of link providers, let's support only link providers.

@marvinthepa
Copy link
Contributor

I have a version that is "working" using linkProviders for doubleClick.

To make it work for right click as well, I would use _selectWordAtCursor for both right click and double click. The only difference (apart from allowWhitespaceOnlySelection, which can be pulled up) is

      this._model.selectionEnd = undefined;

@Tyriar do you remember why this is cleared on right click but not on double click? The commit message on 3c6cab1 just describes what it does, but not why.

marvinthepa added a commit to marvinthepa/xterm.js that referenced this issue Jan 24, 2021
Also works with right click select.
Fixes xtermjs#682.
marvinthepa added a commit to marvinthepa/xterm.js that referenced this issue Jan 24, 2021
Also works with right click select.
Fixes xtermjs#682.
@Tyriar
Copy link
Member Author

Tyriar commented Jan 28, 2021

@marvinthepa I think it was to align with macOS' default behavior (probably testing on Terminal.app).

@marvinthepa
Copy link
Contributor

I do not know how the terminal behaves differently when you do not clear _model.selectionEnd when selecting a word. Maybe we should discuss this on #3230. If this is wrong I have to adapt the pull request.

marvinthepa added a commit to marvinthepa/xterm.js that referenced this issue Feb 10, 2021
Also works with right click select.
Fixes xtermjs#682.
marvinthepa added a commit to marvinthepa/xterm.js that referenced this issue Mar 16, 2021
Also works with right click select.
Fixes xtermjs#682.
marvinthepa added a commit to marvinthepa/xterm.js that referenced this issue Mar 16, 2021
Also works with right click select.
Fixes xtermjs#682.
@Tyriar Tyriar added this to the 4.12.0 milestone Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue help wanted type/enhancement Features or improvements to existing features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants