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

File Actions menu #31220

Open
wants to merge 181 commits into
base: main
Choose a base branch
from
Open

File Actions menu #31220

wants to merge 181 commits into from

Conversation

Aaron-Junker
Copy link
Collaborator

@Aaron-Junker Aaron-Junker commented Feb 1, 2024

Summary of the Pull Request

Adding a new menu invokable by selecting files in explorer and pressing a shortcut set in the settings.

Screenshots

image
image
image
image
image
image
image

PR Checklist

Detailed Description of the Pull Request / Additional comments

Things left to do:

  • Add installer
  • Add icon
  • DPI Awareness

Validation Steps Performed

</ui:MenuItem.Icon>
</ui:MenuItem>
<Separator/>
<ui:MenuItem Header="Copy path of files sperated by">

Check failure

Code scanning / check-spelling

Unrecognized Spelling

[sperated](#security-tab) is not a recognized word. \(unrecognized-spelling\)
@Aaron-Junker
Copy link
Collaborator Author

@niels9001 Do you have an idea why the contextmenu doesn't take the WPFUI style?

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

check-spelling found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

This comment has been minimized.

This comment has been minimized.

@Aaron-Junker
Copy link
Collaborator Author

@stefansjfw All your comments are now adressed

Copy link
Collaborator

@stefansjfw stefansjfw left a comment

Choose a reason for hiding this comment

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

Thanks for addressing comments Aaron. Few more comments to resolve and then I'll trigger release CI to give it a final test. Great work so far!

src/gpo/assets/PowerToys.admx Outdated Show resolved Hide resolved
src/gpo/assets/PowerToys.admx Outdated Show resolved Hide resolved
src/gpo/assets/en-US/PowerToys.adml Outdated Show resolved Hide resolved
@@ -25,7 +25,9 @@ public static Frame Frame
{
if (frame == null)
{
#pragma warning disable WinUIEx1001 // The member will always be null.
Copy link
Collaborator

Choose a reason for hiding this comment

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

we are referencing something that will (according to WinUIEx) always be null. If the version bump of WinUIEx introduced this, this can't go in like this. We should make sure that this is safe to do. Hence, my question above...

@Aaron-Junker
Copy link
Collaborator Author

@stefansjfw I adressed your remaining comments. Thank you for the review.

@Aaron-Junker
Copy link
Collaborator Author

@stefansjfw Thank you so much for fixing the issues. Is there anything left I have to do?

@htcfreek
Copy link
Collaborator

@Aaron-Junker
Merging in Main and updating GPO info bar(s) on the settings page.

@stefansjfw
Copy link
Collaborator

@stefansjfw Thank you so much for fixing the issues. Is there anything left I have to do?

Give it some more testing I guess :) will do the same tomorrow. I also asked @niels9001 to revisit icon and screenshots

Comment on lines +33 to +38
<tkcontrols:SettingsCard
x:Uid="Activation_Shortcut"
HeaderIcon="{ui:FontIcon Glyph=&#xEDA7;}"
IsEnabled="{x:Bind ViewModel.FileActionsMenuIsEnabled, Mode=OneWay}">
<controls:ShortcutControl MinWidth="{StaticResource SettingActionControlMinWidth}" HotkeySettings="{x:Bind Path=ViewModel.FileActionsMenuShortcut, Mode=TwoWay}" />
</tkcontrols:SettingsCard>
Copy link
Collaborator

Choose a reason for hiding this comment

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

To be aligned with other pages, shouldn't this be a settings group?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Review This Pull Request awaits the review of a maintainer.
Projects
Status: 📑In Review
Status: No status
Development

Successfully merging this pull request may close these issues.

7 participants