Add action for Quick Fix and key binding for Suggestions #17502

carlos-zamora · 2024-07-02T00:31:08Z

Adds a keybinding to open the quick fix menu, if one is available. When the action is used, we also open up the button (if it was collapsed) because that looks nice.

The showSuggestions action is bound to ctrl+shift+period by default to align with VS' "quick actions" feature and VS Code's "quick fix" feature. This was chosen over binding to quickFix because it's more helpful. The quick fix button is a route for users that prefer to use the mouse. If users want to add a keybinding to activate the quickFix button, they can do that now.

This PR also performs a bit of miscellaneous polish from the bug bash. This includes:

the suggestions UI now presents quick fixes first
scrolling may result in the button being drawn in the wrong place
- The bug was tracked down this line: TermControl::CursorPositionInDips() --> _core.CursorPosition() --> Terminal::GetViewportRelativeCursorPosition(). The mutable viewport there does not update when the user scrolls. Thus, the button would be drawn on the same position on the screen even though we scrolled. To fix this, I include the _scrollOffset in the calculation. The only other place this function is used is with the suggestions UI, which does not update the UIs position as we scroll (but if we're interested in doing that, we can now).

Closes #17377

carlos-zamora · 2024-07-02T00:33:48Z

src/cascadia/TerminalSettingsModel/defaults.json

@@ -637,6 +638,7 @@
        { "keys":"ctrl+shift+p", "id": "Terminal.ToggleCommandPalette" },
        { "keys":"win+sc(41)", "id": "Terminal.QuakeMode" },
        { "keys": "alt+space", "id": "Terminal.OpenSystemMenu" },
+        { "keys": "ctrl+period", "id": "Terminal.QuickFix" },


The quickFix action is bound to ctrl+period by default to align with VS' "quick actions" feature and VS Code's "quick fix" feature.

@zadjii-msft funny enough, I feel like openSuggestions would be better suited here. Especially since the suggestions UI loads quick fixes automatically. Maybe we want ctrl+period to be used for openSuggestions instead?

Also, we're in a weird spot rn. Quick fixes are behind a feature flag so idk if it makes sense to bind this just yet. Happy to remove changes to this file, just want some clarity on the plan here.

CC @DHowett

hard veto binding just ctrl+anything. Ctrl alone is reserved for a modifier for the CLI application to use.

I'm probably biased, so I probably need to recuse myself from answering if it should be quickFix or showSuggestions

(In case you're curious) Found that "ctrl+period" is used for "repeat the last command" in nano, so yeah, we need a different keybinding.

Changed to "ctrl+shift+period".

The questions above still stand though. How about long-term we do...

ctrl+shift+period --> quick fix

alt+shift+period --> suggestions
Just wanna make sure I don't claim a keybinding you were planning to claim later.

Discussed a bit during today's bug bash. Here's the leading proposal rn:

default kbd opens suggestions UI (source=all)

clicking QF button --> open suggestions UI

As we add more quick fixes (i.e. actual winget suggestions, non-snippets, etc.) and non-snippet suggestions, we can revisit this and change it.

lhecker

(Aside from the question in your comment, LGTM.)

zadjii-msft · 2024-07-02T10:52:17Z

src/cascadia/TerminalSettingsModel/defaults.json

@@ -637,6 +638,7 @@
        { "keys":"ctrl+shift+p", "id": "Terminal.ToggleCommandPalette" },
        { "keys":"win+sc(41)", "id": "Terminal.QuakeMode" },
        { "keys": "alt+space", "id": "Terminal.OpenSystemMenu" },
+        { "keys": "ctrl+period", "id": "Terminal.QuickFix" },


hard veto binding just ctrl+anything. Ctrl alone is reserved for a modifier for the CLI application to use.

I'm probably biased, so I probably need to recuse myself from answering if it should be quickFix or showSuggestions

carlos-zamora · 2024-07-02T22:21:23Z

Quick Fix feedback from bug bash

Sometimes the flyout only opens on the 2nd click on the button
- ...and only closes on the 2nd click into the terminal
- This seems related to the tooltip because moving the cursor into the button and clicking it super quickly doesn't cause the issue
- EDIT: looks like this is a consistent thing with the first time the button appears (regardless of tooltip). Potentially a loading issue? Nah... x:Load was removed :/
- EDIT 2: Not sure how to even approach this one. We don't have a Click handler. The flyout is defined directly in the XAML as a part of the button. The only thing I can think of is that this is a side effect of populating the flyout menu in the Opening() event, but the context menu doesn't have that problem, I believe. Thoughts?
When you open the tooltip with the action, we don't expand the bubble. We should expand the bubble.
When the contents scroll (text prints), the button doesn't change position upwards
- EDIT: ~~having trouble reproing this~~ Repro'd by generating output, then resizing the window down then up (to create a scrollback). Turns out the fix was deep inside the code. Updated the PR body to describe the fix since it's in a bit of a weird place.
[Screen Reader] QF kbd --> "pop up window"
- should probably be renamed to "quick fix list" or something like that
- EDIT: looks like we can't access the "pop up window". The flyout already has a UIA name but the screen reader isn't reading it out. The only thing that can make this better imo is announcing the action that was just run, but that's being tracked over in [Terminal]: Screen Reader does not announce Confirmation message after actions executed successfully. #12055. Marking as resolved for now.
[Screen Reader] Should we add "use kbd to open" to the "quick fix available" notification?
- EDIT: we need the action map to be able to get the key binding, but we don't have access to that in the TermControl. Not sure if it's that valuable. Marking as resolved unless somebody else pushes for it (since I filed this one).
We should put quickfixes in the suggestions UI first

this:

EDIT: should be prevented by

terminal/src/cascadia/TerminalCore/Terminal.cpp

Lines 703 to 710 in de50310

    
           if (vkey == VK_RETURN && !_inAltBuffer()) 
        
           { 
        
               // Treat VK_RETURN as a new prompt, 
        
               // so we should clear the quick fix UI if it's visible. 
        
               if (_pfnClearQuickFix) 
        
               { 
        
                   _pfnClearQuickFix(); 
        
               }

. Pressing "Enter" to run "vim" should clear the quick fix menu.

zadjii-msft

I'll approve but maybe we swap that action around in post

src/cascadia/TerminalSettingsModel/defaults.json

DHowett

2/9

src/cascadia/TerminalApp/AppActionHandlers.cpp

src/cascadia/TerminalControl/TermControl.cpp

DHowett · 2024-07-16T22:27:32Z

I'm bumping this from the merge queue - it took me all of four seconds to find a resource leak 😄

DHowett · 2024-07-16T22:28:04Z

src/cascadia/TerminalControl/TermControl.cpp

@@ -3920,7 +3940,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
        const auto viewportBufferPosition = rd->GetViewport();
        const auto cursorBufferPosition = rd->GetCursorPosition();
        rd->UnlockConsole();
-        if (cursorBufferPosition.y < viewportBufferPosition.Top() || cursorBufferPosition.y > viewportBufferPosition.BottomExclusive())
+        if (cursorBufferPosition.y < viewportBufferPosition.Top() || cursorBufferPosition.y > viewportBufferPosition.BottomInclusive())


what's this change do?

This is an extension of the fix for scrolling may result in the button being drawn in the wrong place.

From some testing, there were some scenarios where the button would be drawn on the bottom left corner of the terminal. The "last visible line of text" would be cut off by the window, so the renderer wouldn't draw the text. But XAML would still draw the button. This didn't seem right to me, so I made this change.

DHowett · 2024-07-16T22:28:48Z

src/cascadia/TerminalCore/Terminal.cpp

@@ -1597,12 +1597,13 @@ void Terminal::ColorSelection(const TextAttribute& attr, winrt::Microsoft::Termi
 }

 // Method Description:
-// - Returns the position of the cursor relative to the active viewport
+// - Returns the position of the cursor relative to the visible viewport
 til::point Terminal::GetViewportRelativeCursorPosition() const noexcept


who else uses this, and are they impacted by this change?

Already did the research. 😉 Here's the relevant stuff from the PR body:

scrolling may result in the button being drawn in the wrong place

The bug was tracked down this line: TermControl::CursorPositionInDips() --> _core.CursorPosition() --> Terminal::GetViewportRelativeCursorPosition(). The mutable viewport there does not update when the user scrolls. Thus, the button would be drawn on the same position on the screen even though we scrolled. To fix this, I include the _scrollOffset in the calculation. The only other place this function is used is with the suggestions UI, which does not update the UIs position as we scroll (but if we're interested in doing that, we can now).

src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw

DHowett · 2024-07-16T22:34:02Z

Scrolling behavior:

fixed in 7f821d4

I removed this from the commit message because individual commits inside a PR get destroyed when it is squashed.

…tion to defaults

src/cascadia/TerminalControl/TermControl.cpp

carlos-zamora commented Jul 2, 2024

View reviewed changes

lhecker approved these changes Jul 2, 2024

View reviewed changes

zadjii-msft requested changes Jul 2, 2024

View reviewed changes

microsoft-github-policy-service bot added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jul 2, 2024

zadjii-msft added this to the Terminal v1.22 milestone Jul 2, 2024

carlos-zamora self-assigned this Jul 12, 2024

carlos-zamora added 2 commits July 12, 2024 10:42

Add a keybinding for Quick Fix

d88389c

ctrl+shift+period

40d7d1e

carlos-zamora force-pushed the dev/cazamor/quick-fix-kbd branch from 1553641 to 40d7d1e Compare July 12, 2024 17:42

carlos-zamora added 4 commits July 12, 2024 11:29

suggestions UI: show QF first

efc3ea1

temporarily expand QF btn when opened via action

a1c2ee4

only find QuickFixButton once

94f83ed

improve scrolling behavior

7f821d4

carlos-zamora assigned zadjii-msft and unassigned carlos-zamora Jul 15, 2024

zadjii-msft approved these changes Jul 16, 2024

View reviewed changes

src/cascadia/TerminalSettingsModel/defaults.json Show resolved Hide resolved

default keybinding for suggestions

5793497

carlos-zamora changed the title ~~Add a keybinding for Quick Fix~~ Add action for Quick Fix and key binding for Suggestions Jul 16, 2024

carlos-zamora unassigned zadjii-msft Jul 16, 2024

carlos-zamora enabled auto-merge July 16, 2024 18:08

zadjii-msft approved these changes Jul 16, 2024

View reviewed changes

carlos-zamora added this pull request to the merge queue Jul 16, 2024

DHowett requested changes Jul 16, 2024

View reviewed changes

src/cascadia/TerminalApp/AppActionHandlers.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved

microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 16, 2024

DHowett removed this pull request from the merge queue due to a manual request Jul 16, 2024

DHowett requested changes Jul 16, 2024

View reviewed changes

microsoft-github-policy-service bot removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 17, 2024

address feedback; get rid of unnecessary FindName; re-add quickFix ac…

f39eccf

…tion to defaults

carlos-zamora assigned DHowett Jul 17, 2024

DHowett reviewed Jul 17, 2024

View reviewed changes

src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved

if constexpr

7e05aad

carlos-zamora requested a review from DHowett July 17, 2024 20:28

DHowett approved these changes Jul 17, 2024

View reviewed changes

DHowett merged commit 1999366 into main Jul 17, 2024
18 of 20 checks passed

DHowett deleted the dev/cazamor/quick-fix-kbd branch July 17, 2024 22:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add action for Quick Fix and key binding for Suggestions #17502

Add action for Quick Fix and key binding for Suggestions #17502

carlos-zamora commented Jul 2, 2024 •

edited by DHowett

Loading

carlos-zamora Jul 2, 2024

zadjii-msft Jul 2, 2024

carlos-zamora Jul 2, 2024

carlos-zamora Jul 2, 2024

lhecker left a comment

zadjii-msft Jul 2, 2024

carlos-zamora commented Jul 2, 2024 •

edited

Loading

zadjii-msft left a comment

DHowett left a comment

DHowett commented Jul 16, 2024

DHowett Jul 16, 2024

carlos-zamora Jul 17, 2024

DHowett Jul 16, 2024

carlos-zamora Jul 17, 2024

DHowett commented Jul 16, 2024

Add action for Quick Fix and key binding for Suggestions #17502

Add action for Quick Fix and key binding for Suggestions #17502

Conversation

carlos-zamora commented Jul 2, 2024 • edited by DHowett Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlos-zamora commented Jul 2, 2024 • edited Loading

Quick Fix feedback from bug bash

zadjii-msft left a comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

DHowett commented Jul 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett commented Jul 16, 2024

carlos-zamora commented Jul 2, 2024 •

edited by DHowett

Loading

carlos-zamora commented Jul 2, 2024 •

edited

Loading