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

AtlasEngine: Remove experimental tag and add tracing #13939

Merged
2 commits merged into from
Sep 8, 2022
Merged

AtlasEngine: Remove experimental tag and add tracing #13939

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
8f4997b
AtlasEngine: Remove experimental tag and add tracing
lhecker Sep 7, 2022
bf204bf
Address feedback
lhecker Sep 7, 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 doc/cascadia/profiles.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2237,10 +2237,10 @@
"description": "Use to set a path to a pixel shader to use with the Terminal. Overrides `experimental.retroTerminalEffect`. This is an experimental feature, and its continued existence is not guaranteed.",
"type": "string"
},
"experimental.useAtlasEngine": {
"description": "Enable using the experimental new rendering engine for this profile. This is an experimental feature, and its continued existence is not guaranteed.",
"useAtlasEngine": {
"description": "Windows Terminal 1.16 and later ship with a new, performant text renderer. Set this to false to revert back to the old text renderer.",
"type": "boolean",
"default": false
"default": true
},
"fontFace": {
"default": "Cascadia Mono",
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalSettingsEditor/MainPage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
}
else if (clickedItemTag == renderingTag)
{
contentFrame().Navigate(xaml_typename<Editor::Rendering>(), winrt::make<RenderingViewModel>(_settingsClone.GlobalSettings()));
contentFrame().Navigate(xaml_typename<Editor::Rendering>(), winrt::make<RenderingViewModel>(_settingsClone));
const auto crumb = winrt::make<Breadcrumb>(box_value(clickedItemTag), RS_(L"Nav_Rendering/Content"), BreadcrumbSubPage::None);
_breadcrumbs.Append(crumb);
}
Expand Down
6 changes: 6 additions & 0 deletions src/cascadia/TerminalSettingsEditor/Rendering.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,12 @@
<TextBlock x:Uid="Globals_RenderingDisclaimer"
Style="{StaticResource DisclaimerStyle}" />

<!-- AtlasEngine -->
<local:SettingContainer x:Uid="Profile_UseAtlasEngine">
<ToggleSwitch IsOn="{x:Bind ViewModel.UseAtlasEngine, Mode=TwoWay}"
Style="{StaticResource ToggleSwitchInExpanderStyle}" />
</local:SettingContainer>

<!-- Force Full Repaint -->
<local:SettingContainer x:Uid="Globals_ForceFullRepaint">
<ToggleSwitch IsOn="{x:Bind ViewModel.ForceFullRepaintRendering, Mode=TwoWay}"
Expand Down
4 changes: 2 additions & 2 deletions src/cascadia/TerminalSettingsEditor/RenderingViewModel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ using namespace winrt::Microsoft::Terminal::Settings::Model;

namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
RenderingViewModel::RenderingViewModel(Model::GlobalAppSettings globalSettings) :
_GlobalSettings{ globalSettings }
RenderingViewModel::RenderingViewModel(Model::CascadiaSettings settings) noexcept :
_settings{ std::move(settings) }
{
}
}
11 changes: 5 additions & 6 deletions src/cascadia/TerminalSettingsEditor/RenderingViewModel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,19 @@

#include "RenderingViewModel.g.h"
#include "ViewModelHelpers.h"
#include "Utils.h"

namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
{
struct RenderingViewModel : RenderingViewModelT<RenderingViewModel>, ViewModelHelper<RenderingViewModel>
{
public:
RenderingViewModel(Model::GlobalAppSettings globalSettings);
explicit RenderingViewModel(Model::CascadiaSettings settings) noexcept;

PERMANENT_OBSERVABLE_PROJECTED_SETTING(_GlobalSettings, ForceFullRepaintRendering);
PERMANENT_OBSERVABLE_PROJECTED_SETTING(_GlobalSettings, SoftwareRendering);
PERMANENT_OBSERVABLE_PROJECTED_SETTING(_settings.ProfileDefaults(), UseAtlasEngine);
PERMANENT_OBSERVABLE_PROJECTED_SETTING(_settings.GlobalSettings(), ForceFullRepaintRendering);
PERMANENT_OBSERVABLE_PROJECTED_SETTING(_settings.GlobalSettings(), SoftwareRendering);

private:
Model::GlobalAppSettings _GlobalSettings;
Model::CascadiaSettings _settings{ nullptr };
};
};

Expand Down
3 changes: 2 additions & 1 deletion src/cascadia/TerminalSettingsEditor/RenderingViewModel.idl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,9 @@ namespace Microsoft.Terminal.Settings.Editor
{
runtimeclass RenderingViewModel : Windows.UI.Xaml.Data.INotifyPropertyChanged
{
RenderingViewModel(Microsoft.Terminal.Settings.Model.GlobalAppSettings globalSettings);
RenderingViewModel(Microsoft.Terminal.Settings.Model.CascadiaSettings settings);

PERMANENT_OBSERVABLE_PROJECTED_SETTING(Boolean, UseAtlasEngine);
PERMANENT_OBSERVABLE_PROJECTED_SETTING(Boolean, ForceFullRepaintRendering);
PERMANENT_OBSERVABLE_PROJECTED_SETTING(Boolean, SoftwareRendering);
}
Expand Down
4 changes: 2 additions & 2 deletions src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1075,8 +1075,8 @@
<comment>A description for what the "bell style" setting does. Presented near "Profile_BellStyle".{Locked="BEL"}</comment>
</data>
<data name="Profile_UseAtlasEngine.Header" xml:space="preserve">
<value>Enable experimental text rendering engine</value>
<comment>An option to enable an experimental text rendering engine</comment>
<value>Use the new text renderer ("AtlasEngine")</value>
<comment>{Locked="AtlasEngine"}</comment>
</data>
<data name="Profile_VtPassthrough.Header" xml:space="preserve">
<value>Enable experimental virtual terminal passthrough</value>
Expand Down
19 changes: 19 additions & 0 deletions src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -893,6 +893,25 @@ try
settings->_hash = _calculateHash(settingsString, lastWriteTime);
}

// GH#13936: We're interested in how many users opt out of useAtlasEngine,
// indicating major issues that would require us to disable it by default again.
{
size_t enabled[2]{};
for (const auto& profile : settings->_activeProfiles)
{
enabled[profile.UseAtlasEngine()]++;
}

TraceLoggingWrite(
g_hSettingsModelProvider,
"AtlasEngine_Usage",
TraceLoggingDescription("Event emitted upon settings load, containing the number of profiles opted-in/out of useAtlasEngine"),
TraceLoggingUIntPtr(enabled[0], "UseAtlasEngineDisabled", "Number of profiles for which AtlasEngine is disabled"),
TraceLoggingUIntPtr(enabled[1], "UseAtlasEngineEnabled", "Number of profiles for which AtlasEngine is enabled"),
Copy link
Member

Choose a reason for hiding this comment

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

Do we maybe want to break this down further?

  • Users that don't have the setting at all (!HasUseAtlasEngineDisabled())
  • Users that explicitly opted in
  • Users that explicitly opted out

and maybe a total number of profiles too? Trying to figure out how many folks actually just set this in profiles.defaults might complicate this...

Copy link
Member Author

Choose a reason for hiding this comment

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

We can infer all the first 3 points from the version number alone...
But I think I just realized that might not be added by default to the event data, right? In that case you're right, I'll have to add something like that.

Since the number of profiles is UseAtlasEngineDisabled + UseAtlasEngineEnabled I think we can infer that if needed for sure.

Copy link
Member

Choose a reason for hiding this comment

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

Ah right, we can infer that the user opted out if Disabled > 0. The version is part of the data we automatically get.

Copy link
Member

Choose a reason for hiding this comment

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

Oh so I guess (Num Preview users)-(preview users with it disabled)=(preview users with it enabled, either by default or explicitly). Okay, that makes sense.

TraceLoggingKeyword(MICROSOFT_KEYWORD_MEASURES),
TelemetryPrivacyDataTag(PDT_ProductAndServiceUsage));
Copy link
Member

Choose a reason for hiding this comment

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

In team sync you mentioned a bunch of performance data we wanted to get insights into as well. Atlas resizing, etc.
Did you want to include those in this PR as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd have to figure out how to implement them first and I felt like getting this crucial change in ASAP is much more important than that.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with Leonard on this one. Let's start with simple

}

return *settings;
}
catch (const SettingsException& ex)
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalSettingsModel/MTSMSettings.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ Author(s):
X(CloseOnExitMode, CloseOnExit, "closeOnExit", CloseOnExitMode::Automatic) \
X(hstring, TabTitle, "tabTitle") \
X(Model::BellStyle, BellStyle, "bellStyle", BellStyle::Audible) \
X(bool, UseAtlasEngine, "experimental.useAtlasEngine", Feature_AtlasEngine::IsEnabled()) \
X(bool, UseAtlasEngine, "useAtlasEngine", Feature_AtlasEngine::IsEnabled()) \
X(Windows::Foundation::Collections::IVector<winrt::hstring>, BellSound, "bellSound", nullptr) \
X(bool, Elevate, "elevate", false) \
X(bool, VtPassthrough, "experimental.connection.passthroughMode", false) \
Expand Down