fix: delay deepin-screen-recorder notify#1428
Closed
qxp930712 wants to merge 2 commits intolinuxdeepin:masterfrom
Closed
fix: delay deepin-screen-recorder notify#1428qxp930712 wants to merge 2 commits intolinuxdeepin:masterfrom
qxp930712 wants to merge 2 commits intolinuxdeepin:masterfrom
Conversation
1. Added QTimer include for asynchronous delay handling 2. Modified Notify method to check for "deepin-screen-recorder" appIcon 3. Implemented 3-second delay using QTimer::singleShot for screen recorder notifications 4. Ensured parameters are captured by value for the delayed execution This change prevents the screen recorder notification from appearing too early, which may interfere with the recording start or UI behavior. Log: Delayed the display time of screen recorder notifications to improve user experience. Influence: 1. Test notification triggering from deepin-screen-recorder and verify the 3-second delay 2. Test notifications from other applications to ensure they appear immediately without delay 3. Verify that mixed notifications (screen recorder and others) are processed correctly 4. Check for potential memory leaks or crashes when notifications are delayed 5. Verify the return value handling for delayed notifications pms: BUG-338965/BUG-336159
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: qxp930712 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 guide (collapsed on small PRs)Reviewer's GuideIntroduces a special-case asynchronous handling path for notifications coming from the deepin-screen-recorder icon by delaying their processing 3 seconds via QTimer::singleShot, while preserving the existing synchronous path for all other notifications and ensuring DBus error replies are still sent if notification creation fails. Sequence diagram for delayed deepin_screen_recorder notification handlingsequenceDiagram
actor Client
participant DbusAdaptor
participant QTimer
participant NotificationManager
participant QDBusConnection
Client->>DbusAdaptor: Notify(appName, replacesId, appIcon="deepin-screen-recorder", summary, body, actions, hints, expireTimeout)
activate DbusAdaptor
DbusAdaptor-->>Client: return 1
deactivate DbusAdaptor
Note over DbusAdaptor,QTimer: Asynchronous path via QTimer::singleShot(3000)
DbusAdaptor->>QTimer: singleShot(3000, this, lambda)
rect rgba(200, 200, 200, 0.1)
Note over QTimer: After 3 seconds
QTimer-->>DbusAdaptor: invoke lambda
activate DbusAdaptor
DbusAdaptor->>NotificationManager: Notify(appName, replacesId, appIcon, summary, body, actions, hints, expireTimeout)
NotificationManager-->>DbusAdaptor: id
alt id == 0
DbusAdaptor->>QDBusConnection: send(QDBusError InternalError "Notify failed.")
end
deactivate DbusAdaptor
end
Flow diagram for conditional notification delay in Notifyflowchart TD
A[Notify called] --> B{appIcon == deepin-screen-recorder}
B -- Yes --> C[Schedule QTimer::singleShot<br/>delay 3000 ms<br/>capture parameters by value]
C --> D[Return 1 to caller immediately]
B -- No --> E[Call manager Notify synchronously]
E --> F{id == 0?}
F -- Yes --> G[Create QDBusError InternalError<br/>Notify failed<br/>Send error via QDBusConnection sessionBus]
F -- No --> H[Return id to caller]
subgraph Delayed_execution_after_3s
I[QTimer fires<br/>lambda invoked] --> J[Call manager Notify with captured parameters]
J --> K{id == 0?}
K -- Yes --> L[Create QDBusError InternalError<br/>Notify failed<br/>Send error via QDBusConnection sessionBus]
K -- No --> M[End async path]
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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:
- Delaying the notification while still returning a hardcoded
1fromNotifychanges the D-Bus method contract (callers won’t get the real notification ID and will see a success path even if the delayedNotifyfails); consider either keepingNotifysynchronous for this case or wiring a proper asynchronous D-Bus reply usingsetDelayedReplyand sending the eventual ID/error back to the original caller. - The delayed error handling uses
QDBusMessage::createErrorandQDBusConnection::sessionBus().send(reply)without relating the reply to the original D-Bus call; if you intend to reply asynchronously, you should construct the reply from the original message to ensure it matches the correct call and service. - The lambda passed to
QTimer::singleShotcapturesthisand runs 3 seconds later; if the adaptor object can be destroyed in that window, this can lead to a dangling pointer—consider using aQPointeror ensuring the object’s lifetime covers the delay.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Delaying the notification while still returning a hardcoded `1` from `Notify` changes the D-Bus method contract (callers won’t get the real notification ID and will see a success path even if the delayed `Notify` fails); consider either keeping `Notify` synchronous for this case or wiring a proper asynchronous D-Bus reply using `setDelayedReply` and sending the eventual ID/error back to the original caller.
- The delayed error handling uses `QDBusMessage::createError` and `QDBusConnection::sessionBus().send(reply)` without relating the reply to the original D-Bus call; if you intend to reply asynchronously, you should construct the reply from the original message to ensure it matches the correct call and service.
- The lambda passed to `QTimer::singleShot` captures `this` and runs 3 seconds later; if the adaptor object can be destroyed in that window, this can lead to a dangling pointer—consider using a `QPointer` or ensuring the object’s lifetime covers the delay.
## Individual Comments
### Comment 1
<location> `panels/notification/server/dbusadaptor.cpp:35-44` </location>
<code_context>
int expireTimeout)
{
+ // Delay processing for 3 seconds when appIcon == "deepin-screen-recorder"
+ if (appIcon == "deepin-screen-recorder") {
+ // Use QTimer to delay processing asynchronously without blocking other notifications
+ QTimer::singleShot(3000, this, [this, appName, replacesId, appIcon, summary, body, actions, hints, expireTimeout]() {
+ uint id = manager()->Notify(appName, replacesId, appIcon, summary, body, actions, hints, expireTimeout);
+ if (id == 0) {
+ QDBusError error(QDBusError::InternalError, "Notify failed.");
+ QDBusMessage reply = QDBusMessage::createError(error);
+ QDBusConnection::sessionBus().send(reply);
+ }
+ });
+ return 1;
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Delaying Notify via QTimer breaks D-Bus method-call semantics and may confuse clients
`Notify` now always returns `1` immediately, while the real notification (which may fail or get a different ID) is created up to 3 seconds later. The async error you send is not associated with the original D-Bus method call, so clients will see a successful reply with ID `1` even when `manager()->Notify(...)` fails or chooses another ID, breaking D-Bus method semantics and potentially causing timeouts or inconsistent client behavior. If a delay is needed, keep the D-Bus call synchronous with the actual `Notify` result (e.g., perform the delay inside `manager()->Notify` or a deeper layer, or use an internal queue/timer while still returning the final ID synchronously).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+35
to
+44
| if (appIcon == "deepin-screen-recorder") { | ||
| // Use QTimer to delay processing asynchronously without blocking other notifications | ||
| QTimer::singleShot(3000, this, [this, appName, replacesId, appIcon, summary, body, actions, hints, expireTimeout]() { | ||
| uint id = manager()->Notify(appName, replacesId, appIcon, summary, body, actions, hints, expireTimeout); | ||
| if (id == 0) { | ||
| QDBusError error(QDBusError::InternalError, "Notify failed."); | ||
| QDBusMessage reply = QDBusMessage::createError(error); | ||
| QDBusConnection::sessionBus().send(reply); | ||
| } | ||
| }); |
There was a problem hiding this comment.
issue (bug_risk): Delaying Notify via QTimer breaks D-Bus method-call semantics and may confuse clients
Notify now always returns 1 immediately, while the real notification (which may fail or get a different ID) is created up to 3 seconds later. The async error you send is not associated with the original D-Bus method call, so clients will see a successful reply with ID 1 even when manager()->Notify(...) fails or chooses another ID, breaking D-Bus method semantics and potentially causing timeouts or inconsistent client behavior. If a delay is needed, keep the D-Bus call synchronous with the actual Notify result (e.g., perform the delay inside manager()->Notify or a deeper layer, or use an internal queue/timer while still returning the final ID synchronously).
18202781743
reviewed
Feb 4, 2026
| QDBusConnection::sessionBus().send(reply); | ||
| } | ||
| }); | ||
| return 1; |
Contributor
There was a problem hiding this comment.
这种是bubbleId，对调用方是有用的，
不应该这样去延迟，
Updated the logic in the AudioApplet to correctly identify and display the mute icon when the volume level is less than or equal to 1. Previously, the code failed to explicitly handle the state where volume was 0 or very low, potentially resulting in incorrect icon rendering or undefined behavior. The change adds an else block to return "osd_volume_mute" in these cases, ensuring accurate UI feedback for users. Log: Fixed the audio OSD mute icon display for silent state Influence: 1. Test volume at 0% to ensure the mute icon is displayed 2. Test volume between 1% and 66% to ensure the low/mid volume icon is displayed 3. Test volume above 66% to ensure the high volume icon is displayed 4. Verify icon switching behavior when sliding the volume slider pms: BUG-349767
deepin pr auto review这段代码包含两个部分的修改，涉及音频图标显示逻辑和屏幕录制通知的延迟处理。以下是对这两处代码的详细审查和改进意见： 1. 音频图标显示逻辑 (
|
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
4 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.
This change prevents the screen recorder notification from appearing too early, which may interfere with the recording start or UI behavior.
Log: Delayed the display time of screen recorder notifications to improve user experience.
Influence:
pms: BUG-338965/BUG-336159
Summary by Sourcery
Bug Fixes: