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

Jump to bottom

Fix broken reset button on some profile settings #12275

Merged
2 commits merged into from
Jan 28, 2022
Merged

Fix broken reset button on some profile settings #12275

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ce33661
Fix broken reset button on some profile settings
carlos-zamora Jan 27, 2022
aac7bb8
fix unreferenced formal param
carlos-zamora Jan 27, 2022
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
6 changes: 3 additions & 3 deletions src/cascadia/TerminalSettingsEditor/Appearances.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,16 +131,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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
void OnNavigatedTo(const winrt::Windows::UI::Xaml::Navigation::NavigationEventArgs& e);

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
Open in desktop
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
Open in desktop
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
Open in desktop
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")
Copy link
Member

Choose a reason for hiding this comment

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

Just double checking... the property is "Antialiasing" (with the "A" of "aliasing") being lowercase while the Notification string is "AntiAliasing" (with a capital "A")?

It may very well have already been like that and you just copy/pasted... but I'm just checking as it stood out to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I was actually really surprised this worked haha.

{
_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");
}
Comment on lines +118 to +133
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 got moved over from Profiles.cpp.

This is step 2 of my PR description.

});

// Do the same for the starting directory
Expand Down
12 changes: 6 additions & 6 deletions src/cascadia/TerminalSettingsEditor/ProfileViewModel.h
View file
Open in desktop
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);
Comment on lines +109 to +111
Copy link
Member Author

Choose a reason for hiding this comment

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

Had to move this down so that it could find the getters/setters defined in the OBSERVABLE_PROJECTED_SETTING macros above.

Copy link
Member

Choose a reason for hiding this comment

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

Do you want to leave a comment that says "don't shuffle this section up or you're gonna have a bad time?"

Copy link
Member Author

Choose a reason for hiding this comment

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

Eh, if somebody moves them, the errors are pretty clear. So I think it's fine.


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

private:
Expand Down
19 changes: 1 addition & 18 deletions src/cascadia/TerminalSettingsEditor/Profiles.cpp
View file
Open in desktop
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")
Copy link
Member

Choose a reason for hiding this comment

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

Oh yeah look the case mismatch was over here too. Ah well.

{
_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.UpdateControlSettings(_Profile.TermSettings(), _Profile.TermSettings());
});

Expand Down
46 changes: 23 additions & 23 deletions src/cascadia/TerminalSettingsEditor/Utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,29 +49,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); \
} \
} \
Comment on lines +52 to +71
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 change is what I was talking about in step 1 of my PR description.

\
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