feat(display): integrate ScreenScale service for scale management#3176
Open
fly602 wants to merge 1 commit intolinuxdeepin:masterfrom
Open
feat(display): integrate ScreenScale service for scale management#3176fly602 wants to merge 1 commit intolinuxdeepin:masterfrom
fly602 wants to merge 1 commit intolinuxdeepin:masterfrom
Conversation
Use ScreenScale service as the single source of truth for display scale: - Get scale factor, available scales and recommended scale from ScreenScale - Set scale via ScreenScale.SetScaleFactor() method - Remove XSettings scale interfaces, simplify code structure - Add PropertiesChanged signal listener for real-time updates Log: 集成ScreenScale服务统一管理显示缩放 PMS: BUG-355227 Influence: 缩放数据从ScreenScale服务获取，设置缩放通过ScreenScale接口，简化代码结构 Change-Id: I53aa2c588631543f2d83234adb16c7f2eb2c7b4a
Reviewer's GuideIntegrates the org.deepin.dde.ScreenScale1 D-Bus service as the single source of truth for UI scaling, wiring its properties and signals into the DisplayDBusProxy/DisplayWorker/DisplayModule stack and QML, while removing the old XSettings-based max scale calculation and asynchronous GetScaleFactor calls. Sequence diagram for setting global scale via ScreenScale servicesequenceDiagram
actor User
participant DisplayMain_qml
participant DisplayModule
participant DisplayWorker
participant DisplayDBusProxy
participant ScreenScaleService as org.deepin_dde_ScreenScale1
participant DisplayModel
User ->> DisplayMain_qml: change scale selection
DisplayMain_qml ->> DisplayModule: setGlobalScale(scale)
DisplayModule ->> DisplayWorker: setUiScale(scale)
alt isTreeland
DisplayWorker ->> DisplayDBusProxy: SetWaylandUIScale(scale)
DisplayWorker ->> DisplayModel: setUIScale(scale)
else X11_or_other
DisplayWorker ->> DisplayDBusProxy: SetScreenScaleFactor(scale)
DisplayDBusProxy ->> ScreenScaleService: SetScaleFactor(scale)
DisplayWorker ->> DisplayModel: setUIScale(scale)
end
ScreenScaleService -->> DisplayDBusProxy: PropertiesChanged ScaleFactor
DisplayDBusProxy ->> DisplayWorker: ScreenScaleFactorChanged(value)
DisplayWorker ->> DisplayModel: setUIScale(value)
Sequence diagram for initializing scale and available scales from ScreenScalesequenceDiagram
participant DisplayWorker
participant DisplayDBusProxy
participant ScreenScaleService as org.deepin_dde_ScreenScale1
participant DisplayModel
DisplayWorker ->> DisplayDBusProxy: active()
Note over DisplayWorker,DisplayDBusProxy: non Treeland branch
DisplayWorker ->> DisplayDBusProxy: screenScaleFactor()
DisplayDBusProxy ->> ScreenScaleService: read property ScaleFactor
ScreenScaleService -->> DisplayDBusProxy: ScaleFactor
DisplayDBusProxy -->> DisplayWorker: double
DisplayWorker ->> DisplayModel: setUIScale(scaleFactor_or_1_0)
DisplayWorker ->> DisplayDBusProxy: availableScales()
DisplayDBusProxy ->> ScreenScaleService: read property AvailableScales
ScreenScaleService -->> DisplayDBusProxy: AvailableScales
DisplayWorker ->> DisplayDBusProxy: recommendedScale()
DisplayDBusProxy ->> ScreenScaleService: read property RecommendedScale
ScreenScaleService -->> DisplayDBusProxy: RecommendedScale
ScreenScaleService -->> DisplayDBusProxy: PropertiesChanged AvailableScales RecommendedScale
DisplayDBusProxy ->> DisplayModule: AvailableScalesChanged
DisplayDBusProxy ->> DisplayModule: RecommendedScaleChanged
Class diagram for updated scaling-related classesclassDiagram
class DisplayDBusProxy {
+DisplayDBusProxy(QObject *parent)
+BrightnessMap brightness()
+QDBusPendingReply~<>~ ApplyChanges()
+double screenScaleFactor() const
+double recommendedScale() const
+QList~double~ availableScales() const
+QDBusPendingReply~<>~ SetScreenScaleFactor(double factor)
+void onScreenScalePropertiesChanged(QString interface, QVariantMap changedProperties, QStringList invalidatedProperties)
+void ScreenScaleFactorChanged(double value) const
+void AvailableScalesChanged() const
+void RecommendedScaleChanged() const
+void BrightnessChanged(BrightnessMap value) const
+void ColorTemperatureEnabledChanged(bool value) const
-Dtk::Core::DDBusInterface *m_dBusDisplayInter
-Dtk::Core::DDBusInterface *m_dBusSystemDisplayInter
-Dtk::Core::DDBusInterface *m_dBusAppearanceInter
-Dtk::Core::DDBusInterface *m_dBusPowerInter
-Dtk::Core::DDBusInterface *m_dBusScreenScaleInter
-void init()
}
class DisplayWorker {
+DisplayWorker(DisplayModel *model, QObject *parent, bool isSync)
+~DisplayWorker()
+void active()
+void saveChanges()
+void switchMode(int mode, QString name)
+void setUiScale(double value)
+void setIndividualScaling(Monitor *m, double scaling)
+DisplayDBusProxy *dbusProxy() const
-void onMonitorListChanged(QList~QDBusObjectPath~ mons)
-void onMonitorsBrightnessChanged(BrightnessMap brightness)
-void onGetScreenScalesFinished(QDBusPendingCallWatcher *w)
-DisplayModel *m_model
-DisplayDBusProxy *m_displayInter
}
class DisplayModule {
<<QObject>>
+Q_PROPERTY(bool isX11)
+Q_PROPERTY(qreal globalScale)
+Q_PROPERTY(qreal maxGlobalScale)
+Q_PROPERTY(QList~qreal~ availableScales)
+Q_PROPERTY(qreal recommendedScale)
+bool isX11() const
+qreal globalScale() const
+void setGlobalScale(qreal scale)
+qreal maxGlobalScale() const
+QList~qreal~ availableScales() const
+qreal recommendedScale() const
+bool colorTemperatureEnabled() const
+void setColorTemperatureEnabled(bool enabled)
+void globalScaleChanged()
+void maxGlobalScaleChanged()
+void availableScalesChanged()
+void recommendedScaleChanged()
-DisplayModulePrivate *d_ptr
}
class DisplayModulePrivate {
+DisplayModulePrivate(DisplayModule *parent)
+void init()
+void updateMonitorList()
+void updatePrimary()
+void updateDisplayMode()
+DccScreen *primary() const
+QString displayMode() const
+void setScreenPosition(QList~ScreenData*~ screensData)
-DisplayWorker *m_worker
-DisplayModel *m_model
-QList~DccScreen*~ m_screens
-QList~DccScreen*~ m_virtualScreens
-DccScreen *m_primary
-QString m_displayMode
}
class DisplayModel {
+void setUIScale(double value)
+void uiscaleChanged()
+void colorTemperatureChanged()
+void customColorTempTimePeriodChanged()
+void adjustCCTmodeChanged()
}
class DisplayMain_qml {
+function getScaleModel(maxScale, scale)
+property real globalScale
+property real maxGlobalScale
+property var availableScales
+property real recommendedScale
}
DisplayWorker --> DisplayDBusProxy : uses
DisplayWorker --> DisplayModel : updates
DisplayModulePrivate --> DisplayWorker : holds
DisplayModulePrivate --> DisplayModel : holds
DisplayModule --> DisplayModulePrivate : aggregates
DisplayMain_qml --> DisplayModule : binds_Q_PROPERTY
DisplayDBusProxy --> DisplayModel : ScreenScaleFactorChanged_to_setUIScale
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 3 issues, and left some high level feedback:
- In
DisplayWorker::setUiScaleyou switched from a synchronousSetScaleFactorcall with error handling to a fire-and-forgetSetScreenScaleFactorwhile unconditionally updatingm_model->setUIScale(rv); consider at least checking theQDBusPendingReplyfor errors or deferring the model update until the DBus call succeeds to avoid UI state diverging from the compositor. - In
DisplayWorker::activethe calls tom_displayInter->availableScales()andrecommendedScale()are unused and just perform synchronous DBus property accesses; either use their results (e.g. to initialize related model fields) or remove them to avoid unnecessary blocking calls during activation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `DisplayWorker::setUiScale` you switched from a synchronous `SetScaleFactor` call with error handling to a fire-and-forget `SetScreenScaleFactor` while unconditionally updating `m_model->setUIScale(rv)`; consider at least checking the `QDBusPendingReply` for errors or deferring the model update until the DBus call succeeds to avoid UI state diverging from the compositor.
- In `DisplayWorker::active` the calls to `m_displayInter->availableScales()` and `recommendedScale()` are unused and just perform synchronous DBus property accesses; either use their results (e.g. to initialize related model fields) or remove them to avoid unnecessary blocking calls during activation.
## Individual Comments
### Comment 1
<location path="src/plugin-display/operation/private/displayworker.cpp" line_range="602" />
<code_context>
void DisplayWorker::setUiScale(const double value)
</code_context>
<issue_to_address>
**issue (bug_risk):** DBus call result is ignored when setting screen scale, which can desync UI state from actual compositor state.
Previously the non-Treeland path only updated `m_model->setUIScale(rv)` after `SetScaleFactor` completed successfully. The new code fire-and-forgets `SetScreenScaleFactor(rv)` and always updates the model, so any DBus failure (missing service, permission error, runtime failure) will leave the UI showing a scale that wasn’t applied. Please either wait on the DBus reply (e.g. via `QDBusPendingCallWatcher`) and update the model only on success, or at least detect errors and revert/refresh the value from `screenScaleFactor()` when the call fails.
</issue_to_address>
### Comment 2
<location path="src/plugin-display/operation/displaymodule.cpp" line_range="361-365" />
<code_context>
qreal DisplayModule::maxGlobalScale() const
{
Q_D(const DisplayModule);
- return d->m_maxGlobalScale;
+ auto scales = d->m_worker->dbusProxy()->availableScales();
+ return scales.isEmpty() ? 1.0 : scales.last();
+}
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Assuming `availableScales()` is sorted may be fragile for deriving `maxGlobalScale`.
This now relies on `availableScales()` returning an ascending list and uses `scales.last()` as the max, but that ordering isn’t guaranteed here. If the backend changes order or returns unsorted data, the max scale in the UI could be incorrect. Consider computing the maximum explicitly with `*std::max_element(scales.cbegin(), scales.cend())` instead of depending on list ordering.
Suggested implementation:
```cpp
qreal DisplayModule::maxGlobalScale() const
{
Q_D(const DisplayModule);
const auto scales = d->m_worker->dbusProxy()->availableScales();
if (scales.isEmpty()) {
return 1.0;
}
return *std::max_element(scales.cbegin(), scales.cend());
}
```
To compile, this file must include the `<algorithm>` header. If it is not already present at the top of `displaymodule.cpp`, add:
`#include <algorithm>`
alongside the other standard library includes.
</issue_to_address>
### Comment 3
<location path="src/plugin-display/operation/private/displayworker.cpp" line_range="85-95" />
<code_context>
- QDBusPendingCallWatcher *scalewatcher = new QDBusPendingCallWatcher(m_displayInter->GetScaleFactor());
- connect(scalewatcher, &QDBusPendingCallWatcher::finished, this, &DisplayWorker::onGetScaleFinished);
+ m_displayInter->availableScales();
+ m_displayInter->recommendedScale();
QDBusPendingCallWatcher *screenscaleswatcher = new QDBusPendingCallWatcher(m_displayInter->GetScreenScaleFactors());
</code_context>
<issue_to_address>
**suggestion (performance):** Calls to `availableScales()` and `recommendedScale()` in `active()` have no observable effect.
These calls fetch values that are never used, so they only introduce extra synchronous DBus traffic during startup. Unless they are intentionally used to force some side-effect (in which case that purpose should be clearly documented), they should be removed or the returned values should be used to initialize or validate related model state (e.g., cache `recommendedScale` or check the current scale against `availableScales`).
```suggestion
double scaleFactor = m_displayInter->screenScaleFactor();
if (scaleFactor <= 0) {
scaleFactor = 1.0;
}
m_model->setUIScale(scaleFactor);
QDBusPendingCallWatcher *screenscaleswatcher = new QDBusPendingCallWatcher(m_displayInter->GetScreenScaleFactors());
connect(screenscaleswatcher, &QDBusPendingCallWatcher::finished, this, &DisplayWorker::onGetScreenScalesFinished);
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
| watcher->deleteLater(); | ||
| // 通过 ScreenScale 设置缩放（作为唯一事实来源，会同步到 XSettings） | ||
| m_displayInter->SetScreenScaleFactor(rv); |
There was a problem hiding this comment.
issue (bug_risk): DBus call result is ignored when setting screen scale, which can desync UI state from actual compositor state.
Previously the non-Treeland path only updated m_model->setUIScale(rv) after SetScaleFactor completed successfully. The new code fire-and-forgets SetScreenScaleFactor(rv) and always updates the model, so any DBus failure (missing service, permission error, runtime failure) will leave the UI showing a scale that wasn’t applied. Please either wait on the DBus reply (e.g. via QDBusPendingCallWatcher) and update the model only on success, or at least detect errors and revert/refresh the value from screenScaleFactor() when the call fails.
Comment on lines
361
to
+365
| qreal DisplayModule::maxGlobalScale() const | ||
| { | ||
| Q_D(const DisplayModule); | ||
| return d->m_maxGlobalScale; | ||
| auto scales = d->m_worker->dbusProxy()->availableScales(); | ||
| return scales.isEmpty() ? 1.0 : scales.last(); |
There was a problem hiding this comment.
suggestion (bug_risk): Assuming availableScales() is sorted may be fragile for deriving maxGlobalScale.
This now relies on availableScales() returning an ascending list and uses scales.last() as the max, but that ordering isn’t guaranteed here. If the backend changes order or returns unsorted data, the max scale in the UI could be incorrect. Consider computing the maximum explicitly with *std::max_element(scales.cbegin(), scales.cend()) instead of depending on list ordering.
Suggested implementation:
qreal DisplayModule::maxGlobalScale() const
{
Q_D(const DisplayModule);
const auto scales = d->m_worker->dbusProxy()->availableScales();
if (scales.isEmpty()) {
return 1.0;
}
return *std::max_element(scales.cbegin(), scales.cend());
}
To compile, this file must include the <algorithm> header. If it is not already present at the top of displaymodule.cpp, add:
#include <algorithm>
alongside the other standard library includes.
Comment on lines
+85
to
95
| double scaleFactor = m_displayInter->screenScaleFactor(); | ||
| if (scaleFactor <= 0) { | ||
| scaleFactor = 1.0; | ||
| } | ||
| m_model->setUIScale(scaleFactor); | ||
|
|
||
| QDBusPendingCallWatcher *scalewatcher = new QDBusPendingCallWatcher(m_displayInter->GetScaleFactor()); | ||
| connect(scalewatcher, &QDBusPendingCallWatcher::finished, this, &DisplayWorker::onGetScaleFinished); | ||
| m_displayInter->availableScales(); | ||
| m_displayInter->recommendedScale(); | ||
|
|
||
| QDBusPendingCallWatcher *screenscaleswatcher = new QDBusPendingCallWatcher(m_displayInter->GetScreenScaleFactors()); | ||
| connect(screenscaleswatcher, &QDBusPendingCallWatcher::finished, this, &DisplayWorker::onGetScreenScalesFinished); |
There was a problem hiding this comment.
suggestion (performance): Calls to availableScales() and recommendedScale() in active() have no observable effect.
These calls fetch values that are never used, so they only introduce extra synchronous DBus traffic during startup. Unless they are intentionally used to force some side-effect (in which case that purpose should be clearly documented), they should be removed or the returned values should be used to initialize or validate related model state (e.g., cache recommendedScale or check the current scale against availableScales).
Suggested change
| double scaleFactor = m_displayInter->screenScaleFactor(); | |
| if (scaleFactor <= 0) { | |
| scaleFactor = 1.0; | |
| } | |
| m_model->setUIScale(scaleFactor); | |
| QDBusPendingCallWatcher *scalewatcher = new QDBusPendingCallWatcher(m_displayInter->GetScaleFactor()); | |
| connect(scalewatcher, &QDBusPendingCallWatcher::finished, this, &DisplayWorker::onGetScaleFinished); | |
| m_displayInter->availableScales(); | |
| m_displayInter->recommendedScale(); | |
| QDBusPendingCallWatcher *screenscaleswatcher = new QDBusPendingCallWatcher(m_displayInter->GetScreenScaleFactors()); | |
| connect(screenscaleswatcher, &QDBusPendingCallWatcher::finished, this, &DisplayWorker::onGetScreenScalesFinished); | |
| double scaleFactor = m_displayInter->screenScaleFactor(); | |
| if (scaleFactor <= 0) { | |
| scaleFactor = 1.0; | |
| } | |
| m_model->setUIScale(scaleFactor); | |
| QDBusPendingCallWatcher *screenscaleswatcher = new QDBusPendingCallWatcher(m_displayInter->GetScreenScaleFactors()); | |
| connect(screenscaleswatcher, &QDBusPendingCallWatcher::finished, this, &DisplayWorker::onGetScreenScalesFinished); |
caixr23
approved these changes
Apr 15, 2026
deepin pr auto review这份代码变更主要针对显示模块的缩放功能进行了重构，将原本在本地计算最大缩放比例的逻辑改为直接通过DBus从 1. 语法逻辑1.1 正确的部分
1.2 潜在问题
2. 代码质量2.1 优点
2.2 改进建议
3. 代码性能3.1 优点
3.2 改进建议
4. 代码安全4.1 潜在问题
5. 其他建议
总结这次代码变更总体上改进了显示模块的缩放功能实现，简化了逻辑，提高了可维护性。主要需要关注的是DBus接口的健壮性和错误处理，以及确保服务依赖的可靠性。建议在合并前进行充分的测试，特别是针对 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: caixr23, fly602 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 |
|
TAG Bot New tag: 6.1.82 |
|
TAG Bot New tag: 6.1.83 |
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.
Use ScreenScale service as the single source of truth for display scale:
Log: 集成ScreenScale服务统一管理显示缩放
PMS: BUG-355227
Influence: 缩放数据从ScreenScale服务获取，设置缩放通过ScreenScale接口，简化代码结构
Change-Id: I53aa2c588631543f2d83234adb16c7f2eb2c7b4a
Summary by Sourcery
Integrate the ScreenScale D-Bus service as the unified source of truth for display scaling and expose its data through the display module and QML UI.
New Features:
Enhancements: