Apply MVVM for global settings in SUI #11853
Conversation
| #include "GlobalSettingsViewModel.g.h" | ||
| #include "Utils.h" | ||
| #include "ViewModelHelpers.h" | ||
| #include "..\TerminalSettingsModel\MTSMSettings.h" |
There was a problem hiding this comment.
I know... I really shouldn't just grab this from TerminalSettingsModel, but it lets me use the X-Macro and it's so nice! Should I move this to inc or something? I feel like it's fine.
There was a problem hiding this comment.
I feel like it's fine. I think the traditional OS code style would be
TerminalSettingsModel/ -> contains the actual soure
- inc/ -> has shared headers like MTSMSettings.h
- lib/ -> builds the .lib
- dll/ -> builds the .dll
but honestly, meh
src/cascadia/TerminalSettingsEditor/GlobalSettingsViewModel.idl
Outdated
Show resolved
Hide resolved
| <ComboBox x:Name="DefaultTerminal" | ||
| ItemsSource="{x:Bind State.Settings.DefaultTerminals}" | ||
| SelectedItem="{x:Bind State.Settings.CurrentDefaultTerminal, Mode=TwoWay}" | ||
| ItemsSource="{x:Bind AppSettings.DefaultTerminals}" | ||
| SelectedItem="{x:Bind AppSettings.CurrentDefaultTerminal, Mode=TwoWay}" |
There was a problem hiding this comment.
This one is weird. We need CascadiaSettings for defterm. But Globals already has a reference to CascadiaSettings. For now, I've been passing it in separately to the navigation state and storing a reference to it on the page. Long term, we should be using CascadiaSettingsViewModel instead, I think. But idk if this abstraction is right. Thoughts?
There was a problem hiding this comment.
meh, this is fine, I don't think it's much worse than it was before
src/cascadia/TerminalSettingsEditor/Microsoft.Terminal.Settings.Editor.vcxproj.filters
Show resolved
Hide resolved
| _NotifyChanges(L"Has" #name, L## #name); \ | ||
| } \ | ||
| } \ | ||
| #define _GET_SETTING_FUNC(target, name) \ |
There was a problem hiding this comment.
A bit annoyed by this change. I wanted to use OBSERVABLE_PROJECTED_SETTING for Default Profile (I think?), but the ClearXXX function wasn't valid. So I had to break things up a bit here.
There was a problem hiding this comment.
wait isn't that why we have the PERMANENT variant of OBSERVABLE...? Like, Name and Guid cannot be cleared.
There was a problem hiding this comment.
yea this is probably the only one I'm not signing off over. Not blocking, over, but it is a little weird
There was a problem hiding this comment.
Sorry, the exact problem is that DefaultProfile only has a getter and a setter (because we do that dance where we get the unparsed default profile, but we always have a default profile). So it doesn't use inheritance at all.
The PERMANENT variant gets rid of ClearXXX, but still uses the HasXXX API. I could introduce a third macro called GET_SET_XXX that would just expose the getter/setter. Idk how valuable that would be though tbh.
|
Closing this PR because we want to make an MVVM class for each page for consistency. @PankajBhojwani will take it over. |
Summary of the Pull Request
Introduces and uses
GlobalSettingsViewModelas the view model for global settings in the settings UI. This makes it so that we don't actually store theXXXNavigationStateon the page objects. Instead, we store the view model. The view model also stores some additional useful information like enum entry lists, current enum entry values, and getters for the settings model.References
#9207 - Apply MVVM
Detailed Description of the Pull Request / Additional comments
Really this just involved me moving stuff into
GlobalSettingsViewModel. There should be no change in functionality.Validation Steps Performed
Changed some global settings and saved/discarded changes. Verified correct behavior from that.