feat: refactor wallpaper slideshow for multi-monitor support - #29
Merged
Merged
Conversation
1. Added getCurrentWorkspaceBackgroundForMonitor method to retrieve wallpaper for specific monitor 2. Refactored configuration storage to use monitor names directly instead of workspace-monitor combinations 3. Implemented per-monitor wallpaper type tracking to handle different wallpaper types on different screens 4. Added screen validation to ensure operations only affect valid monitors 5. Updated configuration migration to convert old workspace-monitor format to monitor-only format 6. Improved wallpaper change detection to handle multi-monitor scenarios Log: Enhanced wallpaper slideshow to better support multi-monitor setups Influence: 1. Test wallpaper slideshow on single monitor setup 2. Test wallpaper slideshow on multi-monitor setup with different wallpapers 3. Verify configuration migration from old format to new format 4. Test wallpaper change detection when switching between different wallpaper types 5. Verify slideshow scheduling works correctly for each monitor independently 6. Test with invalid monitor names to ensure proper error handling feat: 重构壁纸轮播功能以支持多显示器 1. 新增 getCurrentWorkspaceBackgroundForMonitor 方法用于获取特定显示器的 壁纸 2. 重构配置存储，直接使用显示器名称而非工作区-显示器组合 3. 实现按显示器跟踪壁纸类型，以处理不同屏幕上的不同壁纸类型 4. 添加屏幕验证确保操作仅影响有效显示器 5. 更新配置迁移以将旧的工作区-显示器格式转换为仅显示器格式 6. 改进壁纸变更检测以处理多显示器场景 Log: 增强壁纸轮播功能以更好地支持多显示器设置 Influence: 1. 在单显示器设置下测试壁纸轮播功能 2. 在多显示器设置下测试壁纸轮播功能，使用不同壁纸 3. 验证从旧格式到新格式的配置迁移 4. 测试在不同壁纸类型之间切换时的壁纸变更检测 5. 验证每个显示器的轮播调度独立正常工作 6. 使用无效显示器名称测试以确保正确的错误处理 pms: BUG-333269 pms: BUG-333263
Reviewer's GuideRefactors the wallpaper slideshow manager to fully support multi-monitor setups by switching configuration keys to per-monitor, migrating legacy workspace-monitor settings, validating monitors before operations, and tracking wallpaper types and scheduling independently for each screen. Sequence diagram for per-monitor wallpaper change detection and updatesequenceDiagram
participant "SlideshowManager"
participant "QApplication"
participant "QScreen"
participant "AppearanceDBusProxy"
participant "Backgrounds"
"SlideshowManager"->>"QApplication": get screens()
loop for each screen
"SlideshowManager"->>"QScreen": get name()
"SlideshowManager"->>"AppearanceDBusProxy": getCurrentWorkspaceBackgroundForMonitor(screenName)
"SlideshowManager"->>"Backgrounds": getBackgroundType(wallpaper)
alt wallpaper type changed
"SlideshowManager"->>"Backgrounds": update type for screen
end
end
alt any wallpaper type changed
"SlideshowManager"->>"SlideshowManager": updateWSPolicy()
"SlideshowManager"->>"WallpaperLoop": updateLoopList()
end
Entity relationship diagram for wallpaper slideshow configuration migrationerDiagram
CONFIG_OLD {
STRING key
STRING value
}
CONFIG_NEW {
STRING monitorName
STRING value
}
CONFIG_OLD ||--o| CONFIG_NEW : "migrates to"
Class diagram for updated SlideshowManager and AppearanceDBusProxyclassDiagram
class SlideshowManager {
+bool doSetWallpaperSlideShow(monitorName, wallpaperSlideShow)
+QString doGetWallpaperSlideShow(monitorName)
+void updateWSPolicy(policy)
+void loadWSConfig()
+void autoChangeBg(monitorSpace, date)
+void init()
+void loadConfig()
+void handlePrepareForSleep(sleep)
+void onWallpaperChanged()
+bool isValidScreen(screenName)
-QMap<QString, QSharedPointer<WallpaperLoop>> m_wsLoopMap
-QSharedPointer<AppearanceDBusProxy> m_dbusProxy
-QString m_wallpaperSlideShow
-QMap<QString, Backgrounds::BackgroundType> m_wallpaperType
}
class AppearanceDBusProxy {
+QString getCurrentWorkspaceBackground()
+QString getCurrentWorkspaceBackgroundForMonitor(monitor)
+void SetCurrentWorkspaceBackgroundForMonitor(url, screenName)
+void SetGreeterBackground(url)
}
SlideshowManager --> AppearanceDBusProxy : uses
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 there - I've reviewed your changes - here's some feedback:
- doSetWallpaperSlideShow currently swallows JSON parse errors and proceeds with an empty object, so consider logging parse failures or aborting to avoid unintentionally wiping existing keys.
- loadConfig always rewrites the migrated JSON back to DConfig on every startup, which could be optimized by applying the migration only once to reduce repeated writes.
- Relying on QScreen::name() for monitor identity may be fragile across reboots or hardware changes—consider using a more stable identifier like screen serial number or index.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- doSetWallpaperSlideShow currently swallows JSON parse errors and proceeds with an empty object, so consider logging parse failures or aborting to avoid unintentionally wiping existing keys.
- loadConfig always rewrites the migrated JSON back to DConfig on every startup, which could be optimized by applying the migration only once to reduce repeated writes.
- Relying on QScreen::name() for monitor identity may be fragile across reboots or hardware changes—consider using a more stable identifier like screen serial number or index.
## Individual Comments
### Comment 1
<location> `src/plugin-qt/wallpaperslideshow/slideshowmanager.cpp:41-45` </location>
<code_context>
+ qWarning() << "monitor can not found: " << monitorName;
+ return false;
+ }
+ QByteArray jsonData = m_wallpaperSlideShow.toUtf8();
+ QJsonParseError err;
+ QJsonDocument doc = QJsonDocument::fromJson(jsonData, &err);
- QJsonDocument doc = QJsonDocument::fromJson(wallpaperSlideShow.toLatin1());
- QJsonObject cfgObj = doc.object();
+ QJsonObject cfgObj;
+ if (err.error == QJsonParseError::NoError && doc.isObject()) {
+ cfgObj = doc.object();
</code_context>
<issue_to_address>
**issue (bug_risk):** Potential confusion between input and member variable usage for wallpaperSlideShow.
The method uses 'm_wallpaperSlideShow' for JSON parsing instead of the 'wallpaperSlideShow' parameter, which may not reflect the intended behavior. Please verify if the correct variable is being used.
</issue_to_address>
### Comment 2
<location> `src/plugin-qt/wallpaperslideshow/slideshowmanager.cpp:82` </location>
<code_context>
-
- if (tempMap.count(key) == 1) {
- return tempMap[key].toString();
+ if (tempMap.count(monitorName) == 1) {
+ return tempMap[monitorName].toString();
}
</code_context>
<issue_to_address>
**nitpick:** Using count() == 1 for key existence is less idiomatic than contains().
Consider replacing count(monitorName) == 1 with contains(monitorName) for improved readability and idiomatic usage.
</issue_to_address>
### Comment 3
<location> `src/plugin-qt/wallpaperslideshow/slideshowmanager.cpp:116-119` </location>
<code_context>
}
if (m_wsLoopMap.count(iter.first) == 0) {
- m_wsLoopMap[iter.first] = QSharedPointer<WallpaperLoop>(new WallpaperLoop(m_wallpaperType));
+ m_wsLoopMap[iter.first] = QSharedPointer<WallpaperLoop>(new WallpaperLoop(m_wallpaperType[screenName]));
}
- m_wsLoopMap[iter.first]->updateWallpaperType(m_wallpaperType);
+ m_wsLoopMap[iter.first]->updateWallpaperType(m_wallpaperType[screenName]);
- if (m_curMonitorSpace == iter.first && WallpaperLoopConfigManger::isValidWSPolicy(iter.second.toString())) {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Assumes m_wallpaperType[screenName] is always valid; may cause issues if missing.
Accessing m_wallpaperType[screenName] without validation may lead to default or unintended values. Please ensure m_wallpaperType is properly initialized for all relevant screens before use.
```suggestion
if (!m_wallpaperType.contains(screenName)) {
qWarning() << "Wallpaper type not initialized for screen:" << screenName;
continue;
}
if (m_wsLoopMap.count(iter.first) == 0) {
m_wsLoopMap[iter.first] = QSharedPointer<WallpaperLoop>(new WallpaperLoop(m_wallpaperType[screenName]));
}
m_wsLoopMap[iter.first]->updateWallpaperType(m_wallpaperType[screenName]);
```
</issue_to_address>
### Comment 4
<location> `src/plugin-qt/wallpaperslideshow/slideshowmanager.cpp:255` </location>
<code_context>
void SlideshowManager::loadConfig()
{
- m_wallpaperSlideShow = m_settingDconfig->value(GSKEYWALLPAPERSLIDESHOW).toString();
+ QFile::remove(WS_CONFIG_PATH);
+
+ const QString wallpaperSlideShow = m_settingDconfig->value(GSKEYWALLPAPERSLIDESHOW).toString();
</code_context>
<issue_to_address>
**issue (bug_risk):** Unconditionally removing WS_CONFIG_PATH may cause data loss.
If removing the config file is required, implement a backup or recovery process to prevent data loss in case of failure.
</issue_to_address>
### Comment 5
<location> `src/plugin-qt/wallpaperslideshow/slideshowmanager.cpp:270-274` </location>
<code_context>
+ QJsonObject newObject;
+
+ // 兼容老配置，去掉&&
+ for (auto it = rootObject.begin(); it != rootObject.end(); ++it) {
+ QString key = it.key();
+ QJsonValue value = it.value();
+
+ if (key.contains("&&")) {
+ QString newKey = key.split("&&").first();
+ newObject[newKey] = value;
</code_context>
<issue_to_address>
**issue (bug_risk):** Key migration logic may not handle duplicate keys after splitting.
Splitting keys with '&&' may overwrite values if multiple original keys produce the same newKey. Please add logic to detect and handle such duplicates to prevent data loss.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the wallpaper slideshow feature to properly support multi-monitor configurations by tracking wallpaper state per-monitor instead of per-workspace-monitor combination.
Key Changes:
- Migrated configuration storage from
workspace&&monitorformat to monitor-only keys - Changed wallpaper type tracking from single value to per-monitor map
- Added per-monitor wallpaper retrieval and validation
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| slideshowmanager.h | Changed m_wallpaperType from single value to per-monitor map; added screen validation method |
| slideshowmanager.cpp | Implemented monitor-based configuration storage, added config migration logic, and per-monitor wallpaper change detection |
| commondefine.h | Added WS_CONFIG_PATH constant to centralize config file path |
| appearancedbusproxy.h | Added getCurrentWorkspaceBackgroundForMonitor method declaration |
| appearancedbusproxy.cpp | Implemented monitor-specific wallpaper retrieval method |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
18202781743
approved these changes
Oct 21, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, mhduiy 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 |
Contributor
Author
|
/forcemerge |
|
This pr force merged! (status: blocked) |
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
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: Enhanced wallpaper slideshow to better support multi-monitor setups
Influence:
feat: 重构壁纸轮播功能以支持多显示器
Log: 增强壁纸轮播功能以更好地支持多显示器设置
Influence:
pms: BUG-333269
pms: BUG-333263
Summary by Sourcery
Refactor the wallpaper slideshow system to support multi-monitor setups by keying configurations and tracking per-monitor wallpaper state
New Features:
Enhancements: