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

[EXPLORER] Don't use Explorers ID_SHELL_CMD IDs in IShellFolder menu #6872

Merged
merged 3 commits into from
May 21, 2024

Conversation

whindsaks
Copy link
Contributor

@whindsaks whindsaks commented May 12, 2024

Explorer does not control the IDs used by the CFSFolder::QueryContext menu and therefore cannot share its ID range with it. The range passed to CFSFolder::QueryContext also cannot start at 0 because CTrayWindow::TrackCtxMenu would interpret that as user cancel.

Note: IDs clashing is configuration specific, it depends on the shell extensions you have installed that adds its items to folders.

Explorer does not control the IDs used by CFSFolder::QueryContext menu and therefore cannot share its ID range with it. The range passed to CFSFolder::QueryContext also cannot start at 0 because CTrayWindow::TrackCtxMenu would interpret that as user cancel.
@github-actions github-actions bot added the shell All PR's related to the shell (and shell extensions) label May 12, 2024
@HBelusca HBelusca changed the title [Explorer] Don't use Explorers ID_SHELL_CMD IDs in IShellFolder menu [EXPLORER] Don't use Explorers ID_SHELL_CMD IDs in IShellFolder menu May 12, 2024
@whindsaks
Copy link
Contributor Author

This commit changed the meaning of ID_SHELL_CMD causing the ranges to overlap. I don't want to go back to the way it was because that means changes to the Explorer bands. Isolating the IDs for IShellFolder seems to be the best option.

@binarymaster binarymaster added the bugfix For bugfix PRs. label May 12, 2024
@HBelusca
Copy link
Contributor

@whindsaks Do you think that the commit you mentioned was trying to be more correct than what was existing prior to it, but didn't go all the way there?

@whindsaks
Copy link
Contributor Author

whindsaks commented May 13, 2024

was trying to be more correct

I think the previous change simply forgot that startctxmnu.cpp was also using the same ID range.

I'm not going to speculate on what is more correct, I can only point out the two common ways ROS gets IContextMenu wrong:

  • You cannot share your ID range with a IShellFolders IContextMenu if you use constant IDs when adding your own items to the menu/interpret them in WM_COMMAND.
  • If you want to display the menu with TPM_RETURNCMD, a IShellFolders IContextMenu cannot receive a 0 idFirstCmd (because 0 means the user cancelled the menu).

Copy link
Contributor

@HBelusca HBelusca left a comment

Choose a reason for hiding this comment

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

looks OK; I just have a minor question about a code block.

return S_OK;
if (idCmdLast - idCmdFirst >= ID_SHELL_CMD_LAST - ID_SHELL_CMD_FIRST)
{
AddStartContextMenuItems(hPopup);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this code block be run even if the CreateContextMenuFromShellFolderPidl() call from above failed (or wasn't executed because either pidlStart was NULL or some of the if-tests failed) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you would then end up with a menu that just has Properties, Separator, Open/Explore All Users.

@whindsaks whindsaks merged commit 43b3280 into reactos:master May 21, 2024
38 checks passed
@whindsaks whindsaks deleted the shexplstartctxmenuopencommon branch May 21, 2024 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix For bugfix PRs. shell All PR's related to the shell (and shell extensions)
Projects
None yet
4 participants