-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
[Workspaces] Saving app properties on launch and recapture #36751
base: main
Are you sure you want to change the base?
Conversation
} | ||
|
||
inline void StampWorkspacesLaunchedProperty(HWND window) | ||
inline void StampWorkspacesLaunchedProperty(HWND window, const std::wstring appId) |
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.
Please don't copy strings; pass by const reference const std::wstring&
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.
Also, please keep setting different properties separate, and create a new function for setting app IDs.
|
||
const wchar_t WorkspacesAppID[] = L"PowerToys_WorkspacesAppId"; |
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.
Please don't duplicate constants and use one defined in WorkspacesWindowPropertyUtils.h
.
const std::wstring GetGuidFromHwnd(HWND window); | ||
|
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 continue managing properties in a single location. Please move this function to WorkspacesWindowPropertyUtils.h
@@ -157,7 +157,10 @@ namespace SnapshotUtils | |||
rect.bottom = monitorRect.top + monitorRect.height; | |||
} | |||
|
|||
std::wstring guid = Utils::Apps::GetGuidFromHwnd(window); |
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.
There is a possible scenario that could lead to the same app IDs in different workspaces, which could lead to bugs.
For example, if I click Launch and Edit, leave those launched windows opened, and create a new workspace with those windows -> then we'll have the same IDs for apps.
My suggestion is to pass an argument to the snapshot tool with a flag if it's a creation of a new workspace or "Launch and Edit" (the same as we do for Launcher).
When we're creating a new workspace, we should create a new GUID for each app, and when editing, get those from windows.
var appBefore = sameAppBefore.FirstOrDefault(); | ||
app.CommandLineArguments = appBefore.CommandLineArguments; | ||
app.IsElevated = appBefore.IsElevated; | ||
app.PwaAppId = appBefore.PwaAppId; |
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.
Why do you need to rewrite the PWA app ID here? Could it change?
Summary of the Pull Request
Adding GUID to window properties on launch (or move) to be able to recognize the window when re-snapshoting. Copying the properties, which cannot be captured, like: command line arguments
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed
tested locally