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

Added option to disable text selection #1127

Closed
wants to merge 2 commits into from
Closed

Added option to disable text selection #1127

wants to merge 2 commits into from

Conversation

simast
Copy link

@simast simast commented Nov 13, 2017

This PR adds a new terminal option - enableSelection. I think it's a valid use case where regular terminal text selection is undesirable (one example would be a curses based game).

This option is set to true by default - meaning by default there is no impact for existing integrations.

Example usage:

const term = new xterm.Terminal({
  enableSelection: false // To disable text selection
});

const enableSelection = term.getOption("enableSelection");

term.setOption("enableSelection", true);

Some implementation notes:

  • I am not sure how the flag should be named as right now both enable/disable naming formats are used (example would be disableStdin and enableBold).
  • I have preserved the special selection mode (with a modifier key) when terminal is in mouse event mode. Although previously it was quite confusing that SelectionManager.disable() would actually put terminal into this mode (as selection was not disabled). Therefore to fix this I have modified the disable() and enable() methods to be used for disabling and enabling text selection (and mouse input mode is now handled based on terminal.mouseEvents flag).
  • Default mouse cursor is used when text selection is disabled (like with mouse event input mode).
  • This will also disable text selection using existing public APIs (and with a Cmd+A key).
  • I had a look at existing tests - although I could not figure out how to mock and cover this functionality without exposing some private fields as protected.

@Tyriar
Copy link
Member

Tyriar commented Nov 14, 2017

@simast I'm not sure I understand the need for this; a curses-based game would enable mouse events which does disable selection (excluding the forced selection via a modifier)?

@simast
Copy link
Author

simast commented Nov 14, 2017

I'm not sure I understand the need for this; a curses-based game would enable mouse events which does disable selection (excluding the forced selection via a modifier)?

I don't think that enabling mouse events is a proper workaround to disable text selection. Very few roguelikes using actual terminal output will bother implementing mouse events. Enabling, receiving and then ignoring those events when you do not actually support them is also far from ideal. Besides, having shortcuts like Cmd+A select all terminal is also undesirable in a game.

I think those are two different unrelated options as you can have both mouse events with disabled text selection and the other way around.

@Tyriar
Copy link
Member

Tyriar commented Nov 16, 2017

@simast seems unintuitive to force the user to go and configure their terminal when they want to play said game though. Maybe I missed it but I think xterm.js acts just like other terminals right now with the override to force selection (just checked terminator and gnome-terminal). I couldn't see any other terminals that provide such a setting as well.

@simast
Copy link
Author

simast commented Nov 16, 2017

But xterm.js is a library, not a standalone terminal application. This setting is not for the end-user but for the library user - someone who chose to embed xterm.js in a website or an Electron app. I honestly don't understand why is there an issue to make it more configurable?

@Tyriar
Copy link
Member

Tyriar commented Nov 18, 2017

@simast trying to prevent bloat; the larger the API surface, the harder it is to maintain and the larger the payload. I suspect you are one of only a small handful of people who desire such a setting.

To be clear, xterm.js isn't meant to be a platform in which to ship programs with, but rather a library to implement terminals with. This is however a good candidate for the sort of thing we want custom addons to enable, which we're discussing in #1128 and #808.

@Tyriar
Copy link
Member

Tyriar commented Dec 20, 2017

Closing this off as there are no plans to include this in the core lib as explained above.

@Tyriar Tyriar closed this Dec 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants