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

Add suppressApplicationTitle as boolean #2814

Merged
merged 20 commits into from
Nov 22, 2019
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
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
1 change: 1 addition & 0 deletions doc/cascadia/SettingsSchema.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ Properties listed below are specific to each unique profile.
| `snapOnInput` | Optional | Boolean | `true` | When set to `true`, the window will scroll to the command input line when typing. When set to `false`, the window will not scroll when you start typing. |
| `source` | Optional | String | | Stores the name of the profile generator that originated this profile. _There are no discoverable values for this field._ |
| `startingDirectory` | Optional | String | `%USERPROFILE%` | The directory the shell starts in when it is loaded. |
| `suppressApplicationTitle` | Optional | Boolean | | When set to `true`, `tabTitle` overrides the default title of the tab and any title change messages from the application will be suppressed. When set to `false`, `tabTitle` behaves as normal. |
| `tabTitle` | Optional | String | | If set, will replace the `name` as the title to pass to the shell on startup. Some shells (like `bash`) may choose to ignore this initial value, while others (`cmd`, `powershell`) may use this value over the lifetime of the application. |
| `useAcrylic` | Optional | Boolean | `false` | When set to `true`, the window will have an acrylic background. When set to `false`, the window will have a plain, untextured background. |

Expand Down
4 changes: 4 additions & 0 deletions doc/cascadia/profiles.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -519,6 +519,10 @@
"description": "The directory the shell starts in when it is loaded.",
"type": "string"
},
"suppressApplicationTitle": {
"description": "When set to `true`, `tabTitle` overrides the default title of the tab and any title change messages from the application will be suppressed. When set to `false`, `tabTitle` behaves as normal.",
"type": "boolean"
},
"tabTitle": {
"description": "If set, will replace the name as the title to pass to the shell on startup. Some shells (like bash) may choose to ignore this initial value, while others (cmd, powershell) may use this value over the lifetime of the application.",
"type": "string"
Expand Down
31 changes: 31 additions & 0 deletions src/cascadia/TerminalApp/Profile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ static constexpr std::string_view BackgroundKey{ "background" };
static constexpr std::string_view SelectionBackgroundKey{ "selectionBackground" };
static constexpr std::string_view ColorTableKey{ "colorTable" };
static constexpr std::string_view TabTitleKey{ "tabTitle" };
static constexpr std::string_view SuppressApplicationTitleKey{ "suppressApplicationTitle" };
static constexpr std::string_view HistorySizeKey{ "historySize" };
static constexpr std::string_view SnapOnInputKey{ "snapOnInput" };
static constexpr std::string_view CursorColorKey{ "cursorColor" };
Expand Down Expand Up @@ -93,6 +94,7 @@ Profile::Profile(const std::optional<GUID>& guid) :
_selectionBackground{},
_colorTable{},
_tabTitle{},
_suppressApplicationTitle{},
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_suppressApplicationTitle{},
_suppressApplicationTitle{ false },

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't this only if it's set false as default?

_historySize{ DEFAULT_HISTORY_SIZE },
_snapOnInput{ true },
_cursorColor{ DEFAULT_CURSOR_COLOR },
Expand Down Expand Up @@ -187,6 +189,11 @@ TerminalSettings Profile::CreateTerminalSettings(const std::unordered_map<std::w
// use the profile name
terminalSettings.StartingTitle(_tabTitle ? _tabTitle.value() : _name);

if (_suppressApplicationTitle)
{
terminalSettings.SuppressApplicationTitle(_suppressApplicationTitle);
}

if (_schemeName)
{
const auto found = schemes.find(_schemeName.value());
Expand Down Expand Up @@ -322,6 +329,11 @@ Json::Value Profile::ToJson() const
root[JsonKey(TabTitleKey)] = winrt::to_string(_tabTitle.value());
}

if (_suppressApplicationTitle)
{
root[JsonKey(SuppressApplicationTitleKey)] = _suppressApplicationTitle;
}

if (_startingDirectory)
{
root[JsonKey(StartingDirectoryKey)] = winrt::to_string(_startingDirectory.value());
Expand Down Expand Up @@ -669,6 +681,11 @@ void Profile::LayerJson(const Json::Value& json)
auto useAcrylic{ json[JsonKey(UseAcrylicKey)] };
_useAcrylic = useAcrylic.asBool();
}
if (json.isMember(JsonKey(SuppressApplicationTitleKey)))
{
auto suppressApplicationTitle{ json[JsonKey(SuppressApplicationTitleKey)] };
_suppressApplicationTitle = suppressApplicationTitle.asBool();
}
if (json.isMember(JsonKey(CloseOnExitKey)))
{
auto closeOnExit{ json[JsonKey(CloseOnExitKey)] };
Expand Down Expand Up @@ -774,6 +791,15 @@ void Profile::SetTabTitle(std::wstring tabTitle) noexcept
_tabTitle = std::move(tabTitle);
}

// Method Description
// - Sets if the application title will be suppressed in this profile.
// Arguments:
// - suppressApplicationTitle: boolean
void Profile::SetSuppressApplicationTitle(bool suppressApplicationTitle) noexcept
{
_suppressApplicationTitle = suppressApplicationTitle;
}

// Method Description:
// - Sets this profile's icon path.
// Arguments:
Expand Down Expand Up @@ -811,6 +837,11 @@ std::wstring_view Profile::GetName() const noexcept
return _name;
}

bool Profile::GetSuppressApplicationTitle() const noexcept
{
return _suppressApplicationTitle;
}

bool Profile::HasConnectionType() const noexcept
{
return _connectionType.has_value();
Expand Down
3 changes: 3 additions & 0 deletions src/cascadia/TerminalApp/Profile.h
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ class TerminalApp::Profile final
void SetColorScheme(std::optional<std::wstring> schemeName) noexcept;
std::optional<std::wstring>& GetSchemeName() noexcept;
void SetTabTitle(std::wstring tabTitle) noexcept;
void SetSuppressApplicationTitle(bool suppressApplicationTitle) noexcept;
void SetAcrylicOpacity(double opacity) noexcept;
void SetCommandline(std::wstring cmdline) noexcept;
void SetStartingDirectory(std::wstring startingDirectory) noexcept;
Expand All @@ -83,6 +84,7 @@ class TerminalApp::Profile final
void SetIconPath(std::wstring_view path);

bool GetCloseOnExit() const noexcept;
bool GetSuppressApplicationTitle() const noexcept;
bool IsHidden() const noexcept;

void GenerateGuidIfNecessary() noexcept;
Expand Down Expand Up @@ -119,6 +121,7 @@ class TerminalApp::Profile final
std::optional<uint32_t> _selectionBackground;
std::array<uint32_t, COLOR_TABLE_SIZE> _colorTable;
std::optional<std::wstring> _tabTitle;
bool _suppressApplicationTitle;
int32_t _historySize;
bool _snapOnInput;
uint32_t _cursorColor;
Expand Down
4 changes: 4 additions & 0 deletions src/cascadia/TerminalCore/Terminal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,10 @@ void Terminal::UpdateSettings(winrt::Microsoft::Terminal::Settings::ICoreSetting

_copyOnSelect = settings.CopyOnSelect();

_suppressApplicationTitle = settings.SuppressApplicationTitle();

_startingTitle = settings.StartingTitle();

// TODO:MSFT:21327402 - if HistorySize has changed, resize the buffer so we
// have a smaller scrollback. We should do this carefully - if the new buffer
// size is smaller than where the mutable viewport currently is, we'll want
Expand Down
2 changes: 2 additions & 0 deletions src/cascadia/TerminalCore/Terminal.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -162,12 +162,14 @@ class Microsoft::Terminal::Core::Terminal final :
std::unique_ptr<::Microsoft::Console::VirtualTerminal::TerminalInput> _terminalInput;

std::wstring _title;
std::wstring_view _startingTitle;
Copy link
Member

Choose a reason for hiding this comment

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

I'm mildly concerned about storing this as a wstring_view. Won't this explode when the method that sets this goes out of scope, as the backing wstring gets freed?


std::array<COLORREF, XTERM_COLOR_TABLE_SIZE> _colorTable;
COLORREF _defaultFg;
COLORREF _defaultBg;

bool _snapOnInput;
bool _suppressApplicationTitle;

#pragma region Text Selection
enum class SelectionExpansionMode
Expand Down
14 changes: 13 additions & 1 deletion src/cascadia/TerminalCore/TerminalApi.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -365,11 +365,23 @@ bool Terminal::EraseInDisplay(const DispatchTypes::EraseType eraseType)

bool Terminal::SetWindowTitle(std::wstring_view title)
{
// Set the title on Terminal load
if (_title.empty())
{
_title = title;
_pfnTitleChanged(title);
}

_title = title;

if (_suppressApplicationTitle)
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we just not call _pfnTitleChanged when _suppressApplicationTitle is true? That would negate the need to the Terminal core to know about startingTitle, it would just not send title change events at all.

Maybe the _pfnTitleChanged needs to be called on the first time through this method, but not on subsequent calls...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we don't call _pfnTitleChanged when _suppressApplicationTitle is true, refocusing on the tab/pane within the tab (term.GotFocus() in TerminalPage.cpp) will call that tab's stored title, which is stored as the application title.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like we could just set _title to StartingTitle as early as possible (when we apply the settings), and then we never have to make the check here on 368-373. Then, we can just simplify the code in here to:

if (!_suppressApplicationTitle)
{
    _title = title;
    if (_pfnTitleChanged)
    {
        _pfnTitleChanged(_title)
    }
}
return true;

and that's it. We never update _title, we never check if it's empty (settings always sets it), we don't have to store _startingTitle separately 😄

{
_title = _startingTitle;
}

if (_pfnTitleChanged)
{
_pfnTitleChanged(title);
_pfnTitleChanged(_title);
}

return true;
Expand Down
1 change: 0 additions & 1 deletion src/cascadia/TerminalSettings/IControlSettings.idl
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ namespace Microsoft.Terminal.Settings

String Commandline;
String StartingDirectory;
String StartingTitle;
String EnvironmentVariables;

String BackgroundImage;
Expand Down
2 changes: 2 additions & 0 deletions src/cascadia/TerminalSettings/ICoreSettings.idl
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ namespace Microsoft.Terminal.Settings
UInt32 CursorColor;
CursorStyle CursorShape;
UInt32 CursorHeight;
String StartingTitle;
Boolean SuppressApplicationTitle;
String WordDelimiters;
Boolean CopyOnSelect;
};
Expand Down
10 changes: 10 additions & 0 deletions src/cascadia/TerminalSettings/TerminalSettings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,16 @@ namespace winrt::Microsoft::Terminal::Settings::implementation
_startingTitle = value;
}

bool TerminalSettings::SuppressApplicationTitle()
{
return _suppressApplicationTitle;
}

void TerminalSettings::SuppressApplicationTitle(bool value)
{
_suppressApplicationTitle = value;
}

hstring TerminalSettings::EnvironmentVariables()
{
return _envVars;
Expand Down
4 changes: 4 additions & 0 deletions src/cascadia/TerminalSettings/terminalsettings.h
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,9 @@ namespace winrt::Microsoft::Terminal::Settings::implementation
hstring StartingTitle();
void StartingTitle(hstring const& value);

bool SuppressApplicationTitle();
void SuppressApplicationTitle(bool value);

hstring EnvironmentVariables();
void EnvironmentVariables(hstring const& value);

Expand Down Expand Up @@ -125,6 +128,7 @@ namespace winrt::Microsoft::Terminal::Settings::implementation
hstring _commandline;
hstring _startingDir;
hstring _startingTitle;
bool _suppressApplicationTitle;
hstring _envVars;
Settings::IKeyBindings _keyBindings;
Settings::ScrollbarState _scrollbarState;
Expand Down
7 changes: 6 additions & 1 deletion src/cascadia/UnitTests_TerminalCore/MockTermSettings.h
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ namespace TerminalCoreUnitTests
uint32_t CursorHeight() { return 42UL; }
winrt::hstring WordDelimiters() { return winrt::to_hstring(DEFAULT_WORD_DELIMITERS.c_str()); }
bool CopyOnSelect() { return _copyOnSelect; }
bool SuppressApplicationTitle() { return _suppressApplicationTitle; }
uint32_t SelectionBackground() { return COLOR_WHITE; }

// other implemented methods
Expand All @@ -50,15 +51,19 @@ namespace TerminalCoreUnitTests
void CursorHeight(uint32_t) {}
void WordDelimiters(winrt::hstring) {}
void CopyOnSelect(bool copyOnSelect) { _copyOnSelect = copyOnSelect; }
void SuppressApplicationTitle(bool suppressApplicationTitle) { _suppressApplicationTitle = suppressApplicationTitle; }
void SelectionBackground(uint32_t) {}

// other unimplemented methods
void SetColorTableEntry(int32_t /* index */, uint32_t /* value */) {}
void SetColorTableEntry(int32_t /* index */, uint32_t /* value */)
{
}

private:
int32_t _historySize;
int32_t _initialRows;
int32_t _initialCols;
bool _copyOnSelect{ false };
bool _suppressApplicationTitle{ false };
};
}