Skip to content

fix(reader): restore arrow key scrolling by removing QAction shortcut registration - #409

Merged
deepin-bot[bot] merged 1 commit into
linuxdeepin:release/snipefrom
Resurgamz:agent/pms-bug-bot/abc79fed2d0f
Sep 29, 2026
Merged

deepin-bot[bot] merged 1 commit into
linuxdeepin:release/snipefrom
Resurgamz:agent/pms-bug-bot/abc79fed2d0f

Conversation

@Resurgamz

@Resurgamz Resurgamz commented Sep 28, 2026 •

Copy link
Copy Markdown

fix(reader): restore arrow key scrolling by removing QAction shortcut registration

Arrow keys (Left/Right/Up/Down) and Space were registered as QAction
shortcuts in Central.cpp constructor, which intercepted key events and
prevented Qt's default scrolling behavior in document reading mode.
The page navigation logic in CentralDocPage.cpp for these keys was
already commented out, so the QAction had no effect except blocking
default scrolling.

Fix by commenting out the QAction registrations for these keys in
Central.cpp, removing the slide widget key forwarding in
CentralDocPage.cpp, and adding a keyPressEvent override to SlideWidget
with setFocus() in its constructor so slide show mode handles arrow
keys and space independently. This restores the fix from commit
a81e5e1 that was reverted by the v23 merge commit b20fcf9.

Log: 方向键无法滚动文档页面
Bug: https://pms.uniontech.com/bug-view-374547.html

Summary by Sourcery

Restore document scrolling while retaining keyboard navigation for slide presentations.

Bug Fixes:

  • Restore default arrow-key and Space scrolling in document reading mode.
  • Preserve independent arrow-key and Space handling in slide show mode.

Enhancements:

  • Remove centralized shortcut interception and route slide show keyboard input through SlideWidget focus and key events.

@sourcery-ai

sourcery-ai Bot commented Sep 28, 2026

Copy link
Copy Markdown

Reviewer's Guide

Restores Qt's default arrow-key and Space scrolling in document reading mode by removing their central QAction registrations and page-level forwarding, while preserving slide-show navigation through SlideWidget focus and keyPressEvent handling.

Sequence diagram for mode-specific keyboard handling

sequenceDiagram
    actor User
    participant Central
    participant DocumentView
    participant SlideWidget
    participant Qt

    User->>Central: keyPressEvent(arrow or Space)
    alt document reading mode
        Central->>Qt: default scrolling
        Qt-->>DocumentView: scroll document
    else slide show mode
        Central->>SlideWidget: keyPressEvent(event)
        SlideWidget->>SlideWidget: handleKeyPressEvent(key)
        SlideWidget->>Qt: DWidget::keyPressEvent(event)
    end
Loading

File-Level Changes

Change Details Files
Stop central shortcut handling from intercepting document scrolling keys.
  • Comment out QAction registrations for Left, Right, Up, Down, and Space.
  • Remove forwarding of shortcut events from document-page handling to the slide widget.
reader/uiframe/Central.cpp
reader/uiframe/CentralDocPage.cpp
Handle slide-show navigation through the widget's native key-event path.
  • Give SlideWidget focus after initialization.
  • Convert key press events to application shortcuts and dispatch them through the existing slide navigation handler.
  • Expose the handler and override keyPressEvent.
reader/widgets/SlideWidget.cpp
reader/widgets/SlideWidget.h
Update tests for widget-level key handling and removed document-page forwarding.
  • Send synthetic QKeyEvent instances to verify Space and arrow navigation.
  • Adjust CentralDocPage shortcut expectations to ensure slide handling is no longer invoked.
tests/uiframe/ut_centraldocpage.cpp
tests/widgets/ut_slidewidget.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

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

Hey - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨

… registration

Arrow keys (Left/Right/Up/Down) and Space were registered as QAction
shortcuts in Central.cpp constructor, which intercepted key events and
prevented Qt's default scrolling behavior in document reading mode.
The page navigation logic in CentralDocPage.cpp for these keys was
already commented out, so the QAction had no effect except blocking
default scrolling.

Fix by commenting out the QAction registrations for these keys in
Central.cpp, removing the slide widget key forwarding in
CentralDocPage.cpp, and adding a keyPressEvent override to SlideWidget
with setFocus() in its constructor so slide show mode handles arrow
keys and space independently. This restores the fix from commit
a81e5e1 that was reverted by the v23 merge commit b20fcf9.

Log: 方向键无法滚动文档页面
Bug: https://pms.uniontech.com/bug-view-374547.html
@Resurgamz
Resurgamz force-pushed the agent/pms-bug-bot/abc79fed2d0f branch from f417b30 to 6c5612b Compare September 29, 2026 01:31
@deepin-ci-robot

Copy link
Copy Markdown

deepin pr auto review

AI 代码审查报告

项目: linuxdeepin/deepin-reader
PR: #409
分支: agent/pms-bug-bot/abc79fed2d0f → release/snipe
作者: Resurgamz
提交: fix(reader): restore arrow key scrolling by removing QAction shortcut registration
审查时间: 2026-09-29 09:50:00
平台: GitHub


总体评分

维度 评分 状态
语法逻辑 25/25 ✓
代码质量 23/25 ✓
代码性能 20/20 ✓
代码安全 30/30 ✓
总分 98/100 优秀

变更概述

本次 PR 修复了 deepin-reader 中方向键滚动功能被屏蔽的问题。通过移除 QAction 快捷键注册,恢复方向键的默认滚动行为,同时为幻灯片模式(SlideWidget)添加独立的 keyPressEvent 处理。

修改文件:

  1. reader/uiframe/Central.cpp - 注释掉方向键和空格键的快捷键注册
  2. reader/uiframe/CentralDocPage.cpp - 移除幻灯片模式下的 handleKeyPressEvent 调用
  3. reader/widgets/SlideWidget.cpp - 新增 keyPressEvent 重写和焦点设置
  4. reader/widgets/SlideWidget.h - 调整 handleKeyPressEvent 可见性,新增 keyPressEvent 声明

维度1:语法逻辑(25分)✓

语法正确,逻辑清晰

分析:

  1. Central.cpp - 注释快捷键注册:语法正确,注释格式符合 C++ 规范。移除方向键和空格键的快捷键注册后,这些按键不再被 QAction 拦截,可以正常传递给目标控件。

  2. CentralDocPage.cpp - 移除 handleKeyPressEvent 调用:if (m_slideWidget) { return; } 逻辑合理。当幻灯片模式激活时,直接返回不处理快捷键,由 SlideWidget 自身的 keyPressEvent 接管按键处理。

  3. SlideWidget.cpp - 新增 keyPressEvent:

    • Utils::getKeyshortcut(event) 将 QKeyEvent 转换为字符串,经核实转换结果与 Dr::key_left("Left")、Dr::key_right("Right")等常量匹配,逻辑正确
    • handleKeyPressEvent(key) 处理幻灯片翻页(方向键)和播放控制(空格键)
    • 调用 DWidget::keyPressEvent(event) 保留基类默认行为,确保未处理的按键正常传播
  4. SlideWidget.cpp - QTimer::singleShot(0, this, [this](){this->setFocus();}):标准 Qt 模式,在控件 show() 后异步设置焦点,确保 SlideWidget 能接收键盘事件。this 作为接收者参数确保对象销毁时自动断开连接,无悬空指针风险。

结论: 无编译错误,逻辑合理,边界处理完善。


维度2:代码质量(23分)✓

代码结构清晰,注释完整

分析:

  1. 注释代码保留问题(-2分):

    • Central.cpp 第77-83行:注释掉的代码(//keyList.append(...))建议直接删除而非保留注释。移除原因已由上方注释说明,原始代码可通过 git 历史恢复。保留注释代码增加视觉噪音,可能导致后续维护混淆。
    • 建议替换为简洁说明注释:
      // 方向键和空格键不注册为快捷键,以保留主界面的默认滚动行为
  2. 代码结构改进:

    • handleKeyPressEvent 从 public slots 移至 private 区域,符合最小权限原则,因为该方法现在仅由 SlideWidget 内部的 keyPressEvent 调用
    • keyPressEvent 正确放置在 protected 区域,符合 Qt 事件处理函数的惯例
  3. 代码复用: 使用 Utils::getKeyshortcut(event) 统一的按键转换函数,与 SheetSidebar 中的用法一致,避免重复代码

  4. 无残留调试代码: 代码整洁,无遗留的调试输出


维度3:代码性能(20分)✓

性能良好,资源使用合理

分析:

  1. QTimer::singleShot(0, ...): 延迟 0ms 的定时器在事件循环下一次迭代时执行,开销可忽略不计。这是 Qt 中在控件显示后设置焦点的标准模式。

  2. Utils::getKeyshortcut(event): 每次按键触发时进行字符串转换,涉及修饰符检查和字符串拼接。操作轻量,对按键事件频率而言性能影响可忽略。

  3. 无资源泄漏: QTimer::singleShot 为一次性定时器,无需手动管理。lambda 通过 this 参数确保安全。

结论: 无性能瓶颈,算法复杂度合理。


维度4:代码安全(30分)✓

存在0个安全漏洞

分析:

  1. 无命令注入风险: 代码仅处理键盘事件,不涉及外部输入、文件路径或系统命令执行
  2. 无敏感信息泄露: 无硬编码密钥、密码或 Token
  3. 无缓冲区溢出: 不涉及缓冲区操作,QString 自动管理内存
  4. 无路径遍历: 不涉及文件路径操作
  5. 无权限绕过: 不涉及权限控制逻辑

漏洞对比统计:新增漏洞 0 个,减少漏洞 0 个,持平 0 个


OCR 审查结果

OCR(OpenCodeReview)审查发现 1 条建议:

  1. [低级别 · 可维护性] Central.cpp:77-83 - 注释掉的代码建议直接删除而非保留在代码中。移除原因已由注释说明,原始代码可通过版本控制历史恢复。保留注释代码增加视觉噪音,可能导致后续维护混淆。

该建议与代码质量维度发现的问题一致,已计入代码质量扣分。


改进建议

建议1:移除注释代码,替换为说明性注释

文件: reader/uiframe/Central.cpp 第77-83行

当前代码:

//使用下面5个快捷键会屏蔽主界面对应的功能,故注释
//keyList.append(QKeySequence(Qt::Key_Left));
//keyList.append(QKeySequence(Qt::Key_Right));

//keyList.append(QKeySequence(Qt::Key_Up));
//keyList.append(QKeySequence(Qt::Key_Down));
//keyList.append(QKeySequence(Qt::Key_Space));

建议修改:

// 方向键和空格键不注册为快捷键,以保留主界面的默认滚动行为

审查结论

本次 PR 修复了方向键滚动被屏蔽的问题,方案设计合理:

  1. 问题根因正确识别:方向键被注册为 QAction 快捷键后被拦截,导致无法传递给文档视图进行滚动
  2. 修复方案合理:移除快捷键注册 + 为 SlideWidget 添加独立的 keyPressEvent 处理 + 异步设置焦点
  3. 无安全风险:代码不涉及安全敏感操作
  4. 代码质量良好:结构清晰,复用现有工具函数,仅有一处注释代码保留的轻微问题

评分: 98/100(优秀)

@deepin-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: max-lvs, Resurgamz

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

@Resurgamz

Copy link
Copy Markdown
Author

/merge

@deepin-bot
deepin-bot Bot merged commit 85ddd3f into linuxdeepin:release/snipe Sep 29, 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