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

[PowerRename][ImageResizer] Tier1 Win11 Context menu #19000

Merged
merged 32 commits into from
Jun 30, 2022

Conversation

stefansjfw
Copy link
Collaborator

@stefansjfw stefansjfw commented Jun 23, 2022

Summary of the Pull Request

Add tier1 context menu items (Win11) for PowerRename and ImageResizer:
image

Also, with tier1 context menu PowerRename can be started by right-clicking directory background (no need to select items):
image

PR Checklist

Detailed Description of the Pull Request / Additional comments

Tier1 context menu are added by registering sparse packages for both PowerRename and ImageResizer. Packages are registered per user on module->enable() (conditionally if win11 and if not already registered). Packages are unregistered on PowerToys uninstall.

Also, minimum supported windows version is bumped to 10.0.19041.0 as sparse packaged registration API is added there. Also, end of support for previously used min version (10.0.18362.0) was 2022-05-10.

ImageResizer settings and trace files are extracted to ImageResizerLib project, to be able to consume those both from Ext and ContextMenu projects.

As CreateProcess can't run new proces from packaged app (sparse package here), PowerRename and ImageResuzer are being ran with explorer as parent process. Therefore, additional logic is added to use named pipes in that case (comparing to default stdin anonymous pipes when standard context menu is used).

Validation Steps Performed

  • Build installer and install PowerToys on Win11
  • Sign sparse packages manually in order to be registered successfully
  • Enable PowerRename and ImageResizer modules (if not enabled)
  • Check from PowerShell that packages are registered e.g. powershell -c get-appxpackage -name *imageresizer*
  • If packages are registered and items are not visible in tier1 context menu, explorer needs to be restarted (known issue)

Cpp.Build.props Show resolved Hide resolved
src/common/utils/package.h Outdated Show resolved Hide resolved
@Jay-o-Way
Copy link
Collaborator

  • I see you chose these two items in the main menu — no sub-menu?
  • I suppose this doesn't have any effect on Win.10 at all?

PowerRename can be started by right-clicking directory background

Does this also apply to "special" folders? / is there a check to prevent critical (system/user) folders being renamed?

@stefansjfw
Copy link
Collaborator Author

I see you chose these two items in the main menu — no sub-menu?

The idea is to add tier1 support and keep it as it was in standard context menu for now. If we want to group items in single PowerToys menu, it should be like that for both Win10 and Win11 menus. That would need some additional rework for both logics.

I suppose this doesn't have any effect on Win.10 at all?

No, it doesn't.

Does this also apply to "special" folders? / is there a check to prevent critical (system/user) folders being renamed?

Good point, I'll double check this.

@stefansjfw
Copy link
Collaborator Author

Does this also apply to "special" folders? / is there a check to prevent critical (system/user) folders being renamed?

Done

@lncubus
Copy link
Contributor

lncubus commented Jun 23, 2022

This is a big one. Quite a fix for 19k!

@htcfreek
Copy link
Collaborator

htcfreek commented Jun 28, 2022

@stefansjfw

  • What AUMID will the two packages have?

PowerRenameContextMenu_1.0.0.0_neutral__8wekyb3d8bbwe
ImageResizerContextMenu_1.0.0.0_neutral__8wekyb3d8bbwe

I think something like Microsoft.PowerToys.*** is better. This helps for AppX-Whitelists with regex match and makes admins life easier.
And it is more clear where the packages come from.

@stefansjfw
Copy link
Collaborator Author

I think something like Microsoft.PowerToys.*** is better. This helps for AppX-Whitelists with regex match and makes admins life easier.

Makes sense. Added. Thanks

@stefansjfw stefansjfw force-pushed the stefan/tier1_contextmenu branch 2 times, most recently from ff727c9 to 1fb705f Compare June 28, 2022 14:25
…e error

Don't generate empty STRINGTABLE for resx files without data
@microsoft microsoft deleted a comment from github-actions bot Jun 29, 2022
@microsoft microsoft deleted a comment from github-actions bot Jun 29, 2022
@niels9001
Copy link
Contributor

@stefansjfw Ugh, you can't imagine how much joy this brings me haha :). Great job!

Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

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

LGTM! Great work!
After merging this, please open an issue regarding investigating how to avoid needing to restart explorer for the context menu to recognize the new entries under Windows 11.

@htcfreek
Copy link
Collaborator

@stefansjfw
Can you please open the issue for Terminal plugin too?

@htcfreek
Copy link
Collaborator

@stefansjfw
Can you please open the issue for Terminal plugin too?

And an issue to add package check to bugreport tool.

@stefansjfw
Copy link
Collaborator Author

@stefansjfw
Can you please open the issue for Terminal plugin too?

And an issue to add package check to bugreport tool.

Added package check to bugreport tool

}
catch (...)
{
printf("Failed to report installed context menu packages");
Copy link
Collaborator

@htcfreek htcfreek Jun 30, 2022

Choose a reason for hiding this comment

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

We should catch per package and we should write xyz: not found/registered if a package isn't inatalled. This makes reading/analyzing the bug report easier.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you improve this in a second step?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it's a minor difference.. we only have 2 packages to report. If package is not listed in the file - it's not installed/registered, so as soon as someone open the file it's clear whether all packages are there or not

@stefansjfw
Copy link
Collaborator Author

stefansjfw commented Jun 30, 2022

Created
#19124
#19123

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants