Skip to content

fix(browser): intercept PageDown/PageUp to snap to page boundary#263

Merged
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
pengfeixx:fix/pagedown-page-boundary
Apr 21, 2026
Merged

fix(browser): intercept PageDown/PageUp to snap to page boundary#263
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
pengfeixx:fix/pagedown-page-boundary

Conversation

@pengfeixx
Copy link
Copy Markdown
Contributor

Override keyPressEvent in SheetBrowser to handle PageDown/PageUp by jumping to exact page boundaries instead of pixel-based scrolling.

重写SheetBrowser的keyPressEvent,PageDown/PageUp直接跳转到
精确页面边界,避免像素滚动导致的累积偏移。

Log: 修复PageDown翻页时页面边界逐渐偏移的问题
PMS: BUG-353721
Influence: 所有缩放模式下PageDown/PageUp均精确跳转到上/下一页,不再有累积偏移。

Override keyPressEvent in SheetBrowser to handle PageDown/PageUp
by jumping to exact page boundaries instead of pixel-based scrolling.

重写SheetBrowser的keyPressEvent,PageDown/PageUp直接跳转到
精确页面边界,避免像素滚动导致的累积偏移。

Log: 修复PageDown翻页时页面边界逐渐偏移的问题
PMS: BUG-353721
Influence: 所有缩放模式下PageDown/PageUp均精确跳转到上/下一页,不再有累积偏移。
@deepin-ci-robot
Copy link
Copy Markdown

deepin pr auto review

这段代码实现了一个基本的键盘翻页功能,通过拦截 PageDownPageUp 按键来调用 m_sheet 的翻页方法。以下是针对语法逻辑、代码质量、代码性能和代码安全的详细审查及改进建议:

1. 语法逻辑审查

  • 基本逻辑正确:代码正确地检查了 m_sheet 是否存在,并在按下 PageDown/PageUp 时调用相应的方法。
  • 事件传递:在处理完特定按键后,代码正确地调用了 DGraphicsView::keyPressEvent(event),确保其他按键事件能被正常处理。

潜在问题

  • 按键过滤逻辑:当前代码只处理了 PageDownPageUp,但没有考虑其他可能的翻页快捷键(如 Home/End 或方向键)。如果需要更完整的键盘导航支持,可以扩展此逻辑。
  • 自动重复按键:代码通过 !event->isAutoRepeat() 过滤了自动重复按键,这可能是为了避免快速连续翻页。但用户可能期望长按按键时连续翻页,需确认是否符合产品需求。

2. 代码质量审查

  • 命名规范:变量和方法命名清晰,符合 Qt 的命名规范。
  • 注释:头文件中的注释提到了"高度模式",但代码逻辑并未体现这一点,可能需要更新注释或补充逻辑。

改进建议

  • 常量提取:可以将 Qt::Key_PageDownQt::Key_PageUp 提取为常量或枚举,提高可读性。
  • 代码复用:如果未来需要支持更多翻页按键,建议将按键判断逻辑封装为单独的函数。

3. 代码性能审查

  • 性能影响小keyPressEvent 是事件驱动,性能开销主要在于 m_sheet->jumpToNextPage()m_sheet->jumpToPrevPage(),这部分取决于 m_sheet 的实现。
  • 事件处理效率:当前逻辑简单直接,没有明显的性能问题。

4. 代码安全审查

  • 空指针检查:代码检查了 m_sheet 是否为空,这是好的实践。
  • 事件指针安全event 是 Qt 事件循环管理的指针,通常不需要手动释放,当前代码没有内存安全问题。

潜在风险

  • 线程安全:如果 m_sheet 可能在其他线程中被修改,需要加锁保护。但根据上下文,m_sheet 似乎是 UI 线程中的对象,问题不大。

5. 改进后的代码示例

void SheetBrowser::keyPressEvent(QKeyEvent *event)
{
    if (m_sheet) {
        switch (event->key()) {
            case Qt::Key_PageDown:
                if (!event->isAutoRepeat()) {
                    m_sheet->jumpToNextPage();
                    return;
                }
                break;
            case Qt::Key_PageUp:
                if (!event->isAutoRepeat()) {
                    m_sheet->jumpToPrevPage();
                    return;
                }
                break;
            // 可扩展其他翻页按键
            case Qt::Key_Home:
                m_sheet->jumpToFirstPage();
                return;
            case Qt::Key_End:
                m_sheet->jumpToLastPage();
                return;
            default:
                break;
        }
    }
    DGraphicsView::keyPressEvent(event);
}

6. 头文件注释改进

/**
 * @brief keyPressEvent
 * 处理键盘按键事件,支持 PageDown/PageUp/Home/End 等翻页操作
 * @param event 键盘事件对象
 */
void keyPressEvent(QKeyEvent *event) override;

总结

当前代码实现简单且功能正确,但可以从以下方面改进:

  1. 扩展翻页按键支持(如 Home/End)。
  2. 使用 switch-case 替代多个 if,提高可读性。
  3. 更新注释以准确反映功能。
  4. 确认是否需要支持长按连续翻页。

如果产品需求明确只需要 PageDown/PageUp,当前代码已经足够。

@deepin-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lzwind, pengfeixx

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

@pengfeixx
Copy link
Copy Markdown
Contributor Author

/merge

@deepin-bot deepin-bot Bot merged commit a6918d6 into linuxdeepin:master Apr 21, 2026
10 checks passed
@pengfeixx pengfeixx deleted the fix/pagedown-page-boundary branch April 21, 2026 06:05
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