fix: 关闭弹窗时同时关闭启动器 #1397
fix: 关闭弹窗时同时关闭启动器 #1397
Conversation
在 dock 空白区域右键时，启动器应该随其他弹窗一起关闭。 此修改为 dock 的 onRequestClosePopup 处理器添加了可选的启动器支持。 修改内容： - 添加 launcherControllerWrapper 属性以条件性支持启动器 - 在 Component.onCompleted 中使用 try-catch 初始化包装器以保证兼容性 - 在 onRequestClosePopup 中关闭启动器（如果包装器可用） - 移除延迟菜单定时器，关闭弹窗后直接打开菜单 - 修复 MenuHelper 正确断开 aboutToHide 处理器 PMS: BUG-323289
Reviewer's GuideAdds optional dde-launchpad integration so the launcher closes together with other dock popups, and fixes menu cleanup by correctly managing aboutToHide handlers while simplifying popup/menu timing. Sequence diagram for closing dock popups and optional launchersequenceDiagram
actor User
participant Dock as DockWindow
participant Popup as DockPopup
participant MenuHelper
participant LauncherWrapper as LauncherControllerWrapper
participant LauncherController
participant Menu
User->>Dock: rightClickBlankArea()
Dock->>Dock: onRequestClosePopup()
Dock->>Popup: DS.closeChildrenWindows(popup)
alt popup visible
Dock->>Popup: close()
end
alt launcherControllerWrapper exists
Dock->>LauncherWrapper: hide()
LauncherWrapper->>LauncherController: set visible = false
end
Dock->>MenuHelper: openMenu(contextMenu)
alt activeMenu exists
MenuHelper->>MenuHelper: activeMenu.close()
end
MenuHelper->>MenuHelper: disconnect previous aboutToHide handler for menu
MenuHelper->>MenuHelper: handler = function() { activeMenu = null }
MenuHelper->>Menu: menu.open()
MenuHelper->>Menu: menu.aboutToHide.connect(handler)
MenuHelper->>MenuHelper: activeMenu = menu
Updated class diagram for dock main window and MenuHelperclassDiagram
class DockWindow {
int dockRemainingSpaceForCenter
int dockPartSpacing
real dockItemIconSize
var launcherControllerWrapper
onRequestClosePopup()
onDockScreenChanged()
Component_onCompleted()
}
class LauncherControllerWrapper {
hide()
}
class LauncherController {
bool visible
}
class MenuHelper {
Menu activeMenu
var aboutToHideConnections
openMenu(menu)
closeMenu(menu)
}
class Menu {
open()
close()
aboutToHide
}
DockWindow --> LauncherControllerWrapper : optionally creates
LauncherControllerWrapper --> LauncherController : controls visibility
MenuHelper --> Menu : manages
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:
- Using a plain JS object for
aboutToHideConnectionswithmenuobjects as keys will coerce the keys to strings (e.g.[object Object]), so different menu instances can collide; consider using aMapor attaching the handler directly as a property on themenuinstance to avoid incorrect disconnect behavior. aboutToHideConnectionsnever clears entries when a menu is closed or destroyed, so over time this can accumulate stale references and handlers; consider deleting the entry in theaboutToHidehandler or incloseMenuto keep the mapping in sync with live menus.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using a plain JS object for `aboutToHideConnections` with `menu` objects as keys will coerce the keys to strings (e.g. `[object Object]`), so different menu instances can collide; consider using a `Map` or attaching the handler directly as a property on the `menu` instance to avoid incorrect disconnect behavior.
- `aboutToHideConnections` never clears entries when a menu is closed or destroyed, so over time this can accumulate stale references and handlers; consider deleting the entry in the `aboutToHide` handler or in `closeMenu` to keep the mapping in sync with live menus.
## Individual Comments
### Comment 1
<location> `panels/dock/MenuHelper.qml:11` </location>
<code_context>
Item {
property Menu activeMenu: null
+ property var aboutToHideConnections: ({}) // Store connections by menu object
+
function openMenu(menu: Menu) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Using a plain JS object keyed by menu instances is unreliable because keys are coerced to strings.
Because QML/JS object keys are always strings, indexing with `aboutToHideConnections[menu]` will stringify `menu` (e.g. to `"[object Object]"`), so different `Menu` instances can collide and overwrite each other’s handlers. Instead, store the handler directly on the `menu` instance (e.g. `menu._aboutToHideHandler`) or use a small helper component that keeps the connection alongside the specific `Menu` object.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| Item { | ||
| property Menu activeMenu: null | ||
| property var aboutToHideConnections: ({}) // Store connections by menu object |
There was a problem hiding this comment.
issue (bug_risk): Using a plain JS object keyed by menu instances is unreliable because keys are coerced to strings.
Because QML/JS object keys are always strings, indexing with aboutToHideConnections[menu] will stringify menu (e.g. to "[object Object]"), so different Menu instances can collide and overwrite each other’s handlers. Instead, store the handler directly on the menu instance (e.g. menu._aboutToHideHandler) or use a small helper component that keeps the connection alongside the specific Menu object.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Ivy233 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 |
| 'import QtQuick 2.15; import org.deepin.launchpad 1.0; QtObject { function hide() { LauncherController.visible = false } }', | ||
| dock, "LauncherControllerWrapper") |
There was a problem hiding this comment.
啊？这里的目的仅仅是为了获取LauncherController以便你隐藏launchpad是么？
There was a problem hiding this comment.
onRequestClosePopup 的时候关掉启动器的话，下面那一堆仍然是需要的吗？
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where the dde-launchpad launcher doesn't close when right-clicking on the dock's empty area. The fix adds optional launcher support to the dock's popup close handler while maintaining compatibility when the launcher module is unavailable. Additionally, it improves MenuHelper's signal handler management to prevent stale connections.
Changes:
- Added optional LauncherController wrapper to conditionally close the launcher when dock popups are dismissed
- Fixed MenuHelper to properly track and disconnect aboutToHide signal handlers per menu instance
- Cleaned up trailing whitespace in main.qml
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| panels/dock/package/main.qml | Adds launcherControllerWrapper property with try-catch initialization and integrates launcher hiding into onRequestClosePopup handler; removes trailing whitespace |
| panels/dock/MenuHelper.qml | Implements per-menu connection tracking to prevent handler accumulation and properly disconnect previous handlers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| Item { | ||
| property Menu activeMenu: null | ||
| property var aboutToHideConnections: ({}) // Store connections by menu object |
There was a problem hiding this comment.
Using a Menu object as a JavaScript object key can lead to unexpected behavior because QML objects don't have stable string representations. This could cause memory leaks as old menu instances won't be properly cleaned up from the dictionary. Consider using a WeakMap-like pattern or implementing cleanup logic in a Component.onDestruction handler.
| try { | ||
| menu.aboutToHide.disconnect(aboutToHideConnections[menu]) | ||
| } catch (e) { | ||
| // Silently ignore disconnect errors |
There was a problem hiding this comment.
The try-catch block silently suppresses disconnect errors without logging or clearing the stale entry from aboutToHideConnections. If disconnection fails, the stale handler remains in the dictionary, potentially causing memory leaks. Consider logging the error or removing the entry from aboutToHideConnections after the disconnect attempt.
| // Silently ignore disconnect errors | |
| console.warn("Failed to disconnect aboutToHide handler for menu:", menu, "error:", e) | |
| } finally { | |
| delete aboutToHideConnections[menu] |
| launcherControllerWrapper = Qt.createQmlObject( | ||
| 'import QtQuick 2.15; import org.deepin.launchpad 1.0; QtObject { function hide() { LauncherController.visible = false } }', | ||
| dock, "LauncherControllerWrapper") | ||
| } catch (e) { | ||
| // Launchpad module not available, launcherControllerWrapper remains null |
There was a problem hiding this comment.
The inline QML string creates a tight coupling and makes the code harder to maintain. Consider extracting this to a separate QML file (e.g., LauncherControllerWrapper.qml) that can be loaded conditionally using a Loader or Qt.createComponent, which would improve readability and make the wrapper logic easier to test and modify.
| launcherControllerWrapper = Qt.createQmlObject( | |
| 'import QtQuick 2.15; import org.deepin.launchpad 1.0; QtObject { function hide() { LauncherController.visible = false } }', | |
| dock, "LauncherControllerWrapper") | |
| } catch (e) { | |
| // Launchpad module not available, launcherControllerWrapper remains null | |
| var launcherComponent = Qt.createComponent("LauncherControllerWrapper.qml"); | |
| if (launcherComponent.status === Component.Ready) { | |
| launcherControllerWrapper = launcherComponent.createObject(dock); | |
| } | |
| } catch (e) { | |
| // Launchpad module or wrapper component not available, launcherControllerWrapper remains null |
|
|
||
| // Try to create LauncherController wrapper for optional launchpad support | ||
| try { | ||
| launcherControllerWrapper = Qt.createQmlObject( |
There was a problem hiding this comment.
这种不应该在dde-shell中依赖launchpad，
4 participants
在 dock 空白区域右键时，启动器应该随其他弹窗一起关闭。
此修改为 dock 的 onRequestClosePopup 处理器添加了可选的启动器支持。
修改内容：
PMS: BUG-323289
Summary by Sourcery
Ensure the dock closes the launcher along with other popups and improve menu cleanup handling.
Bug Fixes:
Enhancements: