feat: improve tray menu and notification panel interaction#1513
Open
qxp930712 wants to merge 1 commit intolinuxdeepin:masterfrom
Open
feat: improve tray menu and notification panel interaction#1513qxp930712 wants to merge 1 commit intolinuxdeepin:masterfrom
qxp930712 wants to merge 1 commit intolinuxdeepin:masterfrom
Conversation
Added tray menu state tracking and automatic panel management to prevent conflicts between tray menus and notification panel. The changes include: 1. Added autoCloseOnDeactivate property to PanelMenu for conditional auto-close behavior 2. Implemented tray menu state tracking in MenuHelper with hasTrayMenuOpen property 3. Added delayed menu opening with timers to handle panel transitions smoothly 4. Modified notification center panel to respect tray menu state and prevent overlapping 5. Added automatic notification panel closing when opening app item menus or tray menus Log: Improved tray menu behavior and fixed conflicts with notification panel Influence: 1. Test opening tray menus while notification panel is visible - should auto-close notification panel 2. Verify notification panel cannot be opened when tray menu is active 3. Test app item menu opening behavior with notification panel 4. Check tray menu auto-close behavior when losing focus 5. Verify all menu types properly track their open/close state 6. Test timing of menu openings and panel transitions 7. Verify notification panel visibility respects tray menu state PMS: BUG--284867
|
[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 GuideImplements coordinated state tracking between tray menus and the notification center to avoid overlapping UIs, by delaying tray menu opening, conditionally auto-closing menus on deactivation, and blocking or closing the notification panel when tray menus or app item menus are active. Sequence diagram for opening a tray menu while notification panel is visiblesequenceDiagram
actor User
participant TrayIcon as TrayIcon_QML
participant SurfacePopup as SurfacePopup_QML
participant TrayMenu as PanelMenu_tray
participant MenuHelper as MenuHelper_QML
participant NotifyPanel as NotificationCenterPanel
participant NotifyWindow as notification_main_qml
User->>TrayIcon: click()
TrayIcon->>SurfacePopup: onTrayActivated()
SurfacePopup->>NotifyPanel: DS.applet(notificationcenter)
alt notification panel visible
SurfacePopup->>NotifyPanel: close()
NotifyPanel->>NotifyWindow: set Panel.visible false
NotifyPanel->>NotifyWindow: visible = Panel.visible && !Panel.hasTrayMenuOpen
end
SurfacePopup->>TrayMenu: setup bindings(menuX, menuY)
SurfacePopup->>TrayMenu: set autoCloseOnDeactivate false
SurfacePopup->>MenuHelper: set hasTrayMenuOpen true
SurfacePopup->>NotifyPanel: setHasTrayMenuOpen(true)
SurfacePopup->>SurfacePopup: pendingTrayMenu = TrayMenu
SurfacePopup->>SurfacePopup: trayMenuOpenTimer.restart()
SurfacePopup->>SurfacePopup: restoreAutoCloseTimer.restart()
SurfacePopup-->>TrayMenu: trayMenuOpenTimer.onTriggered
TrayMenu->>TrayMenu: open()
SurfacePopup-->>TrayMenu: restoreAutoCloseTimer.onTriggered
TrayMenu->>TrayMenu: autoCloseOnDeactivate = true
TrayMenu-->>MenuHelper: aboutToHide()
MenuHelper->>MenuHelper: activeMenu = null
MenuHelper->>MenuHelper: hasTrayMenuOpen = false
MenuHelper-->>NotifyPanel: setHasTrayMenuOpen(false)
NotifyPanel->>NotifyWindow: visible = Panel.visible && !Panel.hasTrayMenuOpen
Sequence diagram for blocking notification panel when tray menu is opensequenceDiagram
actor User
participant PanelToggle as PanelToggle_UI
participant NotifyPanel as NotificationCenterPanel
participant NotifyWindow as notification_main_qml
participant MenuHelper as MenuHelper_QML
Note over MenuHelper,NotifyPanel: Precondition: MenuHelper.hasTrayMenuOpen = true
User->>PanelToggle: click()
PanelToggle->>NotifyPanel: setVisible(true)
alt tray menu is open
NotifyPanel->>NotifyPanel: if m_hasTrayMenuOpen is true
NotifyPanel-->>PanelToggle: return without changing m_visible
NotifyPanel->>NotifyWindow: visible remains Panel.visible && !Panel.hasTrayMenuOpen
else no tray menu open
NotifyPanel->>NotifyPanel: m_visible = true
NotifyPanel->>NotifyWindow: visible = Panel.visible && !Panel.hasTrayMenuOpen
end
Updated class diagram for menu and notification panel coordinationclassDiagram
class PanelMenu {
+bool autoCloseOnDeactivate
+string windowTitle
+int width
+int height
+QtObject menuWindow
+QtObject control
+onActiveChanged()
}
class MenuHelper {
+QtObject activeMenu
+bool hasTrayMenuOpen
+menuClosed()
+onAboutToHide()
}
class NotificationCenterPanel {
+bool m_visible
+bool m_hasTrayMenuOpen
+NotificationCenterProxy* m_proxy
+NotificationCenterPanel(QObject* parent)
+~NotificationCenterPanel()
+bool visible() const
+void setVisible(bool newVisible)
+void close()
+bool hasTrayMenuOpen() const
+void setHasTrayMenuOpen(bool hasOpen)
+void setBubblePanelEnabled(bool enabled)
+visibleChanged()
+hasTrayMenuOpenChanged()
}
class SurfacePopup_QML {
+var pendingTrayMenu
+Timer trayMenuOpenTimer
+Timer restoreAutoCloseTimer
+onMenuVisibleChanged()
+onActiveChanged()
+openTrayMenu()
}
class TrayItemSurfacePopup_QML {
+bool popupVisible
+var pendingTrayMenu
+Timer trayMenuOpenTimer
+Timer restoreAutoCloseTimer
+onMenuVisibleChanged()
+onActiveChanged()
+openTrayItemMenu()
}
class AppItem_QML {
+requestAppItemMenu()
}
class NotificationMainQML {
+bool visible
+int contentPadding
}
SurfacePopup_QML --> PanelMenu : uses as tray menu
TrayItemSurfacePopup_QML --> PanelMenu : uses as tray menu
SurfacePopup_QML --> MenuHelper : updates hasTrayMenuOpen
TrayItemSurfacePopup_QML --> MenuHelper : updates hasTrayMenuOpen
SurfacePopup_QML --> NotificationCenterPanel : setHasTrayMenuOpen()
TrayItemSurfacePopup_QML --> NotificationCenterPanel : setHasTrayMenuOpen()
NotificationCenterPanel --> NotificationMainQML : exposes Panel.visible
NotificationMainQML --> NotificationCenterPanel : depends on hasTrayMenuOpen
AppItem_QML --> NotificationCenterPanel : close() before menu
MenuHelper --> PanelMenu : manages activeMenu
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 2 issues, and left some high level feedback:
- The
restoreAutoCloseTimerin bothSurfacePopup.qmlandTrayItemSurfacePopup.qmlcheckspendingTrayMenu, but that property is set tonullintrayMenuOpenTimerbeforerestoreAutoCloseTimerfires, soautoCloseOnDeactivatewill never be restored; consider storing the menu reference separately or restoringautoCloseOnDeactivateimmediately after opening instead of relying onpendingTrayMenu. - The tray-menu state handling (timers,
pendingTrayMenu,MenuHelper.hasTrayMenuOpen, notification panel updates) is duplicated betweenSurfacePopup.qmlandTrayItemSurfacePopup.qml; consider extracting the shared behavior into a reusable helper or component to keep the logic consistent and easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `restoreAutoCloseTimer` in both `SurfacePopup.qml` and `TrayItemSurfacePopup.qml` checks `pendingTrayMenu`, but that property is set to `null` in `trayMenuOpenTimer` before `restoreAutoCloseTimer` fires, so `autoCloseOnDeactivate` will never be restored; consider storing the menu reference separately or restoring `autoCloseOnDeactivate` immediately after opening instead of relying on `pendingTrayMenu`.
- The tray-menu state handling (timers, `pendingTrayMenu`, `MenuHelper.hasTrayMenuOpen`, notification panel updates) is duplicated between `SurfacePopup.qml` and `TrayItemSurfacePopup.qml`; consider extracting the shared behavior into a reusable helper or component to keep the logic consistent and easier to maintain.
## Individual Comments
### Comment 1
<location path="panels/dock/tray/SurfacePopup.qml" line_range="27-25" />
<code_context>
+ onTriggered: {
+ if (pendingTrayMenu) {
+ pendingTrayMenu.open()
+ pendingTrayMenu = null
+ }
+ }
+ }
+ Timer {
+ id: restoreAutoCloseTimer
+ interval: 200
+ repeat: false
+ onTriggered: {
+ if (pendingTrayMenu) {
+ pendingTrayMenu.autoCloseOnDeactivate = true
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** autoCloseOnDeactivate is never restored because pendingTrayMenu is nulled before the restore timer runs
Because `pendingTrayMenu` is set to `null` immediately after `open()`, `restoreAutoCloseTimer`’s `if (pendingTrayMenu)` check always fails and `autoCloseOnDeactivate` is never reset for that menu. As a result, later `menuWindow.active` changes won’t auto-close the tray menu. You’ll need to either keep a separate reference for the restore timer, delay nulling `pendingTrayMenu` until after the restore runs, or restore `autoCloseOnDeactivate` in a way that doesn’t depend on `pendingTrayMenu` being non-null.
</issue_to_address>
### Comment 2
<location path="panels/dock/tray/TrayItemSurfacePopup.qml" line_range="31-29" />
<code_context>
+ onTriggered: {
+ if (pendingTrayMenu) {
+ pendingTrayMenu.open()
+ pendingTrayMenu = null
+ }
+ }
+ }
+ Timer {
+ id: restoreAutoCloseTimer
+ interval: 200
+ repeat: false
+ onTriggered: {
+ if (pendingTrayMenu) {
+ pendingTrayMenu.autoCloseOnDeactivate = true
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Same autoCloseOnDeactivate restoration issue as in SurfacePopup.qml
Here too, `trayMenuOpenTimer` clears `pendingTrayMenu` after opening, so when `restoreAutoCloseTimer` fires, `pendingTrayMenu` is null and `autoCloseOnDeactivate` is never restored. This leaves `popupMenu.autoCloseOnDeactivate` stuck at `false`, so deactivation won’t close the menu. Please adjust the logic (as in `SurfacePopup.qml`) so the flag is always restored after the first open.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| interval: 150 | ||
| repeat: false | ||
| onTriggered: { | ||
| if (pendingTrayMenu) { |
There was a problem hiding this comment.
issue (bug_risk): autoCloseOnDeactivate is never restored because pendingTrayMenu is nulled before the restore timer runs
Because pendingTrayMenu is set to null immediately after open(), restoreAutoCloseTimer’s if (pendingTrayMenu) check always fails and autoCloseOnDeactivate is never reset for that menu. As a result, later menuWindow.active changes won’t auto-close the tray menu. You’ll need to either keep a separate reference for the restore timer, delay nulling pendingTrayMenu until after the restore runs, or restore autoCloseOnDeactivate in a way that doesn’t depend on pendingTrayMenu being non-null.
| interval: 150 | ||
| repeat: false | ||
| onTriggered: { | ||
| if (pendingTrayMenu) { |
There was a problem hiding this comment.
issue (bug_risk): Same autoCloseOnDeactivate restoration issue as in SurfacePopup.qml
Here too, trayMenuOpenTimer clears pendingTrayMenu after opening, so when restoreAutoCloseTimer fires, pendingTrayMenu is null and autoCloseOnDeactivate is never restored. This leaves popupMenu.autoCloseOnDeactivate stuck at false, so deactivation won’t close the menu. Please adjust the logic (as in SurfacePopup.qml) so the flag is always restored after the first open.
deepin pr auto review这段代码主要实现了在托盘菜单打开时自动关闭通知中心面板，并防止通知中心在托盘菜单打开时显示。以下是对代码的详细审查和改进建议： 1. 代码逻辑审查优点：
潜在问题：
2. 代码质量改进建议
3. 代码性能改进建议
4. 代码安全改进建议
5. 具体代码改进示例改进 // 创建一个可重用的组件
Component {
id: trayMenuHandler
Timer {
id: trayMenuOpenTimer
interval: 150
repeat: false
onTriggered: {
if (pendingTrayMenu) {
pendingTrayMenu.open()
pendingTrayMenu = null
}
}
}
Timer {
id: restoreAutoCloseTimer
interval: 200
repeat: false
onTriggered: {
if (pendingTrayMenu) {
pendingTrayMenu.autoCloseOnDeactivate = true
}
}
}
Connections {
target: menu
function onMenuVisibleChanged() {
if (!menu.menuVisible) {
MenuHelper.hasTrayMenuOpen = false
updateNotifyPanelState(false)
}
}
}
Connections {
target: menu.menuWindow
function onActiveChanged() {
if (menu.menuWindow && !menu.menuWindow.active && !menu.menuVisible) {
MenuHelper.hasTrayMenuOpen = false
updateNotifyPanelState(false)
}
}
}
function updateNotifyPanelState(isOpen) {
let notifyPanel = DS.applet("org.deepin.ds.notificationcenter")
if (notifyPanel) {
notifyPanel.hasTrayMenuOpen = isOpen
}
}
}改进 void NotificationCenterPanel::setBubblePanelEnabled(bool enabled)
{
if (!applet) {
qWarning(notifyLog) << "Applet is null, cannot set bubble panel enabled";
return;
}
QMetaObject::invokeMethod(applet, "setEnabled", Qt::DirectConnection, Q_ARG(bool, enabled));
}改进 void NotificationCenterPanel::setHasTrayMenuOpen(bool hasOpen)
{
if (m_hasTrayMenuOpen == hasOpen)
return;
// 添加状态变更的验证逻辑
if (hasOpen && m_visible) {
qWarning(notifyLog) << "Inconsistent state: tray menu opened while notification center is visible";
setVisible(false);
}
m_hasTrayMenuOpen = hasOpen;
emit hasTrayMenuOpenChanged();
}6. 其他建议
这些改进将有助于提高代码的可维护性、可靠性和性能，同时减少潜在的错误和问题。 |
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
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.
Added tray menu state tracking and automatic panel management to prevent conflicts between tray menus and notification panel. The changes include:
Log: Improved tray menu behavior and fixed conflicts with notification panel
Influence:
PMS: BUG--284867
Summary by Sourcery
Coordinate tray menus with the notification center panel to avoid overlapping and conflicting behaviors.
New Features:
Bug Fixes:
Enhancements: