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

Terminal doesn't respect "start /max" and "/min" parameters when running a console app as default terminal #12154

Closed
Tracked by #13392
bzzrak opened this issue Jan 13, 2022 · 6 comments
Assignees
Labels
Area-DefApp Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Tag-Fix Doesn't match tag requirements Priority-0 Bugs that we consider release-blocking/recall-class (P0) Product-Terminal The new Windows Terminal.

Comments

@bzzrak
Copy link

bzzrak commented Jan 13, 2022

Windows Terminal version

1.11.3471.0

Windows build number

10.0.22000.434

Other Software

Seemingly any console application (cmd, powershell, python, batch file) -- I'm using python as an example here.

Steps to reproduce

  1. set terminal as default terminal
  2. open command prompt in terminal
  3. run for example "start /max python.exe" (or powershell, or cmd, or some batch file)
  4. the window will show windowed, not maximised

Expected Behavior

New window starts maximised, as it does when the default Windows console is set.
The correct behaviour also happens if you run wt.exe itself, i.e. "start /max wt python"
image
image

Actual Behavior

New window starts windowed
image

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Jan 13, 2022
@bzzrak
Copy link
Author

bzzrak commented Jan 13, 2022

The /min parameter is even worse. "start /min python.exe" starts the classic console, while "start /min wt python.exe" does make a minimised WT window, but you can't switch to it for some reason.

image

image

@bzzrak bzzrak changed the title Terminal doesn't respect "start /max" parameter when running a console app as default terminal Terminal doesn't respect "start /max" and "/min" parameters when running a console app as default terminal Jan 13, 2022
@zadjii-msft
Copy link
Member

zadjii-msft commented Jan 13, 2022

Alright, so there are three elements here. From the bottom up:

  • start /min wt python.exe we're tracking in START /MIN WT? #9053, so feel free to continue that element there.
  • "start /min python.exe opens conhost" This is kinda by design for now. There are a LOT of apps that run commandline tools in "hidden" console windows, so defterm is pretty aggressive about not attaching to those kinds of launches. We don't necessarily want to pop up a Terminal window when a console window wouldn't have been visible to the user.
  • "start /max python.exe doesn't open the Terminal maximized" now, that's a real issue. Or at least, it's something we should have an answer for.

We may need to pass some blob of parameters to defterm invocations describing how the terminal application should be launched. That might take care of both /min and /max.

Lemme add @miniksa, see what he thinks

(fat fingered enter, sorry about that)

@zadjii-msft zadjii-msft added Area-DefApp Needs-Discussion Something that requires a team discussion before we can proceed Product-Terminal The new Windows Terminal. labels Jan 13, 2022
@eryksun
Copy link

eryksun commented Jan 13, 2022

Without knowing the details of what's possible, I'd expect Terminal to honor relevant fields in STARTUPINFO after a handoff from conhost. When a new tab (not a window) is created, I'd expect it to use lpTitle and -- if they're still relevant in any way -- dwXCountChars, dwYCountChars, and dwFillAttribute. When a new window is created, I'd expect it to honor the window's initial show state, position, and size: wShowWindow, dx, dy, dwXSize, and dwYSize. If honoring a setting isn't possible or isn't relevant when using Terminal as the default, please document it for STARTUPINFO, or document it in the console docs.

I understand skipping the handoff to Terminal if wShowWindow is SW_HIDE (0). A hidden Terminal window would be dysfunctional since a console app can't programmatically restore the window, and neither can the user. I don't understand skipping the handoff for SW_SHOWMINIMIZED (2), SW_MINIMIZE (6), SW_SHOWMINNOACTIVE (7), and SW_FORCEMINIMIZE (11). In principle, the user can easily restore a minimized terminal. Maybe this was implemented because Terminal doesn't support wShowWindow properly. For example, as mentioned above, start /min wt is broken. It's better to use the classic console until that gets fixed.

@zadjii-msft zadjii-msft added Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Discussion Something that requires a team discussion before we can proceed labels Jan 31, 2022
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jan 31, 2022
@zadjii-msft zadjii-msft added this to the 22H2 milestone Jan 31, 2022
@miniksa
Copy link
Member

miniksa commented Mar 11, 2022

Without knowing the details of what's possible, I'd expect Terminal to honor relevant fields in STARTUPINFO after a handoff from conhost. When a new tab (not a window) is created, I'd expect it to use lpTitle and -- if they're still relevant in any way -- dwXCountChars, dwYCountChars, and dwFillAttribute. When a new window is created, I'd expect it to honor the window's initial show state, position, and size: wShowWindow, dx, dy, dwXSize, and dwYSize. If honoring a setting isn't possible or isn't relevant when using Terminal as the default, please document it for STARTUPINFO, or document it in the console docs.

I get doing lpTitle. That should be easy even if inserting a tab into an existing window. I guess same for dwFillAttribute.

But for the other ones related to sizing and visibility... I think what I'd do is... if the inbound connection specified any of those, I'd ensure the monarch/peasant granted them a fresh window to manipulate however they specified without tampering with the user's existing window. So they can only "join" automatically if they're not going to mess with what is existing and are flexible with the layout.

I understand skipping the handoff to Terminal if wShowWindow is SW_HIDE (0). A hidden Terminal window would be dysfunctional since a console app can't programmatically restore the window, and neither can the user. I don't understand skipping the handoff for SW_SHOWMINIMIZED (2), SW_MINIMIZE (6), SW_SHOWMINNOACTIVE (7), and SW_FORCEMINIMIZE (11). In principle, the user can easily restore a minimized terminal. Maybe this was implemented because Terminal doesn't support wShowWindow properly. For example, as mentioned above, start /min wt is broken. It's better to use the classic console until that gets fixed.

This was just a matter of scoping it. You're right, Terminal doesn't support wShowWindow completely/properly yet and we hadn't thought through all those scenarios, so the minimum scope to get the "hard part" of the actual handoff out of the way was my goal. Additionally, when I was doing my testing, two things stood out to me: 1. Applications that don't set any of the flags or any settings and ask for a completely blank console session are most commonly destined for an interactive session. 2. Applications that mess with all the flags during the ::CreateProcess call are usually extraordinarily sensitive to changes and performance characteristics, so I didn't want to impact them right away.

@zadjii-msft
Copy link
Member

zadjii-msft commented Jul 14, 2022

Brief notes from 1-1:

  • conhost reads the Cac, decides if it wants to handoff. It does not drain the Cac from the driver
  • openconsole gets handed off to, it reads the driver. The message in flight has the original Cac

so OpenConsole can read the startup info, without rev'ing the defterm interface. We know this works because the OpenConsole does get the title from the STARTUPINFO, so OpenConsole does get that info somehow.

Rev'ing ITerminalHandoff is okay, since that's OpenConsole->WindowsTerminal and that's all in the package.


Verbatims

  • start /max python
  • start minimized? - maybe best we not
  • I'd expect it to use lpTitle
  • I'd expect it to honor the window's initial show state, position, and size: wShowWindow, dx, dy, dwXSize, and dwYSize. If honoring a setting isn't possible or isn't relevant when using Terminal as the default, please document it for STARTUPINFO, or document it in the console docs.
  • I'd ensure the monarch/peasant granted them a fresh window to manipulate however they specified without tampering with the user's existing window. So they can only "join" automatically if they're not going to mess with what is existing and are flexible with the layout.
  • if we get the lnk filename or the exe path from ITerminalHandoff, we can cobble together a fake profile (based on the console settings in the lnk or registry) that "looks-like-" what conhost would have looked like for that lnk or exe
    • font
    • colors (? how will this work with schemes?)
    • cursor
    • icon

Todo's

  • start /max python
  • STARTUP_INFO members:
    • lpTitle
    • wShowWindow
    • dx
    • dy
    • dwXSize
    • dwYSize
    • When x/y/w/h are specified, and the monarch wants to attach to an existing window, instead create a new window with those args.
  • Properties from lnk/reg:
    • font
    • colors (? how will this work with schemes?)
    • cursor
    • icon
      • Probably able to fake with start "C:\WINDOWS\system32\notepad.EXE" cmd

Notes

  • in srvinit.cpp@ConsoleEstablishHandoff, we attempt to handoff->EstablishPtyHandoff to the Terminal. There, we have a PCONSOLE_API_MSG connectMessage we can use.
  • (elsewhere) IoDispatchers::ConsoleHandleConnectionRequest is responsible for unpacking the connection request. It creates a CONSOLE_API_CONNECTINFO via ConsoleInitializeConnectInfo(PCONSOLE_API_MSG,...)
    • srvinit.cpp@ConsoleInitializeConnectInfo shows how we unpack/pack that thing.
    • pay attention to the Cac->ConsoleInfo member, that's a Settings, a la the console settings.
    • srvinit.cpp@ConsoleAllocateConsole shows how we create a conhost with that
    • Especially Settings::ApplyStartupInfo
    • refer to
      // search for the application along the path so that we can load its icons (if we didn't find one explicitly in
      // the shortcut)
      const DWORD dwLinkLen = SearchPathW(pwszCurrDir, pwszAppName, nullptr, ARRAYSIZE(wszIconLocation), wszIconLocation, nullptr);
      // If we cannot find the application in the path, then try to fall back and see if the window title is a valid path and use that.
      if (dwLinkLen <= 0 || dwLinkLen > sizeof(wszIconLocation))
      {
      if (PathFileExistsW(pwszTitle) && (wcslen(pwszTitle) < sizeof(wszIconLocation)))
      {
      StringCchCopyW(wszIconLocation, ARRAYSIZE(wszIconLocation), pwszTitle);
      }
      else
      {
      // If all else fails, just stick the app name into the path and try to resolve just the app name.
      StringCchCopyW(wszIconLocation, ARRAYSIZE(wszIconLocation), pwszAppName);
      }
      }
      for the icon loading
    • OpenConsole!615061

@zadjii-msft zadjii-msft self-assigned this Jul 14, 2022
@zadjii-msft zadjii-msft modified the milestones: 22H2, Terminal v1.16 Jul 14, 2022
@zadjii-msft zadjii-msft added the Priority-0 Bugs that we consider release-blocking/recall-class (P0) label Jul 14, 2022
zadjii-msft added a commit that referenced this issue Aug 26, 2022
This PR by itself doesn't _really_ change much. Technically, now the Terminal will respect the Title of a `.lnk` when started for defterm, but we don't do anything else yet. Primarily, the goal of this PR is to just wire up startup info in OpenConsole to the connected Terminal. 
* This required a bit of changes in `srvinit.cpp:ConsoleEstablishHandoff` to replicate other bits of startup, where we crack open the connect message to get the relevant bits of info.
* We pack that all into a `TERMINAL_STARTUP_INFO`, which we pass along to the registered terminal application.
* `ConptyConnection` accepts the handoff, and gathers that information out of the `TERMINAL_STARTUP_INFO`
* Some other updates to the scratch sln were made to make it build again (related, but unimportant).
* This is a precursor to:
  * #13111
  * #12154
* Closes #9458
* Tested manually
* I work here
DHowett pushed a commit that referenced this issue Oct 14, 2022
This PR by itself doesn't _really_ change much. Technically, now the Terminal will respect the Title of a `.lnk` when started for defterm, but we don't do anything else yet. Primarily, the goal of this PR is to just wire up startup info in OpenConsole to the connected Terminal.
* This required a bit of changes in `srvinit.cpp:ConsoleEstablishHandoff` to replicate other bits of startup, where we crack open the connect message to get the relevant bits of info.
* We pack that all into a `TERMINAL_STARTUP_INFO`, which we pass along to the registered terminal application.
* `ConptyConnection` accepts the handoff, and gathers that information out of the `TERMINAL_STARTUP_INFO`
* Some other updates to the scratch sln were made to make it build again (related, but unimportant).
* This is a precursor to:
  * #13111
  * #12154
* Closes #9458
* Tested manually
* I work here

(cherry picked from commit 7e47f6a)
Service-Card-Id: 86230565
Service-Version: 1.15
ghost pushed a commit that referenced this issue Oct 26, 2022
…enabled (#14222)

## Summary of the Pull Request

- Pipe the `ShowWindow` value through to `ConptyConnection`
- When `TerminalPage` receives the new connection, it checks the `ShowWindow` value and maximizes *IF* there were no other pre-existing tabs (in glomming mode, we don't want to maximize sessions that did not ask for it)

## References
#12154 

## PR Checklist
* [ ] Closes #xxx
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx
* [ ] Schema updated.
* [x] 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
This is just a temporary solution until we change our defterm handoff process. Because of the way the process currently works, we have no way of knowing that the connection has requested the window to be maximized until after we have already started a terminal session. This means that we have to manually maximize the window upon receiving the connection, instead of having the session _start_ maximized, as it probably should. 

## Validation Steps Performed
`start /max python` with defterm enabled opens up python in a maximized WT window
DHowett pushed a commit that referenced this issue Dec 12, 2022
…enabled (#14222)

## Summary of the Pull Request

- Pipe the `ShowWindow` value through to `ConptyConnection`
- When `TerminalPage` receives the new connection, it checks the `ShowWindow` value and maximizes *IF* there were no other pre-existing tabs (in glomming mode, we don't want to maximize sessions that did not ask for it)

## References
#12154

## PR Checklist
* [ ] Closes #xxx
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx
* [ ] Schema updated.
* [x] 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
This is just a temporary solution until we change our defterm handoff process. Because of the way the process currently works, we have no way of knowing that the connection has requested the window to be maximized until after we have already started a terminal session. This means that we have to manually maximize the window upon receiving the connection, instead of having the session _start_ maximized, as it probably should.

## Validation Steps Performed
`start /max python` with defterm enabled opens up python in a maximized WT window

(cherry picked from commit 85ca8f5)
Service-Card-Id: 87207152
Service-Version: 1.16
@zadjii-msft
Copy link
Member

zadjii-msft commented Apr 6, 2023

Note: For discussion:

I'm leaning towards closing this out with the combo of #14222 and #13838. We can pass the startup info into the Terminal now, but I'm reluctant to change the behavior where defterm filters out start /min cli-app.exe invocations. start /min wt cli-app.exe will work, and start /max cli-app.exe will both launch into the terminal, and start /min cli-app.exe will open a minimized conhost running cli-app.exe. As further noted in #14222 (comment)

@zadjii-msft zadjii-msft added the Needs-Discussion Something that requires a team discussion before we can proceed label Apr 6, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Apr 11, 2023
@zadjii-msft zadjii-msft removed the Needs-Discussion Something that requires a team discussion before we can proceed label Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-DefApp Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Tag-Fix Doesn't match tag requirements Priority-0 Bugs that we consider release-blocking/recall-class (P0) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

No branches or pull requests

4 participants