feat(notification): add ActivationToken support for wayland#1627
Open
18202781743 wants to merge 1 commit into
Open
feat(notification): add ActivationToken support for wayland#162718202781743 wants to merge 1 commit into
18202781743 wants to merge 1 commit into
Conversation
Add xdg-activation-v1 token support to notification service: - Emit ActivationToken signal before ActionInvoked for non-extended actions - Pass XDG_ACTIVATION_TOKEN env to dde-am for extended actions - Use async token request without blocking 为通知服务添加wayland激活令牌支持： - 非扩展动作先发送ActivationToken信号再发送ActionInvoked - 扩展动作通过环境变量传递令牌给dde-am - 使用异步方式获取令牌，不阻塞主线程 Log: 为通知服务添加wayland激活令牌支持
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743 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 |
Reviewer's GuideAdds Wayland XDG activation token support to the notification service by emitting a new ActivationToken D-Bus signal for non-extended actions and propagating the token to dde-am via XDG_ACTIVATION_TOKEN for extended actions, using an asynchronous token retrieval mechanism. Sequence diagram for Wayland activation token handling in notification actionssequenceDiagram
actor User
participant NotificationManager
participant XdgActivation
participant DbusAdaptor
participant DdeAm
rect rgb(230,230,255)
User ->> NotificationManager: actionInvoked(id, bubbleId, actionKey)
NotificationManager ->> XdgActivation: new XdgActivation
NotificationManager ->> XdgActivation: requestToken()
XdgActivation -->> NotificationManager: tokenReady(token)
alt [token not empty]
NotificationManager ->> DbusAdaptor: ActivationToken(bubbleId, token)
end
NotificationManager ->> DbusAdaptor: ActionInvoked(bubbleId, actionKey)
NotificationManager ->> DbusAdaptor: NotificationClosed(bubbleId, Closed)
end
rect rgb(230,255,230)
User ->> NotificationManager: doActionInvoked(entity, actionId, actionKey)
NotificationManager ->> XdgActivation: new XdgActivation
NotificationManager ->> XdgActivation: requestToken()
XdgActivation -->> NotificationManager: tokenReady(token)
NotificationManager ->> DdeAm: startDetached("dde-am", amArgs)
opt [token not empty]
NotificationManager ->> DdeAm: set env XDG_ACTIVATION_TOKEN
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
deepin pr auto review我来对这段代码进行审查，从语法逻辑、代码质量、性能和安全性几个方面提出改进意见： 语法逻辑
代码质量
性能
安全性
改进建议
connect(activation, &DS_NAMESPACE::XdgActivation::tokenFailed, this,
[this, bubbleId, actionKey](const QString &error) {
qWarning(notifyLog) << "Failed to get activation token:" << error;
Q_EMIT ActionInvoked(bubbleId, actionKey);
Q_EMIT NotificationClosed(bubbleId, NotifyEntity::Closed);
activation->deleteLater();
}, Qt::SingleShotConnection);
auto activation = QSharedPointer<DS_NAMESPACE::XdgActivation>::create(this);
void NotificationManager::handleActionWithToken(uint bubbleId, const QString &actionKey,
std::function<void(const QString &token)> callback)
{
auto activation = QSharedPointer<DS_NAMESPACE::XdgActivation>::create(this);
connect(activation.get(), &DS_NAMESPACE::XdgActivation::tokenReady, this,
[this, bubbleId, actionKey, callback, activation](const QString &token) {
callback(token);
Q_EMIT ActionInvoked(bubbleId, actionKey);
Q_EMIT NotificationClosed(bubbleId, NotifyEntity::Closed);
}, Qt::SingleShotConnection);
connect(activation.get(), &DS_NAMESPACE::XdgActivation::tokenFailed, this,
[this, bubbleId, actionKey, activation](const QString &error) {
qWarning(notifyLog) << "Failed to get activation token:" << error;
Q_EMIT ActionInvoked(bubbleId, actionKey);
Q_EMIT NotificationClosed(bubbleId, NotifyEntity::Closed);
}, Qt::SingleShotConnection);
activation->requestToken();
}
if (!token.isEmpty() && token.isValid()) { // 假设 isValid() 是验证方法
proEnv.insert("XDG_ACTIVATION_TOKEN", token);
}
这些改进将提高代码的健壮性、可维护性和安全性，同时保持良好的性能表现。 |
![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:
- Both in
actionInvokedanddoActionInvokedthe asynchronousXdgActivationflow can indefinitely delay emittingActionInvoked/NotificationClosedor startingdde-amiftokenReadyis never emitted; consider adding an error/timeout path or a fallback that proceeds without a token. - You unconditionally create a new
XdgActivationinstance for every action, which may be unnecessary on non-Wayland sessions and could add overhead; consider guarding this logic with a Wayland/session check or reusing a shared helper instead of allocating per-action. - The
XdgActivationsetup and cleanup code is duplicated in two places; consider extracting a small helper (e.g.,requestActivationToken(callback)) to centralize token acquisition and ensure consistent behavior and resource management.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Both in `actionInvoked` and `doActionInvoked` the asynchronous `XdgActivation` flow can indefinitely delay emitting `ActionInvoked`/`NotificationClosed` or starting `dde-am` if `tokenReady` is never emitted; consider adding an error/timeout path or a fallback that proceeds without a token.
- You unconditionally create a new `XdgActivation` instance for every action, which may be unnecessary on non-Wayland sessions and could add overhead; consider guarding this logic with a Wayland/session check or reusing a shared helper instead of allocating per-action.
- The `XdgActivation` setup and cleanup code is duplicated in two places; consider extracting a small helper (e.g., `requestActivationToken(callback)`) to centralize token acquisition and ensure consistent behavior and resource management.
## Individual Comments
### Comment 1
<location path="panels/notification/server/notificationmanager.cpp" line_range="139" />
<code_context>
- Q_EMIT ActionInvoked(bubbleId, actionKey);
- Q_EMIT NotificationClosed(bubbleId, NotifyEntity::Closed);
+ // For non-extended actions, emit ActivationToken first if available
+ auto *activation = new DS_NAMESPACE::XdgActivation(this);
+ connect(activation, &DS_NAMESPACE::XdgActivation::tokenReady, this,
+ [this, bubbleId, actionKey, activation](const QString &token) {
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the repeated XdgActivation setup into a reusable helper that takes a handler functor to simplify control flow and avoid duplication in both call sites.
The `XdgActivation` usage is duplicated and adds nested lambdas and lifetime handling in both call sites. Extracting a small helper that owns `XdgActivation` and just calls a functor when the token is ready will keep both sites linear and remove the extra `amArgsCopy`.
Example helper (private method in `NotificationManager`):
```c++
class NotificationManager : public QObject
{
Q_OBJECT
// ...
private:
template <typename F>
void withActivationToken(F &&handler)
{
auto *activation = new DS_NAMESPACE::XdgActivation(this);
connect(activation, &DS_NAMESPACE::XdgActivation::tokenReady, this,
[activation, handler = std::forward<F>(handler)](const QString &token) mutable {
handler(token);
activation->deleteLater();
},
Qt::SingleShotConnection);
activation->requestToken();
}
};
```
Then `actionInvoked` becomes simpler and closer to the original structure:
```c++
void NotificationManager::actionInvoked(qint64 id, uint bubbleId, const QString &actionKey)
{
qInfo(notifyLog) << "Action invoked, bubbleId:" << bubbleId << ", id:" << id << ", actionKey" << actionKey;
actionInvoked(id, actionKey);
withActivationToken([this, bubbleId, actionKey](const QString &token) {
if (!token.isEmpty()) {
Q_EMIT ActivationToken(bubbleId, token);
qDebug(notifyLog) << "Emitted ActivationToken for non-extended action:" << token;
}
Q_EMIT ActionInvoked(bubbleId, actionKey);
Q_EMIT NotificationClosed(bubbleId, NotifyEntity::Closed);
});
}
```
And the extended action (`dde-am` launch) stays linear and avoids `amArgsCopy`:
```c++
if (!args.isEmpty()) {
QString cmd = args.takeFirst();
QStringList amArgs;
amArgs << "-c" << cmd;
if (!args.isEmpty()) {
amArgs << "--" << args;
}
withActivationToken([amArgs = std::move(amArgs)](const QString &token) {
QProcess pro;
pro.setProgram("dde-am");
pro.setArguments(amArgs);
QProcessEnvironment proEnv = QProcessEnvironment::systemEnvironment();
proEnv.remove("DSG_APP_ID");
if (!token.isEmpty()) {
proEnv.insert("XDG_ACTIVATION_TOKEN", token);
qDebug(notifyLog) << "Set XDG_ACTIVATION_TOKEN for extended action:" << token;
}
pro.setProcessEnvironment(proEnv);
pro.startDetached();
});
}
```
This keeps all behavior (including token emission, `ActivationToken` signal, and process environment setup) while reducing duplication, nesting, and manual `new`/`deleteLater` in the main logic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| Q_EMIT ActionInvoked(bubbleId, actionKey); | ||
| Q_EMIT NotificationClosed(bubbleId, NotifyEntity::Closed); | ||
| // For non-extended actions, emit ActivationToken first if available | ||
| auto *activation = new DS_NAMESPACE::XdgActivation(this); |
There was a problem hiding this comment.
issue (complexity): Consider extracting the repeated XdgActivation setup into a reusable helper that takes a handler functor to simplify control flow and avoid duplication in both call sites.
The XdgActivation usage is duplicated and adds nested lambdas and lifetime handling in both call sites. Extracting a small helper that owns XdgActivation and just calls a functor when the token is ready will keep both sites linear and remove the extra amArgsCopy.
Example helper (private method in NotificationManager):
class NotificationManager : public QObject
{
Q_OBJECT
// ...
private:
template <typename F>
void withActivationToken(F &&handler)
{
auto *activation = new DS_NAMESPACE::XdgActivation(this);
connect(activation, &DS_NAMESPACE::XdgActivation::tokenReady, this,
[activation, handler = std::forward<F>(handler)](const QString &token) mutable {
handler(token);
activation->deleteLater();
},
Qt::SingleShotConnection);
activation->requestToken();
}
};Then actionInvoked becomes simpler and closer to the original structure:
void NotificationManager::actionInvoked(qint64 id, uint bubbleId, const QString &actionKey)
{
qInfo(notifyLog) << "Action invoked, bubbleId:" << bubbleId << ", id:" << id << ", actionKey" << actionKey;
actionInvoked(id, actionKey);
withActivationToken([this, bubbleId, actionKey](const QString &token) {
if (!token.isEmpty()) {
Q_EMIT ActivationToken(bubbleId, token);
qDebug(notifyLog) << "Emitted ActivationToken for non-extended action:" << token;
}
Q_EMIT ActionInvoked(bubbleId, actionKey);
Q_EMIT NotificationClosed(bubbleId, NotifyEntity::Closed);
});
}And the extended action (dde-am launch) stays linear and avoids amArgsCopy:
if (!args.isEmpty()) {
QString cmd = args.takeFirst();
QStringList amArgs;
amArgs << "-c" << cmd;
if (!args.isEmpty()) {
amArgs << "--" << args;
}
withActivationToken([amArgs = std::move(amArgs)](const QString &token) {
QProcess pro;
pro.setProgram("dde-am");
pro.setArguments(amArgs);
QProcessEnvironment proEnv = QProcessEnvironment::systemEnvironment();
proEnv.remove("DSG_APP_ID");
if (!token.isEmpty()) {
proEnv.insert("XDG_ACTIVATION_TOKEN", token);
qDebug(notifyLog) << "Set XDG_ACTIVATION_TOKEN for extended action:" << token;
}
pro.setProcessEnvironment(proEnv);
pro.startDetached();
});
}This keeps all behavior (including token emission, ActivationToken signal, and process environment setup) while reducing duplication, nesting, and manual new/deleteLater in the main logic.
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
2 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.
Summary
为通知服务添加 wayland 激活令牌支持：
ActivationToken信号再发送ActionInvokedXDG_ACTIVATION_TOKEN传递令牌给dde-amChanges
panels/notification/server/notificationmanager.h: 添加ActivationToken信号panels/notification/server/notificationmanager.cpp: 实现异步令牌请求和传递逻辑panels/notification/server/dbusadaptor.h: 添加ActivationTokenD-Bus 信号Test Plan
XDG_ACTIVATION_TOKEN环境变量ActivationToken信号Summary by Sourcery
Add Wayland activation token support to notification actions and expose it via D-Bus.
New Features:
Enhancements: