fix: Use dde-am to execute command in notifications #1409
Merged
Conversation
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
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRoutes notification action commands through dde-am instead of executing them directly, wrapping the original command and its arguments into dde-am’s CLI format while preserving the environment tweak that removes DSG_APP_ID. Sequence diagram for executing notification action via dde_amsequenceDiagram
participant User
participant NotificationManager
participant QProcess
participant dde_am as dde_am_process
participant TargetApp
User->>NotificationManager: Clicks notification action
NotificationManager->>NotificationManager: Extract cmd and args from NotifyEntity
NotificationManager->>NotificationManager: Build amArgs = ["-c", cmd, "--", args]
NotificationManager->>QProcess: Configure program "/usr/bin/dde-am" and arguments amArgs
NotificationManager->>QProcess: Get system environment
NotificationManager->>QProcess: Remove DSG_APP_ID from environment
NotificationManager->>QProcess: Start process
QProcess->>dde_am: Launch /usr/bin/dde-am -c cmd -- args
dde_am->>TargetApp: Execute original command cmd with args
TargetApp-->>dde_am: Command completes
dde_am-->>QProcess: Exit status
QProcess-->>NotificationManager: Process finished
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
BLumia
reviewed
Jan 27, 2026
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider avoiding the hardcoded "/usr/bin/dde-am" path and instead either rely on the executable being in PATH or centralize the path in a configurable/constant location to improve portability and maintainability.
- It may be worth checking and handling the QProcess start/exit status (e.g., via startDetached or signals) so that failures to launch
dde-amfor notification actions can be detected or logged more clearly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider avoiding the hardcoded "/usr/bin/dde-am" path and instead either rely on the executable being in PATH or centralize the path in a configurable/constant location to improve portability and maintainability.
- It may be worth checking and handling the QProcess start/exit status (e.g., via startDetached or signals) so that failures to launch `dde-am` for notification actions can be detected or logged more clearly.
## Individual Comments
### Comment 1
<location> `panels/notification/server/notificationmanager.cpp:543` </location>
<code_context>
QProcess pro;
- pro.setProgram(cmd);
- pro.setArguments(args);
+ pro.setProgram("/usr/bin/dde-am");
+ pro.setArguments(amArgs);
QProcessEnvironment proEnv = QProcessEnvironment::systemEnvironment();
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid hardcoding the absolute path to `dde-am` to reduce fragility across environments.
This path ties the code to one filesystem layout and may fail silently if `dde-am` is installed elsewhere or provided via a symlink/alternative prefix. Prefer resolving the path at runtime (e.g., from configuration, a helper that locates `dde-am`, or by relying on `PATH` and using just `"dde-am"`) to keep it portable across different installations.
Suggested implementation:
```cpp
QProcess pro;
// Use command name and rely on PATH instead of hardcoded absolute path
pro.setProgram(QStringLiteral("dde-am"));
pro.setArguments(amArgs);
```
If the project has a configuration system or a central helper for resolving tool paths, you may want to:
1. Replace the string literal `"dde-am"` with a configurable value (e.g., read from settings) or a utility function like `AppLocator::ddeAmPath()`.
2. Ensure that the environment in which this process runs has `dde-am` available on `PATH` or that any wrapper scripts/symlinks are set up appropriately.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
use dde-am -c cmd -- args Log: Use dde-am to execute command in notifications Issues: linuxdeepin/developer-center#12695
deepin pr auto review这段代码的修改主要目的是将通知动作的执行方式从直接执行命令改为通过 1. 语法逻辑
2. 代码质量
3. 代码性能
4. 代码安全
总结与改进建议这段代码在逻辑上是可行的，将执行入口统一收归到 建议修改后的代码示例（假设需要异步执行）： void NotificationManager::doActionInvoked(const NotifyEntity &entity, const QString &actionKey)
{
// ... 前面的逻辑 ...
if (!args.isEmpty()) {
QString cmd = args.takeFirst(); // 命令
QStringList amArgs;
amArgs << "-c" << cmd;
if (!args.isEmpty()) {
amArgs << "--" << args;
}
// 改进点 1: 使用 QPointer 或者在类中管理 QProcess 指针，防止野指针
// 改进点 2: 分配在堆上，防止函数结束后对象被销毁导致子进程被杀
QProcess *pro = new QProcess();
// 连接 finished 信号，确保进程结束后自动删除对象，防止内存泄漏
connect(pro, QOverload<int, QProcess::ExitStatus>::of(&QProcess::finished),
pro, &QProcess::deleteLater);
pro->setProgram("dde-am");
pro->setArguments(amArgs);
QProcessEnvironment proEnv = QProcessEnvironment::systemEnvironment();
proEnv.remove("DSG_APP_ID");
pro->setProcessEnvironment(proEnv);
// 启动进程
pro->start();
}
// ...
}如果必须保持栈对象（同步执行），请确保代码中包含了 |
BLumia
approved these changes
Jan 27, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: BLumia, yixinshark The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Contributor
Author
|
/forcemerge |
|
This pr force merged! (status: blocked) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
use dde-am -c cmd -- args
Log: Use dde-am to execute command in notifications
Issues: linuxdeepin/developer-center#12695
Summary by Sourcery
Bug Fixes: