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.

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

Make Settings UI the default experience #9224

Merged
3 commits merged into from
Feb 24, 2021
Merged

Conversation

carlos-zamora
Copy link
Member

@carlos-zamora carlos-zamora commented Feb 19, 2021

This makes the settings UI the default settings experience.

As shown below, the following bindings are now default:

  • ctrl+, --> settings ui
  • ctrl+shift+, --> settings.json
  • ctrl+alt+, --> defaults.json

The dropdown settings button aligns with this heuristic:

  • click --> settings ui
  • shift+click --> settings.json
  • alt+click --> defaults.json
  • if alt and shift both pressed, open settings.json

#6800 - Settings UI Epic

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

I have opinions on this matter and it's 5pm on a friday

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Feb 19, 2021
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

With #9253, I'm cool with this for preview

ghost pushed a commit that referenced this pull request Feb 23, 2021
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
@@ -444,7 +444,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
OpenSettingsArgs() = default;
OpenSettingsArgs(const SettingsTarget& target) :
_Target{ target } {}
GETSET_PROPERTY(SettingsTarget, Target, SettingsTarget::SettingsFile);
GETSET_PROPERTY(SettingsTarget, Target, SettingsTarget::SettingsUI);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right. But I don't like that you're right hahaha. Begrudgingly making the change :P

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

We can make the decision to break peoples' settings in the future.

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

Qs

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Feb 23, 2021
@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 24, 2021
@ghost
Copy link

ghost commented Feb 24, 2021

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 35e1168 into main Feb 24, 2021
@ghost ghost deleted the dev/cazamor/sui/make-default branch February 24, 2021 00:14
ghost pushed a commit that referenced this pull request Feb 25, 2021
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
DHowett pushed a commit that referenced this pull request Feb 25, 2021
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)
@ghost
Copy link

ghost commented Mar 1, 2021

🎉Windows Terminal Preview v1.7.572.0 has been released which incorporates this pull request.:tada:

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants