fix: Fix issue where system sound effects cannot be selected using th…

[pengfeixx]

…e Tab key

Fix issue where system sound effects cannot be selected using the Tab key

Log: Fix issue where system sound effects cannot be selected using the Tab key
pms: BUG-346643

Summary by Sourcery

Improve keyboard focus handling for selecting system sound effects via the Tab key.

Bug Fixes:

• Allow system sound effect items in the device list to receive focus when navigating with the Tab key.
• Prevent the sound effects page container from incorrectly taking active focus on Tab, ensuring focus moves to individual list items instead.

[sourcery-ai]

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Enables keyboard Tab navigation to system sound effect items by making each device entry focusable via Tab while preventing the parent page container from stealing active focus on Tab, ensuring correct focus behavior within the sound effects page.

Sequence diagram for Tab key focus behavior in sound effects page

sequenceDiagram
    actor User
    participant SoundEffectsPage
    participant DeviceListView
    participant DeviceItem as DeviceListViewItem

    User->>SoundEffectsPage: Press Tab key
    SoundEffectsPage->>SoundEffectsPage: onParentItemChanged sets parent activeFocusOnTab = false
    SoundEffectsPage->>DeviceListView: Forward focus within page
    DeviceListView->>DeviceItem: Move focus to next item
    DeviceItem->>DeviceItem: focusPolicy = Qt.TabFocus
    DeviceItem->>User: Item shows keyboard focus outline

Loading

File-Level Changes

Change	Details	Files
Make device list entries focusable via Tab so sound effects can be selected using keyboard navigation.	Set button-like delegate components to accept focus via the Tab key Enable active focus when navigating to an entry with the Tab key	src/plugin-sound/qml/DeviceListView.qml
Prevent the parent sound effects page container from capturing Tab focus so that focus moves correctly into the device list entries.	Add a handler to clear activeFocusOnTab on the parent item when it becomes available	src/plugin-sound/qml/SoundEffectsPage.qml

---

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

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey - I've left some high level feedback:

• In SoundEffectsPage.qml, the onParentItemChanged: item => { ... } handler doesn't follow typical QML syntax for property change handlers; consider using onParentItemChanged: { if (parentItem) parentItem.activeFocusOnTab = false } instead so you rely on the parentItem property rather than an argument.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `SoundEffectsPage.qml`, the `onParentItemChanged: item => { ... }` handler doesn't follow typical QML syntax for property change handlers; consider using `onParentItemChanged: { if (parentItem) parentItem.activeFocusOnTab = false }` instead so you rely on the `parentItem` property rather than an argument.

---

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

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[deepin-ci-robot]

deepin pr auto review

这段代码主要涉及QML中键盘焦点管理的改进，以下是对语法逻辑、代码质量、代码性能和代码安全方面的详细审查意见：

1. 语法逻辑

DeviceListView.qml

• 修改点：为某个控件添加了 focusPolicy: Qt.TabFocus 和 activeFocusOnTab: true。
• 分析：
  • 语法正确。Qt.TabFocus 表示该控件仅接受Tab键的焦点切换，不接受点击获取焦点。
  • activeFocusOnTab: true 配合 focusPolicy 使用，确保当Tab键导航到该元素时，它会主动获取焦点。
  • 潜在逻辑问题：在列表视图中，通常希望用户可以通过Tab键进入列表，然后使用方向键（上下键）在列表项之间导航。如果列表项本身设置了 focusPolicy: Qt.TabFocus，用户可能需要按多次Tab键才能遍历完所有列表项，而不是按一次Tab进入列表，再用方向键选择。这取决于具体的UI交互设计需求。如果该控件是一个独立的按钮或开关，这个修改是合理的；如果它是可滚动列表中的一项，可能需要重新评估交互逻辑。

SoundEffectsPage.qml

• 修改点：在 DccObject 中添加了 onParentItemChanged 信号处理器。
• 分析：
  • 语法正确，使用了箭头函数 item => { ... }。
  • 逻辑意图：这段代码的目的是防止焦点意外停留在 SoundEffectsPage 的容器项上。当该页面作为子项被加载到父容器时，强制将父容器的 activeFocusOnTab 设为 false。
  • 逻辑风险：强制修改父项的属性是一种具有侵入性的操作。如果 item（父容器）被多个子页面复用，或者父容器本身需要通过Tab键获取焦点，这个修改可能会导致其他页面的焦点行为异常。建议确认父容器的生命周期和复用情况。

2. 代码质量

• 可读性：代码简短清晰，使用了Qt的标准属性，易于理解。
• 注释：建议添加注释。为什么需要在这里修改焦点策略？特别是 SoundEffectsPage.qml 中修改父对象属性的行为比较特殊，如果不加注释，后续维护者可能会困惑为什么要这样做，甚至误认为这是bug。
  • 建议示例：

    // 防止焦点停留在父容器，确保Tab键能直接进入子控件
    onParentItemChanged: item => { if (item) item.activeFocusOnTab = false }

3. 代码性能

• 影响：这两处修改对性能影响极小。
  • focusPolicy 和 activeFocusOnTab 是属性赋值，仅在对象创建或状态改变时生效，不涉及高频计算。
  • onParentItemChanged 仅在对象树结构发生变化时触发，属于低频事件。
• 结论：无需担心性能问题。

4. 代码安全

• 健壮性：
  • 在 SoundEffectsPage.qml 中，使用了 if (item) 进行了判空检查，这是很好的做法，防止了在父对象未准备好时访问属性导致的空指针异常（虽然在QML中通常是安全的，但保持这种习惯很好）。
• 副作用：
  • 如前所述，修改父对象属性（item.activeFocusOnTab = false）会产生副作用。如果父对象是动态创建的，或者该页面被频繁卸载/重载，需要确保这个副作用不会破坏其他页面的状态。

5. 改进建议

1. 关于 DeviceListView.qml 的交互逻辑：

   • 请确认该控件是否真的需要 focusPolicy: Qt.TabFocus。如果这是一个列表，通常建议列表容器接受Tab焦点（focusPolicy: Qt.TabFocus），而列表项接受方向键导航。如果确实需要每个项都支持Tab导航，请确保这是预期的UX设计。

2. 关于 SoundEffectsPage.qml 的实现方式：

   • 更好的解耦方式：直接修改父对象的属性通常是不推荐的，因为它破坏了封装性。建议检查是否可以通过修改 DccObject 或父容器的定义来达成同样的效果，而不是由子组件去“污染”父组件的状态。
   • 例如，可以在父容器加载 SoundEffectsPage 时，由父容器自己决定是否设置 activeFocusOnTab: false。

3. 代码注释：

   • 务必为这两处修改添加清晰的注释，说明修改焦点策略的原因（例如：为了优化无障碍访问体验，或者修复特定的焦点跳转问题）。

总结

这段代码在语法和逻辑上没有明显错误，主要是为了优化键盘导航体验。主要的改进空间在于：

1. 确认列表项的Tab导航逻辑是否符合UX预期。
2. 避免子组件直接修改父组件属性，以降低耦合度。
3. 补充必要的代码注释。

[deepin-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: caixr23, 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:

• debian/deepin/OWNERS

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

[pengfeixx]

/forcemerge

[deepin-bot]

This pr force merged! (status: blocked)