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

suppressApplicationTitle will suppress initial application title #3837

Merged
merged 3 commits into from
Dec 4, 2019

Conversation

cinnamon-msft
Copy link
Contributor

Summary of the Pull Request

Fixed bug where suppressApplicationTitle didn't suppress initial title from application

References

PR Checklist

  • Closes suppressApplicationTitle doesn't seem to work in all cases #3743
  • 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

@cinnamon-msft cinnamon-msft requested a review from a team December 4, 2019 21:34
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

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

One last thing -- and this might actually fix the Azure Cloud Shell issue (!) -- is that you could set the _title to be _startingTitle if _suppressApplicationTitle is enabled in the Terminal::CreateFromSettings function. If you do that, it will get set to the correct value on creation!

@cinnamon-msft
Copy link
Contributor Author

One last thing -- and this might actually fix the Azure Cloud Shell issue (!) -- is that you could set the _title to be _startingTitle if _suppressApplicationTitle is enabled in the Terminal::CreateFromSettings function. If you do that, it will get set to the correct value on creation!

Done!

@cinnamon-msft cinnamon-msft added the Needs-Second It's a PR that needs another sign-off label Dec 4, 2019
@ghost ghost requested review from zadjii-msft, miniksa and carlos-zamora December 4, 2019 22:08
@carlos-zamora carlos-zamora merged commit 54966c3 into master Dec 4, 2019
@carlos-zamora carlos-zamora deleted the cinnamon/suppress-app-title-fix branch December 4, 2019 22:57
if (_suppressApplicationTitle)
{
_title = _startingTitle;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

er, this is not in the right place and will never trigger 😦

It needs to be after line 98, UpdateSettings, because these values are never being set to anything otherwise..

Copy link
Member

Choose a reason for hiding this comment

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

...oops...I should not have merged that huh... Sorry!

Copy link
Contributor

Choose a reason for hiding this comment

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

I shouldn't have Approved with pending "please write new code" comments 😄

@ghost ghost removed the Needs-Second It's a PR that needs another sign-off label Dec 4, 2019
cinnamon-msft added a commit that referenced this pull request Dec 6, 2019
DHowett-MSFT pushed a commit that referenced this pull request Dec 11, 2019
Fixed bug where suppressApplicationTitle didn't suppress initial title from application
Also fixes the Azure Cloud Shell issue.

(cherry picked from commit 54966c3)
DHowett-MSFT pushed a commit that referenced this pull request Dec 11, 2019
@ghost
Copy link

ghost commented Dec 12, 2019

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

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

suppressApplicationTitle doesn't seem to work in all cases
3 participants