PowerToys icon indicating copy to clipboard operation
PowerToys copied to clipboard

[Settings] Reset Activation Key to Default Value

Open Svenlaa opened this issue 1 year ago • 2 comments

Summary of the Pull Request

  • Add Default value to Properties
  • Add Reset button next to shortcut
  • Clicking the Reset button on Video Conferencing will crash the Settings UI

Screenshot of the new Reset button

PR Checklist

  • [x] Closes: #22568
  • [ ] Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
  • [ ] Tests: Added/updated and all pass
  • [x] Localization: All end user facing strings can be localized
  • [ ] Dev docs: Added/updated
  • [ ] New binaries: Added on the required places
  • [ ] Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Svenlaa avatar May 29 '23 19:05 Svenlaa

@microsoft-github-policy-service agree

Svenlaa avatar May 29 '23 19:05 Svenlaa

I don't like how my implementation of the button looks. Do you have any suggestions on the location or appearance of the button? Should it be placed inside the shortcut menu? Is an "Undo" icon the right label?

Svenlaa avatar May 30 '23 06:05 Svenlaa

I don't like how my implementation of the button looks. Do you have any suggestions on the location or appearance of the button? Should it be placed inside the shortcut menu? Is an "Undo" icon the right label?

I would prefer if the reset button is inside the pop-up which appears when you click the Edit button on the Shortcut. Also the icon should be more like , the current one you have used is more Undo than Reset and people might expect Undo behavior which can be different from Reset.

sanidhyas3s avatar Jun 01 '23 03:06 sanidhyas3s

@svenlaa To keep the UI as clean as possible, would it make sense to have the reset button in the actual shortcut dialog?

niels9001 avatar Jun 04 '23 12:06 niels9001

Personal ideas:

  • I would - at least - place the reset Button inside the same area, in line with the edit button. Moving it to the fly-out is also possible, but that does mean more clicks, which I find not user friendly.
  • The second StackPanel can be removed.
  • The Click action can be moved to the actual edit ~FontIcon~ → Button (in stead of the larger area)

Jay-o-Way avatar Jun 04 '23 12:06 Jay-o-Way

Awesome feature! Thanks a lot for this contribution! Is it ready for review?

jaimecbernardo avatar Jun 06 '23 14:06 jaimecbernardo

Awesome feature! Thanks a lot for this contribution! Is it ready for review?

Not quite. A few other folks have commented on it and I'd like to implement their feedback. I got a few personal things going on, so I won't be available for coding 'till Thursday

Svenlaa avatar Jun 06 '23 19:06 Svenlaa

Thanks for the contribution and the update :) Just press "Ready for review" when you want the core team to review. Thanks a lot, once again.

jaimecbernardo avatar Jun 07 '23 10:06 jaimecbernardo

Personal ideas:

  • I would - at least - place the reset Button inside the same area, in line with the edit button. Moving it to the fly-out is also possible, but that does mean more clicks, which I find not user friendly.

It's a secondary action however, so an additional click might not be that big of a deal? My worry is that we now have 3 buttons next to each other - the shortcut button, reset button and the expand/collapse icon. Users might accidently hit the reset button which would be annoying.

I'd put a reset functionality in the dialog (or next to the title similar to Terminal?)

image

niels9001 avatar Jun 07 '23 14:06 niels9001

Sure, it not that bad. What about Save-Reset-Cancel in one line?

Jay-o-Way avatar Jun 07 '23 14:06 Jay-o-Way

Sure, it not that bad. What about Save-Reset-Cancel in one line?

I really like this one, and it was easier to implement than I thought. I used a "Use Default" label instead of "Reset" which is clearer in my opinion. What should it reset to? The default shortcut

Svenlaa avatar Jun 10 '23 09:06 Svenlaa

I think "use default" and "reset" are both possible. Personally I would go for "reset" because it's nice and short. Also thinking about translation + available space.

@niels9001 is the comment (in shortcutcontrol.cs) about C# versus XAML still valid?

Jay-o-Way avatar Jun 10 '23 10:06 Jay-o-Way

When running/debugging: It looks and works good. When building, I get a few issues. (don't know how much it's related to this branch) image

Try removing the obj and bin folder and/or restart Visual Studio.. weird cache issue you see sometimes, but not related to the branch :).

@Svenlaa do you have an updated screenshot in the main post? I like the 3 button approach!

niels9001 avatar Jun 13 '23 13:06 niels9001

@niels9001. I just updated the screenshot in the main post. Glad you like the approach!

Svenlaa avatar Jun 13 '23 14:06 Svenlaa

Yep, simple and clean solution. 👌🏻 Just hoping it will work when a different language uses long words. 🤞🏻

Jay-o-Way avatar Jun 13 '23 19:06 Jay-o-Way

Merged it. Thank you for the contribution!

jaimecbernardo avatar Jun 20 '23 13:06 jaimecbernardo