Repository navigation
fix: apply scale factor to GTK cursor theme size - #57
Merged
Merged
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates GTK cursor theme size handling so that the derived gtk-cursor-theme-size is computed from the base size multiplied by the current scale factor, with validation and logging for debugging. Sequence diagram for applying scale factor to GTK cursor theme sizesequenceDiagram
actor User
participant Application
participant XSettingsManager
participant DConfigStore
User->>Application: Change cursor size or display scaling
Application->>XSettingsManager: handleDConfigChangedCb(key = gtk-cursor-theme-size-base)
XSettingsManager->>DConfigStore: value(gtk-cursor-theme-size-base)
DConfigStore-->>XSettingsManager: cursorSizeBase:int
XSettingsManager->>DConfigStore: value(dcKeyScaleFactor)
DConfigStore-->>XSettingsManager: scale:double
alt invalid scale (scale <= 0)
XSettingsManager->>XSettingsManager: scale = 1.0
XSettingsManager->>Application: qWarning invalid scale factor
end
XSettingsManager->>Application: qWarning update gtk-cursor-theme-size
XSettingsManager->>DConfigStore: setValue(gtk-cursor-theme-size, cursorSizeBase * scale)
DConfigStore-->>XSettingsManager: ack
XSettingsManager-->>Application: return
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:
- The scale factor logging on every
gtk-cursor-theme-size-basechange usesqWarning(), which may be too noisy for normal operation; consider using a lower log level (e.g.,qDebug()or a conditional debug flag) for the non-error update message. - The multiplication of
cursorSizeBase(int) andscale(double) produces a double that is passed directly tosetValue; if the downstream consumer expects an integer cursor size, consider explicitly rounding or casting and documenting the chosen behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The scale factor logging on every `gtk-cursor-theme-size-base` change uses `qWarning()`, which may be too noisy for normal operation; consider using a lower log level (e.g., `qDebug()` or a conditional debug flag) for the non-error update message.
- The multiplication of `cursorSizeBase` (int) and `scale` (double) produces a double that is passed directly to `setValue`; if the downstream consumer expects an integer cursor size, consider explicitly rounding or casting and documenting the chosen behavior.
## Individual Comments
### Comment 1
<location path="src/plugin-qt/xsettings/impl/xsettingsmanager.cpp" line_range="222-227" />
<code_context>
+ qWarning() << "invalid scale factor:" << scale << ", fallback to 1.0";
+ scale = 1.0;
+ }
+ qWarning() << "update gtk-cursor-theme-size to" << cursorSizeBase * scale;
+ m_settingDconfig->setValue("gtk-cursor-theme-size", cursorSizeBase * scale);
return;
</code_context>
<issue_to_address>
**suggestion:** Using qWarning for the normal update path may be too noisy in production logs.
Logging every successful update with `qWarning` will clutter production logs and may be interpreted as errors by log aggregation tools. Use `qDebug` (or a categorized debug log) for the normal update, and keep `qWarning` for exceptional cases like the invalid scale path.
```suggestion
if (scale <= 0) {
qWarning() << "invalid scale factor:" << scale << ", fallback to 1.0";
scale = 1.0;
}
qDebug() << "update gtk-cursor-theme-size to" << cursorSizeBase * scale;
m_settingDconfig->setValue("gtk-cursor-theme-size", cursorSizeBase * scale);
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
When the DConfig key "gtk-cursor-theme-size-base" changes, the derived "gtk-cursor-theme-size" value is now calculated by multiplying the base size with the current scale factor. Previously, the base size was used directly without scaling, which could lead to incorrect cursor sizes on high-DPI displays. The change includes validation for the scale factor, defaulting to 1.0 if an invalid value is read, and logs a warning in both error and update cases for debugging purposes. fix: 应用缩放因子到GTK光标主题大小 当DConfig键"gtk-cursor-theme-size-base"发生变化时，现在通过将基础大小乘 以当前缩放因子来计算派生的"gtk-cursor-theme-size"值。之前，基础大小被直 接使用而没有进行缩放，这可能导致在高DPI显示器上光标大小不正确。此更改包 括对缩放因子的验证，如果读取到无效值则回退到1.0，并在错误和更新情况下记 录警告信息以便调试。 pms: BUG-343215
deepin pr auto review这段代码主要修改了光标主题尺寸的计算逻辑，从直接使用基础尺寸改为根据缩放因子(scale factor)动态计算。以下是对这段代码的审查意见和改进建议： 1. 语法逻辑当前状态：代码逻辑基本正确，但存在一些潜在问题。 改进建议：
2. 代码质量当前状态：代码可读性较好，有基本的错误处理和日志输出。 改进建议：
3. 代码性能当前状态：性能影响较小，但可以优化。 改进建议：
4. 代码安全当前状态：有基本的错误处理，但可以增强。 改进建议：
改进后的代码示例：void XSettingsManager::handleDConfigChangedCb(const QString &key)
{
if (key == "gtk-cursor-theme-size-base") {
int cursorSizeBase = m_settingDconfig->value(key).toInt();
if (cursorSizeBase <= 0) {
qWarning() << "XSettingsManager: Invalid cursor size base:" << cursorSizeBase;
return;
}
double scale = m_settingDconfig->value(dcKeyScaleFactor).toDouble();
if (scale <= 0 || qIsNaN(scale)) {
qWarning() << "XSettingsManager: Invalid scale factor:" << scale << ", fallback to 1.0";
scale = 1.0;
}
int finalSize = static_cast<int>(cursorSizeBase * scale);
qDebug() << "XSettingsManager: Update gtk-cursor-theme-size to" << finalSize;
m_settingDconfig->setValue("gtk-cursor-theme-size", finalSize);
return;
}
// 其他处理逻辑...
}总结这段代码的功能是合理的，但可以通过增强错误处理、优化性能和改进日志输出来提升代码质量。建议在合并前进行单元测试，特别是针对边界情况（如 |
yixinshark
approved these changes
Feb 27, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mhduiy, yixinshark 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
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.
When the DConfig key "gtk-cursor-theme-size-base" changes, the derived "gtk-cursor-theme-size" value is now calculated by multiplying the base size with the current scale factor. Previously, the base size was used directly without scaling, which could lead to incorrect cursor sizes on high-DPI displays. The change includes validation for the scale factor, defaulting to 1.0 if an invalid value is read, and logs a warning in both error and update cases for debugging purposes.
fix: 应用缩放因子到GTK光标主题大小
当DConfig键"gtk-cursor-theme-size-base"发生变化时，现在通过将基础大小乘 以当前缩放因子来计算派生的"gtk-cursor-theme-size"值。之前，基础大小被直
接使用而没有进行缩放，这可能导致在高DPI显示器上光标大小不正确。此更改包
括对缩放因子的验证，如果读取到无效值则回退到1.0，并在错误和更新情况下记
录警告信息以便调试。
pms: BUG-343215
Summary by Sourcery
Bug Fixes: