Skip to content

Conversation

@mhduiy
Copy link
Contributor

@mhduiy mhduiy commented Jan 26, 2026

  1. Modified scale factor retrieval logic to only query XSettings DBus interface when running under X11 session
  2. Added session type check using XDG_SESSION_TYPE environment variable
  3. Added TODO comment explaining that XSettings is not available on Wayland during early startup
  4. This change prevents failed DBus calls and potential startup delays on Wayland sessions
  5. The scale factor is temporarily defaulted to 1.0 on Wayland, affecting only double-click distance calculation

Log: Fixed Wayland session startup issue by skipping XSettings scale factor query

Influence:

  1. Test application startup on both X11 and Wayland sessions
  2. Verify double-click behavior works correctly with default scale factor on Wayland
  3. Ensure no DBus errors appear during Wayland session initialization
  4. Confirm X11 sessions still properly retrieve scale factor from XSettings
  5. Test that QT_QPA_PLATFORM environment variable is correctly set based on session type

fix: Wayland 下暂时跳过缩放因子获取

  1. 修改缩放因子获取逻辑,仅在 X11 会话下查询 XSettings DBus 接口
  2. 添加使用 XDG_SESSION_TYPE 环境变量的会话类型检查
  3. 添加 TODO 注释说明 Wayland 在早期启动阶段无法使用 XSettings
  4. 此更改防止 Wayland 会话上的 DBus 调用失败和潜在的启动延迟
  5. Wayland 上的缩放因子暂时默认为 1.0,仅影响双击距离计算

Log: 修复 Wayland 会话启动问题,跳过 XSettings 缩放因子查询

Influence:

  1. 测试在 X11 和 Wayland 会话上的应用程序启动
  2. 验证 Wayland 上使用默认缩放因子的双击行为正常工作
  3. 确保 Wayland 会话初始化期间不出现 DBus 错误
  4. 确认 X11 会话仍能正确从 XSettings 获取缩放因子
  5. 测试 QT_QPA_PLATFORM 环境变量根据会话类型正确设置

Summary by Sourcery

Guard XSettings-based scale factor retrieval behind an X11 session check to avoid Wayland startup issues while keeping existing behavior for X11.

Bug Fixes:

  • Prevent failed XSettings DBus calls and potential startup delays when starting a session under Wayland.

Enhancements:

  • Default the scale factor to 1.0 on non-X11 sessions and document the limitation and future work for Wayland scale detection.

1. Modified scale factor retrieval logic to only query XSettings DBus
interface when running under X11 session
2. Added session type check using XDG_SESSION_TYPE environment variable
3. Added TODO comment explaining that XSettings is not available on
Wayland during early startup
4. This change prevents failed DBus calls and potential startup delays
on Wayland sessions
5. The scale factor is temporarily defaulted to 1.0 on Wayland,
affecting only double-click distance calculation

Log: Fixed Wayland session startup issue by skipping XSettings scale
factor query

Influence:
1. Test application startup on both X11 and Wayland sessions
2. Verify double-click behavior works correctly with default scale
factor on Wayland
3. Ensure no DBus errors appear during Wayland session initialization
4. Confirm X11 sessions still properly retrieve scale factor from
XSettings
5. Test that QT_QPA_PLATFORM environment variable is correctly set based
on session type

fix: Wayland 下暂时跳过缩放因子获取

1. 修改缩放因子获取逻辑,仅在 X11 会话下查询 XSettings DBus 接口
2. 添加使用 XDG_SESSION_TYPE 环境变量的会话类型检查
3. 添加 TODO 注释说明 Wayland 在早期启动阶段无法使用 XSettings
4. 此更改防止 Wayland 会话上的 DBus 调用失败和潜在的启动延迟
5. Wayland 上的缩放因子暂时默认为 1.0,仅影响双击距离计算

Log: 修复 Wayland 会话启动问题,跳过 XSettings 缩放因子查询

Influence:
1. 测试在 X11 和 Wayland 会话上的应用程序启动
2. 验证 Wayland 上使用默认缩放因子的双击行为正常工作
3. 确保 Wayland 会话初始化期间不出现 DBus 错误
4. 确认 X11 会话仍能正确从 XSettings 获取缩放因子
5. 测试 QT_QPA_PLATFORM 环境变量根据会话类型正确设置
@deepin-ci-robot
Copy link

deepin pr auto review

这段代码主要是在会话启动时设置环境变量,特别是针对 X11 和 Wayland 会话类型的不同处理。以下是对该 git diff 的详细审查意见,涵盖语法逻辑、代码质量、性能和安全性四个方面:

1. 语法逻辑

  • 逻辑优化
    • QByteArray sessionType = qgetenv("XDG_SESSION_TYPE"); 提取到函数开头是合理的。这避免了重复调用系统函数,且让代码逻辑更清晰。
    • 问题点:在 else 分支(即 Wayland 环境)中,代码留空并仅添加了 TODO 注释。这意味着在 Wayland 环境下,scaleFactor 将保持默认值 1.0。虽然注释解释了原因(XSettings 服务未就绪),但这会导致 QT_DBL_CLICK_DIST(双击距离)在 Wayland 高分屏下可能设置不正确,影响用户体验。建议至少记录一条警告日志,提示当前缩放比例可能不准确。

2. 代码质量

  • 变量作用域
    • QDBusInterface dbusInterface 现在被限制在 if (sessionType == "x11") 的作用域内。这比原来的代码更好,因为它明确表示该接口仅在 X11 下有效,避免了在 Wayland 下创建无意义的 DBus 对象。
  • 注释质量
    • TODO 注释写得很详细,解释了为什么不能在 Wayland 下获取缩放因子,以及产生的影响。这对于后续维护非常有帮助。
  • 硬编码字符串
    • 代码中存在多处硬编码字符串,如 "org.deepin.dde.XSettings1""x11""wayland" 等。建议将这些定义为类顶部的静态常量(例如 static const char* kServiceName = "org.deepin.dde.XSettings1";),以提高可读性和可维护性。

3. 代码性能

  • 系统调用优化
    • qgetenv 是一次系统调用(读取环境变量)。将其从原来的中间位置移至顶部并复用 sessionType 变量,消除了冗余调用,虽然性能提升微乎其微,但习惯良好。
  • DBus 调用
    • DBus 调用 GetScaleFactor 是同步进行的(dbusInterface.call),这会阻塞当前线程直到收到回复。考虑到这是在 init 阶段,通常是可以接受的。但如果在 UI 线程中调用且 DBus 响应慢,可能会导致界面卡顿。虽然目前改动未涉及此点,但值得注意。

4. 代码安全

  • 空指针/有效性检查
    • 代码对 dbusInterface.isValid()scaleFactorReply.isValid() 进行了检查,这是很好的防御性编程实践,防止了 DBus 调用失败导致程序崩溃或使用未初始化的值。
  • 默认值处理
    • scaleFactor 初始化为 1.0。如果在 X11 下 DBus 调用失败,或者在 Wayland 下无法获取,程序会回退到默认值。这保证了程序的健壮性,不会因为配置获取失败而出现异常行为(如除零错误或无效的距离设置)。

改进建议

基于以上分析,建议对代码进行如下微调:

void EnvironmentsManager::createGeneralEnvironments()
{
    // 1. 定义常量以提高可读性(建议放在类头文件中)
    // static const char* kSessionTypeX11 = "x11";
    // static const char* kSessionTypeWayland = "wayland";

    // 2. 提前获取环境变量
    QByteArray sessionType = qgetenv("XDG_SESSION_TYPE");
    double scaleFactor = 1.0;

    // 3. 仅在 X11 下尝试获取缩放因子
    if (sessionType == "x11") { // 建议使用常量 kSessionTypeX11
        QDBusInterface dbusInterface("org.deepin.dde.XSettings1", "/org/deepin/dde/XSettings1",
                                     "org.deepin.dde.XSettings1", QDBusConnection::sessionBus());
        if (dbusInterface.isValid()) {
            QDBusReply<double> scaleFactorReply = dbusInterface.call("GetScaleFactor");
            if (scaleFactorReply.isValid()) {
                scaleFactor = scaleFactorReply.value();
            }
        }
    } else if (sessionType == "wayland") { // 建议使用常量 kSessionTypeWayland
        // 4. 添加日志记录,便于调试
        qWarning() << "Running under Wayland, scale factor detection is skipped, using default 1.0. "
                   << "This might affect double-click distance settings.";
        // TODO: (保留原有注释)
        // wayland环境下在这里通过xsettings获取是不合理的,此时xsettings服务还没有启动...
    }

    auto envs = QProcessEnvironment::systemEnvironment();
    // ... 后续代码保持不变 ...
}

总结
这段代码的改动是正确且必要的,它修复了在 Wayland 环境下尝试连接 X11 服务可能导致的潜在问题或无意义的等待。通过将环境变量读取前置,代码逻辑更加清晰。主要的改进空间在于处理 Wayland 分支时的日志记录以及消除硬编码字符串。

@sourcery-ai
Copy link

sourcery-ai bot commented Jan 26, 2026

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Restricts scale factor retrieval via XSettings D-Bus to X11 sessions only, defaults to scale factor 1.0 on Wayland, and keeps QT_QPA_PLATFORM selection based on session type while documenting current Wayland limitations.

Sequence diagram for session-based scale factor retrieval in createGeneralEnvironments

sequenceDiagram
    participant EnvironmentsManager
    participant SystemEnv as SystemEnvironment
    participant XSettingsService
    participant QProcessEnv as QProcessEnvironment

    EnvironmentsManager->>SystemEnv: qgetenv(XDG_SESSION_TYPE)
    SystemEnv-->>EnvironmentsManager: sessionType

    alt X11 session
        EnvironmentsManager->>XSettingsService: DBus call GetScaleFactor
        XSettingsService-->>EnvironmentsManager: scaleFactor (double)
        EnvironmentsManager->>EnvironmentsManager: set scaleFactor from reply
    else Wayland or other session
        EnvironmentsManager->>EnvironmentsManager: keep default scaleFactor = 1.0
    end

    EnvironmentsManager->>QProcessEnv: QProcessEnvironment::systemEnvironment
    QProcessEnv-->>EnvironmentsManager: envs

    EnvironmentsManager->>EnvironmentsManager: insert DDE related envs into m_envMap
    EnvironmentsManager->>EnvironmentsManager: insert QT_DBL_CLICK_DIST = 15 * scaleFactor

    alt X11 session
        EnvironmentsManager->>EnvironmentsManager: insert QT_QPA_PLATFORM = dxcb and xcb
    else Wayland session
        EnvironmentsManager->>EnvironmentsManager: insert QT_QPA_PLATFORM = wayland
    else other session
        EnvironmentsManager->>EnvironmentsManager: do not set QT_QPA_PLATFORM
    end
Loading

Flow diagram for session-type dependent environment and scale factor handling

flowchart TD
    A["Start createGeneralEnvironments"] --> B["Read XDG_SESSION_TYPE into sessionType"]
    B --> C["Set scaleFactor = 1.0"]
    C --> D{sessionType == x11?}
    D -->|Yes| E["Create QDBusInterface to org.deepin.dde.XSettings1"]
    E --> F{dbusInterface isValid?}
    F -->|Yes| G["Call GetScaleFactor via DBus"]
    G --> H{scaleFactorReply isValid?}
    H -->|Yes| I["Set scaleFactor = scaleFactorReply.value"]
    H -->|No| J["Keep scaleFactor = 1.0"]
    F -->|No| J
    D -->|No| K["Skip XSettings DBus call for Wayland or other sessions (TODO for Wayland handling)"]

    I --> L["Load system environment via QProcessEnvironment::systemEnvironment"]
    J --> L
    K --> L

    L --> M["Insert common envs into m_envMap including XDG_CURRENT_DESKTOP = DDE"]
    M --> N["Insert QT_DBL_CLICK_DIST = 15 * scaleFactor"]
    N --> O{sessionType == x11?}
    O -->|Yes| P["Set QT_QPA_PLATFORM = dxcb;xcb"]
    O -->|No| Q{sessionType == wayland?}
    Q -->|Yes| R["Set QT_QPA_PLATFORM = wayland"]
    Q -->|No| S["Do not set QT_QPA_PLATFORM"]
    P --> T["End createGeneralEnvironments"]
    R --> T
    S --> T
Loading

File-Level Changes

Change Details Files
Guard scale-factor retrieval behind X11 session check and default to 1.0 on Wayland, while preserving session-specific QT_QPA_PLATFORM setup.
  • Read XDG_SESSION_TYPE once at the top of createGeneralEnvironments and reuse it for both scale-factor logic and platform selection.
  • On X11 sessions, instantiate the XSettings D-Bus interface and, if valid, call GetScaleFactor to compute the scaleFactor value.
  • On non-X11 sessions, skip the XSettings D-Bus call entirely, leaving scaleFactor at the default 1.0.
  • Keep QT_QPA_PLATFORM set to dxcb;xcb for x11 and wayland;dxcb for wayland based on the sessionType value.
  • Add a TODO comment explaining why XSettings cannot be used during early Wayland startup and that only double-click distance is currently affected.
src/dde-session/environmentsmanager.cpp

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've left some high level feedback:

  • The sessionType checks are now used in two separate places in this method; consider factoring the environment lookup and session-type handling into a small helper or enum to avoid string literals and keep the logic centralized.
  • In the new else branch for non-x11 sessions, you implicitly fall back to a scale factor of 1.0; consider making this default explicit via a named constant or comment near the initialization so the behavior is clear even without reading the TODO.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The `sessionType` checks are now used in two separate places in this method; consider factoring the environment lookup and session-type handling into a small helper or enum to avoid string literals and keep the logic centralized.
- In the new `else` branch for non-`x11` sessions, you implicitly fall back to a scale factor of 1.0; consider making this default explicit via a named constant or comment near the initialization so the behavior is clear even without reading the TODO.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@deepin-ci-robot
Copy link

[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.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mhduiy mhduiy merged commit bccde6b into linuxdeepin:master Jan 26, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants