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

Search behavior in Terminal -- add capability to insert the matched hit #8274

Closed
brunzefb opened this issue Nov 14, 2020 · 12 comments · Fixed by #13765
Closed

Search behavior in Terminal -- add capability to insert the matched hit #8274

brunzefb opened this issue Nov 14, 2020 · 12 comments · Fixed by #13765
Assignees
Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone

Comments

@brunzefb
Copy link

brunzefb commented Nov 14, 2020

Allow Search functionality to easily expand found text, and insert the expanded found text at current cursor location

as an example take a look at this Kubernetes example:

╭─root@nonprod /home/nonprod
╰─# k get po -n dev | grep notifications
oom-ent-notifications-1-0-775c6fb9dc-n476x                1/1     Running   3          71d
╭─root@nonprod /home/nonprod
╰─#

The system listed a container name - a complex generated name. I need the full container name to delete the container, for instance, what I want is to execute the following

k delete po -n dev oom-ent-notifications-1-0-775c6fb9dc-n476x

Now the problem is -- how do I get the long name in? The mac iterm2 solves this by allowing a search

  1. User hits ctrl+f (or ctrl+shift+f)
  2. User types in oom-ent
  3. System matches oom-ent (below the line ending in grep notifications)
  4. User presses Tab
  5. System highlights oom-ent-notifications-1-0-775c6fb9dc-n476x AND copies it to clipboard
  6. User types k delete po -n dev [Ctrl+V]
  7. System displays k delete po -n dev oom-ent-notifications-1-0-775c6fb9dc-n476x
  8. User hits enter to execute the command.
    Shift-Tab should work to select up to the beginning of the word. Shift-Tab followed by Tab should select the whole word if letters in the middle of the word are matched.

Allowing this functionality is super-useful.
F.

@brunzefb brunzefb added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Nov 14, 2020
@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 Nov 14, 2020
@skyline75489
Copy link
Collaborator

Out of curiosity, is there any terminal out there that actually has this particular feature?

@brunzefb
Copy link
Author

brunzefb commented Nov 16, 2020 via email

@zadjii-msft
Copy link
Member

So, I'll throw this on the Search v2 backlog, but we'll need to definitely think about how this will work exactly. Right now, tab in the find dialog will move the focus to the "Find Up" button. We'd need to be able to offer some sort of configurable way of intercepting that and performing some other action - in this case, something like "expandSelection(word)".

Presumably, in your example, if the user had searched for ent-not, that would expand in both directions, to select the entire oom-ent-notifications-1-0-775c6fb9dc-n476x word. Nevermind, just read the following:

Shift-Tab should work to select up to the beginning of the word. Shift-Tab followed by Tab should select the whole word if letters in the middle of the word are matched.

So idk if we have actions for both of those spec'd in #2840, I'd have to check

Then, we'd need to figure out how this plays with copy on select - I'm thinking that we'd probably not have it auto-copy (unless copyOnSelect was enabled, and instead require the user to manually press the copy keybinding.

@zadjii-msft zadjii-msft added Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Area-User Interface Issues pertaining to the user interface of the Console or Terminal Product-Terminal The new Windows Terminal. labels Nov 18, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Nov 18, 2020
@zadjii-msft zadjii-msft added this to the Terminal Backlog milestone Nov 18, 2020
@zadjii-msft zadjii-msft mentioned this issue Nov 18, 2020
20 tasks
@brunzefb
Copy link
Author

brunzefb commented Nov 18, 2020 via email

@zadjii-msft
Copy link
Member

I would love to help you write a spec 😃

Right now, we've got a spec template over here. I'm gonna level with you, I don't love the template, and I'm working on updating it now, but it's certainly a good enough starting point. I'd think that the "Capabilities" header should definitely be renamed to "Concerns" or something similar. There's a bunch of other completed specs in the same folder, with a variety of levels of detail (less, more, most). Those can be a good reference for what we're looking for.

As far as details go, I'm a little worried that this feature might run up against the "Keyboard Selection Spec" over in #2840. I'm pretty sure that spec is actually in its final form and has all the details that the team discussed, but we're mostly just backed up a little bit bureaucratically on that on. So I think that's fine to use as a base to build off of.

I'd also keep an eye on the fact that currently, the TermControl itself is the thing responsible for handling the key events to try and dispatch an action. If we wanted to have the keybinding for this action configurable, then we'd need some way of identifying that the action is being dispatched from the find dialog, not the terminal control. If you can describe the UX well enough, we can almost certainly help figure the plumbing out.

I'm super happy to keep pushing on the discussion and keep bumping the review as much as possible, though I'd just also warn you that as we're heading into the end of the year, there are a lot more holidays and vacation days used, so our response times might be a little slower than usual.

The codebase is all C++ (more specifically, C++/WinRT). If you're interested in poking around the code, this directory is probably the most relevant to the discussion.

@DHowett DHowett removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Nov 20, 2020
@brunzefb
Copy link
Author

brunzefb commented Nov 21, 2020 via email

@zadjii-msft
Copy link
Member

Take however long you need! If you only want to work on the spec, that's perfectly fine too. C++/WinRT is a bit of a weird hybrid of C++ and C#, and it can be a little tricky to get things exactly right. It's really nice once you've gotten the hang of it, but it can have some really nasty edge cases.

As far as the process goes, what you've outlined is great. I think we're still trying to find the best way to handle community spec contributions. You'll probably get a couple reviews from the team after submitting the PR, and we tend to hold specs to a 3-approval minimum, to ensure there's a broader consensus across the team. We usually have team syncs on Mondays, so we might have additional feedback for you the Monday after the PR is submitted.

I think technically our code of conduct is here, but we operate pretty informally. If you follow the unofficial COC of "don't be an asshole", then you'll get along just fine 😄

@zadjii-msft zadjii-msft modified the milestones: Terminal Backlog, Backlog Jan 4, 2022
@zadjii-msft
Copy link
Member

An idle thought: We've got mark mode now. Maybe we could have the find dialog trigger a mark mode selection, use the dialog to get to the hit you want, then hit esc to dismiss the dialog. At this point, you've got part of a word selected. Maybe we could bind tab[1] here to "expand selection to word". Then you could ctrl+c to copy it to the clipboard and ctrl+v to paste.

/cc @carlos-zamora to run my crazy idea past you

[1]: I'm aware that as of 1.16 tab in mark mode jumps to the next hyperlink. We didn't really make that rebindable. Maybe we go with ctrl+space for expandToWord? I know that's what Sublime does by default.

@carlos-zamora
Copy link
Member

An idle thought: We've got mark mode now. Maybe we could have the find dialog trigger a mark mode selection, use the dialog to get to the hit you want, then hit esc to dismiss the dialog. At this point, you've got part of a word selected. Maybe we could bind tab[1] here to "expand selection to word". Then you could ctrl+c to copy it to the clipboard and ctrl+v to paste.

[1]: I'm aware that as of 1.16 tab in mark mode jumps to the next hyperlink. We didn't really make that rebindable. Maybe we go with ctrl+space for expandToWord? I know that's what Sublime does by default.

Hmm... I don't like making ctrl+space expandToWord. It feels unintuitive imo. But I can't think of an intuitive default key binding. So for that reason, I think it shouldn't be bound to mark mode.

That said, I'll meet you halfway. What if we did this...

  • Setup:
    • (new work) introduce expandToWord() action. Unbound by default. Operates like "copy" as in it only expands the selection when a selection is present (otherwise the key chord just goes through to the terminal).
    • User binds tab to multipleActions([ expandToWord(), copy(), paste() ]) action. This means that tab will expand the selection to encompass the current word, copy the selection, and immediately paste it.
    • We do not make "Find" enter mark mode (this is the behavior today, I just want to highlight this).
  • Scenario (paraphrasing what OP said):
    • User hits ctrl+shift+f to search text
    • User types in oom-ent
    • System matches oom-ent
    • User presses Tab --> this executes the multiple actions introduced in the setup, which then highlights oom-ent-notifications-1-0-775c6fb9dc-n476x, copies it, and immediately pastes it (NOTE: if you don't want to paste it, we can just edit the multipleActions() in setup)
    • Done

tldr: add an expandToWord() action and if the user binds it to Tab, tab works as expected. Just don't bind it by default (but heck, that's even debatable tbh since it won't interfere with mark mode's tab behavior)

@zadjii-msft shipit? This is a pretty easy task tbh. TerminalSelection already has logic to expand by word. We'd just expand "start" to the beginning of the word and "end" to the end of the word haha.

@zadjii-msft
Copy link
Member

I don't hate it. tab seems like a dangerous keybinding for this (or anything), just cause like... I think even in the find dialog, we check keybindings (so binding tab to something predictably prevents tab navigation of the UI)[1]. BUT I think otherwise, this should all work as expected, yea?

We may want to sanity check something like returning false out of a multipleActions action - do we keep going with the rest of the actions? We do. (Maybe we shouldn't?). But that's an aside.

I'd say :shipit:, and I'll let @brunzefb ACK if this idea would work for them

[1]: this is a mild fact, I haven't actually verified it.

@carlos-zamora
Copy link
Member

Oh. Minor correction for what I said above. We should escape the find dialog first. Then press tab. That way, tab is bound to something.

Hoping to write expandToWord() in my spare time soon. Just for fun ;)

@ghost ghost added the In-PR This issue has a related PR label Aug 17, 2022
@ghost ghost closed this as completed in #13765 Aug 31, 2022
ghost pushed a commit that referenced this issue Aug 31, 2022
## Summary of the Pull Request
Introduces a new action `expandSelectionToWord()` which expands the beginning and end of the selection to encompass the word(s) it's on. This was implemented as a conditional keybinding where the key chord is passed through to the terminal if no selection is active (similar to `copy()`).
It is not bound to anything by default.

## PR Checklist
* [x] Closes #8274
* [x] Schema updated.

## Validation Steps Performed
- Scenario in #8274:
	- search for some text in the find dialog
	- ESC to close the dialog
	- execute `expandSelectionToWord()`
	- the new selection encompasses the whole word
- mark mode
	- move onto a word
	- execute `expandSelectionToWord()`
- mouse selection (same as above)
- select a portion of two words --> new selection fully encompasses both words
@ghost ghost removed the In-PR This issue has a related PR label Aug 31, 2022
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Aug 31, 2022
@ghost
Copy link

ghost commented Sep 13, 2022

🎉This issue was addressed in #13765, 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.) Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. 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