-
Notifications
You must be signed in to change notification settings - Fork 7.4k
Shortcut Guide V2 #40834
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
base: main
Are you sure you want to change the base?
Shortcut Guide V2 #40834
Conversation
else if (wcscmp(class_name, L"Shell_SecondaryTrayWnd") == 0) | ||
{ | ||
// Secondary taskbar structure | ||
HWND workerw = FindWindowExW(taskbar_hwnd, 0, L"WorkerW", nullptr); |
Check failure
Code scanning / check-spelling
Unrecognized Spelling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can rename the variable like worker_hwnd?
{ | ||
// Secondary taskbar structure | ||
HWND workerw = FindWindowExW(taskbar_hwnd, 0, L"WorkerW", nullptr); | ||
if (!workerw) |
Check failure
Code scanning / check-spelling
Unrecognized Spelling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can rename the variable like worker_hwnd?
HWND workerw = FindWindowExW(taskbar_hwnd, 0, L"WorkerW", nullptr); | ||
if (!workerw) | ||
return; | ||
tasklist_hwnd = FindWindowExW(workerw, 0, L"MSTaskListWClass", nullptr); |
Check failure
Code scanning / check-spelling
Unrecognized Spelling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can rename the variable like worker_hwnd?
using ShortcutGuide.Models; | ||
using YamlDotNet.Serialization; | ||
|
||
// This should all be outsourced to WinGet in the future |
Check failure
Code scanning / check-spelling
Unrecognized Spelling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's pick other words.
Alt: false | ||
Keys: | ||
- "/" | ||
- Name: Open Gamebar |
Check failure
Code scanning / check-spelling
Unrecognized Spelling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we make it as "Open Game Bar", may be the spelling check will be happier.
And I see its official name is two words as well.
} | ||
|
||
[GeneratedRegex(@"# <Populate start>[\s\S\n\r]*# <Populate end>")] | ||
private static partial Regex PoulateRegex(); |
Check failure
Code scanning / check-spelling
Unrecognized Spelling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is just a typo, should rename to PopulateRegex.
{ | ||
internal struct ShortcutPageParameters | ||
{ | ||
public static SeachFilterObservable SearchFilter = new(); |
Check failure
Code scanning / check-spelling
Unrecognized Spelling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be typo? It should be SearchFilterObservable.
|
||
public static FrameHeightObservable FrameHeight = new(); | ||
|
||
internal sealed class SeachFilterObservable |
Check failure
Code scanning / check-spelling
Unrecognized Spelling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be typo? It should be SearchFilterObservable.
</data> | ||
<data name="PinShortcut" xml:space="preserve"> | ||
<value>Pin</value> | ||
<comment>Refers to the action of pinning something on a pinboard</comment> |
Check failure
Code scanning / check-spelling
Unrecognized Spelling
</data> | ||
<data name="UnpinShortcut" xml:space="preserve"> | ||
<value>Unpin</value> | ||
<comment>Refers to the action of unpinning something from a pinboard</comment> |
Check failure
Code scanning / check-spelling
Unrecognized Spelling
…t/PowerToys into feature/shortcutguidev2
src/modules/ShortcutGuide/ShortcutGuide.CPPProject/tasklist_positions.cpp
Fixed
Show fixed
Hide fixed
src/modules/ShortcutGuide/ShortcutGuide.CPPProject/tasklist_positions.cpp
Fixed
Show fixed
Hide fixed
src/modules/ShortcutGuide/ShortcutGuide.CPPProject/tasklist_positions.cpp
Fixed
Show fixed
Hide fixed
src/modules/ShortcutGuide/ShortcutGuide.IndexYmlGenerator/IndexYmlGenerator.cs
Fixed
Show fixed
Hide fixed
src/modules/ShortcutGuide/ShortcutGuide.Ui/Assets/ShortcutGuide/+WindowsNT.Shell.en-US.yml
Fixed
Show fixed
Hide fixed
src/modules/ShortcutGuide/ShortcutGuide.Ui/Helpers/PowerToysShortcutsPopulator.cs
Fixed
Show fixed
Hide fixed
src/modules/ShortcutGuide/ShortcutGuide.Ui/Models/ShortcutPageParameters.cs
Fixed
Show fixed
Hide fixed
src/modules/ShortcutGuide/ShortcutGuide.Ui/Models/ShortcutPageParameters.cs
Fixed
Show fixed
Hide fixed
This comment has been minimized.
This comment has been minimized.
…t/PowerToys into feature/shortcutguidev2
…t/PowerToys into feature/shortcutguidev2
This comment has been minimized.
This comment has been minimized.
…r pages then Windows -> Overview and fix height of Windows -> Overview
2dbc631
to
e7582eb
Compare
@noraa-junker, I rebased this with latest main. While, this PR will conflict with another two PRs that will be going in soon.
Given we want to cut the build at the end of this week for 0.94, the chance we resolve all the conflict will really need extra effort in case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shawn will help further on review, since his PR on shortcut conflict detection (for PowerToys itself + system key) should have many commonality and knowledge on code change.
|
||
content = new(PopulateRegex().Replace(content.ToString(), populateStartString + Environment.NewLine)); | ||
|
||
ISettingsUtils settingsUtils = new SettingsUtils(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shuaiyuanxx, I believe the following code can be replace with your one?
Surprisingly or not, PowerToys has many "naming conversion" to work correctly, even they defined in seperate file, e.g.
PowerToys/src/settings-ui/Settings.UI.Library/Utilities/GetSettingCommandLineCommand.cs
Line 60 in 8737de2
moduleSettings.Add("Enabled", typeof(EnabledModules).GetProperty(moduleName).GetValue(enabledModules)); |
Actually the EnabledModules' PropertyName should be the same as the Setting's ModuleName.
BTW, I see it need two resource string below for each, why we only need to return one in our interface in the shortcut conflict detection one? (I see below one is the Module's name, one is the shortcut's name)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file can be simplified by leveraging the ModuleHelper class. It provides method for checking the module is enabled or not, and can get the module header.
} | ||
|
||
[GeneratedRegex(@"# <Populate start>[\s\S\n\r]*# <Populate end>")] | ||
private static partial Regex PoulateRegex(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is just a typo, should rename to PopulateRegex.
Alt: false | ||
Keys: | ||
- "/" | ||
- Name: Open Gamebar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we make it as "Open Game Bar", may be the spelling check will be happier.
And I see its official name is two words as well.
Shift: false | ||
Alt: false | ||
Keys: | ||
- "<PrtSc>" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it PrtScn?
const string populateStartString = "# <Populate start>"; | ||
const string populateEndString = "# <Populate end>"; | ||
|
||
content = PoulateRegex().Replace(content, populateStartString + Environment.NewLine); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is just a typo, should rename to PopulateRegex.
VerticalAlignment = VerticalAlignment.Center, | ||
|
||
// Use monospaced font to ensure the text doesn't move | ||
FontFamily = new("Courier New"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@niels9001, any suggestion here?
// Todo: Take the default shell name from the settings or environment variable, default to "+WindowsNT.Shell" | ||
indexFile.DefaultShellName = "+WindowsNT.Shell"; | ||
|
||
Serializer serializer = new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the search logic now.
e.g.
I go to PowerToys General, search for "H"
It shows me 3 results, but I think only first one one make sense?
And then I try to search for "P" and /" and see... "P" gives me many many results, and "/" gives me no result.
I also try to search for Shift, Ctrl, Alt, no result as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see... It search for the "Name" only now, shortcut value is not included.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vanzue, given you are doing setting search now, any suggestion from you to this PR related to search?
Close(); | ||
} | ||
|
||
private void SearchBox_TextChanged(object sender, TextChangedEventArgs e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I already put something as part of search, e.g. "H"
Then when I switch to another Tab, e.g. Windows -> Clipboard to PowerToys, it will not obey with the search filter.
- Application-specific shortcuts based on the active application | ||
- Additional shortcuts beyond Windows key combinations | ||
- PowerToys shortcuts | ||
- Implementing with WinGet to get new shortcut manifest files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean in the future, there will be really like "+WindowsNT.Shell" avaliable in winget, and it can also define the localized Shortcut information there?
// Todo: Take the default shell name from the settings or environment variable, default to "+WindowsNT.Shell" | ||
indexFile.DefaultShellName = "+WindowsNT.Shell"; | ||
|
||
Serializer serializer = new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see... It search for the "Name" only now, shortcut value is not included.
// Todo: Take the default shell name from the settings or environment variable, default to "+WindowsNT.Shell" | ||
indexFile.DefaultShellName = "+WindowsNT.Shell"; | ||
|
||
Serializer serializer = new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vanzue, given you are doing setting search now, any suggestion from you to this PR related to search?
Summary of the Pull Request
PR Checklist
Detailed Description of the Pull Request / Additional comments
Todo list:
Work for future PRs:
Images
Validation Steps Performed