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.

Sign up for GitHub

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

Jump to bottom

Multi-Click Selection: Triple-Click Settings + Viewport Selection #1302

Closed
wants to merge 3 commits into from
Closed

Multi-Click Selection: Triple-Click Settings + Viewport Selection #1302

wants to merge 3 commits into from

Conversation

carlos-zamora
Copy link
Member

@carlos-zamora carlos-zamora commented Jun 17, 2019 •
edited
Loading

Summary of the Pull Request

Introduces "viewport selection" which creates a selection of the entire viewport. Also, adds per-profile settings to decide whether a triple-click performs a line selection or viewport selection (disabling it is also an option).

References

Builds on #1197. Similar to #1273.
Closes #1084.

PR Checklist

  • Closes Feature Request: Take screenshot of selection/viewport #1084
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan.

Detailed Description of the Pull Request / Additional comments

  • TerminalSettings:
    • add property to winrt TerminalSettings
  • Terminal App:
    • read/write JSON for this new property
    • default set to line selection
  • TerminalCore:
    • import/use selection mode from ICoreSettings
    • perform proper action
  • UnitTests_TerminalCore:
    • Added setting to MockTermSettings

Validation Steps Performed

  • modified profiles.json. Then performed triple click in an instance of that profile

@carlos-zamora carlos-zamora self-assigned this Jun 17, 2019
@carlos-zamora carlos-zamora changed the title Multi-Click Selection: Triple-Click Settings Multi-Click Selection: Triple-Click Settings + Viewport Selection Jun 17, 2019
@DHowett-MSFT DHowett-MSFT added the Area-User Interface Issues pertaining to the user interface of the Console or Terminal label Jun 18, 2019
zadjii-msft
zadjii-msft reviewed Jun 18, 2019
View reviewed changes
src/cascadia/TerminalSettings/ICoreSettings.idl Outdated
@@ -12,6 +12,13 @@ namespace Microsoft.Terminal.Settings
EmptyBox
};

enum SelectionMode
Copy link
Member

Choose a reason for hiding this comment

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

mild concern that "Selection Mode" might be too broad a name here - though I don't have a good example of why we wouldn't want to call it that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed to TripleClickSelectionMode. I feel like we don't wanna attach this functionality just to TripleClick though. So I'm open to suggestions :)

src/cascadia/TerminalApp/Profile.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalCore/Terminal.hpp Outdated Show resolved Hide resolved
@carlos-zamora carlos-zamora mentioned this pull request Jun 28, 2019
Merged
10 tasks
@carlos-zamora
Viewport selection is now an option
d2e1be1 
TODO: connect to setting
@carlos-zamora
Triple Click Settings
9db18ab
@carlos-zamora
SelectionMode --> TripleClickSelectionMode
23ae33c 
Terminal: save winrt version of enum
Added doc comments
@carlos-zamora carlos-zamora force-pushed the dev/cazamor/mcs/viewport-selection branch from 3bce793 to 23ae33c Compare June 28, 2019 17:15
miniksa
miniksa reviewed Jun 28, 2019
View reviewed changes
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

I mean I'm not totally convinced that triple click needs a settable behavior. The linked thread isn't super relevant to making this a preference that customers are actually looking for.

I also don't know why you just didn't go for broke at this point and put viewport on quadruple click and the entire buffer on quintuple click and the entire known universe of data on sextuple click or something.

@carlos-zamora
Copy link
Member Author

I mean I'm not totally convinced that triple click needs a settable behavior. The linked thread isn't super relevant to making this a preference that customers are actually looking for.

Honestly, it'd probably just be better to have this functionality come in after we do pointer bindings (#1553 ). Then you could just set the behavior you want on a triple click. Should we just back this out then?

I also don't know why you just didn't go for broke at this point and put viewport on quadruple click and the entire buffer on quintuple click and the entire known universe of data on sextuple click or something.

We can doesn't mean we should. I don't think I've ever actually seen anything have 4+ click functionality.

@carlos-zamora
Copy link
Member Author

Abandoning this PR so that it can be done right/better with pointer bindings.

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

@miniksa miniksa miniksa left review comments

@zadjii-msft zadjii-msft zadjii-msft left review comments

Assignees

@carlos-zamora carlos-zamora

Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@carlos-zamora @miniksa @zadjii-msft @DHowett-MSFT