Repository navigation
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: newerit1 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 |
|
Hi @newerit1. Thanks for your PR. 😃 |
|
Hi @newerit1. Thanks for your PR. I'm waiting for a linuxdeepin member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
CLA Assistant Lite bot: You can retrigger this bot by commenting recheck in this Pull Request |
Reviewer's GuideData structuresAdd persistent state and Qt properties/signals for enabling docks on all screens, including the new dock mirror instance metadata consumed by the shell and dock.
ImplementationCreate, track, and remove mirrored dock loaders per monitor, including delayed creation while hotplugged screen geometry settles and fallback to a single dock when mirroring is disabled.
Scope mirrored dock wake areas to the selected screen and rebuild areas when the dock's target screen changes.
Select and position each mirrored dock on its target monitor, and synchronize the dock and related components with the global appearance theme.
Other changesFiles not grouped.
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 7 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="shell/main.cpp" line_range="149" />
<code_context>
- QObject::connect(loader, &DAppletLoader::failed, qApp, [this, loader, pluginIds](const QString &pluginId) {
- if (pluginIds.contains(pluginId)) {
- m_loaders.removeOne(loader);
- loader->deleteLater();
- }
+ if (m_dockConfig && m_dockConfig->isValid()) {
</code_context>
<issue_to_address>
**Failed docks leave stale pointers**
When a mirrored dock loader fails and its screen is later removed or the dock is retried, the failed-loader handler removes the loader from `m_loaders` and schedules it for deletion without removing it from `m_dockLoaders`. `removeDockForScreen()` can dereference the stale pointer and crash, while `addDockForScreen()` treats the screen as already handled and prevents a retry.
Remove the failed loader's screen entry from `m_dockLoaders` before scheduling the loader for deletion.
Also at `shell/main.cpp:166`, `shell/main.cpp:174-178`.
</issue_to_address>
### Comment 2
<location path="shell/main.cpp" line_range="193" />
<code_context>
+ const auto screens = m_dockLoaders.keys();
+ for (auto *screen : screens)
+ removeDockForScreen(screen);
+ if (m_showOnAllScreens) {
+ for (auto *screen : qApp->screens())
+ addDockForScreen(screen);
</code_context>
<issue_to_address>
**Duplicate docks remain visible**
When all-screen mode is enabled at runtime after starting with it disabled, or is toggled off and on, `setShowOnAllScreens()` removes only loaders in `m_dockLoaders`, but the ordinary dock loader remains in `m_loaders` and is not tracked there. The ordinary dock therefore stays visible alongside the mirrors; toggling back can also create another ordinary dock, so users see duplicates.
Track and remove the ordinary dock loader when changing modes, and avoid creating a replacement while one is still active.
Also at `shell/main.cpp:70-72`, `shell/main.cpp:74`, `shell/main.cpp:194-195`, `shell/main.cpp:197`.
</issue_to_address>
### Comment 3
<location path="shell/main.cpp" line_range="165" />
<code_context>
+ data.insert(QStringLiteral("DockMirror"), true);
+ data.insert(QStringLiteral("DockScreen"), screen->name());
+ data.insert(QStringLiteral("DockMirrorPrimary"), screen == qApp->primaryScreen());
+ if (auto *loader = createLoader(DAppletData(data), QStringList{dockPluginId()})) {
+ m_dockLoaders.insert(screen, loader);
+ if (m_started)
</code_context>
<issue_to_address>
**Dock calls reach the wrong mirror**
When all-screen mode creates more than one dock instance, each `DockPanel` registers the same D-Bus service names and object paths, but consumers have a single endpoint for those names. Calls and properties therefore resolve to whichever mirror registered first instead of the intended screen's dock.
Ensure mirrored docks do not compete for the same D-Bus endpoints, and route dock calls to the intended screen's instance.
Also at `shell/main.cpp:72`.
</issue_to_address>
### Comment 4
<location path="shell/main.cpp" line_range="164" />
<code_context>
+ data.insert(QStringLiteral("Id"), QUuid::createUuid().toString(QUuid::WithoutBraces));
+ data.insert(QStringLiteral("DockMirror"), true);
+ data.insert(QStringLiteral("DockScreen"), screen->name());
+ data.insert(QStringLiteral("DockMirrorPrimary"), screen == qApp->primaryScreen());
+ if (auto *loader = createLoader(DAppletData(data), QStringList{dockPluginId()})) {
+ m_dockLoaders.insert(screen, loader);
</code_context>
<issue_to_address>
**Tray plugins stay on former primary**
When the primary monitor changes while mirrored docks are running, `addDockForScreen()` records `DockMirrorPrimary` only when each mirror is created, and `DockPanel` uses that fixed value to decide which dock loads tray plugins. Existing mirrors are not updated after a primary change, so the former primary keeps the plugins and the new primary does not load them.
Update existing mirrors' primary status and move the tray plugins when the primary screen changes.
Also at `panels/dock/dockpanel.cpp:54-55`.
</issue_to_address>
### Comment 5
<location path="shell/main.cpp" line_range="194" />
<code_context>
- qCWarning(dsLog) << "Loading plugin failed:" << pluginId;
- continue;
+ if (pluginId == dockPluginId() && m_showOnAllScreens) {
+ for (auto *screen : qApp->screens())
+ addDockForScreen(screen);
+ } else {
</code_context>
<issue_to_address>
**Excluded dock appears after toggle**
When the dock was omitted or disabled at startup and the all-screen setting changes, `setShowOnAllScreens()` creates dock loaders without checking whether the dock was included in the original `pluginIds`. A dock excluded at startup therefore appears when mirroring is enabled, and a regular dock can also be created when mirroring is disabled.
Preserve the original dock selection and create dock loaders only when that selection includes the dock.
Also at `shell/main.cpp:197`.
</issue_to_address>
### Comment 6
<location path="panels/dock/dockpanel.cpp" line_range="209" />
<code_context>
+ QMetaObject::invokeMethod(this, &DockPanel::onWindowGeometryChanged);
// 监听屏幕几何信息变化，当主屏位置变化时重新计算 frontendWindowRect
- if (auto screen = window()->screen()) {
+ if (auto screen = m_dockScreen) {
connect(screen, &QScreen::geometryChanged, this, &DockPanel::onWindowGeometryChanged);
}
</code_context>
<issue_to_address>
**Dock stays at old screen coordinates**
When a mirrored monitor's geometry changes after the dock is placed, `onWindowGeometryChanged()` updates the reported frontend rectangle but does not reposition the window. After a monitor geometry or layout change, the mirrored dock stays at its old coordinates instead of the configured screen edge.
Recalculate and apply the window position when the assigned screen's geometry changes.
Also at `panels/dock/dockpanel.cpp:552-553`.
</issue_to_address>
### Comment 7
<location path="panels/dock/dockpanel.cpp" line_range="475" />
<code_context>
+
+void DockPanel::setShowOnAllScreens(bool newShowOnAllScreens)
+{
+ SETTINGS->setShowOnAllScreens(newShowOnAllScreens);
+}
+
</code_context>
<issue_to_address>
**Dock property bindings stay stale**
When the setting changes while a caller observes the `DockPanel` property, `DockSettings` emits `showOnAllScreensChanged`, but `DockPanel` does not forward that signal or emit its own notifier. Bindings and observers of the exposed property therefore keep the old value.
Forward `DockSettings::showOnAllScreensChanged` through `DockPanel::showOnAllScreensChanged`.
Also at `panels/dock/dockpanel.h:35`.
</issue_to_address>| QObject::connect(loader, &DAppletLoader::failed, qApp, [this, loader, rootPluginIds](const QString &pluginId) { | ||
| if (rootPluginIds.contains(pluginId)) { | ||
| m_loaders.removeOne(loader); | ||
| loader->deleteLater(); |
There was a problem hiding this comment.
🟠 High · Failed docks leave stale pointers
When a mirrored dock loader fails and its screen is later removed or the dock is retried, the failed-loader handler removes the loader from m_loaders and schedules it for deletion without removing it from m_dockLoaders. removeDockForScreen() can dereference the stale pointer and crash, while addDockForScreen() treats the screen as already handled and prevents a retry.
Remove the failed loader's screen entry from m_dockLoaders before scheduling the loader for deletion.
Also at shell/main.cpp:166, shell/main.cpp:174-178.
Prompt for AI agents
In `shell/main.cpp` at line 149:
**Failed docks leave stale pointers**
When a mirrored dock loader fails and its screen is later removed or the dock is retried, the failed-loader handler removes the loader from `m_loaders` and schedules it for deletion without removing it from `m_dockLoaders`. `removeDockForScreen()` can dereference the stale pointer and crash, while `addDockForScreen()` treats the screen as already handled and prevents a retry.
Remove the failed loader's screen entry from `m_dockLoaders` before scheduling the loader for deletion.
Also at `shell/main.cpp:166`, `shell/main.cpp:174-178`.| const auto screens = m_dockLoaders.keys(); | ||
| for (auto *screen : screens) | ||
| removeDockForScreen(screen); | ||
| if (m_showOnAllScreens) { |
There was a problem hiding this comment.
🟠 High · Duplicate docks remain visible
When all-screen mode is enabled at runtime after starting with it disabled, or is toggled off and on, setShowOnAllScreens() removes only loaders in m_dockLoaders, but the ordinary dock loader remains in m_loaders and is not tracked there. The ordinary dock therefore stays visible alongside the mirrors; toggling back can also create another ordinary dock, so users see duplicates.
Track and remove the ordinary dock loader when changing modes, and avoid creating a replacement while one is still active.
Also at shell/main.cpp:70-72, shell/main.cpp:74, shell/main.cpp:194-195, shell/main.cpp:197.
Prompt for AI agents
In `shell/main.cpp` at line 193:
**Duplicate docks remain visible**
When all-screen mode is enabled at runtime after starting with it disabled, or is toggled off and on, `setShowOnAllScreens()` removes only loaders in `m_dockLoaders`, but the ordinary dock loader remains in `m_loaders` and is not tracked there. The ordinary dock therefore stays visible alongside the mirrors; toggling back can also create another ordinary dock, so users see duplicates.
Track and remove the ordinary dock loader when changing modes, and avoid creating a replacement while one is still active.
Also at `shell/main.cpp:70-72`, `shell/main.cpp:74`, `shell/main.cpp:194-195`, `shell/main.cpp:197`.| data.insert(QStringLiteral("DockMirror"), true); | ||
| data.insert(QStringLiteral("DockScreen"), screen->name()); | ||
| data.insert(QStringLiteral("DockMirrorPrimary"), screen == qApp->primaryScreen()); | ||
| if (auto *loader = createLoader(DAppletData(data), QStringList{dockPluginId()})) { |
There was a problem hiding this comment.
🟡 Medium · Dock calls reach the wrong mirror
When all-screen mode creates more than one dock instance, each DockPanel registers the same D-Bus service names and object paths, but consumers have a single endpoint for those names. Calls and properties therefore resolve to whichever mirror registered first instead of the intended screen's dock.
Ensure mirrored docks do not compete for the same D-Bus endpoints, and route dock calls to the intended screen's instance.
Also at shell/main.cpp:72.
Prompt for AI agents
In `shell/main.cpp` at line 165:
**Dock calls reach the wrong mirror**
When all-screen mode creates more than one dock instance, each `DockPanel` registers the same D-Bus service names and object paths, but consumers have a single endpoint for those names. Calls and properties therefore resolve to whichever mirror registered first instead of the intended screen's dock.
Ensure mirrored docks do not compete for the same D-Bus endpoints, and route dock calls to the intended screen's instance.
Also at `shell/main.cpp:72`.| data.insert(QStringLiteral("Id"), QUuid::createUuid().toString(QUuid::WithoutBraces)); | ||
| data.insert(QStringLiteral("DockMirror"), true); | ||
| data.insert(QStringLiteral("DockScreen"), screen->name()); | ||
| data.insert(QStringLiteral("DockMirrorPrimary"), screen == qApp->primaryScreen()); |
There was a problem hiding this comment.
🟠 High · Tray plugins stay on former primary
When the primary monitor changes while mirrored docks are running, addDockForScreen() records DockMirrorPrimary only when each mirror is created, and DockPanel uses that fixed value to decide which dock loads tray plugins. Existing mirrors are not updated after a primary change, so the former primary keeps the plugins and the new primary does not load them.
Update existing mirrors' primary status and move the tray plugins when the primary screen changes.
Also at panels/dock/dockpanel.cpp:54-55.
Prompt for AI agents
In `shell/main.cpp` at line 164:
**Tray plugins stay on former primary**
When the primary monitor changes while mirrored docks are running, `addDockForScreen()` records `DockMirrorPrimary` only when each mirror is created, and `DockPanel` uses that fixed value to decide which dock loads tray plugins. Existing mirrors are not updated after a primary change, so the former primary keeps the plugins and the new primary does not load them.
Update existing mirrors' primary status and move the tray plugins when the primary screen changes.
Also at `panels/dock/dockpanel.cpp:54-55`.| for (auto *screen : screens) | ||
| removeDockForScreen(screen); | ||
| if (m_showOnAllScreens) { | ||
| for (auto *screen : qApp->screens()) |
There was a problem hiding this comment.
🟡 Medium · Excluded dock appears after toggle
When the dock was omitted or disabled at startup and the all-screen setting changes, setShowOnAllScreens() creates dock loaders without checking whether the dock was included in the original pluginIds. A dock excluded at startup therefore appears when mirroring is enabled, and a regular dock can also be created when mirroring is disabled.
Preserve the original dock selection and create dock loaders only when that selection includes the dock.
Also at shell/main.cpp:197.
Prompt for AI agents
In `shell/main.cpp` at line 194:
**Excluded dock appears after toggle**
When the dock was omitted or disabled at startup and the all-screen setting changes, `setShowOnAllScreens()` creates dock loaders without checking whether the dock was included in the original `pluginIds`. A dock excluded at startup therefore appears when mirroring is enabled, and a regular dock can also be created when mirroring is disabled.
Preserve the original dock selection and create dock loaders only when that selection includes the dock.
Also at `shell/main.cpp:197`.| QMetaObject::invokeMethod(this, &DockPanel::onWindowGeometryChanged); | ||
| // 监听屏幕几何信息变化，当主屏位置变化时重新计算 frontendWindowRect | ||
| if (auto screen = window()->screen()) { | ||
| if (auto screen = m_dockScreen) { |
There was a problem hiding this comment.
🟡 Medium · Dock stays at old screen coordinates
When a mirrored monitor's geometry changes after the dock is placed, onWindowGeometryChanged() updates the reported frontend rectangle but does not reposition the window. After a monitor geometry or layout change, the mirrored dock stays at its old coordinates instead of the configured screen edge.
Recalculate and apply the window position when the assigned screen's geometry changes.
Also at panels/dock/dockpanel.cpp:552-553.
Prompt for AI agents
In `panels/dock/dockpanel.cpp` at line 209:
**Dock stays at old screen coordinates**
When a mirrored monitor's geometry changes after the dock is placed, `onWindowGeometryChanged()` updates the reported frontend rectangle but does not reposition the window. After a monitor geometry or layout change, the mirrored dock stays at its old coordinates instead of the configured screen edge.
Recalculate and apply the window position when the assigned screen's geometry changes.
Also at `panels/dock/dockpanel.cpp:552-553`.|
|
||
| void DockPanel::setShowOnAllScreens(bool newShowOnAllScreens) | ||
| { | ||
| SETTINGS->setShowOnAllScreens(newShowOnAllScreens); |
There was a problem hiding this comment.
🟡 Medium · Dock property bindings stay stale
When the setting changes while a caller observes the DockPanel property, DockSettings emits showOnAllScreensChanged, but DockPanel does not forward that signal or emit its own notifier. Bindings and observers of the exposed property therefore keep the old value.
Forward DockSettings::showOnAllScreensChanged through DockPanel::showOnAllScreensChanged.
Also at panels/dock/dockpanel.h:35.
Prompt for AI agents
In `panels/dock/dockpanel.cpp` at line 475:
**Dock property bindings stay stale**
When the setting changes while a caller observes the `DockPanel` property, `DockSettings` emits `showOnAllScreensChanged`, but `DockPanel` does not forward that signal or emit its own notifier. Bindings and observers of the exposed property therefore keep the old value.
Forward `DockSettings::showOnAllScreensChanged` through `DockPanel::showOnAllScreensChanged`.
Also at `panels/dock/dockpanel.h:35`.Add an opt-in dock instance for every connected screen. Keep mirrored wake areas and tray loading scoped to the target screen, synchronize the global appearance theme, and recover dock geometry when monitors are hotplugged or their layout is still settling.
Hallo Guys --- this is my idea for a dual monitor with dock mirror
Summary by Sourcery
Support multi-monitor dock mirroring with configurable per-screen instances and synchronized theming.
New Features:
Enhancements: