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 shouldn't be offered as an option in the context menu for Quick Access background #12578

Closed
zadjii-msft opened this issue Feb 25, 2022 · 15 comments · Fixed by #13206 or #14048
Closed
Labels
Area-ShellExtension For issues related to the explorer right-click context menu Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone

Comments

@zadjii-msft
Copy link
Member

From MSFT:37831236

When I right-click on the background of File Explorer "Quick Access," Terminal shouldn't be offered as an option because it doesn't do anything there

Presumably we can filter if we're visible based on the path that was provided to us.

@zadjii-msft zadjii-msft added Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Priority-3 A description (P3) Area-ShellExtension For issues related to the explorer right-click context menu labels Feb 25, 2022
@zadjii-msft zadjii-msft added this to the Backlog milestone Feb 25, 2022
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Feb 25, 2022
@zadjii-msft
Copy link
Member Author

From internal mail:

So, in their IExplorerCommand::GetState, I think they could have a check for that against the IShellItemArray that gets passed in.

bool IsFileSystemItem(IShellItem* shellItem)
{
    SFGAOF attributes;
    return (shellItem->GetAttributes(SFGAO_FILESYSTEM, &attributes) == S_OK);
}

We may also want to compare notes with MSFT:36909951

@zadjii-msft zadjii-msft added good first issue This is a fix that might be easier for someone to do as a first contribution and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Feb 25, 2022
@Shomnipotence
Copy link

Open with... dialog cannot choose the All the Apps from Microsoft Store

@ghost ghost added the In-PR This issue has a related PR label May 30, 2022
@ghost ghost closed this as completed in #13206 May 31, 2022
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels May 31, 2022
ghost pushed a commit that referenced this issue May 31, 2022
This commit hides the "Open in Terminal" context menu option when the
context menu is opened in a non-filesystem path like "Quick Actions".

Closes #12578
DHowett pushed a commit that referenced this issue Jun 30, 2022
This commit hides the "Open in Terminal" context menu option when the
context menu is opened in a non-filesystem path like "Quick Actions".

Closes #12578

(cherry picked from commit fb1491a)
Service-Card-Id: 83524152
Service-Version: 1.14
@ghost
Copy link

ghost commented Jul 6, 2022

🎉This issue was addressed in #13206, which has now been successfully released as Windows Terminal v1.14.186.:tada:

Handy links:

@ghost
Copy link

ghost commented Jul 6, 2022

🎉This issue was addressed in #13206, which has now been successfully released as Windows Terminal Preview v1.15.186.:tada:

Handy links:

@ghost
Copy link

ghost commented Aug 17, 2022

🎉This issue was addressed in #13206, which has now been successfully released as Windows Terminal v1.14.228.:tada:

Handy links:

@ghost
Copy link

ghost commented Aug 17, 2022

🎉This issue was addressed in #13206, which has now been successfully released as Windows Terminal Preview v1.15.228.:tada:

Handy links:

@zadjii-msft
Copy link
Member Author

Lol the bot thought that it should comment here because of the revert 😅

I'm reopening this cause we reverted this to fix #13523. I think there's an OS bug, at least on Windows 11, that doesn't let us know the difference between "My PC" and the desktop background. Both show up with an empty array and a null path from explorer. So that's useless.

@zadjii-msft zadjii-msft reopened this Aug 18, 2022
@zadjii-msft zadjii-msft removed Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. good first issue This is a fix that might be easier for someone to do as a first contribution labels Aug 18, 2022
@zadjii-msft
Copy link
Member Author

Upstream issue is now MSFT:41029797

@leejy12
Copy link
Contributor

leejy12 commented Sep 7, 2022

Regarding #13523

The reason that "Open in Terminal" isn't shown consistently when desktop is right clicked is that on Windows 11, the default context menu is made of fancy XAML controls, and the Window Class name of the foreground window is sometimes reported as XamlExplorerHostIslandWindow, not Progman or WorkerW.

In OpenTerminalHere::_GetPathFromExplorer(), I think we can add an additional check to work around this issue.

if (0 == StrCmp(szName, L"WorkerW") ||
    0 == StrCmp(szName, L"Progman") ||
    0 == StrCmp(szName, L"XamlExplorerHostIslandWindow"))

From #13206 (comment)

Okay I'm pretty sure this caused #13523. Turns out that there's a non-zero rate of the shell giving us 0 items in the array. I can check for that easy enough, but it seems like still returning ECS_HIDDEN for that results in the menu entry not always appearing for the desktop.

I'll need to noodle more

Is there a way to reproduce the case where psiItemArray is not nullptr but psiItemArray->GetCount returns 0? (I assume this is why OpenTerminalHere::GetState() was crashing often). I've never succeeded in observing this behavior.

Also, the PR that originally fixed this issue was reverted in the internal repo, right? Will the revert make its way to this public repo?

@DHowett
Copy link
Member

DHowett commented Sep 7, 2022

Also, the #13206 that originally fixed this issue was reverted in the internal repo, right? Will the revert make its way to this public repo?

This PR The revert was made against and merged into the public repository. There is no private/internal copy of the Windows Terminal source; we do all development right out here in the open! 😄

Edited to correct for me having misread your question. The commit that reverts it is 2d00432 on release-1.14 and 76daf71 on release-1.15. We kept it on in main so we could have builds to repro the issue with.

@DHowett
Copy link
Member

DHowett commented Sep 7, 2022

Good find on the Xaml...Window thing! Do you think that the rest of the explorer lookup logic will work with that HWND as well?

@leejy12
Copy link
Contributor

leejy12 commented Sep 7, 2022

Edited to correct for me having misread your question. The commit that reverts it is 2d00432 on release-1.14 and 76daf71 on release-1.15. We kept it on in main so we could have builds to repro the issue with.

Thanks for the clarification!

Good find on the Xaml...Window thing! Do you think that the rest of the explorer lookup logic will work with that HWND as well?

I think so. Even when the context menu is open in Explorer, the foreground window's class name is always reported as CabinetWClass.

DHowett added a commit that referenced this issue Sep 20, 2022
When we first introduced the shell extension, it didn't work properly
for some folders (such as the Desktop, or perhaps any "background"
click) due to a bug in Windows. We worked around that bug with the help
of an awesome community member, who contributed code that would pull up
the topmost Explorer window and query its location.

That Windows bug was eventually fixed, but we still had trouble with
items appearing correctly. On Windows 11, the Open in Terminal menu item
appears and disappears at random when you right-click the desktop, but
it always appears when you right-click a folder. It sometimes appears
for Quick Access, even though it shouldn't.

We tried to fix that in #13206, but the fix caused more issues than it
solved. We reverted it for 1.15 and 1.16.

At the end of the day, it turns out that getting the path from the
toplevel explorer window is fragile. Fortunately, the shell does offer
us a way to get that information: the site chain.

This pull request replaces GetPathFromExplorer() with an implementation
of `IObjectWithSite`, which allows us to use the site chain to look up
from whence a context menu request was initiated. It also makes item
lookup generally more robust.

✅ Tested on Windows 11
  ✅ Desktop
  ✅ Folder Background
  ✅ Folder Selected
  ✅ Quick Access (does not appear)
  ✅ This PC (does not appear)

References #13206
References #13523
Closes #12578
@ghost ghost added the In-PR This issue has a related PR label Sep 20, 2022
DHowett added a commit that referenced this issue Sep 21, 2022
When we first introduced the shell extension, it didn't work properly
for some folders (such as the Desktop, or perhaps any "background"
click) due to a bug in Windows. We worked around that bug with the help
of an awesome community member, who contributed code that would pull up
the topmost Explorer window and query its location.

That Windows bug was eventually fixed, but we still had trouble with
items appearing correctly. On Windows 11, the Open in Terminal menu item
appears and disappears at random when you right-click the desktop, but
it always appears when you right-click a folder. It sometimes appears
for Quick Access, even though it shouldn't.

We tried to fix that in #13206, but the fix caused more issues than it
solved. We reverted it for 1.15 and 1.16.

At the end of the day, it turns out that getting the path from the
toplevel explorer window is fragile. Fortunately, the shell does offer
us a way to get that information: the site chain.

This pull request replaces GetPathFromExplorer() with an implementation
of `IObjectWithSite`, which allows us to use the site chain to look up
from whence a context menu request was initiated. It also makes item
lookup generally more robust.

* ✅ Tested on Windows 11
  * ✅ Desktop
  * ✅ Folder Background
  * ✅ Folder Selected
  * ✅ Quick Access (does not appear)
  * ✅ This PC (does not appear)
* ✅ Tested on Windows 10
  * ✅ Desktop
  * ✅ Folder Background
  * ✅ Folder Selected
  * ✅ Quick Access (does not appear)
  * ✅ This PC (does not appear)

References #13206
References #13523
Closes #12578

Co-authored-by: John Lueders <johnlue@microsoft.com>
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Needs-Tag-Fix Doesn't match tag requirements and removed In-PR This issue has a related PR labels Sep 21, 2022
DHowett added a commit that referenced this issue Sep 21, 2022
When we first introduced the shell extension, it didn't work properly
for some folders (such as the Desktop, or perhaps any "background"
click) due to a bug in Windows. We worked around that bug with the help
of an awesome community member, who contributed code that would pull up
the topmost Explorer window and query its location.

That Windows bug was eventually fixed, but we still had trouble with
items appearing correctly. On Windows 11, the Open in Terminal menu item
appears and disappears at random when you right-click the desktop, but
it always appears when you right-click a folder. It sometimes appears
for Quick Access, even though it shouldn't.

We tried to fix that in #13206, but the fix caused more issues than it
solved. We reverted it for 1.15 and 1.16.

At the end of the day, it turns out that getting the path from the
toplevel explorer window is fragile. Fortunately, the shell does offer
us a way to get that information: the site chain.

This pull request replaces GetPathFromExplorer() with an implementation
of `IObjectWithSite`, which allows us to use the site chain to look up
from whence a context menu request was initiated. It also makes item
lookup generally more robust.

* ✅ Tested on Windows 11
  * ✅ Desktop
  * ✅ Folder Background
  * ✅ Folder Selected
  * ✅ Quick Access (does not appear)
  * ✅ This PC (does not appear)
* ✅ Tested on Windows 10
  * ✅ Desktop
  * ✅ Folder Background
  * ✅ Folder Selected
  * ✅ Quick Access (does not appear)
  * ✅ This PC (does not appear)

References #13206
References #13523
Closes #12578

Co-authored-by: John Lueders <johnlue@microsoft.com>
(cherry picked from commit 5027c80)
Service-Card-Id: 85788409
Service-Version: 1.16
@ghost
Copy link

ghost commented Sep 23, 2022

🎉This issue was addressed in #14048, which has now been successfully released as Windows Terminal Preview v1.16.2641.0.:tada:

Handy links:

DHowett added a commit that referenced this issue Sep 27, 2022
When we first introduced the shell extension, it didn't work properly
for some folders (such as the Desktop, or perhaps any "background"
click) due to a bug in Windows. We worked around that bug with the help
of an awesome community member, who contributed code that would pull up
the topmost Explorer window and query its location.

That Windows bug was eventually fixed, but we still had trouble with
items appearing correctly. On Windows 11, the Open in Terminal menu item
appears and disappears at random when you right-click the desktop, but
it always appears when you right-click a folder. It sometimes appears
for Quick Access, even though it shouldn't.

We tried to fix that in #13206, but the fix caused more issues than it
solved. We reverted it for 1.15 and 1.16.

At the end of the day, it turns out that getting the path from the
toplevel explorer window is fragile. Fortunately, the shell does offer
us a way to get that information: the site chain.

This pull request replaces GetPathFromExplorer() with an implementation
of `IObjectWithSite`, which allows us to use the site chain to look up
from whence a context menu request was initiated. It also makes item
lookup generally more robust.

* ✅ Tested on Windows 11
  * ✅ Desktop
  * ✅ Folder Background
  * ✅ Folder Selected
  * ✅ Quick Access (does not appear)
  * ✅ This PC (does not appear)
* ✅ Tested on Windows 10
  * ✅ Desktop
  * ✅ Folder Background
  * ✅ Folder Selected
  * ✅ Quick Access (does not appear)
  * ✅ This PC (does not appear)

References #13206
References #13523
Closes #12578

Co-authored-by: John Lueders <johnlue@microsoft.com>
(cherry picked from commit 5027c80)
Service-Card-Id: 85788408
Service-Version: 1.15
@ghost
Copy link

ghost commented Oct 18, 2022

🎉This issue was addressed in #14048, which has now been successfully released as Windows Terminal v1.15.2874.:tada:

Handy links:

@ghost
Copy link

ghost commented Dec 14, 2022

🎉This issue was addressed in #14048, which has now been successfully released as Windows Terminal Preview v1.16.3463.0 and v1.16.3464.0.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-ShellExtension For issues related to the explorer right-click context menu Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
4 participants