-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Console icon? #11759
Comments
Seems to be a deliberate feature: terminal/src/interactivity/win32/SystemConfigurationProvider.cpp Lines 172 to 188 in 7b77568
The logic there seems somewhat odd though; the comment talks about searching "along the path" but the code calls SearchPathW with only pwszCurrDir as the path. Plus it uses sizeof(wszIconLocation) in two comparisons where ARRAYSIZE would be correct. Filed as #11761. |
I can imagine trying to get an icon from STARTUPINFO::lpTitle if STARTUPINFO::dwFlags contains STARTF_TITLEISLINKNAME (or maybe even STARTF_TITLEISAPPID). Otherwise it makes no sense to me. I wouldn't call it a feature. Of STARUPINFO::lpTitle, the docs say
That hardly tells the whole story. |
@vefatica if you remove the extension, the icon won't be displayed: I don't have the knowledge why it behaves like that! |
In fact, the code posted by @KalleOlaviNiemitalo is done outside the |
I assume it is a counterpart to some code in cmd.exe or kernelbase.dll that uses the file name as the window title. Which then makes me wonder if the "Administrator:" prefix can make this not find the correct icon. Perhaps not, because it still has pwszAppName. |
Back to my first post ... setting the icon to that of notepad is just plain wrong. It's a title. It is not a specification of where to look for an icon. |
honestly, I'm not sure anyone on the team is going to have an answer as to why. This is gonna be one of those "features" of the console that's simply always existed, since before we formally owned the console codebase. I don't think we'd really have an answer other than "Huh, yep, it sure does seem to do that". I'd be extremely reluctant to change that, undoutably someone somewhere is relying on that behavior to set the icon of their console windows for some reason. |
Maybe so, but it doesn't happen in Windows 7. I wish I had more Windows versions to test on.
That's humorous. |
When you've been here as long as us, you can be 100% sure that someone, somewhere, is relying on all sorts of wacky bugs in your codebase 😄 |
Changing this seems unlikely to halt a production line though. Unless they are then calling GetConsoleWindow and extracting the icon from there. Or they are using an UNC path as the title and waking up a network device that way. On the other hand, the current strange use of the title shouldn't really have a business impact, either. |
It is too early for me to be reading code -- this is all wrong. Kept for posterityIt looks like this was an "intent error" (like, we may not have understood the original code.) when we brought the v2 console online! The v1 console loads the icon from:
The v2 console does the exact same thing, except in step 2 it overwrites the resolved app name with the title. Oops. Here's the v2 code- terminal/src/interactivity/win32/SystemConfigurationProvider.cpp Lines 174 to 182 in 7b77568
Note the copy from title into IconLocation just after IconLocation was filled and validated, on line 181. Here's the v1 code- dwLinkLen = RtlDosSearchPath_U(CurDir,
AppName,
NULL,
sizeof(LinkName),
LinkName,
NULL);
if (dwLinkLen > 0 && dwLinkLen < sizeof(LinkName)) {
pszIconLocation = LinkName; // there is no equivalent in v2-- the else block is omitted for linkLen being valid
} else {
pszIconLocation = AppName; |
This bug only occurs when conhost.exe is the default terminal application, and it uses the V2 implementation. It doesn't occur with V1, i.e. when "HKCU\Console\ForceV2" is 0. I don't know why the fallback to the Anyway, the strange fallback code is not actually the bug here. This looks to be a bug in Windows. |
I checked the initialization of |
@vefatica just a question: how do you find this stuff? How did you find this one? |
I can't reconstruct the whole route but it started with my investigating #11704. That led me to mess around with the START command. It is also vaguely connected to a TCC issue wherein the initialization directive "UpdateTitle=No" is not always honored. |
The way I read this, szAppName is probably fully qualified. If it's not fully qualified, SearchPathW is used to resolve it to something fully qualified. It's intentionally not searching the PATH because the real purpose is relative to absolute name resolution. The interesting part in this issue isn't the SearchPathW, it's the fallback to use the title as a path. That code appears to have been added in 2017, before the Github release, but after the code was maintained outside the Windows tree, so I don't have visibility into why it was done. Since it's 2017 I don't think it's true to say that this is ancient behavior that the current console team inherited, or that it will be widely depended on. It's very unclear to me why this would be useful - before that change, the code tried SearchPath, and if that failed, fell back to szAppName, which I understand. Falling back to the title, particularly in preference to szAppName, seems like a curious choice. |
Here you go!
Cherry picked from Pull request !615061. Related work items: MSFT-11750393 If I had to guess: something about app execution aliases made this really wig out. This would be especially true if the AppName/AppPath was to the app execution alias (though I suspect it would not be, given that this data originates from within conclnt after the process has already started.) |
That bug states: Title: Console doesn't pick up the icon of Distro Launcher exe's Body: It is quite terse. 😁 |
Does the current start menu use STARTF_TITLEISLINKNAME? If so, the title value is (re)initialized from the shortcut, and I think my later change in #6277 will fix it, by taking the icon from the shortcut target. If it doesn't, then, uhh, sad. |
Alas. I think this applied to package launches, which don't even have a link and likely don't pass |
At first, it looked like it was supposed to be, but it would have been obvious that it's not had I checked the definition of The app name was intended for the console's per-executable command-line history and aliases. Code was added some versions later to read the icon on the host side by resolving the base executable name against the client's current working directory, i.e.
Falling back to just Using the title sometimes works because |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
It'll be best for us to keep this issue on its original: conhost's erroneous selection of icon based on the title. 😄 All discussion of windows terminal as the default terminal will need to be moved to a new thread where we can triage it independently. |
The funny thing is that way back when we had
Windows 8.1 does not use the icon from the title path. |
This comment was marked as off-topic.
This comment was marked as off-topic.
Hmmm. Okay, so breaking this thread down:
Did I miss anything? I'm down in these weeds as a part of #9458, so this is a prime opportunity to resolve this, if resolvable at all. |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
Does WT use openconsole/conhost under the hood? If not, we could revert the behaviour regressed just for WSL support from conhost/openconsole? As anyone on WSL would also be on WT imo..
@LinHeLurking, this is related to: #12582 #12755 |
I don't know if this has anything to do with conhost, but I can't think of a better place to ask "What's going on here?".
If I give this command to CMD,
start "C:\WINDOWS\system32\notepad.EXE" cmd
, I get a new instance of CMD in a console with notepad's icon. Why does specifying a title affect the icon?The text was updated successfully, but these errors were encountered: