Skip to content

Commit

Permalink
Fix broken reset button on some profile settings (#12275)
Browse files Browse the repository at this point in the history 
This fixes a bug where several settings would not show the reset button. The root cause of this issue is two fold:
1. Hooking up `CurrentXXX`
   - `GETSET_BINDABLE_ENUM_SETTING` was hooked up to the **settings** model profile object instead of the **view** model profile object. Since the settings model has no `PropertyChanged` system, any changes were directly being applied to the setting, but not notifying the view model (and thus, the view, by extension) to update themselves.
   - This fix required me to slightly modify the macro. Rather than using two parameters (object and function name), I used one parameter (path to getter/setter).
2. Responding to the `PropertyChanged` notifications
   - Now that we're actually dispatching the `PropertyChanged` notifications, we need to actually respond to them. This behavior was defined in `Profiles::OnNavigatedTo()` in the `PropertyChanged()` handler. Funny enough, that code was still there, it just didn't do anything because it was trying to notify that `Profiles::CurrentXXX` changed. This is invalid because `CurrentXXX` got moved to `ProfileViewModel`.
   - The fix here was pretty easy. Just move the property changed handler to `ProfileViewModel`'s `PropertyChanged` handler that is defined in the ctor.

Bug introduced in #11877

✅ Profile termination behavior
✅ Bell notification style
✅ Text antialiasing
✅ Scrollbar visibility
  • Loading branch information
@carlos-zamora @zadjii-msft
carlos-zamora authored and zadjii-msft committed Mar 3, 2022
1 parent 49b7978 commit 57ec119
Show file tree
Hide file tree
Showing 8 changed files with 56 additions and 57 deletions.
6 changes: 3 additions & 3 deletions src/cascadia/TerminalSettingsEditor/Appearances.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,16 +128,16 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
bool IsCustomFontWeight();
WINRT_PROPERTY(Windows::Foundation::Collections::IObservableVector<Microsoft::Terminal::Settings::Editor::EnumEntry>, FontWeightList);

GETSET_BINDABLE_ENUM_SETTING(CursorShape, Microsoft::Terminal::Core::CursorStyle, Appearance(), CursorShape);
GETSET_BINDABLE_ENUM_SETTING(CursorShape, Microsoft::Terminal::Core::CursorStyle, Appearance().CursorShape);
WINRT_PROPERTY(Windows::Foundation::Collections::IObservableVector<Model::ColorScheme>, ColorSchemeList, nullptr);

WINRT_CALLBACK(PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler);
DEPENDENCY_PROPERTY(Editor::AppearanceViewModel, Appearance);
WINRT_PROPERTY(Editor::ProfileViewModel, SourceProfile, nullptr);

GETSET_BINDABLE_ENUM_SETTING(BackgroundImageStretchMode, Windows::UI::Xaml::Media::Stretch, Appearance(), BackgroundImageStretchMode);
GETSET_BINDABLE_ENUM_SETTING(BackgroundImageStretchMode, Windows::UI::Xaml::Media::Stretch, Appearance().BackgroundImageStretchMode);

GETSET_BINDABLE_ENUM_SETTING(IntenseTextStyle, Microsoft::Terminal::Settings::Model::IntenseStyle, Appearance(), IntenseTextStyle);
GETSET_BINDABLE_ENUM_SETTING(IntenseTextStyle, Microsoft::Terminal::Settings::Model::IntenseStyle, Appearance().IntenseTextStyle);

private:
bool _ShowAllFonts;
Expand Down
4 changes: 2 additions & 2 deletions src/cascadia/TerminalSettingsEditor/GlobalAppearance.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
bool FeatureNotificationIconEnabled() const noexcept;

WINRT_PROPERTY(Editor::GlobalAppearancePageNavigationState, State, nullptr);
GETSET_BINDABLE_ENUM_SETTING(Theme, winrt::Windows::UI::Xaml::ElementTheme, State().Globals(), Theme);
GETSET_BINDABLE_ENUM_SETTING(TabWidthMode, winrt::Microsoft::UI::Xaml::Controls::TabViewWidthMode, State().Globals(), TabWidthMode);
GETSET_BINDABLE_ENUM_SETTING(Theme, winrt::Windows::UI::Xaml::ElementTheme, State().Globals().Theme);
GETSET_BINDABLE_ENUM_SETTING(TabWidthMode, winrt::Microsoft::UI::Xaml::Controls::TabViewWidthMode, State().Globals().TabWidthMode);

public:
// LanguageDisplayConverter maps the given BCP 47 tag to a localized string.
Expand Down
4 changes: 2 additions & 2 deletions src/cascadia/TerminalSettingsEditor/Interaction.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation

WINRT_PROPERTY(Editor::InteractionPageNavigationState, State, nullptr);

GETSET_BINDABLE_ENUM_SETTING(TabSwitcherMode, Model::TabSwitcherMode, State().Globals(), TabSwitcherMode);
GETSET_BINDABLE_ENUM_SETTING(CopyFormat, winrt::Microsoft::Terminal::Control::CopyFormat, State().Globals(), CopyFormatting);
GETSET_BINDABLE_ENUM_SETTING(TabSwitcherMode, Model::TabSwitcherMode, State().Globals().TabSwitcherMode);
GETSET_BINDABLE_ENUM_SETTING(CopyFormat, winrt::Microsoft::Terminal::Control::CopyFormat, State().Globals().CopyFormatting);
};
}

Expand Down
6 changes: 3 additions & 3 deletions src/cascadia/TerminalSettingsEditor/Launch.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation

WINRT_PROPERTY(Editor::LaunchPageNavigationState, State, nullptr);

GETSET_BINDABLE_ENUM_SETTING(FirstWindowPreference, Model::FirstWindowPreference, State().Settings().GlobalSettings(), FirstWindowPreference);
GETSET_BINDABLE_ENUM_SETTING(LaunchMode, Model::LaunchMode, State().Settings().GlobalSettings(), LaunchMode);
GETSET_BINDABLE_ENUM_SETTING(WindowingBehavior, Model::WindowingMode, State().Settings().GlobalSettings(), WindowingBehavior);
GETSET_BINDABLE_ENUM_SETTING(FirstWindowPreference, Model::FirstWindowPreference, State().Settings().GlobalSettings().FirstWindowPreference);
GETSET_BINDABLE_ENUM_SETTING(LaunchMode, Model::LaunchMode, State().Settings().GlobalSettings().LaunchMode);
GETSET_BINDABLE_ENUM_SETTING(WindowingBehavior, Model::WindowingMode, State().Settings().GlobalSettings().WindowingBehavior);
};
}

Expand Down
16 changes: 16 additions & 0 deletions src/cascadia/TerminalSettingsEditor/ProfileViewModel.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,22 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
Opacity(1.0);
}
}
else if (viewModelProperty == L"AntialiasingMode")
{
_NotifyChanges(L"CurrentAntiAliasingMode");
}
else if (viewModelProperty == L"CloseOnExit")
{
_NotifyChanges(L"CurrentCloseOnExitMode");
}
else if (viewModelProperty == L"BellStyle")
{
_NotifyChanges(L"IsBellStyleFlagSet");
}
else if (viewModelProperty == L"ScrollState")
{
_NotifyChanges(L"CurrentScrollState");
}
});

// Do the same for the starting directory
Expand Down
12 changes: 6 additions & 6 deletions src/cascadia/TerminalSettingsEditor/ProfileViewModel.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,12 +77,6 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
void DeleteUnfocusedAppearance();
bool AtlasEngineAvailable() const noexcept;

WINRT_PROPERTY(bool, IsBaseLayer, false);
WINRT_PROPERTY(IHostedInWindow, WindowRoot, nullptr);
GETSET_BINDABLE_ENUM_SETTING(AntiAliasingMode, Microsoft::Terminal::Control::TextAntialiasingMode, _profile, AntialiasingMode);
GETSET_BINDABLE_ENUM_SETTING(CloseOnExitMode, Microsoft::Terminal::Settings::Model::CloseOnExitMode, _profile, CloseOnExit);
GETSET_BINDABLE_ENUM_SETTING(ScrollState, Microsoft::Terminal::Control::ScrollbarState, _profile, ScrollState);

PERMANENT_OBSERVABLE_PROJECTED_SETTING(_profile, Guid);
PERMANENT_OBSERVABLE_PROJECTED_SETTING(_profile, ConnectionType);
OBSERVABLE_PROJECTED_SETTING(_profile, Name);
Expand Down Expand Up @@ -110,6 +104,12 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
OBSERVABLE_PROJECTED_SETTING(_profile, BellStyle);
OBSERVABLE_PROJECTED_SETTING(_profile, UseAtlasEngine);

WINRT_PROPERTY(bool, IsBaseLayer, false);
WINRT_PROPERTY(IHostedInWindow, WindowRoot, nullptr);
GETSET_BINDABLE_ENUM_SETTING(AntiAliasingMode, Microsoft::Terminal::Control::TextAntialiasingMode, AntialiasingMode);
GETSET_BINDABLE_ENUM_SETTING(CloseOnExitMode, Microsoft::Terminal::Settings::Model::CloseOnExitMode, CloseOnExit);
GETSET_BINDABLE_ENUM_SETTING(ScrollState, Microsoft::Terminal::Control::ScrollbarState, ScrollState);

TYPED_EVENT(DeleteProfile, Editor::ProfileViewModel, Editor::DeleteProfileEventArgs);

private:
Expand Down
19 changes: 1 addition & 18 deletions src/cascadia/TerminalSettingsEditor/Profiles.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,24 +56,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
// Subscribe to some changes in the view model
// These changes should force us to update our own set of "Current<Setting>" members,
// and propagate those changes to the UI
_ViewModelChangedRevoker = _Profile.PropertyChanged(winrt::auto_revoke, [=](auto&&, const PropertyChangedEventArgs& args) {
const auto settingName{ args.PropertyName() };
if (settingName == L"AntialiasingMode")
{
_PropertyChangedHandlers(*this, PropertyChangedEventArgs{ L"CurrentAntiAliasingMode" });
}
else if (settingName == L"CloseOnExit")
{
_PropertyChangedHandlers(*this, PropertyChangedEventArgs{ L"CurrentCloseOnExitMode" });
}
else if (settingName == L"BellStyle")
{
_PropertyChangedHandlers(*this, PropertyChangedEventArgs{ L"IsBellStyleFlagSet" });
}
else if (settingName == L"ScrollState")
{
_PropertyChangedHandlers(*this, PropertyChangedEventArgs{ L"CurrentScrollState" });
}
_ViewModelChangedRevoker = _Profile.PropertyChanged(winrt::auto_revoke, [=](auto&&, auto&&) {
_previewControl.Settings(_Profile.TermSettings());
_previewControl.UpdateSettings();
});
Expand Down
46 changes: 23 additions & 23 deletions src/cascadia/TerminalSettingsEditor/Utils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,29 +51,29 @@
// of EnumEntries so that we may display all possible values of the given
// enum type and its localized names. It also provides a getter and setter
// for the setting we wish to bind to.
#define GETSET_BINDABLE_ENUM_SETTING(name, enumType, settingsModelName, settingNameInModel) \
public: \
winrt::Windows::Foundation::Collections::IObservableVector<winrt::Microsoft::Terminal::Settings::Editor::EnumEntry> name##List() \
{ \
return _##name##List; \
} \
\
winrt::Windows::Foundation::IInspectable Current##name() \
{ \
return winrt::box_value<winrt::Microsoft::Terminal::Settings::Editor::EnumEntry>(_##name##Map.Lookup(settingsModelName.settingNameInModel())); \
} \
\
void Current##name(const winrt::Windows::Foundation::IInspectable& enumEntry) \
{ \
if (auto ee = enumEntry.try_as<winrt::Microsoft::Terminal::Settings::Editor::EnumEntry>()) \
{ \
auto setting = winrt::unbox_value<enumType>(ee.EnumValue()); \
settingsModelName.settingNameInModel(setting); \
} \
} \
\
private: \
winrt::Windows::Foundation::Collections::IObservableVector<winrt::Microsoft::Terminal::Settings::Editor::EnumEntry> _##name##List; \
#define GETSET_BINDABLE_ENUM_SETTING(name, enumType, viewModelSettingGetSet) \
public: \
winrt::Windows::Foundation::Collections::IObservableVector<winrt::Microsoft::Terminal::Settings::Editor::EnumEntry> name##List() \
{ \
return _##name##List; \
} \
\
winrt::Windows::Foundation::IInspectable Current##name() \
{ \
return winrt::box_value<winrt::Microsoft::Terminal::Settings::Editor::EnumEntry>(_##name##Map.Lookup(viewModelSettingGetSet())); \
} \
\
void Current##name(const winrt::Windows::Foundation::IInspectable& enumEntry) \
{ \
if (auto ee = enumEntry.try_as<winrt::Microsoft::Terminal::Settings::Editor::EnumEntry>()) \
{ \
auto setting = winrt::unbox_value<enumType>(ee.EnumValue()); \
viewModelSettingGetSet(setting); \
} \
} \
\
private: \
winrt::Windows::Foundation::Collections::IObservableVector<winrt::Microsoft::Terminal::Settings::Editor::EnumEntry> _##name##List; \
winrt::Windows::Foundation::Collections::IMap<enumType, winrt::Microsoft::Terminal::Settings::Editor::EnumEntry> _##name##Map;

// This macro defines a dependency property for a WinRT class.
Expand Down

0 comments on commit 57ec119

Please sign in to comment.