[QuickAccent] Adding Enable/Disable Hotkey #31413

Mahfoudh94 · 2024-02-14T00:01:48Z

Summary of the Pull Request

it adds the ability to enable/disable QuickAccent using a global hotkey without the need to enter the settings every time you don't need this utility (the clip is blocky because of the recorder + conversion to GIF)

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Mahfoudh94 · 2024-02-14T00:04:43Z

@microsoft-github-policy-service agree

…

________________________________

Mahfoudh94 · 2024-02-14T00:36:16Z

Help is welcome as I have a problem formatting src\settings-ui\Settings.UI\SettingsXAML\Views\PowerAccentPage.xaml

Jay-o-Way · 2024-02-14T20:00:05Z

Was it intentional to upgrade winrt? It causes a huge number of file changes. Ref #31396

Mahfoudh94 · 2024-02-14T21:28:42Z

Was it intentional to upgrade winrt?

No, it wasn't, as of one time trying to get into the repositories management system, I pulled again from the main branch which was at the time updated with that winrt thing. As such I reverted the changes at the end, if you know how to remove them entirely from the fork, I'd be thankful for you to tell me how.

Jay-o-Way · 2024-02-14T23:13:50Z

Okay. The merge includes changes from the main branch.

Mahfoudh94 · 2024-02-15T09:41:13Z

It shouldn't have been implemented in my commit, however it got included as a merge, therefor I removed it

Jay-o-Way · 2024-02-15T10:44:39Z

If it were me, I think I would create a new, fresh branch. Then maybe cherry-pick the correct commits(s) to the new one. Or, sometimes when I think a branch/pr is getting too messy, I just start over.
@crutkas your tip?

jaimecbernardo · 2024-02-15T16:05:23Z

Yeah, this is weird. Why is there a revert?
Can you please update your branch to only have the commits that include your changes?

jaimecbernardo · 2024-02-15T16:05:39Z

Thanks for opening this PR, by the way.

jaimecbernardo

Hi @Mahfoudh94 , I've tried reverting your changes to fix the PR. Can you please look at https://github.com/microsoft/PowerToys/pull/31413/files and make sure that's the changes you've intended for the PR?
I'm quite sure that PowerToys.sln and Resources file have unintended changes. Can you please fix those?

Mahfoudh94 · 2024-02-15T16:21:00Z

I would create a new, fresh branch. Then maybe cherry-pick the correct commits(s)

So here comes the cherry-pick handy..., okay I'll give it a try, thanks

Thanks for opening this PR

Don't mention it, it is thanks to you two for being here helping me

as for why the revert, when the new code where implemented from the pull, it raises problems from "Audit deps.json files for all applications" step, you can check it on azure pipelines

jaimecbernardo · 2024-02-15T16:31:34Z

Actually, that "Audit deps.json files for all applications" step was caused by the Visual Studio 17.9 update in the CI. A fix has been done in main already, so I've merged main in, which should fix that.

Mahfoudh94 · 2024-02-15T16:36:14Z

understood, now I should go and cherry-pick only those code changes I've done, then what? open a new pull request or is there a way to update this PR and take it from another branch?
EDIT: I think the last question was a dumb question on my end, like I forgot I was a developer 😅

…" and "Hotkey change requires full restart to take effect"

Mahfoudh94 · 2024-02-15T22:04:28Z

There it is, cleaned and cherry-picked. Hoping it all goes well.

Mahfoudh94 · 2024-02-16T17:16:52Z

@jaimecbernardo, still waiting for your approval, and it happens there is a merge conflict because I added to the strings Resources.resw

Jay-o-Way

First quick comments

Jay-o-Way · 2024-02-17T20:29:20Z

src/settings-ui/Settings.UI.Library/PowerAccentProperties.cs

@@ -9,6 +9,11 @@ namespace Microsoft.PowerToys.Settings.UI.Library
 {
    public class PowerAccentProperties
    {
+        public static readonly HotkeySettings DefaultHotkeyValue = new HotkeySettings(true, false, false, false, 0x2D);


How does this translate to keys?

Yes, I think we're missing a comment here to understand the defaults.

I think it is clear by the name of it, seeing it from my end ofc.

Sorry, what I meant was: which keys are this?

Ah yes 😅, it is (Win + INS)

@Mahfoudh94 , can you please add some code comments to avoid having magic numbers like this that are not understood? Thank you!

Jay-o-Way · 2024-02-17T20:31:13Z

src/settings-ui/Settings.UI/SettingsXAML/Views/PowerAccentPage.xaml

+                        <ToggleSwitch x:Uid="ToggleSwitch" IsOn="{x:Bind ViewModel.IsEnabled, Mode=TwoWay}" />
+                    </tkcontrols:SettingsCard>
+                    <tkcontrols:SettingsCard x:Uid="QuickAccent_Enable_Hotkey" HeaderIcon="{ui:FontIcon Glyph=&#xEDA7;}">
+                        <controls:ShortcutControl MinWidth="{StaticResource SettingActionControlMinWidth}" HotkeySettings="{x:Bind Path=ViewModel.Hotkey, Mode=TwoWay}" />


Path= is unneeded :⁠-⁠)

if you say so, let's get rid of it, it doesn't violate the guidelines anyway XD

@Mahfoudh94 , can you please address this feedback? Thank you!

jaimecbernardo

Thanks for opening the PR and cleaning it up. 😄
I've done a code review now. Can you please address the issues?

jaimecbernardo · 2024-02-27T14:42:02Z

src/modules/poweraccent/PowerAccentModuleInterface/PowerAccentModuleInterface.vcxproj.filters

@@ -47,4 +47,7 @@
  <ItemGroup>
    <None Include="packages.config" />
  </ItemGroup>
+  <ItemGroup>
+    <Natvis Include="$(MSBuildThisFileDirectory)..\..\natvis\wil.natvis" />
+  </ItemGroup>


Why is this file being added?

jaimecbernardo · 2024-02-27T14:42:26Z

src/modules/poweraccent/PowerAccentModuleInterface/PowerAccentModuleInterface.vcxproj

+      <AdditionalDependencies Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">$(CoreLibraryDependencies);%(AdditionalDependencies);winmm.lib</AdditionalDependencies>
+      <AdditionalDependencies Condition="'$(Configuration)|$(Platform)'=='Debug|ARM64'">winmm.lib;%(AdditionalDependencies)</AdditionalDependencies>
+      <AdditionalDependencies Condition="'$(Configuration)|$(Platform)'=='Release|x64'">winmm.lib;$(CoreLibraryDependencies);%(AdditionalDependencies)</AdditionalDependencies>
+      <AdditionalDependencies Condition="'$(Configuration)|$(Platform)'=='Release|ARM64'">winmm.lib;$(CoreLibraryDependencies);%(AdditionalDependencies)</AdditionalDependencies>


Can't all these lines be merged into one?

jaimecbernardo · 2024-02-27T14:46:56Z

src/modules/poweraccent/PowerAccentModuleInterface/dllmain.cpp

+        if (m_enabled)
+        {
+            disable();
+            PlaySound(TEXT("Media\\Windows Notify Messaging.wav"), NULL, SND_FILENAME | SND_ASYNC);


I think it doesn't make sense to have the runner process play a sound.
Although I see that it can't likely be the QuickAccent process because it would need to play while exiting...
Can we turn this into a setting that's turned off by default? Always on top also has this as a setting, for example.

jaimecbernardo · 2024-02-27T14:47:40Z

src/modules/poweraccent/PowerAccentModuleInterface/dllmain.cpp

+            m_hotkey.alt = false;
+            m_hotkey.shift = false;
+            m_hotkey.ctrl = false;
+            m_hotkey.key = 0x2D;


Need a comment here for what 0x2D means as well.

jaimecbernardo · 2024-02-27T14:48:22Z

PowerToys.sln

@@ -577,7 +577,6 @@ Global
 		{9412D5C6-2CF2-4FC2-A601-B55508EA9B27}.Debug|ARM64.ActiveCfg = Debug|ARM64
 		{9412D5C6-2CF2-4FC2-A601-B55508EA9B27}.Debug|ARM64.Build.0 = Debug|ARM64
 		{9412D5C6-2CF2-4FC2-A601-B55508EA9B27}.Debug|x64.ActiveCfg = Debug|x64
-		{9412D5C6-2CF2-4FC2-A601-B55508EA9B27}.Debug|x64.Build.0 = Debug|x64


PowerToys.sln has lots of lines removed. I don't see a reason for this. Can you please fix it?

htcfreek · 2024-09-23T21:15:01Z

@jaimecbernardo
Please check if all review comments are fixed.

Adding QuickAccent Activat/Disactivate functionality

a02f657

Mahfoudh94 marked this pull request as ready for review February 14, 2024 14:53

jaimecbernardo reviewed Feb 15, 2024

View reviewed changes

Mahfoudh94 added 4 commits February 15, 2024 22:48

finishing the implementation: fixing "UI update needs settings reopen…

0415d9c

…" and "Hotkey change requires full restart to take effect"

styling/formating

6cba6d2

linking winmm.lib to PowerAccent Interface

3f2e9cb

fix: revert tool version number change

16b7852

Mahfoudh94 force-pushed the quickaccent_toggle_hotkey branch from 0280686 to 16b7852 Compare February 15, 2024 21:50

Jay-o-Way reviewed Feb 17, 2024

View reviewed changes

Mahfoudh94 mentioned this pull request Feb 17, 2024

Quick accent - global enable/disable hotkey #22205

Open

jaimecbernardo requested changes Feb 27, 2024

View reviewed changes

jaimecbernardo added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 12, 2024

crutkas mentioned this pull request Mar 25, 2024

🚧 Would you like to contribute to PowerToys? 🚧 #28769

Open

Adding QuickAccent Activat/Disactivate functionality

da669b8

Mahfoudh94 requested a review from jaimecbernardo April 26, 2024 00:01

Merge branch 'main' into quickaccent_toggle_hotkey

468502c

htcfreek added Needs-Team-Response An issue author responded so the team needs to follow up Needs-Review This Pull Request awaits the review of a maintainer. and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Sep 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[QuickAccent] Adding Enable/Disable Hotkey #31413

[QuickAccent] Adding Enable/Disable Hotkey #31413

Mahfoudh94 commented Feb 14, 2024

Mahfoudh94 commented Feb 14, 2024 via email •

edited by microsoft-github-policy-service bot

Loading

Mahfoudh94 commented Feb 14, 2024

Jay-o-Way commented Feb 14, 2024 •

edited

Loading

Mahfoudh94 commented Feb 14, 2024 •

edited

Loading

Jay-o-Way commented Feb 14, 2024

Mahfoudh94 commented Feb 15, 2024

Jay-o-Way commented Feb 15, 2024

jaimecbernardo commented Feb 15, 2024

jaimecbernardo commented Feb 15, 2024

jaimecbernardo left a comment

Mahfoudh94 commented Feb 15, 2024

jaimecbernardo commented Feb 15, 2024

Mahfoudh94 commented Feb 15, 2024 •

edited

Loading

Mahfoudh94 commented Feb 15, 2024

Mahfoudh94 commented Feb 16, 2024

Jay-o-Way left a comment

Jay-o-Way Feb 17, 2024

jaimecbernardo Feb 20, 2024

Mahfoudh94 Feb 20, 2024

Jay-o-Way Feb 20, 2024

Mahfoudh94 Feb 20, 2024

jaimecbernardo Feb 27, 2024

Jay-o-Way Feb 17, 2024

Mahfoudh94 Feb 17, 2024 •

edited

Loading

jaimecbernardo Feb 27, 2024

jaimecbernardo left a comment

jaimecbernardo Feb 27, 2024

jaimecbernardo Feb 27, 2024

jaimecbernardo Feb 27, 2024

jaimecbernardo Feb 27, 2024

jaimecbernardo Feb 27, 2024

htcfreek commented Sep 23, 2024

[QuickAccent] Adding Enable/Disable Hotkey #31413

Are you sure you want to change the base?

[QuickAccent] Adding Enable/Disable Hotkey #31413

Conversation

Mahfoudh94 commented Feb 14, 2024

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Mahfoudh94 commented Feb 14, 2024 via email • edited by microsoft-github-policy-service bot Loading

Mahfoudh94 commented Feb 14, 2024

Jay-o-Way commented Feb 14, 2024 • edited Loading

Mahfoudh94 commented Feb 14, 2024 • edited Loading

Jay-o-Way commented Feb 14, 2024

Mahfoudh94 commented Feb 15, 2024

Jay-o-Way commented Feb 15, 2024

jaimecbernardo commented Feb 15, 2024

jaimecbernardo commented Feb 15, 2024

jaimecbernardo left a comment

Choose a reason for hiding this comment

Mahfoudh94 commented Feb 15, 2024

jaimecbernardo commented Feb 15, 2024

Mahfoudh94 commented Feb 15, 2024 • edited Loading

Mahfoudh94 commented Feb 15, 2024

Mahfoudh94 commented Feb 16, 2024

Jay-o-Way left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mahfoudh94 Feb 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaimecbernardo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htcfreek commented Sep 23, 2024

Mahfoudh94 commented Feb 14, 2024 via email •

edited by microsoft-github-policy-service bot

Loading

Jay-o-Way commented Feb 14, 2024 •

edited

Loading

Mahfoudh94 commented Feb 14, 2024 •

edited

Loading

Mahfoudh94 commented Feb 15, 2024 •

edited

Loading

Mahfoudh94 Feb 17, 2024 •

edited

Loading