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

Mark Mode should preempt key bindings, and work with anything that causes scrolling... #13533

Closed
Jaykul opened this issue Jul 18, 2022 · 8 comments · Fixed by #13659
Closed
Assignees
Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@Jaykul
Copy link
Contributor

Jaykul commented Jul 18, 2022

Windows Terminal version

1.15.1862.0

Windows build number

10.0.19042.0

Other Software

No response

Steps to reproduce

I know the mark mode keyboard selection is still very early days. I just wanted to make sure the expected behaviors are all covered, and I didn't see this called out.

Any hotkey that pre-exists the mark mode currently interfere with selection. That is, currently:

  • Mark mode seems to only works with the default windows text navigation keys: the arrows, PgUp/PgDn, home/end.
  • Selecting requires turning on mark mode and then holding shift while pressing those keys.

If the user has an "Action" configured for Ctrl+Shift+Home or Ctrl+Shift+PgUp or whatever, that action will trigger instead of the selection.

Additionally, nothing else that scrolls the buffer causes the mark to move or select:

  • Scrolling with the mouse
  • Scrolling with scrollToMark
  • Scrolling via the command palette (Ctrl+Shift+P)

On top of that, we can't usefully turn on mark mode after scrolling, because it always appears at the bottom.

Expected Behavior

Mark mode should probably be called "selection" mode, to disambiguate with the new "marks" ...

Mark mode should disregard actions that consume Arrows/PgUp/PgDn/Home/End -- or possibly, should ignore all actions except selection-specific actions or Esc to cancel, changing all the hotkeys like a vi mode does.

The "mark" should follow the scroll buffer if the buffer is scrolled via other actions. Ideally, it should select when that happens with SHIFT pressed.

The mark should always appear within the visible buffer when it's invoked, not only at the bottom/prompt.

Actual Behavior

The mark only ever appears at the prompt, and can only be used if you haven't configured actions over the default text selection keys.

@Jaykul Jaykul added the Issue-Bug It either shouldn't be doing this or needs an investigation. label Jul 18, 2022
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Jul 18, 2022
@zadjii-msft zadjii-msft added the Needs-Discussion Something that requires a team discussion before we can proceed label Jul 18, 2022
@zadjii-msft
Copy link
Member

zadjii-msft commented Jul 18, 2022

There's a lot to unpack here, I'll make sure this is at the top of the discussion order this afternoon EDIT: tomorrow due to some oofages. Thanks!

@zadjii-msft
Copy link
Member

zadjii-msft commented Jul 19, 2022

quick discussion notes:

Mark mode should probably be called "selection" mode, to disambiguate with the new "marks" ...

  • meh, I think we're more attached to calling this "Mark Mode" than we are to calling the scrollbar marks "marks".

Mark mode should disregard actions that consume Arrows/PgUp/PgDn/Home/End -- or possibly, should ignore all actions except selection-specific actions or Esc to cancel, changing all the hotkeys like a vi mode does.

  • yes - we think we have an issue for this. This gets more complicated with things like nextMatch/prevMatch, find in general. We'll noodle on this one.

The "mark" should follow the scroll buffer if the buffer is scrolled via other actions. Ideally, it should select when that happens with SHIFT pressed.

  • maybe not?

The mark should always appear within the visible buffer when it's invoked, not only at the bottom/prompt.

  • for sure

over to @carlos-zamora

@carlos-zamora
Copy link
Member

Ok, I added this to the main epic #4993. I also have a few notes on a few of these:

Mark mode should disregard actions that consume Arrows/PgUp/PgDn/Home/End -- or possibly, should ignore all actions except selection-specific actions or Esc to cancel, changing all the hotkeys like a vi mode does.

I was wrong. No issue tracking this. So congrats, this issue is now tracking that haha.

The "mark" should follow the scroll buffer if the buffer is scrolled via other actions. Ideally, it should select when that happens with SHIFT pressed.

This one has an issue: #4985. I closed it because selecting by viewport automatically scrolls, which mostly satisfies the need. That said since this is the second time it's been asked for (especially after mark mode is available), I'll reopen it.

@carlos-zamora carlos-zamora removed the Needs-Discussion Something that requires a team discussion before we can proceed label Jul 25, 2022
@zadjii-msft zadjii-msft added Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Priority-1 A description (P1) labels Jul 26, 2022
@ghost ghost added the In-PR This issue has a related PR label Aug 2, 2022
@carlos-zamora
Copy link
Member

carlos-zamora commented Aug 2, 2022

Mark mode should probably be called "selection" mode, to disambiguate with the new "marks" ...

Mark mode should disregard actions that consume Arrows/PgUp/PgDn/Home/End -- or possibly, should ignore all actions except selection-specific actions or Esc to cancel, changing all the hotkeys like a vi mode does.

Fixed by #13659

The mark should always appear within the visible buffer when it's invoked, not only at the bottom/prompt.

Fixed by #13405. I need to backport that change still. Backported via #13660

@carlocardella
Copy link
Member

quick discussion notes:

Mark mode should probably be called "selection" mode, to disambiguate with the new "marks" ...

  • meh, I think we're more attached to calling this "Mark Mode" than we are to calling the scrollbar marks "marks".

Uhm... seems counterintuitive to me... "mark mode" allows to select some text, make a selection. Also, most (all) tools/editors I am aware of call this "selection", why do we want to call it "mark"?

@zadjii-msft
Copy link
Member

why do we want to call it "mark"?

Kinda just because that's what the vintage console called it:
image

We've used "mark mode" internally to refer to "the editing mode in which the keyboard input modifies the selection region" for years.

That being said, "Selection mode" is pretty obvious what it's doing. And, calling it "selection mode" would immediately disambiguate it from the implementation in conhost. Idk, back to the discussion board it goes.

@zadjii-msft zadjii-msft added the Needs-Discussion Something that requires a team discussion before we can proceed label Aug 3, 2022
@DHowett DHowett removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Discussion Something that requires a team discussion before we can proceed labels Aug 12, 2022
@zadjii-msft zadjii-msft added the Product-Terminal The new Windows Terminal. label Aug 15, 2022
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Aug 15, 2022
@ghost ghost closed this as completed in #13659 Aug 22, 2022
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Aug 22, 2022
ghost pushed a commit that referenced this issue Aug 22, 2022
## Summary of the Pull Request
This PR moves the key handling for mark mode into a helper method that is then called before an action/key binding is attempted. 

## References
Epic: #4993 
Closes #13533

## Validation Steps Performed
- Add custom keybinding to "down" arrow key
- in mark mode --> selection updates appropriately
- out of mark mode --> keybinding executed
DHowett pushed a commit that referenced this issue Sep 6, 2022
This PR moves the key handling for mark mode into a helper method that is then called before an action/key binding is attempted.

Epic: #4993
Closes #13533

- Add custom keybinding to "down" arrow key
- in mark mode --> selection updates appropriately
- out of mark mode --> keybinding executed

Amended by DHowett to remove URL targeting features.

(cherry picked from commit 70313db)
Service-Card-Id: 84711521
Service-Version: 1.15
@ghost
Copy link

ghost commented Sep 13, 2022

🎉This issue was addressed in #13659, which has now been successfully released as Windows Terminal v1.15.252.:tada:

Handy links:

@ghost
Copy link

ghost commented Sep 13, 2022

🎉This issue was addressed in #13659, which has now been successfully released as Windows Terminal Preview v1.16.252.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants