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

TerminalPaneContent.cpp - TerminalPaneContent::TerminalPaneContent never knows the profile settings updated with the wt.exe cmdline arguments #17473

Open
htcfreek opened this issue Jun 25, 2024 · 4 comments
Labels
Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Attention The core contributors need to come back around and look at this ASAP. Needs-Tag-Fix Doesn't match tag requirements

Comments

@htcfreek
Copy link

htcfreek commented Jun 25, 2024

Windows Terminal version

#16497 (which is in sync with main) - PR-Commit: 756d7b3

Windows build number

10.0.19045.4529

Other Software

No response

Steps to reproduce

Try to access the profile configuration used by the current terminal pane content in a version that is updated based on the command line parameter used to start wt.exe .

Expected Behavior

The _profile variable contains the current profile settings from settings file.
And the _cache contains the settings updated based on the wt.exe command line arguments.

Actual Behavior

The _profile variable contains the current profile settings from settings file.
And the _cache contains the current profile settings from settings file too.

Debug information

You can use my PR #16497 and check the value of const auto closeMode in TerminalPaneContent::_controlConnectionStateChangedHandler. It is always the one from settings and the same as the one from _profile.

If you compare the value against the value returned in AppCommandlineArgs::_getNewTerminalArgs you will see that the command line parsing works.

@htcfreek htcfreek added Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jun 25, 2024
@DHowett
Copy link
Member

DHowett commented Jun 25, 2024

This is because it only checks the profile, and there is no feature that override the profile's value for CloseOnExit here. I would somewhat expect a PR that adds --keepOpen support to plumb through overriding the CloseOnExit value in an appropriate way.

Expected Behavior

And the _cache contains the settings updated based on the wt.exe command line arguments.

FWIW: _cache is supposed to be a cache; it is NOT supposed to store novel and important values.

@htcfreek
Copy link
Author

htcfreek commented Jun 26, 2024

I did the exact same as done for other commandline arguments.

Today I tried to implement a new profile property (that is not a setting) for holding the override value. But I failed because I wasn't able to access it in the command line parser logic. (The point is that implementing a setting makes no sense to me as it would be written to/read from json.)

@htcfreek
Copy link
Author

htcfreek commented Jun 26, 2024

I would somewhat expect a PR that adds --keepOpen support to plumb through overriding the CloseOnExit value in an appropriate way.

@DHowett
Wher in the code would you override it? The place wher I override it (TerminalSettings::CreateWithNewTerminalArgs()) doesn't work as I can't acces the overridden value in TerminalPaneContent::_controlConnectionStateChangedHandler().

@PankajBhojwani PankajBhojwani added Needs-Attention The core contributors need to come back around and look at this ASAP. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jun 26, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Jun 26, 2024
@htcfreek
Copy link
Author

@zadjii-msft
I know you have many things to do. But it would be great if you can take a look. (I have not enough c++ knowledge to find the cause of my problem.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Attention The core contributors need to come back around and look at this ASAP. Needs-Tag-Fix Doesn't match tag requirements
Projects
None yet
Development

No branches or pull requests

3 participants