Conversation
Why is this a draft?
Figured I'd publish this as a draft PR for now because there may be some major changes people may want to see at this point in the implementation, and I want to move on to work on other stuff for a bit. |
|
Ctrl+Enter, cause clicking on a link requires Ctrl 🤔? <back to vacation> |
2e48423 to
02d6a07
Compare
02d6a07 to
5e49772
Compare
6f4cc16 to
23f2f65
Compare
PankajBhojwani
left a comment
There was a problem hiding this comment.
Discovered a bug that causes a hang (think its a deadlock) - repro steps:
- Select a link via keyboard
- Hit 'Enter'
- Hang
|
@msftbot merge this in 10 minutes |
|
Hello @carlos-zamora! Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:
If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you". |
| _terminal->SelectAll(); | ||
| _updateSelectionUI(); | ||
| return true; | ||
| if (modifiers.IsCtrlPressed() && vkey == 'A') |
There was a problem hiding this comment.
FWIW: I feel like this is below a "dotted line" boundary where ControlInteractivity and ControlCore meet. This is all interactive stuff, and all specifies the interaction model for the control. @zadjii-msft may be more (or less?) opinionated about this, but like... it feels like we're blending the layers in a way that is not mutually beneficial to all the components.
We should discuss!
There was a problem hiding this comment.
oof, just ran into this:
terminal/src/cascadia/TerminalControl/ControlCore.cpp
Lines 1606 to 1616 in 574a72b
I think this may just be another example of the dotted lines not working right. :/
There was a problem hiding this comment.
Maybe this is a case of bad naming. In my head, both WPF and UWP controls would use Interactivity as the owner for the core. I'm not sure there's a world where they'd be separate.
Curiously, TrySendKeyEvent doesn't exist on Interactivity, it only exists on Core. That's kinda weird. Probably an artifact of how the control/core refactor happened - moving this to the right place wasn't needed so it was left in a weird place. In an ideal world:
- yea
TrySendKeyEventshould be exposed off interactivity, with this selection code in it - Core should expose to Interactivity stuff like
SelectAll,SelectHyperlink,TryOpenSelectedHyperlink(ew), etc.
I think. But I wouldn't call this blocking.
| event Windows.Foundation.TypedEventHandler<Object, ShowWindowArgs> ShowWindowChanged; | ||
| event Windows.Foundation.TypedEventHandler<Object, UpdateSelectionMarkersEventArgs> UpdateSelectionMarkers; | ||
|
|
||
| event Windows.Foundation.TypedEventHandler<Object, OpenHyperlinkEventArgs> OpenHyperlink; |
There was a problem hiding this comment.
FWIW: I had my feelings about ControlCore/Interactivity when I got to this line. I was mostly fine before I saw this, then it made me think twice. This is a user-interaction-originated event.
Did you deduplicate existing code while you were here? Mark scrolling also needs to scroll to a point, and there is an active feature request that discusses where exactly to scroll. We should make sure these things use the same implementation. #13349
What? |
| _altGrAliasing{ true }, | ||
| _blockSelection{ false }, | ||
| _selectionMode{ SelectionInteractionMode::None }, | ||
| _isTargetingUrl{ false }, |
There was a problem hiding this comment.
Terminals don't necessarily "target" things; what does this mean?
There was a problem hiding this comment.
This means that the selection is currently targeting a URL. (I'll rename it)
Side note, there's a lot of members that are specific to selection. It would be nice to pull all of these out into a class or struct like this...
struct SelectionState
{
// also add declarations for SelectionAnchors, ExpansionMode, InteractionMode, Endpoint
std::optional<SelectionAnchors> _selection;
bool _blockMode;
std::wstring _wordDelimiters;
ExpansionMode _multiClickMode;
InteractionMode _interactionMode;
bool _isTargetingUrl;
Endpoint _selectionEndpoint;
bool _anchorInactiveSelectionEndpoint;
};
I kinda want to pull all of TerminalSelection.cpp into its own class too but idk what the right way to clean all of this up would be. The reason I bring this up is because I hate having to add the word "selection" to every var related to it (i.e. _isTargettingUrl --> _selectionIsTargetingUrl). Thoughts? Maybe this should be a task I file under CodeHealth?
| Down | ||
| }; | ||
|
|
||
| enum class SearchDirection |
There was a problem hiding this comment.
Huh. Don't we already have an enum for this?
There was a problem hiding this comment.
src\buffer\out\search.h
27-
28-class Search final
29-{
30-public:
31: enum class Direction
Whether it is usable here is a different story...
There was a problem hiding this comment.
Yeah, it's just not really worth it imo. I had a bool before but I think it's ok to have this enum be a bit redundant (at least for now).
| // Clipboard Integration | ||
| { "command": { "action": "copy", "singleLine": false }, "keys": "ctrl+shift+c" }, | ||
| { "command": { "action": "copy", "singleLine": false }, "keys": "ctrl+insert" }, | ||
| { "command": { "action": "copy", "singleLine": false }, "keys": "enter" }, |
There was a problem hiding this comment.
now users cannot unbind it, and it no longer works for mouse selection.........?
There was a problem hiding this comment.
Correct. I mean, I see the argument for both sides, so I'm not sure what the right answer is.
Enter as a key binding
Pros...
- Configurable (aka, unbindable)
- works with mouse selection "out-of-the-box"
Other notes...
- we should probably bind Shift+Enter to copy w/ singleLine = false then? For consistency with mouse-copy?
- we should probably bind Ctrl+Enter to a new action like "open hyperlink"?
Enter as a mark mode binding
Pros...
- these features are explicitly a part of "mark mode"
Other notes...
- we're technically taking key chords away from the user, but a selection is present so key chords are designed to clear the selection anyways... so not really?
Enter as a "that's just how things work" binding
(aka we make it so that "Enter" and "Shift+Enter" copy when a selection is active... always... regardless of how the selection was made)
Pros...
- consistency
Cons...
- no configurability
Thoughts?
There was a problem hiding this comment.
Also:
frankly I never loved binding enter by default, I was just like, okay with it. The shift+enter point is a good one, and I similarly thought this would be a openHyperlink action.
Hmm. I think after reading
but a selection is present so key chords are designed to clear the selection anyways... so not really
I'm kinda cool with either of the second two.
Uhg idk.
- What if I'm a user that wants enter to "copy, not dismiss"?
- What if I want to bind Ctrl+Enter to
openAutoSuggestMenu(as an example). When would a user haveOkay this scenario doesn't seem to make sense.ctrl+enterbound in their client application s.t. they would not want a selection to cause a hyperlink to be opened when pressed...
I'm not sure I've convinced myself one way or the other
| _scrollOffset -= amtBelowView; | ||
| } | ||
| _NotifyScrollEvent(); | ||
| _activeBuffer().TriggerScroll(); |
There was a problem hiding this comment.
Is it right to tell the text buffer that scrolling happened? Does this kick the renderer?
There was a problem hiding this comment.
Yup.
_NotifyScrollEvent()--> tell TermControl to update the scroll barTriggerScroll()--> tell the renderer to re-render the contents
| _selection->start = result->first; | ||
| _selection->pivot = result->first; | ||
| _selection->end = result->second; | ||
| bufferSize.DecrementInBounds(_selection->end); |
There was a problem hiding this comment.
when you make selection exclusive, rename end so things like this immediately become build breaks. you don't have to keep the rename, just use it to highlight issues where we might be mistreating the endpoint!
| { | ||
| _selection = std::nullopt; | ||
| _selectionMode = SelectionInteractionMode::None; | ||
| _isTargetingUrl = false; |
There was a problem hiding this comment.
who resets _isTargetingUrl when somebody (1) makes a mouse selection? or (2) extends an automatic URL search selection by shift-clicking?
There was a problem hiding this comment.
It's only set in SelectHyperlink() (you tabbed to the hyperlink).
It's then reset when you...
- clear the selection
- enter/exit mark mode (I guess this one is unnecessary)
- use keyboard selection to modify the selection
So if you make a mouse selection (or extend it via shift+click), we're no longer in mark mode so ctrl+enter doesn't work.
That said, if we choose a different path here, _isTargetingUrl's value does matter and should be reset there. Heck, in general, it's probably just good practice to update _isTargetingUrl then so I might as well do it.
Just looked into it. On the left, we're using
Do this:
|
|
Ah so
(quoted from #11000) I ultimately just went with always scrolling to the top cause it was easier to implement off the bat, and because it seemed sensible to always plop the prompt at the start of the viewport (with it's output beneath it). There's been some discussion on this subject |
## Summary of the Pull Request Polishes #13405 with some additional feedback found in testing and post-mortem reviews. Such feedback includes: - [x] ControlInteractivity vs ControlCore split ([link](#13405 (comment))) - [x] clearing the selection should be under lock when copying text via "Enter" - [x] move mark mode keybindings into a helper function - [x] decide if "Enter" should be configurable or non-configurable ([link](#13405 (comment))) - [x] rename `_isTargetingUrl` - [x] bugfix: ctrl+enter when the link is outside of the viewport ## References Original PR: #13405 Relevant issue: #6649 Epic: #4993
|
🎉 Handy links: |
## Summary of the Pull Request
There's already logic to tab to a hyperlink when we're in mark mode. We
do this by looking at the automatically detected hyperlinks and finding
the next one of interest. This adds an extra step afterwards to find any
embedded hyperlinks and tab to them too.
Since embedded hyperlinks are stored as text attributes, we need to
iterate through the buffer to find the hyperlink and it's buffer
boundaries. This PR tries to reduce the workload of that by first
finding the automatically detected hyperlinks (since that's a fairly
quick process), then using the reduced search area to find the embedded
hyperlink (if one exists).
## Validation Steps Performed
In PowerShell, add an embedded hyperlink as such:
```powershell
${ESC}=[char]27
Write-Host "${ESC}]8;;https://github.com/microsoft/terminal${ESC}\This is a link!${ESC}]8;;${ESC}\"
```
Enter mark mode (ctrl+shift+m) then shift+tab to it.
✅ The "This is a link!" is selected
✅ Verified that this works when searching forwards and backwards
Closes #18310
Closes #15194
Follow-up from #13405
OSC 8 support added in #7251
## Summary of the Pull Request
There's already logic to tab to a hyperlink when we're in mark mode. We
do this by looking at the automatically detected hyperlinks and finding
the next one of interest. This adds an extra step afterwards to find any
embedded hyperlinks and tab to them too.
Since embedded hyperlinks are stored as text attributes, we need to
iterate through the buffer to find the hyperlink and it's buffer
boundaries. This PR tries to reduce the workload of that by first
finding the automatically detected hyperlinks (since that's a fairly
quick process), then using the reduced search area to find the embedded
hyperlink (if one exists).
## Validation Steps Performed
In PowerShell, add an embedded hyperlink as such:
```powershell
${ESC}=[char]27
Write-Host "${ESC}]8;;https://github.com/microsoft/terminal${ESC}\This is a link!${ESC}]8;;${ESC}\"
```
Enter mark mode (ctrl+shift+m) then shift+tab to it.
✅ The "This is a link!" is selected
✅ Verified that this works when searching forwards and backwards
Closes #18310
Closes #15194
Follow-up from #13405
OSC 8 support added in #7251
(cherry picked from commit 35bd607)
Service-Card-Id: PVTI_lADOAF3p4s4AxadtzgXBDb8
Service-Version: 1.23


Summary of the Pull Request
Adds support to navigate to clickable hyperlinks using only the keyboard. When in mark mode, the user can press [shift+]tab to go the previous/next hyperlink in the text buffer. Once a hyperlink is selected, the user can press Ctrl+Enter to open the hyperlink.
References
#4993
PR Checklist
Detailed Description of the Pull Request / Additional comments
OpenHyperlinkevent needs to be piped down toControlCorenow so that we can open a hyperlink at that layer.SelectHyperlink(dir)searches the buffer in a given direction and finds the first hyperlink, then selects it.convertToSearchArea()lambda was used to convert a given coordinate into the search area coordinate system. So if the search area is the visible viewport, we spit out a viewport position. If the search area is the next viewport, we spit out a position relative to that.extractResultFromList()lambda takes the list of patterns from the pattern tree and spits out the hyperlink we want. If we're searching forwards, we get the next one. Otherwise, we get the previous one. We explicitly ignore the one we're already on. If we don't find any, we returnnullopt.copyaction is no longer bound toEnter. Instead, we manually handle it inControlCore.cpp. This also lets us handle Shift+Enter appropriately without having to take another key binding._ScrollToPointwas added. It's a simple function that just scrolls the viewport such that the provided buffer position is in view. This was used to de-duplicate code._ScrollToPointwas added into theToggleMarkMode()function. Turns out, we don't scroll to the new selection when we enter mark mode (whoops!). We should do that and we should backport this part of the change too. I'll handle that.Validation Steps Performed