Add suppressApplicationTitle as boolean #2814

cinnamon-msft · 2019-09-19T17:33:32Z

Summary of the Pull Request

Add suppressApplicationTitle as a setting

References

PR Checklist

Closes "suppressApplicationTitle" doesn't work (or exist.) #2645
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Requires documentation to be updated
I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

If there's no tabTitle, suppressApplicationTitle does nothing
If suppressApplicationTitle is set to false, tabTitle acts as normal
If they both exist, tabTitle persists as the tab title

src/cascadia/TerminalApp/Profile.cpp

doc/cascadia/SettingsSchema.md

src/cascadia/TerminalApp/TerminalPage.cpp

miniksa

A few questions here

src/cascadia/TerminalApp/Profile.cpp

src/cascadia/TerminalSettings/terminalsettings.h

src/cascadia/TerminalApp/TerminalPage.cpp

ghost · 2019-10-01T16:00:41Z

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

thongtrinh2204 · 2019-10-09T08:41:59Z

hi @cinnamon-msft @zadjii-msft do we have any update on it? i really need this feature for WSL.

ghost · 2019-10-24T14:00:55Z

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

zadjii-msft

Couple questions here - I really don't think that we need startingTitle down in the Terminal core, but I could be wrong

zadjii-msft · 2019-11-18T17:44:53Z

src/cascadia/TerminalCore/TerminalApi.cpp

    _title = title;

+    if (_suppressApplicationTitle)


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...

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.

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 😄

zadjii-msft · 2019-11-18T17:56:43Z

src/cascadia/TerminalCore/Terminal.hpp

@@ -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;


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?

carlos-zamora

Minor nits. Not worth blocking.

carlos-zamora · 2019-11-20T22:45:40Z

src/cascadia/TerminalApp/Profile.cpp

@@ -93,6 +94,7 @@ Profile::Profile(const std::optional<GUID>& guid) :
    _selectionBackground{},
    _colorTable{},
    _tabTitle{},
+    _suppressApplicationTitle{},


Suggested change

_suppressApplicationTitle{},

_suppressApplicationTitle{ false },

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

src/cascadia/UnitTests_TerminalCore/MockTermSettings.h

DHowett-MSFT · 2019-11-20T23:57:08Z

src/cascadia/TerminalCore/TerminalApi.cpp

    _title = title;

+    if (_suppressApplicationTitle)


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 😄

src/cascadia/TerminalCore/TerminalApi.cpp

miniksa

I concur with the one outstanding comment from Mike and Dustin and would like to see the resolution before I sign.

DHowett-MSFT · 2019-11-21T21:58:35Z

My comment no longer applies, and we reverted the code that make it happen because it was actually worse

I'm dismissing myself so I no longer block - I'm thoroughly confused about the state of this re: Dustin's comments, so I'm handing off to him

ghost · 2019-11-26T17:28:13Z

🎉Windows Terminal Preview v0.7.3291.0 has been released which incorporates this pull request.:tada:

Handy links:

cinnamon-msft and others added 2 commits September 17, 2019 22:06

first take at suppressApplicationTitle rewrite

993da81

Rebased tab title fixes

84dbedb

cinnamon-msft requested a review from a team September 19, 2019 17:33

cinnamon-msft mentioned this pull request Sep 19, 2019

Add suppressApplicationTitle setting #2750

Closed

5 tasks

carlos-zamora reviewed Sep 19, 2019

View reviewed changes

src/cascadia/TerminalApp/Profile.cpp Show resolved Hide resolved

cinnamon-msft requested review from DHowett-MSFT, miniksa and zadjii-msft September 20, 2019 17:48

miniksa reviewed Sep 23, 2019

View reviewed changes

doc/cascadia/SettingsSchema.md Outdated Show resolved Hide resolved

miniksa reviewed Sep 23, 2019

View reviewed changes

src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved

miniksa reviewed Sep 23, 2019

View reviewed changes

zadjii-msft previously requested changes Sep 24, 2019

View reviewed changes

src/cascadia/TerminalApp/Profile.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalSettings/terminalsettings.h Show resolved Hide resolved

src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 24, 2019

zadjii-msft mentioned this pull request Sep 25, 2019

A better fix for #tab-titles-are-too-long #2373

Merged

5 tasks

ghost added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Oct 1, 2019

DHowett-MSFT removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. labels Oct 7, 2019

pulled from master

154fb40

cinnamon-msft added 3 commits October 15, 2019 09:56

updated settings doc

bad47df

Merge branch 'master' into cinnamon/suppress-application-title

829b635

Merge branch 'master' into cinnamon/suppress-application-title

d8fa7b3

zadjii-msft added Area-Settings Issues related to settings and customizability, for console or terminal Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something Product-Terminal The new Windows Terminal. labels Oct 24, 2019

ghost added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Oct 24, 2019

cinnamon-msft added the Help Wanted We encourage anyone to jump in on these. label Oct 24, 2019

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 24, 2019

cinnamon-msft added 3 commits November 15, 2019 16:46

code cleanup

aac3f57

merged with master

6b45626

added suppressApplicationTitle to JSON schema

06ffafe

cinnamon-msft requested review from carlos-zamora, zadjii-msft and miniksa November 16, 2019 00:52

cinnamon-msft removed the Help Wanted We encourage anyone to jump in on these. label Nov 16, 2019

cinnamon-msft self-assigned this Nov 16, 2019

more code cleanup

536ddda

zadjii-msft reviewed Nov 18, 2019

View reviewed changes

changed starting title from wstring_view to wstring

9b9c971

cinnamon-msft requested a review from zadjii-msft November 19, 2019 23:42

carlos-zamora approved these changes Nov 20, 2019

View reviewed changes

Formatting fix

5badf2c

DHowett-MSFT suggested changes Nov 20, 2019

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Nov 20, 2019

cinnamon-msft requested a review from DHowett-MSFT November 21, 2019 00:36

DHowett-MSFT reviewed Nov 21, 2019

View reviewed changes

src/cascadia/TerminalCore/TerminalApi.cpp Show resolved Hide resolved

cinnamon-msft force-pushed the cinnamon/suppress-application-title branch from 5d96c7a to 5badf2c Compare November 21, 2019 17:24

cinnamon-msft requested a review from DHowett-MSFT November 21, 2019 17:56

miniksa reviewed Nov 21, 2019

View reviewed changes

miniksa approved these changes Nov 21, 2019

View reviewed changes

Merge branch 'master' into cinnamon/suppress-application-title

8a9a9b0

cinnamon-msft merged commit 99a8337 into master Nov 22, 2019

cinnamon-msft deleted the cinnamon/suppress-application-title branch November 22, 2019 00:18

ghost mentioned this pull request Nov 26, 2019

"suppressApplicationTitle" doesn't work (or exist.) #2645

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add suppressApplicationTitle as boolean #2814

Add suppressApplicationTitle as boolean #2814

cinnamon-msft commented Sep 19, 2019

miniksa left a comment

ghost commented Oct 1, 2019

thongtrinh2204 commented Oct 9, 2019

ghost commented Oct 24, 2019

zadjii-msft left a comment

zadjii-msft Nov 18, 2019

cinnamon-msft Nov 18, 2019

DHowett-MSFT Nov 20, 2019

zadjii-msft Nov 18, 2019

carlos-zamora left a comment

carlos-zamora Nov 20, 2019

cinnamon-msft Nov 20, 2019

DHowett-MSFT Nov 20, 2019

miniksa left a comment

DHowett-MSFT commented Nov 21, 2019

ghost commented Nov 26, 2019

	_suppressApplicationTitle{},
	_suppressApplicationTitle{ false },

Add suppressApplicationTitle as boolean #2814

Add suppressApplicationTitle as boolean #2814

Conversation

cinnamon-msft commented Sep 19, 2019

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

miniksa left a comment

Choose a reason for hiding this comment

ghost commented Oct 1, 2019

thongtrinh2204 commented Oct 9, 2019

ghost commented Oct 24, 2019

zadjii-msft left a comment

Choose a reason for hiding this comment

zadjii-msft Nov 18, 2019

Choose a reason for hiding this comment

cinnamon-msft Nov 18, 2019

Choose a reason for hiding this comment

DHowett-MSFT Nov 20, 2019

Choose a reason for hiding this comment

zadjii-msft Nov 18, 2019

Choose a reason for hiding this comment

carlos-zamora left a comment

Choose a reason for hiding this comment

carlos-zamora Nov 20, 2019

Choose a reason for hiding this comment

cinnamon-msft Nov 20, 2019

Choose a reason for hiding this comment

DHowett-MSFT Nov 20, 2019

Choose a reason for hiding this comment

miniksa left a comment

Choose a reason for hiding this comment

DHowett-MSFT commented Nov 21, 2019

ghost commented Nov 26, 2019