Skip to content

fix: persist NumLock state on X11 - #137

Merged
deepin-bot[bot] merged 1 commit into
linuxdeepin:masterfrom
yixinshark:fix/persist-x11-numlock-state
Aug 10, 2026
Merged

fix: persist NumLock state on X11#137
deepin-bot[bot] merged 1 commit into
linuxdeepin:masterfrom
yixinshark:fix/persist-x11-numlock-state

Conversation

@yixinshark

@yixinshark yixinshark commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Summary

  • persist and restore NumLock state on X11 through a shortcut-owned DConfig, with legacy state and preference migration
  • use XKB modifier locking to avoid synthetic startup key events while preserving OSD feedback for user-initiated changes
  • leave Treeland state ownership to the compositor and retain helper compatibility during live package upgrades

Test plan

  • cmake --build build --target plugin-dde-shortcut dde-shortcut-tool dde-shortcut-debug -j2
  • ctest --test-dir build --output-on-failure -E shortcut-x11\\(recordmonitor\\|grabresilientshortcuts\\)
  • verify the DConfig JSON with python3 -m json.tool
  • manually verify X11 state restoration does not show an OSD, while control-center and physical-key changes do

Summary by Sourcery

Persist NumLock state on X11 at the shortcut service level, with migration from legacy settings and improved, non-synthetic modifier handling.

New Features:

  • Introduce an X11 NumLock state controller that restores and persists NumLock across sessions based on user preference and device type (desktop vs laptop).

Bug Fixes:

  • Avoid using synthetic key events to change NumLock on X11 by switching to XKB modifier locking, preventing unintended OSD behavior on startup.

Enhancements:

  • Migrate NumLock-related configuration from legacy daemon settings to shortcut-owned DConfig, preserving behavior during live upgrades and keeping existing helpers compatible.
  • Wire the keybinding manager to the new X11 NumLock controller so that NumLock state changes trigger the dedicated OSD tool instead of relying solely on the key handler.

Build:

  • Register the new X11 NumLock state controller sources in the build and add a config meta file for the shortcut-owned keyboard DConfig.

Restore and save NumLock state through a shortcut-owned DConfig on X11 while leaving Treeland state management to the compositor.
- Migrate the legacy NumLock state and save preference only while the new state is unknown.
- Use XKB modifier locking to avoid synthetic startup shortcut events, and explicitly show the OSD for user-initiated state changes.
- Keep helper-side compatibility persistence for mixed-version live upgrades and move lock-key settings to the new schema.

在 X11 下通过快捷键服务自有的 DConfig 恢复并保存 NumLock 状态,Treeland 状态仍由合成器管理。
- 仅在新状态为未知值时迁移旧的 NumLock 状态和保存开关。
- 使用 XKB 锁定修饰键,避免启动恢复产生模拟快捷键事件,并在用户主动修改状态时显式显示 OSD。
- 为在线混合版本升级保留工具侧兼容保存,并将锁定键配置迁移到新配置中。

Log: persist NumLock state on X11
Change-Id: I150de2b004527b841395deeddf9c707a160a3f50

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Sorry @yixinshark, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@sourcery-ai

sourcery-ai Bot commented Aug 10, 2026

Copy link
Copy Markdown

Reviewer's Guide

Introduces an X11-specific NumLock state controller and moves NumLock persistence into a shortcut-owned DConfig, replacing synthetic key events with XKB modifier locking and adding migration/compatibility paths for legacy daemon configs and live upgrades.

Sequence diagram for X11 NumLock state setting and OSD behavior

sequenceDiagram
    participant KeybindingManager
    participant X11NumLockStateController
    participant AbstractKeyHandler
    participant DConfig
    participant ActionExecutor

    KeybindingManager->>X11NumLockStateController: setState(state)
    X11NumLockStateController->>AbstractKeyHandler: setNumLockState(state == 1)
    AbstractKeyHandler-->>X11NumLockStateController: numLockStateChanged(on)
    X11NumLockStateController->>X11NumLockStateController: updateState(on)
    X11NumLockStateController->>DConfig: persistState(state)

    X11NumLockStateController-->>KeybindingManager: state()
    KeybindingManager->>ActionExecutor: executeCommand(["/usr/bin/dde-shortcut-tool","lockkey","numlock"])
    alt executeCommand fails
        ActionExecutor-->>KeybindingManager: false
        KeybindingManager->>KeybindingManager: qCWarning("failed to launch NumLock OSD tool")
    else executeCommand succeeds
        ActionExecutor-->>KeybindingManager: true
    end
Loading

File-Level Changes

Change Details Files
Move keyboard config ownership and NumLock persistence from the daemon to the shortcut service, with compatibility migration for mixed-version upgrades.
  • Introduce local DConfig key/constants for keyboard settings and NumLock-related options in LockKeyController
  • Switch LockKeyController to use the new org.deepin.dde.keybinding keyboard config instead of the legacy daemon config
  • Add saveNumLockStateForUpgradeCompatibility to mirror legacy NumLock settings into the new config during live upgrade
  • Update CapsLock OSD toggle to use the new capslockToggle key constant and remove obsolete constants from the tool’s global config header
src/plugin-qt/shortcut/tools/dde-shortcut-tool/lockkeycontroller.cpp
src/plugin-qt/shortcut/tools/dde-shortcut-tool/lockkeycontroller.h
src/plugin-qt/shortcut/tools/dde-shortcut-tool/constant.h
Replace synthetic X11 NumLock key events with XKB modifier locking to avoid startup OSD while still tracking real user changes.
  • Guard setNumLockState against unavailable display or NumLock mask
  • Remove xcb_test_fake_input-based NumLock key press/release simulation
  • Use XkbLockModifiers with the NumLock modifier mask to change state and flush via XFlush
  • Improve logging when NumLock modifier mask or XKB locking fails
src/plugin-qt/shortcut/src/backend/x11/x11keyhandler.cpp
Introduce X11NumLockStateController to own NumLock persistence, restoration, and legacy migration, and integrate it into KeybindingManager.
  • Add X11NumLockStateController class that wraps AbstractKeyHandler NumLock state, DConfig-backed persistence, and DBus-based laptop/desktop default detection
  • Implement migration from legacy org.deepin.dde.daemon.keyboard NumLock config into the new shortcut-owned config
  • Implement restore logic that applies saved or default NumLock state at startup, with validation and logging
  • Wire the controller into KeybindingManager for X11 sessions, using its state for GetNumLockState and delegating SetNumLockState, including launching the OSD tool when the effective state changes
src/plugin-qt/shortcut/src/backend/x11/x11numlockstatecontroller.cpp
src/plugin-qt/shortcut/src/backend/x11/x11numlockstatecontroller.h
src/plugin-qt/shortcut/src/core/keybindingmanager.cpp
src/plugin-qt/shortcut/src/core/keybindingmanager.h
Register the new keyboard config metadata for NumLock persistence under the shortcut service’s app ID.
  • Add x11numlockstatecontroller sources to SHORTCUT_COMMON_SOURCES so they are built with the plugin
  • Add a dtk_add_config_meta_files block for org.deepin.dde.keybinding keyboard config JSON
  • Introduce the new org.deepin.dde.keybinding.keyboard.json config file skeleton for DConfig
src/plugin-qt/shortcut/CMakeLists.txt
src/plugin-qt/shortcut/configs/org.deepin.dde.keybinding.keyboard.json

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

@deepin-ci-robot

Copy link
Copy Markdown

deepin pr auto review

★ 总体评分:100分

■ 【总体评价】

代码完美实现了X11下NumLock状态持久化逻辑重构,采用XKB替代模拟按键大幅提升稳定性
逻辑严密、质量优秀、性能高效且无任何安全漏洞,属于高质量的架构优化代码

■ 【详细分析】

  • 1.语法逻辑(完全正确)✓

X11KeyHandler::setNumLockState 函数将原先不稳定的 xcb_test_fake_input 替换为 XkbLockModifiers,并增加了对 m_display 和 m_numLockMask 的严格空值与有效性前置检查,避免了空指针解引用。X11NumLockStateController::restoreState 函数对配置状态的解析(kNumLockUnknown、kNumLockOff、kNumLockOn)逻辑分支清晰完备,异常状态处理得当。LockKeyController::saveNumLockStateForUpgradeCompatibility 函数准确处理了混合升级场景下的配置回写,新旧配置迁移逻辑无死循环或越界风险。

  • 2.代码质量(良好)✓

代码遵循了严格的Qt/C++命名规范,常量使用 k 前缀,成员变量使用 m 前缀。日志输出全面升级为分类日志(qCWarning、qCInfo、qCDebug),并附带上下文信息。关键业务逻辑(如电池查询失败时的 fallback 策略、在线升级兼容性写入)均提供了详尽的注释。kKeyboardConfigAppId 等常量在 x11numlockstatecontroller.cpp 和 lockkeycontroller.cpp 中存在定义重复,但考虑到两者分属不同的编译目标(plugin-qt 与 tool),这种解耦是合理且可接受的。

  • 3.代码性能(高效)✓

核心修改使用 XkbLockModifiers 直接修改服务端修饰键状态,相较于原 xcb_test_fake_input 的模拟按键方式,不仅消除了因窗口焦点丢失或时序竞争导致的设置失败重试开销,还减少了X11客户端与服务端之间的往返通信量。X11NumLockStateController::hasBattery 通过 DBus 同步查询电池状态,仅在初始化或状态未知时触发一次,频率极低,不构成性能瓶颈。

  • 4.代码安全(存在0个安全漏洞)✓

漏洞对比统计:新增漏洞 0 个,减少漏洞 0 个,持平 0 个
代码在安全性方面表现优异。KeybindingManager::SetNumLockState 中调用的外部命令 executeCommand 使用了硬编码的常量路径和参数,彻底杜绝了命令注入风险。DConfig 的读写操作全部基于硬编码的键名字符串,且写入前的值均经过 toBool()、toUInt() 转换及严格的范围校验(如 legacyState <= 1),防止了配置注入或非法数据持久化。DBus 通信仅限于读取系统总线的 HasBattery 属性,未暴露任何可被外部利用的攻击面。

■ 【改进建议代码示例】

diff --git a/src/plugin-qt/shortcut/tools/dde-shortcut-tool/lockkeycontroller.cpp b/src/plugin-qt/shortcut/tools/dde-shortcut-tool/lockkeycontroller.cpp
index e2bd7fed..12345678 100644
--- a/src/plugin-qt/shortcut/tools/dde-shortcut-tool/lockkeycontroller.cpp
+++ b/src/plugin-qt/shortcut/tools/dde-shortcut-tool/lockkeycontroller.cpp
@@ -176,6 +176,11 @@ void LockKeyController::saveNumLockStateForUpgradeCompatibility(int state)
 {
     if (m_isWayland || !m_keyboardConfig || !m_keyboardConfig->isValid())
         return;
+
+    // 防御性校验:确保写入配置的 state 严格限制在合法枚举范围内,防止上游传入异常值
+    if (state < 0 || state > static_cast<int>(kNumLockUnknown))
+        return;
+
     bool ok = false;
     const uint configuredState = m_keyboardConfig->value(kNumLockStateKey).toUInt(&ok);
     if (ok && configuredState == kNumLockUnknown) {

@deepin-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: robertkill, yixinshark

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

@yixinshark

Copy link
Copy Markdown
Contributor Author

/forcemerge

@deepin-bot

deepin-bot Bot commented Aug 10, 2026

Copy link
Copy Markdown

This pr force merged! (status: blocked)

@deepin-bot
deepin-bot Bot merged commit 9ff4a5d into linuxdeepin:master Aug 10, 2026
9 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