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

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

Merged
merged 9 commits into from
Jul 17, 2024
Merged

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d88389c
Add a keybinding for Quick Fix
carlos-zamora Jul 2, 2024
40d7d1e
ctrl+shift+period
carlos-zamora Jul 2, 2024
efc3ea1
suggestions UI: show QF first
carlos-zamora Jul 12, 2024
a1c2ee4
temporarily expand QF btn when opened via action
carlos-zamora Jul 12, 2024
94f83ed
only find QuickFixButton once
carlos-zamora Jul 12, 2024
7f821d4
improve scrolling behavior
carlos-zamora Jul 15, 2024
5793497
default keybinding for suggestions
carlos-zamora Jul 16, 2024
f39eccf
address feedback; get rid of unnecessary FindName; re-add quickFix ac…
carlos-zamora Jul 17, 2024
7e05aad
if constexpr
carlos-zamora Jul 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 23 additions & 13 deletions src/cascadia/TerminalApp/AppActionHandlers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1461,7 +1461,19 @@ namespace winrt::TerminalApp::implementation
}

// Aggregate all the commands from the different sources that
// the user selected.
// the user selected. This is the order presented to the user

if (WI_IsFlagSet(source, SuggestionsSource::QuickFixes) &&
context != nullptr &&
context.QuickFixes() != nullptr)
{
// \ue74c --> OEM icon
const auto recentCommands = Command::HistoryToCommands(context.QuickFixes(), hstring{ L"" }, false, hstring{ L"\ue74c" });
for (const auto& t : recentCommands)
{
commandsCollection.push_back(t);
}
}

// Tasks are all the sendInput commands the user has saved in
// their settings file. Ask the ActionMap for those.
Expand All @@ -1488,18 +1500,6 @@ namespace winrt::TerminalApp::implementation
}
}

if (WI_IsFlagSet(source, SuggestionsSource::QuickFixes) &&
context != nullptr &&
context.QuickFixes() != nullptr)
{
// \ue74c --> OEM icon
const auto recentCommands = Command::HistoryToCommands(context.QuickFixes(), hstring{ L"" }, false, hstring{ L"\ue74c" });
for (const auto& t : recentCommands)
{
commandsCollection.push_back(t);
}
}

// Open the palette with all these commands in it.
_OpenSuggestions(_GetActiveControl(),
winrt::single_threaded_vector<Command>(std::move(commandsCollection)),
Expand Down Expand Up @@ -1593,4 +1593,14 @@ namespace winrt::TerminalApp::implementation
_ShowAboutDialog();
args.Handled(true);
}

void TerminalPage::_HandleQuickFix(const IInspectable& /*sender*/,
const ActionEventArgs& args)
{
if (const auto& control{ _GetActiveControl() })
{
const auto handled = control.OpenQuickFixMenu();
args.Handled(handled);
}
}
}
36 changes: 32 additions & 4 deletions src/cascadia/TerminalControl/TermControl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,19 @@ namespace winrt::Microsoft::Terminal::Control::implementation
}
}
});
if constexpr (Feature_QuickFix::IsEnabled())
{
QuickFixMenu().Closed([weakThis = get_weak()](auto&&, auto&&) {
if (auto control{ weakThis.get() }; control && !control->_IsClosing())
{
// Expand the quick fix button if it's collapsed (looks nicer)
if (control->_quickFixButtonCollapsible)
{
VisualStateManager::GoToState(*control, StateCollapsed, false);
}
}
});
}
}

void TermControl::_QuickFixButton_PointerEntered(const IInspectable& /*sender*/, const PointerRoutedEventArgs& /*e*/)
Expand Down Expand Up @@ -3555,8 +3568,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation

if (Feature_QuickFix::IsEnabled())
{
auto quickFixBtn = FindName(L"QuickFixButton").as<Controls::Button>();
quickFixBtn.Height(args.Height() / dpiScale);
QuickFixButton().Height(args.Height() / dpiScale);
QuickFixIcon().FontSize(static_cast<double>(args.Width() / dpiScale));
RefreshQuickFixMenu();
}
Expand Down Expand Up @@ -3892,14 +3904,30 @@ namespace winrt::Microsoft::Terminal::Control::implementation
return std::max(CharacterDimensions().Width * 2.0 / 3.0, GetPadding().Left);
}

bool TermControl::OpenQuickFixMenu()
{
if (Feature_QuickFix::IsEnabled() && _core.QuickFixesAvailable())
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
{
// Expand the quick fix button if it's collapsed (looks nicer)
if (_quickFixButtonCollapsible)
{
VisualStateManager::GoToState(*this, StateNormal, false);
}
auto quickFixBtn = QuickFixButton();
quickFixBtn.Flyout().ShowAt(quickFixBtn);
return true;
}
return false;
}

void TermControl::RefreshQuickFixMenu()
{
if (!Feature_QuickFix::IsEnabled())
{
return;
}

auto quickFixBtn = FindName(L"QuickFixButton").as<Controls::Button>();
auto quickFixBtn = QuickFixButton();
if (!_core.QuickFixesAvailable())
{
quickFixBtn.Visibility(Visibility::Collapsed);
Expand All @@ -3920,7 +3948,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())
Copy link
Member

Choose a reason for hiding this comment

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

what's this change do?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

{
quickFixBtn.Visibility(Visibility::Collapsed);
return;
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalControl/TermControl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void RawWriteString(const winrt::hstring& text);

void ShowContextMenu();
bool OpenQuickFixMenu();
void RefreshQuickFixMenu();
void ClearQuickFix();

Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalControl/TermControl.idl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ namespace Microsoft.Terminal.Control
Double QuickFixButtonCollapsedWidth { get; };

void ShowContextMenu();
Boolean OpenQuickFixMenu();
void RefreshQuickFixMenu();
void ClearQuickFix();

Expand Down
7 changes: 4 additions & 3 deletions src/cascadia/TerminalCore/Terminal.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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).

{
const auto absoluteCursorPosition{ GetCursorPosition() };
const auto viewport{ _GetMutableViewport() };
return absoluteCursorPosition - viewport.Origin();
const auto mutableViewport{ _GetMutableViewport() };
const auto relativeCursorPos = absoluteCursorPosition - mutableViewport.Origin();
return { relativeCursorPos.x, relativeCursorPos.y + _scrollOffset };
}

void Terminal::PreviewText(std::wstring_view input)
Expand Down
2 changes: 2 additions & 0 deletions src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ static constexpr std::string_view RestartConnectionKey{ "restartConnection" };
static constexpr std::string_view ToggleBroadcastInputKey{ "toggleBroadcastInput" };
static constexpr std::string_view OpenScratchpadKey{ "experimental.openScratchpad" };
static constexpr std::string_view OpenAboutKey{ "openAbout" };
static constexpr std::string_view QuickFixKey{ "quickFix" };

static constexpr std::string_view ActionKey{ "action" };

Expand Down Expand Up @@ -438,6 +439,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
{ ShortcutAction::ToggleBroadcastInput, RS_(L"ToggleBroadcastInputCommandKey") },
{ ShortcutAction::OpenScratchpad, RS_(L"OpenScratchpadKey") },
{ ShortcutAction::OpenAbout, RS_(L"OpenAboutCommandKey") },
{ ShortcutAction::QuickFix, RS_(L"QuickFixCommandKey") },
};
}();

Expand Down
3 changes: 2 additions & 1 deletion src/cascadia/TerminalSettingsModel/AllShortcutActions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,8 @@
ON_ALL_ACTIONS(RestartConnection) \
ON_ALL_ACTIONS(ToggleBroadcastInput) \
ON_ALL_ACTIONS(OpenScratchpad) \
ON_ALL_ACTIONS(OpenAbout)
ON_ALL_ACTIONS(OpenAbout) \
ON_ALL_ACTIONS(QuickFix)

#define ALL_SHORTCUT_ACTIONS_WITH_ARGS \
ON_ALL_ACTIONS_WITH_ARGS(AdjustFontSize) \
Expand Down
3 changes: 3 additions & 0 deletions src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -734,4 +734,7 @@
<data name="SaveSnippetNamePrefix" xml:space="preserve">
<value>Save Snippet</value>
</data>
<data name="QuickFixCommandKey" xml:space="preserve">
<value>Open quick fix menu</value>
</data>
</root>
3 changes: 3 additions & 0 deletions src/cascadia/TerminalSettingsModel/defaults.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,6 +447,8 @@
{ "command": "restoreLastClosed", "id": "Terminal.RestoreLastClosed" },
{ "command": "openAbout", "id": "Terminal.OpenAboutDialog" },
{ "command": "experimental.openTasks", "id": "Terminal.OpenTasks" },
{ "command": "quickFix", "id": "Terminal.QuickFix" },
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
{ "command": { "action": "showSuggestions", "source": "all"}, "id": "Terminal.Suggestions" },

// Tab Management
// "command": "closeTab" is unbound by default.
Expand Down Expand Up @@ -639,6 +641,7 @@
{ "keys":"ctrl+shift+p", "id": "Terminal.ToggleCommandPalette" },
{ "keys":"win+sc(41)", "id": "Terminal.QuakeMode" },
{ "keys": "alt+space", "id": "Terminal.OpenSystemMenu" },
{ "keys": "ctrl+shift+period", "id": "Terminal.Suggestions" },

// Tab Management
// "command": "closeTab" is unbound by default.
Expand Down
Loading