Skip to content

fix: Handle exceptions when hovering over up/down arrows#334

Closed
JWWTSL wants to merge 1 commit intolinuxdeepin:masterfrom
JWWTSL:master
Closed

fix: Handle exceptions when hovering over up/down arrows#334
JWWTSL wants to merge 1 commit intolinuxdeepin:masterfrom
JWWTSL:master

Conversation

@JWWTSL
Copy link

@JWWTSL JWWTSL commented Feb 5, 2026

log: Modify arrow scrolling logic to adjust only contentY (scroll by step size in pixels), completely bypassing currentIndex. Also add single-step scrolling when clicking arrows.

pms: bug-335545

Summary by Sourcery

Adjust arrow list view buttons to scroll by pixel-based content offsets instead of index changes and support single-step scrolling on click.

Bug Fixes:

  • Prevent exceptions when hovering over arrow list view buttons by guarding scroll operations when the view is unavailable or not scrollable.

Enhancements:

  • Introduce configurable step size for arrow list view button scrolling based on item height.
  • Unify hover and click behavior so both trigger the same bounded scroll operation.

log: Modify arrow scrolling logic to adjust only contentY (scroll by step size in pixels), completely bypassing currentIndex. Also add single-step scrolling when clicking arrows.

pms: bug-335545
@sourcery-ai
Copy link

sourcery-ai bot commented Feb 5, 2026

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Adjusts ArrowListView arrow button scrolling to operate directly on contentY with a pixel step size and adds single-step scrolling on click, while wiring the step size from ArrowListView into the button component.

Sequence diagram for ArrowListViewButton hover and click scrolling

sequenceDiagram
    actor User
    participant ArrowListViewButton
    participant ActionButton
    participant ListView as view

    User->>ArrowListViewButton: hover over up/down arrow
    ArrowListViewButton->>ActionButton: hovered = true
    ActionButton->>ActionButton: shouldAutoScroll = hovered && enabled
    ActionButton->>ActionButton: start initialDelayTimer (300ms)
    ActionButton->>ActionButton: delayCompleted = true on trigger
    ActionButton->>ActionButton: hoverScrollTimer running = shouldAutoScroll && delayCompleted
    loop while hovered and enabled
        ActionButton->>ActionButton: performScroll()
        ActionButton->>ListView: read contentHeight, height, contentY
        ActionButton->>ActionButton: compute maxY = max(0, contentHeight - height)
        ActionButton->>ActionButton: step = max(1, stepSize)
        ActionButton->>ActionButton: nextY = contentY ± step (by direction)
        ActionButton->>ListView: set contentY = clamp(0, maxY, nextY)
    end
    User-->>ArrowListViewButton: stop hover
    ArrowListViewButton->>ActionButton: shouldAutoScroll = false
    ActionButton->>ActionButton: delayCompleted = false

    User->>ArrowListViewButton: click up/down arrow
    ArrowListViewButton->>ActionButton: onClicked
    ActionButton->>ActionButton: performScroll() (single step)
    ActionButton->>ListView: update contentY by ±stepSize pixels
Loading

Class diagram for ArrowListView and ArrowListViewButton scrolling changes

classDiagram
    class ArrowListViewButton {
        +Item view
        +int direction
        +int stepSize
    }

    class ActionButton {
        +bool hovered
        +bool enabled
        +bool shouldAutoScroll
        +bool delayCompleted
        +performScroll()
        +onShouldAutoScrollChanged()
        +onClicked()
    }

    class InitialDelayTimer {
        +int interval
        +bool running
        +onTriggered()
    }

    class HoverScrollTimer {
        +int interval
        +bool repeat
        +bool running
        +onTriggered()
    }

    class ArrowListView {
        +Item itemsView
        +int itemHeight
    }

    ArrowListView o-- ArrowListViewButton : creates_buttons
    ArrowListViewButton o-- ActionButton : loads
    ActionButton o-- InitialDelayTimer : uses
    ActionButton o-- HoverScrollTimer : uses
    ActionButton --> ArrowListView : modifies_contentY_via_view

    note for ActionButton "performScroll reads view.contentHeight, view.height, view.contentY and sets view.contentY using stepSize"
Loading

File-Level Changes

Change Details Files
Change arrow button scroll behavior to manipulate view.contentY by a bounded pixel step instead of relying on currentIndex.
  • Add a configurable stepSize property to ArrowListViewButton defaulting to DS.Style.arrowListView.itemHeight.
  • Rewrite performScroll to early-return when view is missing or not scrollable, compute maxY, clamp nextY between 0 and maxY, and update view.contentY by +/- step.
  • Ensure a minimum step of 1 pixel via Math.max(1, stepSize).
qt6/src/qml/private/ArrowListViewButton.qml
Refine hover auto-scroll state handling and add click-based single-step scroll for the arrows.
  • Keep using two timers (initialDelayTimer and hoverScrollTimer) but simplify comments and logic while still driving performScroll on hover.
  • On shouldAutoScroll change, reset delayCompleted to false when auto-scroll stops.
  • Wire ActionButton.onClicked to performScroll to enable one-step scrolling on click.
qt6/src/qml/private/ArrowListViewButton.qml
Propagate the list view item height as the scroll step size into the arrow buttons.
  • Pass control.itemHeight as stepSize to the top ArrowListViewButton instance.
  • Pass control.itemHeight as stepSize to the bottom ArrowListViewButton instance.
qt6/src/qml/ArrowListView.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

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.

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 ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@deepin-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: JWWTSL

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
Contributor

deepin pr auto review

Git Diff 代码审查

总体评价

这段代码修改了 Qt6 QML 中的 ArrowListViewArrowListViewButton 组件,主要改变了滚动行为:从基于索引的滚动(incrementCurrentIndex/decrementCurrentIndex)改为基于像素的滚动(直接修改 contentY)。同时,添加了 stepSize 属性来控制滚动步长。

语法逻辑分析

  1. 新增属性的使用

    • ArrowListView.qml 中为上下按钮添加了 stepSize: control.itemHeight 属性
    • ArrowListViewButton.qml 中定义了 stepSize 属性,并设置了默认值 DS.Style.arrowListView.itemHeight
    • 逻辑上保持一致,但 stepSize 的默认值与传入的值可能不同,需要确认 control.itemHeightDS.Style.arrowListView.itemHeight 是否相同
  2. 滚动逻辑改变

    • 原代码使用 incrementCurrentIndex()/decrementCurrentIndex() 实现基于索引的滚动
    • 新代码改为直接修改 contentY 实现基于像素的滚动
    • 添加了边界检查,确保滚动不会超出内容范围
  3. 按钮点击行为

    • 新增了 onClicked: performScroll() 处理,确保点击按钮也能触发滚动

代码质量建议

  1. 注释移除

    • 移除了一些注释,如"Unified scroll operation function"、"Auto-scroll control properties using state machine approach"等
    • 建议:保留有意义的注释,帮助理解代码逻辑
  2. 条件语句格式

    // 当前代码
    if (!shouldAutoScroll)
        delayCompleted = false
    
    // 建议
    if (!shouldAutoScroll) {
        delayCompleted = false
    }

    使用大括号包裹单行条件语句,提高可读性和可维护性

  3. 变量命名

    • maxYnextY 的命名可以更明确,如 maxContentYnewContentY

代码性能建议

  1. 滚动计算优化

    • 每次滚动都重新计算 maxYstep,这些值在大多数情况下是固定的
    • 建议:考虑将这些计算移到属性绑定或初始化阶段,避免重复计算
  2. 定时器性能

    • 两个定时器(initialDelayTimerhoverScrollTimer)的实现看起来合理
    • 建议:考虑根据内容大小动态调整 hoverScrollTimer 的间隔,内容越大可以适当增加间隔

代码安全建议

  1. 空值检查

    • 添加了 if (!view) return 检查,这是一个很好的安全措施
    • 建议:同样检查 direction 属性是否有效
  2. 边界检查

    • 使用 Math.max(0, Math.min(maxY, nextY)) 确保滚动不会超出范围
    • 这是很好的安全措施,防止无效滚动位置
  3. 步长验证

    • 使用 Math.max(1, stepSize) 确保步长至少为1
    • 建议:考虑添加最大步长限制,防止过大值导致滚动过快

改进建议代码

// ArrowListViewButton.qml 中的改进版本
Loader {
    required property Item view
    property int direction
    property int stepSize: DS.Style.arrowListView.itemHeight
    
    // 添加最大步长限制
    readonly property int maxStepSize: 100
    readonly property int effectiveStepSize: Math.min(stepSize, maxStepSize)
    
    active: view.interactive

    sourceComponent: ActionButton {
        // ... 其他属性保持不变 ...

        function performScroll() {
            if (!view || !view.hasOwnProperty("contentY"))
                return
                
            // 验证方向是否有效
            if (direction !== ArrowListViewButton.UpButton && 
                direction !== ArrowListViewButton.DownButton)
                return
                
            var maxContentY = Math.max(0, view.contentHeight - view.height)
            if (maxContentY <= 0)
                return
                
            var step = Math.max(1, effectiveStepSize)
            var newContentY = direction === ArrowListViewButton.UpButton 
                ? (view.contentY - step) 
                : (view.contentY + step)
                
            view.contentY = Math.max(0, Math.min(maxContentY, newContentY))
        }

        property bool shouldAutoScroll: hovered && enabled
        property bool delayCompleted: false

        Timer {
            id: initialDelayTimer
            interval: 300
            running: shouldAutoScroll
            onTriggered: delayCompleted = true
        }

        Timer {
            id: hoverScrollTimer
            // 根据内容大小动态调整间隔
            interval: view.contentHeight > 1000 ? 150 : 100
            repeat: true
            running: shouldAutoScroll && delayCompleted
            onTriggered: performScroll()
        }

        onShouldAutoScrollChanged: {
            if (!shouldAutoScroll) {
                delayCompleted = false
            }
        }

        onClicked: performScroll()
    }
}

总结

这段代码修改了滚动行为,从基于索引的滚动改为基于像素的滚动,提供了更精细的滚动控制。总体上代码质量良好,但可以进一步改进注释、变量命名和性能优化。添加了一些安全检查,但还可以进一步完善边界条件和属性验证。

@18202781743 18202781743 closed this Feb 5, 2026
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