feat: 實作 Win32 的啟動與關閉應用 / Impliment start app and stop app for Win32#1376
Open
kaihuang1122 wants to merge 8 commits into
Open
feat: 實作 Win32 的啟動與關閉應用 / Impliment start app and stop app for Win32#1376kaihuang1122 wants to merge 8 commits into
kaihuang1122 wants to merge 8 commits into
Conversation
Contributor
There was a problem hiding this comment.
Hey - 我发现了两个问题,并留下了一些高层次的反馈:
- 在
stop_app中,直接将intent与PROCESSENTRY32W::szExeFile匹配会在intent是完整路径(正如文档中建议的那样)而不仅仅是可执行文件名时失败;建议将intent规范化为文件名,或者显式限制为基础文件名(basename),以使行为更可预测。 - 当前的
stop_app逻辑始终使用TerminateProcess,这相当强硬;当hwnd_可用时,可以先尝试优雅关闭(例如发送WM_CLOSE/WM_QUERYENDSESSION),再在必要时回退到强制终止,从而降低数据丢失或应用状态不一致的风险。
给 AI Agent 的提示
Please address the comments from this code review:
## Overall Comments
- In `stop_app`, matching `intent` directly against `PROCESSENTRY32W::szExeFile` will fail when `intent` is a full path (as suggested by the docs) rather than just the bare executable name; consider normalizing `intent` to a filename or explicitly constraining it to a basename to make behavior predictable.
- The current `stop_app` logic always uses `TerminateProcess`, which is quite forceful; when `hwnd_` is available you may want to attempt a graceful shutdown first (e.g., sending `WM_CLOSE`/`WM_QUERYENDSESSION`) before falling back to termination to reduce the risk of data loss or inconsistent app state.
## Individual Comments
### Comment 1
<location path="source/MaaWin32ControlUnit/Manager/Win32ControlUnitMgr.cpp" line_range="260-269" />
<code_context>
+ return false;
+ }
+
+ std::wstring cmd = MAA_NS::to_u16(intent);
+
+ STARTUPINFOW si;
+ PROCESS_INFORMATION pi;
+ ZeroMemory(&si, sizeof(si));
+ si.cb = sizeof(si);
+ ZeroMemory(&pi, sizeof(pi));
+
+ if (!CreateProcessW(nullptr, cmd.data(), nullptr, nullptr, FALSE, 0, nullptr, nullptr, &si, &pi)) {
+ DWORD err = GetLastError();
+ if (err == ERROR_ELEVATION_REQUIRED) {
+ LogInfo << "Elevation required. Falling back to ShellExecuteExW...";
+ SHELLEXECUTEINFOW sei = { sizeof(sei) };
+ sei.fMask = SEE_MASK_NOCLOSEPROCESS;
+ sei.lpVerb = L"runas";
+ sei.lpFile = cmd.c_str();
+ sei.nShow = SW_SHOWNORMAL;
+ if (ShellExecuteExW(&sei)) {
</code_context>
<issue_to_address>
**issue (bug_risk):** 将完整命令行作为 `ShellExecuteExW` 的 `lpFile` 使用时,可能会把参数误解为可执行文件路径的一部分。
这里的 `cmd` 包含完整命令行(可执行文件 + 参数),但 `ShellExecuteExW` 期望 `lpFile` 只包含可执行文件,剩余部分放在 `lpParameters` 中。如果 `intent` 包含参数,这里可能会失败,或者指向错误的可执行文件。请将 `intent` 拆分为可执行文件和参数(例如使用类似 `CommandLineToArgvW` 的解析方式),把可执行文件赋值给 `sei.lpFile`,其余参数赋值给 `sei.lpParameters`。
</issue_to_address>
### Comment 2
<location path="source/MaaWin32ControlUnit/Manager/Win32ControlUnitMgr.cpp" line_range="331-340" />
<code_context>
+ return true;
+ }
+
+ std::wstring target_exe = MAA_NS::to_u16(intent);
+ HANDLE hSnap = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0);
+ if (hSnap == INVALID_HANDLE_VALUE) {
+ LogError << "CreateToolhelp32Snapshot failed, error:" << GetLastError();
+ return false;
+ }
- return false;
+ PROCESSENTRY32W pe;
+ pe.dwSize = sizeof(pe);
+ bool terminated_any = false;
+
+ if (Process32FirstW(hSnap, &pe)) {
+ do {
+ if (target_exe == pe.szExeFile) {
+ HANDLE hProcess = OpenProcess(PROCESS_TERMINATE, FALSE, pe.th32ProcessID);
+ if (hProcess) {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** 直接将 `target_exe` 与 `pe.szExeFile` 比较,在处理完整路径和大小写时比较脆弱。
在 Windows 上,`PROCESSENTRY32W::szExeFile` 通常只是可执行文件名(例如 `foo.exe`),而 `intent` 可能是完整路径,且/或使用不同的大小写。直接用 `std::wstring` 做相等比较是区分大小写的,在这些情况下会失败。建议先从 `target_exe` 中提取基础文件名(例如使用 `PathFindFileNameW` 或类似方法),再与 `szExeFile` 进行不区分大小写的比较。
建议实现:
```cpp
std::wstring target_exe = MAA_NS::to_u16(intent);
// Extract executable basename (without path) for robust comparison with PROCESSENTRY32W::szExeFile
std::wstring target_exe_name = target_exe;
const auto last_sep_pos = target_exe_name.find_last_of(L"\\/");
if (last_sep_pos != std::wstring::npos && last_sep_pos + 1 < target_exe_name.size()) {
target_exe_name = target_exe_name.substr(last_sep_pos + 1);
}
HANDLE hSnap = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0);
```
```cpp
if (Process32FirstW(hSnap, &pe)) {
do {
// Compare executable names case-insensitively to handle differing casing
if (_wcsicmp(target_exe_name.c_str(), pe.szExeFile) == 0) {
```
如果该翻译单元中尚未包含对应声明,你可能需要:
1. 在文件顶部添加 `#include <cwchar>`(或其它提供 `_wcsicmp` 的头文件)。
2. 确保构建设置允许使用 `_wcsicmp`(它是常见的 MSVC 扩展;在 Windows 项目中通常已经可用)。
</issue_to_address>帮我变得更有用!请在每条评论上点 👍 或 👎,我会根据你的反馈改进评审质量。
Original comment in English
Hey - I've found 2 issues, and left some high level feedback:
- In
stop_app, matchingintentdirectly againstPROCESSENTRY32W::szExeFilewill fail whenintentis a full path (as suggested by the docs) rather than just the bare executable name; consider normalizingintentto a filename or explicitly constraining it to a basename to make behavior predictable. - The current
stop_applogic always usesTerminateProcess, which is quite forceful; whenhwnd_is available you may want to attempt a graceful shutdown first (e.g., sendingWM_CLOSE/WM_QUERYENDSESSION) before falling back to termination to reduce the risk of data loss or inconsistent app state.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `stop_app`, matching `intent` directly against `PROCESSENTRY32W::szExeFile` will fail when `intent` is a full path (as suggested by the docs) rather than just the bare executable name; consider normalizing `intent` to a filename or explicitly constraining it to a basename to make behavior predictable.
- The current `stop_app` logic always uses `TerminateProcess`, which is quite forceful; when `hwnd_` is available you may want to attempt a graceful shutdown first (e.g., sending `WM_CLOSE`/`WM_QUERYENDSESSION`) before falling back to termination to reduce the risk of data loss or inconsistent app state.
## Individual Comments
### Comment 1
<location path="source/MaaWin32ControlUnit/Manager/Win32ControlUnitMgr.cpp" line_range="260-269" />
<code_context>
+ return false;
+ }
+
+ std::wstring cmd = MAA_NS::to_u16(intent);
+
+ STARTUPINFOW si;
+ PROCESS_INFORMATION pi;
+ ZeroMemory(&si, sizeof(si));
+ si.cb = sizeof(si);
+ ZeroMemory(&pi, sizeof(pi));
+
+ if (!CreateProcessW(nullptr, cmd.data(), nullptr, nullptr, FALSE, 0, nullptr, nullptr, &si, &pi)) {
+ DWORD err = GetLastError();
+ if (err == ERROR_ELEVATION_REQUIRED) {
+ LogInfo << "Elevation required. Falling back to ShellExecuteExW...";
+ SHELLEXECUTEINFOW sei = { sizeof(sei) };
+ sei.fMask = SEE_MASK_NOCLOSEPROCESS;
+ sei.lpVerb = L"runas";
+ sei.lpFile = cmd.c_str();
+ sei.nShow = SW_SHOWNORMAL;
+ if (ShellExecuteExW(&sei)) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Using the full command line as `lpFile` for `ShellExecuteExW` can misinterpret arguments as part of the executable path.
Here `cmd` contains the full command line (exe + args), but `ShellExecuteExW` expects only the executable in `lpFile` and the rest in `lpParameters`. If `intent` includes arguments, this can fail or target the wrong executable. Please split `intent` into executable and arguments (e.g., via `CommandLineToArgvW`-style parsing), assign the executable to `sei.lpFile` and the remaining arguments to `sei.lpParameters`.
</issue_to_address>
### Comment 2
<location path="source/MaaWin32ControlUnit/Manager/Win32ControlUnitMgr.cpp" line_range="331-340" />
<code_context>
+ return true;
+ }
+
+ std::wstring target_exe = MAA_NS::to_u16(intent);
+ HANDLE hSnap = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0);
+ if (hSnap == INVALID_HANDLE_VALUE) {
+ LogError << "CreateToolhelp32Snapshot failed, error:" << GetLastError();
+ return false;
+ }
- return false;
+ PROCESSENTRY32W pe;
+ pe.dwSize = sizeof(pe);
+ bool terminated_any = false;
+
+ if (Process32FirstW(hSnap, &pe)) {
+ do {
+ if (target_exe == pe.szExeFile) {
+ HANDLE hProcess = OpenProcess(PROCESS_TERMINATE, FALSE, pe.th32ProcessID);
+ if (hProcess) {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Directly comparing `target_exe` to `pe.szExeFile` is brittle regarding full paths and case sensitivity.
On Windows, `PROCESSENTRY32W::szExeFile` is typically just the executable name (e.g., `foo.exe`), while `intent` may be a full path and/or use different casing. A direct `std::wstring` equality is case-sensitive and will fail in those cases. Consider normalizing by extracting the basename from `target_exe` (e.g., via `PathFindFileNameW` or similar) and comparing it to `szExeFile` case-insensitively.
Suggested implementation:
```cpp
std::wstring target_exe = MAA_NS::to_u16(intent);
// Extract executable basename (without path) for robust comparison with PROCESSENTRY32W::szExeFile
std::wstring target_exe_name = target_exe;
const auto last_sep_pos = target_exe_name.find_last_of(L"\\/");
if (last_sep_pos != std::wstring::npos && last_sep_pos + 1 < target_exe_name.size()) {
target_exe_name = target_exe_name.substr(last_sep_pos + 1);
}
HANDLE hSnap = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0);
```
```cpp
if (Process32FirstW(hSnap, &pe)) {
do {
// Compare executable names case-insensitively to handle differing casing
if (_wcsicmp(target_exe_name.c_str(), pe.szExeFile) == 0) {
```
If not already present in this translation unit, you may need to:
1. Add `#include <cwchar>` (or another header that provides `_wcsicmp`) near the top of the file.
2. Ensure your build settings allow use of `_wcsicmp` (it's a common MSVC extension; typically already available in Windows projects).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Author
|
針對 sourcery-ai 提出的反饋:
因此,我認為應該拒絕 sourcery-ai 的建議。 |
Member
|
嵌套太多了,尽量用守卫模式。然后不要用 do while(0) |
Author
Fixed,多謝指教,僅存的 do while 是 微軟寫法 且我認為具有必要性 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
实现 Win32 控制器对应用启动和停止的支持,并记录新的行为。
新功能:
文档:
Original summary in English
Summary by Sourcery
Implement Win32 controller support for starting and stopping applications and document the new behavior.
New Features:
Documentation: