Skip to content

Conversation

@wjyrich
Copy link
Contributor

@wjyrich wjyrich commented Feb 2, 2026

  1. 将属性从 scrolledByWheel 重命名为 changedByNonKeyboard 以更好地反 映其用途
  2. 更新了整个代码中对重命名属性的所有引用
  3. 添加了当页面指示器更改当前索引时设置 changedByNonKeyboard 的逻辑
  4. 该属性现在跟踪任何非键盘导航发起的页面更改,包括鼠标滚轮滚动和指示器 点击

Log: 改进了页面导航跟踪以实现更好的焦点管理

Influence:

  1. 测试使用鼠标滚轮进行页面导航 - 焦点应转到新页面的第一个应用
  2. 测试点击页面指示器 - 焦点应转到新页面的第一个应用
  3. 测试键盘页面导航 - 焦点应遵循键盘导航规则
  4. 验证在不同导航方法之间切换时的焦点行为
  5. 测试边缘情况,如从最后一页循环到第一页

PMS: BUG-339605

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.

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

Please try again later or upgrade to continue using Sourcery

1. 将属性从 `scrolledByWheel` 重命名为 `changedByNonKeyboard` 以更好地反
映其用途
2. 更新了整个代码中对重命名属性的所有引用
3. 添加了当页面指示器更改当前索引时设置 `changedByNonKeyboard` 的逻辑
4. 该属性现在跟踪任何非键盘导航发起的页面更改,包括鼠标滚轮滚动和指示器
点击

Log: 改进了页面导航跟踪以实现更好的焦点管理

Influence:
1. 测试使用鼠标滚轮进行页面导航 - 焦点应转到新页面的第一个应用
2. 测试点击页面指示器 - 焦点应转到新页面的第一个应用
3. 测试键盘页面导航 - 焦点应遵循键盘导航规则
4. 验证在不同导航方法之间切换时的焦点行为
5. 测试边缘情况,如从最后一页循环到第一页

PMS: BUG-339605
@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: BLumia, wjyrich

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

@deepin-ci-robot
Copy link

deepin pr auto review

这段代码修改主要涉及对 FullscreenFrame.qml 中翻页逻辑的变量重命名和功能扩展。以下是对该 diff 的详细审查意见:

1. 语法逻辑审查

优点:

  • 语法正确,符合 QML 语言规范。
  • 变量重命名 scrolledByWheelchangedByNonKeyboard 逻辑连贯,且所有引用处都已正确更新。

潜在问题:

  • onCurrentIndexChanged 事件中添加了新逻辑:
    onCurrentIndexChanged: {
        if (listviewPage.currentIndex !== currentIndex) {
            listviewPage.changedByNonKeyboard = true
        }
    }
    这里存在逻辑隐患。listviewPage 本身是一个 ListView,而 onCurrentIndexChanged 是其内部某个子元素(可能是 GridViewListView)的事件。当子元素的 currentIndex 改变时,会触发 listviewPage.changedByNonKeyboard = true。这可能会导致误判
    • 如果用户通过键盘导航导致子元素 currentIndex 变化,也会触发此标志位为 true
    • 这与变量名 changedByNonKeyboard(非键盘方式改变)的含义相悖。
    • 原本的 scrolledByWheel 只在滚轮翻页时设为 true,逻辑更明确。

2. 代码质量审查

优点:

  • 注释更新及时,准确描述了新变量的含义("如果是通过非键盘方式(滚轮/indicator)翻页...")。
  • 变量命名 changedByNonKeyboardscrolledByWheel 更具通用性,符合扩展需求(如支持 indicator 点击翻页)。

改进建议:

  • 变量命名:虽然 changedByNonKeyboard 更通用,但可能过长。若上下文清晰,可考虑 changedExternallynonKeyboardNav 等更简洁的名称。
  • 代码结构onCurrentIndexChanged 中的逻辑可能与后续键盘导航冲突,建议明确区分触发来源(如通过鼠标/触控板/键盘)。

3. 代码性能审查

  • 新增的 onCurrentIndexChanged 逻辑会在每次 currentIndex 变化时执行,可能增加不必要的开销。但考虑到是 UI 事件,性能影响可忽略。
  • 无明显性能问题。

4. 代码安全审查

  • 状态一致性风险changedByNonKeyboard 标志位在多个地方被修改(滚轮翻页、indicator 点击、onCurrentIndexChanged),可能导致状态难以追踪。
  • 竞态条件:如果 onCurrentIndexChanged 和键盘导航同时触发,可能产生不可预期的行为(如焦点错误)。

5. 改进建议

(1) 修复 onCurrentIndexChanged 逻辑

如果目标是支持 indicator 点击翻页,建议明确区分触发来源:

onCurrentIndexChanged: {
    // 假设 indicator 点击会触发此事件,且可以通过某种方式区分(如信号参数)
    if (listviewPage.currentIndex !== currentIndex && isIndicatorClick) {
        listviewPage.changedByNonKeyboard = true
    }
}

或避免在 onCurrentIndexChanged 中设置标志位,改为在 indicator 点击事件中显式设置。

(2) 统一标志位管理

changedByNonKeyboard 的设置集中到一个函数中,减少分散修改:

function markNonKeyboardChange() {
    listviewPage.changedByNonKeyboard = true
    // 可在此添加日志或调试信息
}

然后在滚轮翻页、indicator 点击等处调用此函数。

(3) 添加状态重置逻辑

确保 changedByNonKeyboard 在不需要时及时重置,避免影响后续操作:

onKeyboardNavigation: {
    listviewPage.changedByNonKeyboard = false
}

6. 总结

  • 主要问题onCurrentIndexChanged 中的逻辑可能导致非键盘操作被误判为键盘操作,需明确触发来源。
  • 改进方向:统一标志位管理,明确触发条件,避免状态冲突。
  • 整体评价:代码修改方向合理,但需细化逻辑以确保状态一致性。

@wjyrich wjyrich merged commit 57ef20e into linuxdeepin:master Feb 2, 2026
10 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