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

Separate runtime TerminalSettings from profile-TerminalSettings #8602

Merged
19 commits merged into from
Feb 8, 2021
Merged

Separate runtime TerminalSettings from profile-TerminalSettings #8602

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
8b64425
begin
PankajBhojwani Dec 16, 2020
403646f
everything updates except color scheme?
PankajBhojwani Dec 17, 2020
7b5e04e
color scheme fix
PankajBhojwani Dec 17, 2020
21139f5
dead code
PankajBhojwani Dec 17, 2020
7079e60
remove unnecessary comment
PankajBhojwani Dec 17, 2020
1fc3e8b
format
PankajBhojwani Dec 17, 2020
5151a7f
Merge branch 'main' of https://github.com/microsoft/terminal into dev…
PankajBhojwani Dec 18, 2020
8d2c583
Merge branch 'main' of https://github.com/microsoft/terminal into dev…
PankajBhojwani Jan 4, 2021
a288001
pragma warning
PankajBhojwani Jan 4, 2021
ceb1d00
conflict
PankajBhojwani Jan 4, 2021
b57ee23
color table fix
PankajBhojwani Jan 5, 2021
ab36124
fix comment
PankajBhojwani Jan 8, 2021
b9ef9f1
Merge branch 'main' of https://github.com/microsoft/terminal into dev…
PankajBhojwani Jan 22, 2021
54e5123
incomplete
PankajBhojwani Jan 29, 2021
8d27531
comments
PankajBhojwani Feb 5, 2021
efd9176
couple of clarifications left
PankajBhojwani Feb 8, 2021
b29183f
Merge branch 'main' of https://github.com/microsoft/terminal into dev…
PankajBhojwani Feb 8, 2021
8cb6501
fix ambiguous symbols in tests
PankajBhojwani Feb 8, 2021
f402a8c
expose campbell color table
PankajBhojwani Feb 8, 2021
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
2 changes: 1 addition & 1 deletion src/cascadia/TerminalApp/AppActionHandlers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ namespace winrt::TerminalApp::implementation
{
auto controlSettings = activeControl.Settings().as<TerminalSettings>();
controlSettings->ApplyColorScheme(scheme);
activeControl.UpdateSettings(*controlSettings);
activeControl.UpdateSettings();
args.Handled(true);
}
}
Expand Down
8 changes: 7 additions & 1 deletion src/cascadia/TerminalApp/Pane.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -612,7 +612,13 @@ void Pane::UpdateSettings(const TerminalSettings& settings, const GUID& profile)
{
if (profile == _profile)
{
_control.UpdateSettings(settings);
// Update the parent of the control's settings object (and not the object itself) so
// that any overrides made by the control don't get affected by the reload
auto child = winrt::get_self<winrt::TerminalApp::implementation::TerminalSettings>(_control.Settings());
auto parent = winrt::get_self<winrt::TerminalApp::implementation::TerminalSettings>(settings);
child->ClearParents();
child->InsertParent(0, parent->get_strong());
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
_control.UpdateSettings();
}
}
}
Expand Down
4 changes: 3 additions & 1 deletion src/cascadia/TerminalApp/TerminalPage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -739,7 +739,9 @@ namespace winrt::TerminalApp::implementation
}
}

TermControl term{ settings, connection };
// Give term control a child of the settings so that any overrides go in the child
// This way, when we do a settings reload we just update the parent and the overrides remain
TermControl term{ *(winrt::get_self<TerminalSettings>(settings)->CreateChild()), connection };

auto newTabImpl = winrt::make_self<TerminalTab>(profileGuid, term);

Expand Down
99 changes: 47 additions & 52 deletions src/cascadia/TerminalApp/TerminalSettings.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ Author(s):
#pragma once

#include "TerminalSettings.g.h"
#include "../TerminalSettingsModel/IInheritable.h"
Copy link
Member

Choose a reason for hiding this comment

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

(This is a much larger discussion that doesn't have to be implemented as a part of this PR. So really I'm thinking out loud.)
I'm wondering if we should move TerminalSettings over to TerminalSettingsModel. Hear me out:

  • if we're making TerminalSettings IInheritable, might as well move TerminalSettings over to TSM. It's a bit weird for us to grab this random file from a separate project, right? haha
  • With regards to creating a TermControl preview in Settings UI...
    • I hit a problem today. TSE cannot depend on TermApp because that'd create a circular dependency (TermApp <--> TSE). So I tried making it so that TSE builds a TerminalSettings by firing an event to TermApp, but then TermApp would need to somehow get the synthesized TerminalSettings object down to the profile page and update on each change. Having TerminalSettings be its own project or a part of TSM makes it it so much easier to construct TerminalSettings and just have one readily available for a TermControl preview.

What do you think?

CC @DHowett for thoughts on this matter too. I might just be approaching the two things above completely wrong haha

Copy link
Member

Choose a reason for hiding this comment

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

You might have a point here (but it should probably be a follow up)

Copy link
Member

Choose a reason for hiding this comment

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

Bumping this thread for @DHowett because this is important for the terminal preview in SUI.

Copy link
Member

Choose a reason for hiding this comment

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

(Chatted in meeting.)

#include "../inc/cppwinrt_utils.h"
#include <DefaultSettings.h>
#include <conattrs.hpp>
Expand All @@ -27,7 +28,7 @@ namespace TerminalAppLocalTests

namespace winrt::TerminalApp::implementation
{
struct TerminalSettings : TerminalSettingsT<TerminalSettings>
struct TerminalSettings : TerminalSettingsT<TerminalSettings>, winrt::Microsoft::Terminal::Settings::Model::implementation::IInheritable<TerminalSettings>
{
TerminalSettings() = default;
TerminalSettings(const Microsoft::Terminal::Settings::Model::CascadiaSettings& appSettings,
Expand All @@ -40,35 +41,29 @@ namespace winrt::TerminalApp::implementation

void ApplyColorScheme(const Microsoft::Terminal::Settings::Model::ColorScheme& scheme);

// TECHNICALLY, the hstring copy assignment can throw, but the GETSET_PROPERTY
// macro defines the operator as `noexcept`. We're not really worried about it,
// because the only time it will throw is when we're out of memory, and then
// we've got much worse problems. So just suppress that warning for now.
#pragma warning(push)
#pragma warning(disable : 26447)
// --------------------------- Core Settings ---------------------------
// All of these settings are defined in ICoreSettings.

// GetColorTableEntry needs to be implemented manually, to get a
// particular value from the array.
uint32_t GetColorTableEntry(int32_t index) const noexcept;

GETSET_PROPERTY(uint32_t, DefaultForeground, DEFAULT_FOREGROUND_WITH_ALPHA);
GETSET_PROPERTY(uint32_t, DefaultBackground, DEFAULT_BACKGROUND_WITH_ALPHA);
GETSET_PROPERTY(uint32_t, SelectionBackground, DEFAULT_FOREGROUND);
GETSET_PROPERTY(int32_t, HistorySize, DEFAULT_HISTORY_SIZE);
GETSET_PROPERTY(int32_t, InitialRows, 30);
GETSET_PROPERTY(int32_t, InitialCols, 80);
GETSET_SETTING(uint32_t, DefaultForeground, DEFAULT_FOREGROUND_WITH_ALPHA);
GETSET_SETTING(uint32_t, DefaultBackground, DEFAULT_BACKGROUND_WITH_ALPHA);
GETSET_SETTING(uint32_t, SelectionBackground, DEFAULT_FOREGROUND);
GETSET_SETTING(int32_t, HistorySize, DEFAULT_HISTORY_SIZE);
GETSET_SETTING(int32_t, InitialRows, 30);
GETSET_SETTING(int32_t, InitialCols, 80);

GETSET_PROPERTY(bool, SnapOnInput, true);
GETSET_PROPERTY(bool, AltGrAliasing, true);
GETSET_PROPERTY(uint32_t, CursorColor, DEFAULT_CURSOR_COLOR);
GETSET_PROPERTY(Microsoft::Terminal::TerminalControl::CursorStyle, CursorShape, Microsoft::Terminal::TerminalControl::CursorStyle::Vintage);
GETSET_PROPERTY(uint32_t, CursorHeight, DEFAULT_CURSOR_HEIGHT);
GETSET_PROPERTY(hstring, WordDelimiters, DEFAULT_WORD_DELIMITERS);
GETSET_PROPERTY(bool, CopyOnSelect, false);
GETSET_SETTING(bool, SnapOnInput, true);
GETSET_SETTING(bool, AltGrAliasing, true);
GETSET_SETTING(uint32_t, CursorColor, DEFAULT_CURSOR_COLOR);
GETSET_SETTING(Microsoft::Terminal::TerminalControl::CursorStyle, CursorShape, Microsoft::Terminal::TerminalControl::CursorStyle::Vintage);
GETSET_SETTING(uint32_t, CursorHeight, DEFAULT_CURSOR_HEIGHT);
GETSET_SETTING(hstring, WordDelimiters, DEFAULT_WORD_DELIMITERS);
GETSET_SETTING(bool, CopyOnSelect, false);

GETSET_PROPERTY(Windows::Foundation::IReference<uint32_t>, TabColor, nullptr);
GETSET_SETTING(Windows::Foundation::IReference<uint32_t>, TabColor, nullptr);

// When set, StartingTabColor allows to create a terminal with a "sticky" tab color.
// This color is prioritized above the TabColor (that is usually initialized based on profile settings).
Expand All @@ -78,48 +73,48 @@ namespace winrt::TerminalApp::implementation
// TODO: to ensure that this property is not populated during settings reload,
// we should consider moving this property to a separate interface,
// passed to the terminal only upon creation.
GETSET_PROPERTY(Windows::Foundation::IReference<uint32_t>, StartingTabColor, nullptr);
GETSET_SETTING(Windows::Foundation::IReference<uint32_t>, StartingTabColor, nullptr);

// ------------------------ End of Core Settings -----------------------

GETSET_PROPERTY(hstring, ProfileName);
GETSET_PROPERTY(bool, UseAcrylic, false);
GETSET_PROPERTY(double, TintOpacity, 0.5);
GETSET_PROPERTY(hstring, Padding, DEFAULT_PADDING);
GETSET_PROPERTY(hstring, FontFace, DEFAULT_FONT_FACE);
GETSET_PROPERTY(int32_t, FontSize, DEFAULT_FONT_SIZE);
GETSET_SETTING(hstring, ProfileName);
GETSET_SETTING(bool, UseAcrylic, false);
GETSET_SETTING(double, TintOpacity, 0.5);
GETSET_SETTING(hstring, Padding, DEFAULT_PADDING);
GETSET_SETTING(hstring, FontFace, DEFAULT_FONT_FACE);
GETSET_SETTING(int32_t, FontSize, DEFAULT_FONT_SIZE);

GETSET_PROPERTY(winrt::Windows::UI::Text::FontWeight, FontWeight);
GETSET_SETTING(winrt::Windows::UI::Text::FontWeight, FontWeight);

GETSET_PROPERTY(hstring, BackgroundImage);
GETSET_PROPERTY(double, BackgroundImageOpacity, 1.0);
GETSET_SETTING(hstring, BackgroundImage);
GETSET_SETTING(double, BackgroundImageOpacity, 1.0);

GETSET_PROPERTY(winrt::Windows::UI::Xaml::Media::Stretch,
BackgroundImageStretchMode,
winrt::Windows::UI::Xaml::Media::Stretch::UniformToFill);
GETSET_PROPERTY(winrt::Windows::UI::Xaml::HorizontalAlignment,
BackgroundImageHorizontalAlignment,
winrt::Windows::UI::Xaml::HorizontalAlignment::Center);
GETSET_PROPERTY(winrt::Windows::UI::Xaml::VerticalAlignment,
BackgroundImageVerticalAlignment,
winrt::Windows::UI::Xaml::VerticalAlignment::Center);
GETSET_SETTING(winrt::Windows::UI::Xaml::Media::Stretch,
BackgroundImageStretchMode,
winrt::Windows::UI::Xaml::Media::Stretch::UniformToFill);
GETSET_SETTING(winrt::Windows::UI::Xaml::HorizontalAlignment,
BackgroundImageHorizontalAlignment,
winrt::Windows::UI::Xaml::HorizontalAlignment::Center);
GETSET_SETTING(winrt::Windows::UI::Xaml::VerticalAlignment,
BackgroundImageVerticalAlignment,
winrt::Windows::UI::Xaml::VerticalAlignment::Center);

GETSET_PROPERTY(Microsoft::Terminal::TerminalControl::IKeyBindings, KeyBindings, nullptr);
GETSET_SETTING(Microsoft::Terminal::TerminalControl::IKeyBindings, KeyBindings, nullptr);

GETSET_PROPERTY(hstring, Commandline);
GETSET_PROPERTY(hstring, StartingDirectory);
GETSET_PROPERTY(hstring, StartingTitle);
GETSET_PROPERTY(bool, SuppressApplicationTitle);
GETSET_PROPERTY(hstring, EnvironmentVariables);
GETSET_SETTING(hstring, Commandline);
GETSET_SETTING(hstring, StartingDirectory);
GETSET_SETTING(hstring, StartingTitle);
GETSET_SETTING(bool, SuppressApplicationTitle);
GETSET_SETTING(hstring, EnvironmentVariables);

GETSET_PROPERTY(Microsoft::Terminal::TerminalControl::ScrollbarState, ScrollState, Microsoft::Terminal::TerminalControl::ScrollbarState::Visible);
GETSET_SETTING(Microsoft::Terminal::TerminalControl::ScrollbarState, ScrollState, Microsoft::Terminal::TerminalControl::ScrollbarState::Visible);

GETSET_PROPERTY(Microsoft::Terminal::TerminalControl::TextAntialiasingMode, AntialiasingMode, Microsoft::Terminal::TerminalControl::TextAntialiasingMode::Grayscale);
GETSET_SETTING(Microsoft::Terminal::TerminalControl::TextAntialiasingMode, AntialiasingMode, Microsoft::Terminal::TerminalControl::TextAntialiasingMode::Grayscale);

GETSET_PROPERTY(bool, RetroTerminalEffect, false);
GETSET_PROPERTY(bool, ForceFullRepaintRendering, false);
GETSET_PROPERTY(bool, SoftwareRendering, false);
GETSET_PROPERTY(bool, ForceVTInput, false);
GETSET_SETTING(bool, RetroTerminalEffect, false);
GETSET_SETTING(bool, ForceFullRepaintRendering, false);
GETSET_SETTING(bool, SoftwareRendering, false);
GETSET_SETTING(bool, ForceVTInput, false);

GETSET_PROPERTY(hstring, PixelShaderPath);
#pragma warning(pop)
Expand Down
7 changes: 1 addition & 6 deletions src/cascadia/TerminalControl/TermControl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,9 +280,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
// - newSettings: New settings values for the profile in this terminal.
PankajBhojwani marked this conversation as resolved.
Show resolved Hide resolved
// Return Value:
// - <none>
winrt::fire_and_forget TermControl::UpdateSettings(IControlSettings newSettings)
winrt::fire_and_forget TermControl::UpdateSettings()
{
_settings = newSettings;
auto weakThis{ get_weak() };

// Dispatch a call to the UI thread to apply the new settings to the
Expand Down Expand Up @@ -559,10 +558,6 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
{
solidColor.Color(newBgColor);
}

// Set the default background as transparent to prevent the
// DX layer from overwriting the background image or acrylic effect
_settings.DefaultBackground(static_cast<COLORREF>(newBgColor.with_alpha(0)));
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalControl/TermControl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
{
TermControl(IControlSettings settings, TerminalConnection::ITerminalConnection connection);

winrt::fire_and_forget UpdateSettings(IControlSettings newSettings);
winrt::fire_and_forget UpdateSettings();

hstring Title();
hstring GetProfileName() const;
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalControl/TermControl.idl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ namespace Microsoft.Terminal.TerminalControl

static Windows.Foundation.Size GetProposedDimensions(Microsoft.Terminal.TerminalControl.IControlSettings settings, UInt32 dpi);

void UpdateSettings(Microsoft.Terminal.TerminalControl.IControlSettings newSettings);
void UpdateSettings();

Microsoft.Terminal.TerminalControl.IControlSettings Settings { get; };

Expand Down
5 changes: 4 additions & 1 deletion src/cascadia/TerminalCore/Terminal.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,11 @@ void Terminal::CreateFromSettings(ICoreSettings settings,
// - settings: an ICoreSettings with new settings values for us to use.
void Terminal::UpdateSettings(ICoreSettings settings)
{
// Set the default background as transparent to prevent the
// DX layer from overwriting the background image or acrylic effect
til::color newBackgroundColor{ static_cast<COLORREF>(settings.DefaultBackground()) };
_defaultBg = newBackgroundColor.with_alpha(0);
_defaultFg = settings.DefaultForeground();
_defaultBg = settings.DefaultBackground();

CursorType cursorShape = CursorType::VerticalBar;
switch (settings.CursorShape())
Expand Down
5 changes: 5 additions & 0 deletions src/cascadia/TerminalSettingsModel/IInheritable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,11 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
return child;
}

void ClearParents()
{
_parents.clear();
}

void InsertParent(com_ptr<T> parent)
{
_parents.push_back(parent);
Expand Down