Implement UI for choosing default terminal inside Settings page #9907

miniksa · 2021-04-20T18:12:52Z

Implement dropdown menu for choosing a default terminal application from inside the Windows Terminal Settings UI

PR Checklist

Closes [DefApp] Complete Terminal-side UI chooser of registered defapp #9463
I work here.
Manual tests passed
[Defterm] Document Launch page drop down option to choose default terminal application MicrosoftDocs/terminal#314 (and cross reference [DefApp] Document how it works #9462)

Detailed Description of the Pull Request / Additional comments

Adds dropdown menu and a template card for displaying the available default applications (using the same lookup code as the console property sheet console.dll)
Adds model to TSM for adapting the data for display and binding on XAML
Lookup occurs on every page reload. Persistence only happens on Save Changes.
Manifest changed for Terminal to add capability to opt-out of registry redirection so we can edit this setting

Validation Steps Performed

Flipped the menu and pressed Save Changes and launched cmd from run box... it moved between the two.
Flipped system theme from light to dark and ensured secondary color looked good
Flipped the status with a different mechanism (conhost propsheet) and then reopened settings page and confirmed it loaded the updated status

…ce in the TSM and re-enabling the XAML

src/cascadia/TerminalSettingsModel/DefaultTerminal.cpp

src/cascadia/LocalTests_SettingsModel/SettingsModel.LocalTests.vcxproj

src/cascadia/TerminalSettingsEditor/Launch.xaml

src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp

src/cascadia/TerminalSettingsModel/DefaultTerminal.h

src/propslib/DelegationConfig.cpp

src/cascadia/ut_app/TerminalApp.UnitTests.vcxproj

src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw

src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj

miniksa · 2021-04-21T18:02:49Z

reg.exe

…form.

miniksa · 2021-04-21T23:31:07Z

Check what reg.exe do when key is missing.

DHowett · 2021-04-22T14:45:30Z

src/propslib/DelegationConfig.cpp

+        wil::unique_cotaskmem_string str;
+        RETURN_IF_FAILED(StringFromCLSID(clsid, &str));
+
+        auto regExePath = wil::ExpandEnvironmentStringsW<std::wstring>(L"%WINDIR%\\System32\\reg.exe");


Technically we can GetSystemDirectory (for which wil has a wrapper) -- might be (delta) "easier"

That sounds like making one more call and concatenate.... I think I'll just leave it this way especially since I stole this from another part of our codebase already (and subbed the exe name)

miniksa · 2021-04-22T18:15:53Z

Check what reg.exe do when key is missing.

It just works. Makes the tree of keys and puts the key/value pair in. Good enough.

carlos-zamora

Looks good to me

zadjii-msft

yea these are just nits. Check the theming thing though, and watch out for #9716

zadjii-msft · 2021-04-27T13:26:14Z

src/cascadia/TerminalSettingsModel/DefaultTerminal.cpp

+    return _current.value();
+}
+
+void DefaultTerminal::Current(const Model::DefaultTerminal& term)


nit: I'd maybe add a doc comment here that this will write to the registry. Everything else in this class are all just reads, but this one's a write.

zadjii-msft · 2021-04-27T13:26:58Z

src/cascadia/TerminalSettingsModel/DefaultTerminal.h

+        hstring Version() const;
+        hstring Icon() const;
+
+        static void Refresh();


super nit: this can be private, right?

zadjii-msft · 2021-04-27T13:32:36Z

src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp

+// - <none>
+void CascadiaSettings::CurrentDefaultTerminal(winrt::Microsoft::Terminal::Settings::Model::DefaultTerminal terminal)
+{
+    _currentDefaultTerminal = terminal;


nit: maybe note that this doesn't write the defterm choice to the registry, it just sets the value internally, and that the value is persisted to the reg in CascadiaSettings::WriteSettingsToDisk

zadjii-msft · 2021-04-27T13:33:59Z

src/cascadia/TerminalSettingsEditor/Launch.xaml

+
+                                    <TextBlock Grid.Row="1"
+                                               Grid.Column="1"
+                                               Foreground="{StaticResource SystemBaseMediumColor}"


Does this work in both dark and light mode? What if the OS is set to dark mode, but the app is set to light?

I checked OS set to light and OS set to dark with the app following. If you switch it while it is open, it looks terrible. But if you close and reopen the page it's fine. I wasn't going to cry over that.

I can check and cross-reference the other bug I guess.

src/propslib/DelegationConfig.cpp

src/cascadia/TerminalSettingsModel/DefaultTerminal.cpp

ghost · 2021-04-28T10:43:27Z

Hello @zadjii-msft!

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.

DHowett · 2021-04-28T16:05:03Z

Just to be sure- we didn't fix the truncated version number on the right side, right?

carlos-zamora · 2021-04-28T16:41:36Z

Just to be sure- we didn't fix the truncated version number on the right side, right?

                                    <Grid.ColumnDefinitions>
                                        <!--  icon  -->
                                        <ColumnDefinition Width="24" />
                                        <!--  profile name and author  -->
                                        <ColumnDefinition Width="*" />
                                        <!--  version  -->
                                        <ColumnDefinition Width="48" />
                                    </Grid.ColumnDefinitions>

Hmm... Doesn't look like it. We could add a TextTrimming property to the TextBlock to truncate the version instead of clipping it.

ghost · 2021-05-25T19:11:57Z

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

Handy links:

jantari · 2021-05-25T21:52:35Z

I absolutely love that this possibility is coming, so thanks a ton everyone for all the work on this.

I have a question though, and maybe it's obvious, but I was kind of expecting this setting to be in "Windows Settings -> Apps -> Default apps", which is this menu here:

click to expand large-ish screenshot:

Is that planned at all by / with the Windows team or is it the end-goal to permanently host the setting inside the Windows Terminal settings UI?

ghost · 2021-10-20T17:03:10Z

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

Handy links:

Kein · 2021-10-20T21:28:48Z

If it matters and relevant, I'm running 1.11.211019001-release1.11 on W10-19042.685 and there is no such option in the menu. On first launch of new version no pop-up appeared either.
Terminal was installed manually.

zadjii-msft · 2021-10-20T21:30:28Z

on W10-19042.685

Yep, that's by design. Defterm only works on Windows 11.

DHowett · 2021-10-20T21:31:39Z

AH! Another thing:
Right now, Terminal can only be set as default when it was installed as an app package (either from the store or by double-clicking the msixbundle.)

When Terminal is installed in a directory by extracting the archive (which scoop does!), there are required OS registration steps that get skipped.

Kein · 2021-10-20T21:33:02Z

I see. The release notes were written in such way that it implies the Windows 11 sub-remark was just describing different behavior on Windows 11 and the main bulletpoint applied in general to all supportsed OSes.

DHowett · 2021-10-20T21:33:14Z

Thanks, we should clarify that!

5-Hydroxytryptamin · 2021-11-10T23:51:55Z

Will the defterm functionality be added to Win10 in the future?

zadjii-msft · 2021-11-11T12:52:51Z

Nope. That feature is going to remain a Windows 11-only feature. It's powered by a fairly large feature change to the OS, which is unlikely to be serviced as a "bugfix" to earlier OS versions.

miniksa added 5 commits April 16, 2021 16:22

Start implementing default terminal by loading it into a suitable pla…

9d51bc5

…ce in the TSM and re-enabling the XAML

Get it to work.

26a1df7

Pipe through logo from applications

3827fe8

Move strings to localization

d244012

Use empty version for the OS one instead of looking it up.

0b3d8e5

ghost added Area-DefApp Area-Settings UI Anything specific to the SUI Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Apr 20, 2021

miniksa added 2 commits April 20, 2021 11:14

Spelling things from the appxmanifest.

558dd8a

undo change to profile dropdown

dbad474

This comment has been minimized.

Sign in to view

miniksa commented Apr 20, 2021

View reviewed changes

src/cascadia/TerminalSettingsModel/DefaultTerminal.cpp Show resolved Hide resolved

miniksa self-assigned this Apr 20, 2021

miniksa added 2 commits April 20, 2021 11:20

Add IUri with the other interface APIs.

07334dd

Code format pass.

f16b57a

carlos-zamora reviewed Apr 20, 2021

View reviewed changes

miniksa added 3 commits April 20, 2021 16:29

Refine layout of drop down

13cb2b6

restore carlos's pretty newlines

047feb4

Some PR feedback

0472516

miniksa added 2 commits April 21, 2021 15:24

Only load default terminal choice on OS builds where it should work.

463a853

Do not use resource for version pattern.

2474b83

This comment has been minimized.

Sign in to view

Code format!

e5397e2

This comment has been minimized.

Sign in to view

miniksa added 2 commits April 21, 2021 15:37

Spell check!

606d7b1

Switch registry writes to go through reg.exe to sidestep the app plat…

1c64c76

…form.

DHowett reviewed Apr 22, 2021

View reviewed changes

Oh my goodness spell check and code format.

9ea6484

miniksa marked this pull request as ready for review April 26, 2021 17:59

carlos-zamora approved these changes Apr 26, 2021

View reviewed changes

carlos-zamora unassigned miniksa Apr 27, 2021

carlos-zamora added the Needs-Second It's a PR that needs another sign-off label Apr 27, 2021

ghost requested review from zadjii-msft, leonMSFT, PankajBhojwani and lhecker April 27, 2021 01:05

zadjii-msft approved these changes Apr 27, 2021

View reviewed changes

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 28, 2021

ghost merged commit b7fa328 into main Apr 28, 2021

ghost deleted the dev/miniksa/term_def_ui branch April 28, 2021 10:43

PankajBhojwani mentioned this pull request Apr 30, 2021

Crash in main when writing settings to disk twice #10003

Closed

ghost mentioned this pull request May 25, 2021

[DefApp] Complete Terminal-side UI chooser of registered defapp #9463

Closed

1 task

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement UI for choosing default terminal inside Settings page #9907

Implement UI for choosing default terminal inside Settings page #9907

miniksa commented Apr 20, 2021 •

edited

Loading

This comment has been minimized.

miniksa commented Apr 21, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

miniksa commented Apr 21, 2021

DHowett Apr 22, 2021

miniksa Apr 26, 2021

miniksa commented Apr 22, 2021

carlos-zamora left a comment

zadjii-msft left a comment

zadjii-msft Apr 27, 2021

zadjii-msft Apr 27, 2021

zadjii-msft Apr 27, 2021

zadjii-msft Apr 27, 2021

miniksa Apr 27, 2021

zadjii-msft Apr 27, 2021

ghost commented Apr 28, 2021

DHowett commented Apr 28, 2021

carlos-zamora commented Apr 28, 2021

ghost commented May 25, 2021

jantari commented May 25, 2021

ghost commented Oct 20, 2021

Kein commented Oct 20, 2021

zadjii-msft commented Oct 20, 2021

DHowett commented Oct 20, 2021 •

edited

Loading

Kein commented Oct 20, 2021

DHowett commented Oct 20, 2021

5-Hydroxytryptamin commented Nov 10, 2021

zadjii-msft commented Nov 11, 2021

Implement UI for choosing default terminal inside Settings page #9907

Implement UI for choosing default terminal inside Settings page #9907

Conversation

miniksa commented Apr 20, 2021 • edited Loading

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

This comment has been minimized.

miniksa commented Apr 21, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

miniksa commented Apr 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miniksa commented Apr 22, 2021

carlos-zamora left a comment

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Apr 28, 2021

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.

DHowett commented Apr 28, 2021

carlos-zamora commented Apr 28, 2021

ghost commented May 25, 2021

jantari commented May 25, 2021

ghost commented Oct 20, 2021

Kein commented Oct 20, 2021

zadjii-msft commented Oct 20, 2021

DHowett commented Oct 20, 2021 • edited Loading

Kein commented Oct 20, 2021

DHowett commented Oct 20, 2021

5-Hydroxytryptamin commented Nov 10, 2021

zadjii-msft commented Nov 11, 2021

miniksa commented Apr 20, 2021 •

edited

Loading

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.

DHowett commented Oct 20, 2021 •

edited

Loading