fix: Notification bar progress indicator anomaly#1050
Open
JWWTSL wants to merge 1 commit intolinuxdeepin:masterfrom
Open
fix: Notification bar progress indicator anomaly#1050JWWTSL wants to merge 1 commit intolinuxdeepin:masterfrom
JWWTSL wants to merge 1 commit intolinuxdeepin:masterfrom
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts Bluetooth OBEX file transfer notifications to correctly replace the final progress notification with the completion/failed notification, and updates gesture handling to use a generic window action API for maximize/unmaximize behavior. Sequence diagram for updated OBEX transfer notification replacementsequenceDiagram
participant obexAgent
participant Notifications
participant DesktopShell
obexAgent->>Notifications: notifyProgress(notify, 0, filename, device, 1-99)
Notifications-->>DesktopShell: ShowOrUpdateProgressNotification(id = notifyID, percent < 100)
obexAgent-->>obexAgent: notifyID = notifyID
obexAgent-->>obexAgent: lock notifyMu
obexAgent-->>obexAgent: currentID = notifyID
obexAgent-->>obexAgent: unlock notifyMu
obexAgent->>Notifications: notifyProgress(notify, currentID, realFileName, deviceName, 100)
Notifications->>DesktopShell: Notify(appId, replaces_id = currentID, iconSuccess, summary, body)
DesktopShell-->>DesktopShell: ReplaceProgressNotification(currentID, newNotification)
obexAgent-->>obexAgent: notifyID = newID
Class diagram for updated OBEX agent notification handlingclassDiagram
class obexAgent {
notify notifications_Notifications
notifyID uint32
receiveProgress(transfer *transferObj)
notifyProgress(notify notifications_Notifications, replaceID uint32, filename string, device string, percent int) uint32
notifyFailed(notify notifications_Notifications, replaceID uint32, summary string) uint32
}
class notifications_Notifications {
Notify(serial uint32, appName string, replacesID uint32, appIcon string, summary string, body string, actions []string, hints map_string_dbus_Variant, expireTimeout int32) (uint32, error)
CloseNotification(serial uint32, id uint32) error
}
class transferObj {
oriFilename string
deviceName string
}
obexAgent --> notifications_Notifications : uses
obexAgent --> transferObj : reads
Class diagram for gesture Manager maximizing via generic window action APIclassDiagram
class Manager {
wm WindowManager
doHandle4Or5FingersSwipeDown() error
doMaximizeActiveWindow() error
doUnMaximizeActiveWindow() error
doMinimize() error
}
class WindowManagerBefore {
MaximizeActiveWindow(display uint32) error
UnMaximizeActiveWindow(display uint32) error
}
class WindowManagerAfter {
PerformAction(display uint32, action WindowAction) error
}
class WindowAction {
wmActionToggleMaximize
}
Manager --> WindowManagerAfter : calls PerformAction
WindowManagerBefore <|-- WindowManagerAfter : replacedBy
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: JWWTSL 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 |
![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 left some high level feedback:
- In the gesture manager,
doMaximizeActiveWindowanddoUnMaximizeActiveWindownow both callPerformAction(0, wmActionToggleMaximize), which makes them identical; if callers depend on strictly-maximize vs strictly-unmaximize semantics, this toggle-based implementation may introduce unexpected behavior and might be better handled with explicit maximize/unmaximize actions. - In the notification changes,
CloseNotificationwas removed andreplaceIDis now passed tonotify.Notify; it’s worth double-checking thatreplaceIDis always a valid existing notification ID (or zero when starting fresh), and that the DBus notification spec implementation you use interprets this argument asreplaces_idrather than as an unrelated field, to avoid leaking stale notifications or mis-threading updates.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the gesture manager, `doMaximizeActiveWindow` and `doUnMaximizeActiveWindow` now both call `PerformAction(0, wmActionToggleMaximize)`, which makes them identical; if callers depend on strictly-maximize vs strictly-unmaximize semantics, this toggle-based implementation may introduce unexpected behavior and might be better handled with explicit maximize/unmaximize actions.
- In the notification changes, `CloseNotification` was removed and `replaceID` is now passed to `notify.Notify`; it’s worth double-checking that `replaceID` is always a valid existing notification ID (or zero when starting fresh), and that the DBus notification spec implementation you use interprets this argument as `replaces_id` rather than as an unrelated field, to avoid leaking stale notifications or mis-threading updates.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
log: Incorrect replaces_id used when sending the “Received Successfully” notification upon completion (or failure to use the ID from the progress bar notification), causing the 99% progress notification to remain unchanged and persist on the desktop. When the status changes to complete, first read the current `a.notifyID` within the lock and save it as `currentID`. Then use this `currentID` as the `replaces_id` to send the 100% completion notification. This ensures the completion notification replaces the last progress notification (including the 99% progress notification). pms: bug-351711
deepin pr auto review我对这段代码 diff 进行了审查，以下是关于语法逻辑、代码质量、代码性能和代码安全的详细分析及改进建议： 总体评价这段代码主要涉及蓝牙文件传输 (OBEX) 代理和手势管理器的逻辑修改。整体代码风格符合 Go 语言规范，修改主要集中在通知机制的优化和窗口管理动作的统一。代码逻辑清晰，没有明显的安全隐患，但在通知处理逻辑上存在微小的优化空间。 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
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.
log: Incorrect replaces_id used when sending the “Received Successfully” notification upon completion (or failure to use the ID from the progress bar notification), causing the 99% progress notification to remain unchanged and persist on the desktop. When the status changes to complete, first read the current
a.notifyIDwithin the lock and save it ascurrentID. Then use thiscurrentIDas thereplaces_idto send the 100% completion notification. This ensures the completion notification replaces the last progress notification (including the 99% progress notification).pms: bug-351711
Translated with DeepL.com (free version)
Summary by Sourcery
Fix Bluetooth file transfer notifications to correctly replace in-progress toasts on completion and align gesture window maximize behavior with the generic window manager action API.
Bug Fixes:
Enhancements: