Skip to content

Commit

Permalink
Stop updating AutoSuggestBox on selection redux (#18010)
Browse files Browse the repository at this point in the history
I wrote a big comment next to the changes I made.
This is a redo of #17961 which had various issues.

Closes #17916
Closes #18070 

## Validation Steps Performed
* Pressing Enter within the input line doesn't crash ✅
* Type "Cour" and pick Courier New, press Save = Saved ✅
* Pick any other font from the dropdown, press Save = Saved ✅
* Picking an option dismisses focus but not to the tab row ✅
* The first time after launching the SUI, when the setting is still
  unmodified, when you focus the box and pick an option,
  it'll unfocus the box ✅
* When the setting is unmodified, and you pick the default
  (Cascadia Mono), it'll still unfocus the box ✅
  • Loading branch information
lhecker authored Oct 23, 2024
1 parent b58aa26 commit 8d3f12b
Show file tree
Hide file tree
Showing 4 changed files with 47 additions and 16 deletions.
2 changes: 2 additions & 0 deletions .github/actions/spelling/expect/expect.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1464,6 +1464,7 @@ QWER
Qxxxxxxxxxxxxxxx
qzmp
RAII
Ralph
RALT
rasterbar
rasterfont
Expand Down Expand Up @@ -2013,6 +2014,7 @@ WHelper
wic
WIDTHSCROLL
Widthx
Wiggum
wil
WImpl
WINAPI
Expand Down
49 changes: 37 additions & 12 deletions src/cascadia/TerminalSettingsEditor/Appearances.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1052,22 +1052,18 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation

void Appearances::FontFaceBox_LostFocus(const IInspectable& sender, const RoutedEventArgs&)
{
const auto appearance = Appearance();
const auto fontSpec = sender.as<AutoSuggestBox>().Text();
_updateFontName(sender.as<AutoSuggestBox>().Text());
}

if (fontSpec.empty())
{
appearance.ClearFontFace();
}
else
void Appearances::FontFaceBox_QuerySubmitted(const AutoSuggestBox& sender, const AutoSuggestBoxQuerySubmittedEventArgs& args)
{
// When pressing Enter within the input line, this callback will be invoked with no suggestion.
const auto font = unbox_value_or<Editor::Font>(args.ChosenSuggestion(), nullptr);
if (!font)
{
appearance.FontFace(fontSpec);
return;
}
}

void Appearances::FontFaceBox_SuggestionChosen(const AutoSuggestBox& sender, const AutoSuggestBoxSuggestionChosenEventArgs& args)
{
const auto font = unbox_value<Editor::Font>(args.SelectedItem());
const auto fontName = font.Name();
auto fontSpec = sender.Text();

Expand All @@ -1084,6 +1080,22 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
}

sender.Text(fontSpec);

// Normally we'd just update the model property in LostFocus above, but because WinUI is the Ralph Wiggum
// among the UI frameworks, it raises the LostFocus event _before_ the QuerySubmitted event.
// So, when you press Save, the model will have the wrong font face string, because LostFocus was raised too early.
// Also, this causes the first tab in the application to be focused, so when you press Enter it'll switch tabs.
//
// You can't just assign focus back to the AutoSuggestBox, because the FocusState() within the GotFocus event handler
// contains random values. This prevents us from avoiding the IsSuggestionListOpen(true) in our GotFocus event handler.
// You can't just do IsSuggestionListOpen(false) either, because you can show the list with that property but not hide it.
// So, we update the model manually and assign focus to the parent container.
//
// BUT you can't just focus the parent container, because of a weird interaction with AutoSuggestBox where it'll refuse to lose
// focus if you picked a suggestion that matches the current fontSpec. So, we unfocus it first and then focus the parent container.
_updateFontName(fontSpec);
sender.Focus(FocusState::Unfocused);
FontFaceContainer().Focus(FocusState::Programmatic);
}

void Appearances::FontFaceBox_TextChanged(const AutoSuggestBox& sender, const AutoSuggestBoxTextChangedEventArgs& args)
Expand All @@ -1106,6 +1118,19 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
_updateFontNameFilter(filter);
}

void Appearances::_updateFontName(hstring fontSpec)
{
const auto appearance = Appearance();
if (fontSpec.empty())
{
appearance.ClearFontFace();
}
else
{
appearance.FontFace(std::move(fontSpec));
}
}

void Appearances::_updateFontNameFilter(std::wstring_view filter)
{
if (_fontNameFilter != filter)
Expand Down
4 changes: 3 additions & 1 deletion src/cascadia/TerminalSettingsEditor/Appearances.h
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation

void FontFaceBox_GotFocus(const Windows::Foundation::IInspectable& sender, const Windows::UI::Xaml::RoutedEventArgs& e);
void FontFaceBox_LostFocus(const Windows::Foundation::IInspectable& sender, const Windows::UI::Xaml::RoutedEventArgs& e);
void FontFaceBox_SuggestionChosen(const winrt::Windows::UI::Xaml::Controls::AutoSuggestBox&, const winrt::Windows::UI::Xaml::Controls::AutoSuggestBoxSuggestionChosenEventArgs&);
void FontFaceBox_QuerySubmitted(const winrt::Windows::UI::Xaml::Controls::AutoSuggestBox&, const winrt::Windows::UI::Xaml::Controls::AutoSuggestBoxQuerySubmittedEventArgs&);
void FontFaceBox_TextChanged(const winrt::Windows::UI::Xaml::Controls::AutoSuggestBox&, const winrt::Windows::UI::Xaml::Controls::AutoSuggestBoxTextChangedEventArgs&);
void DeleteFontKeyValuePair_Click(const Windows::Foundation::IInspectable& sender, const Windows::UI::Xaml::RoutedEventArgs& e);
safe_void_coroutine BackgroundImage_Click(const Windows::Foundation::IInspectable& sender, const Windows::UI::Xaml::RoutedEventArgs& e);
Expand Down Expand Up @@ -222,9 +222,11 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
Windows::Foundation::Collections::IObservableVector<winrt::hstring> _FontFeaturesNames;
std::wstring _fontNameFilter;
bool _ShowAllFonts = false;
bool _suppressFontFaceBoxList = false;

static void _ViewModelChanged(const Windows::UI::Xaml::DependencyObject& d, const Windows::UI::Xaml::DependencyPropertyChangedEventArgs& e);

void _updateFontName(winrt::hstring fontSpec);
void _updateFontNameFilter(std::wstring_view filter);
void _updateFilteredFontList();
void _UpdateBIAlignmentControl(const int32_t val);
Expand Down
8 changes: 5 additions & 3 deletions src/cascadia/TerminalSettingsEditor/Appearances.xaml
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,8 @@
</local:SettingContainer>

<!-- Font Face -->
<local:SettingContainer x:Uid="Profile_FontFace"
<local:SettingContainer x:Name="FontFaceContainer"
x:Uid="Profile_FontFace"
ClearSettingValue="{x:Bind Appearance.ClearFontFace}"
HasSettingValue="{x:Bind Appearance.HasFontFace, Mode=OneWay}"
SettingOverrideSource="{x:Bind Appearance.FontFaceOverrideSource, Mode=OneWay}"
Expand All @@ -214,10 +215,11 @@
ItemTemplate="{StaticResource FontFaceComboBoxItemTemplate}"
ItemsSource="{x:Bind FilteredFontList, Mode=OneWay}"
LostFocus="FontFaceBox_LostFocus"
SuggestionChosen="FontFaceBox_SuggestionChosen"
QuerySubmitted="FontFaceBox_QuerySubmitted"
Text="{x:Bind Appearance.FontFace, Mode=OneWay}"
TextBoxStyle="{StaticResource TextBoxSettingStyle}"
TextChanged="FontFaceBox_TextChanged" />
TextChanged="FontFaceBox_TextChanged"
UpdateTextOnSelect="False" />
<CheckBox x:Name="ShowAllFontsCheckbox"
x:Uid="Profile_FontFaceShowAllFonts"
IsChecked="{x:Bind ShowAllFonts, Mode=TwoWay}" />
Expand Down

0 comments on commit 8d3f12b

Please sign in to comment.