Conversation
zadjii-msft
left a comment
zadjii-msft
left a comment
There was a problem hiding this comment.
I have opinions on this matter and it's 5pm on a friday
ghost
added
the
Needs-Author-Feedback
zadjii-msft
left a comment
zadjii-msft
left a comment
There was a problem hiding this comment.
With #9253, I'm cool with this for preview
This was the only thing blocking me from signing off on #9224 in 1.7. ! CHANGE WARNING ! If we bind to `T.S.M.Command`s in XAML, then the compiler gets _very angry_ at us. It generates two different versions of `GetReferenceTypeMember_Icon` in `XamlTypeInfo.g.cpp`. Presumably because there's an Icon on a NavViewItem and an Icon on a Command. We don't really know why. Fortunately, the fix is "rename Command::Icon" to "Command::IconPath". It's dumb, but it works. Thanks for the help with that one Carlos☺️ Unblocks #9224
| OpenSettingsArgs(const SettingsTarget& target) : | ||
| _Target{ target } {} | ||
| GETSET_PROPERTY(SettingsTarget, Target, SettingsTarget::SettingsFile); | ||
| GETSET_PROPERTY(SettingsTarget, Target, SettingsTarget::SettingsUI); |
There was a problem hiding this comment.
last q: should we leave this the same and only update the target in defaults.json? I'd like to make sure we don't break people who added their own openSettings key bindings.
There was a problem hiding this comment.
You're right. But I don't like that you're right hahaha. Begrudgingly making the change :P
There was a problem hiding this comment.
On a more serious note, should we consider this a breaking change we want to commit to for v2? Or for stable when SUI goes to stable?
There was a problem hiding this comment.
It costs us nothing to let users have their old bindings work the way they always have, right? Here's my thinking. If a user bound settings to open, IN their JSON file, that's probably what they are expecting.
There was a problem hiding this comment.
That's fair. And when the actual Actions page goes in, the user will probably have to explicitly choose what they want to open. So this seems fine :)
There was a problem hiding this comment.
Gah I hate to come in here and disagree after this has merged, but I think moving it to SettingsTarget::SettingsUI makes sense. I know we're going to be breaking people now, but a year from now I feel like we would have wished we moved the target to the settings UI.
There was a problem hiding this comment.
We can make the decision to break peoples' settings in the future.
ghost
removed
the
Needs-Author-Feedback
DHowett
added
the
AutoMerge
|
Hello @DHowett! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
ghost
merged commit 
Fixes a bug introduced by #9224 where the wrong keyboard accelerator would appear in the new tab dropdown. We were looking for the "settings file" version of the action, as opposed to the "settings UI" version. ## References #9224 - Settings UI as default #6800 - Settings UI Epic (cherry picked from commit 649c546)
|
🎉 Handy links: |
4 participants
This makes the settings UI the default settings experience.
As shown below, the following bindings are now default:
The dropdown settings button aligns with this heuristic:
#6800 - Settings UI Epic