Skip to content

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

Merged
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
JWWTSL:master
Feb 5, 2026
Merged

fix: Handle exceptions when hovering over up/down arrows#565
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
JWWTSL:master

Conversation

@JWWTSL
Copy link
Contributor

@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 button scrolling to operate on view content offset with pixel step size and support click-based single-step scrolling.

Bug Fixes:

  • Prevent errors when hovering scroll arrows by avoiding reliance on current index and guarding against invalid view state.

Enhancements:

  • Introduce configurable pixel step size for arrow-based scrolling using the list view’s item height.

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

Refactors the ArrowListView button scrolling to operate directly on contentY using a configurable pixel step size (item height), adds single-step scrolling on click, and wires the new step size through ArrowListView, reducing reliance on currentIndex-based scrolling and improving hover/arrow behavior.

Sequence diagram for hover and click scrolling behavior in ArrowListViewButton

sequenceDiagram
    actor User
    participant ArrowListViewButton
    participant ActionButton
    participant itemsView

    User->>ArrowListViewButton: hover over up/down arrow
    ArrowListViewButton->>ActionButton: set hovered = true
    ActionButton->>ActionButton: shouldAutoScroll = hovered && enabled
    ActionButton->>ActionButton: start initialDelayTimer (300ms)
    initialDelayTimer-->>ActionButton: onTriggered
    ActionButton->>ActionButton: delayCompleted = true
    ActionButton->>hoverScrollTimer: start (interval 100ms)
    loop while hovered and enabled
        hoverScrollTimer-->>ActionButton: onTriggered
        ActionButton->>ActionButton: performScroll()
        ActionButton->>itemsView: compute maxY, step, nextY
        ActionButton->>itemsView: set contentY = clamp(nextY, 0, maxY)
    end
    User-->>ArrowListViewButton: stop hover / disable button
    ArrowListViewButton->>ActionButton: shouldAutoScroll = false
    ActionButton->>ActionButton: delayCompleted = false

    User->>ActionButton: click up/down arrow
    ActionButton->>ActionButton: onClicked
    ActionButton->>ActionButton: performScroll()
    ActionButton->>itemsView: set contentY = clamp(contentY ± step, 0, maxY)
Loading

Updated class diagram for ArrowListView and ArrowListViewButton QML components

classDiagram
    class ArrowListViewButton {
        +Item view
        +int direction
        +int stepSize
        +bool active
        +bool shouldAutoScroll
        +bool delayCompleted
        +performScroll()
        +onShouldAutoScrollChanged()
        +onClicked()
    }

    class ActionButton {
        +bool hovered
        +bool enabled
        +iconWidth
        +iconHeight
        +onClicked()
    }

    class InitialDelayTimer {
        +int interval = 300
        +bool repeat = false
        +start()
        +stop()
        +onTriggered()
    }

    class HoverScrollTimer {
        +int interval = 100
        +bool repeat = true
        +start()
        +stop()
        +onTriggered()
    }

    class ArrowListView {
        +Item itemsView
        +Item control
        +int itemHeight
    }

    class UpArrowButton {
        +int direction
        +int stepSize
    }

    class DownArrowButton {
        +int direction
        +int stepSize
    }

    ArrowListViewButton o-- ActionButton
    ArrowListViewButton o-- InitialDelayTimer
    ArrowListViewButton o-- HoverScrollTimer

    ArrowListView o-- ArrowListViewButton : uses
    ArrowListView o-- UpArrowButton
    ArrowListView o-- DownArrowButton

    UpArrowButton <|-- ArrowListViewButton
    DownArrowButton <|-- ArrowListViewButton

    ArrowListViewButton --> ArrowListView : view
    ArrowListView --> ArrowListViewButton : stepSize=itemHeight
Loading

File-Level Changes

Change Details Files
Change arrow button scrolling to adjust ListView.contentY by a configurable pixel step instead of modifying currentIndex.
  • Introduce a stepSize property on ArrowListViewButton defaulting to DS.Style.arrowListView.itemHeight.
  • Rewrite performScroll() to early-return if view is missing or non-scrollable, compute maxY from contentHeight and height, clamp nextY between 0 and maxY, and update view.contentY by +/- stepSize based on direction.
  • Ensure stepSize is at least 1 pixel before applying it.
qt6/src/qml/private/ArrowListViewButton.qml
Improve hover and click interaction for the arrow buttons while keeping the auto-scroll state machine behavior.
  • Keep shouldAutoScroll and delayCompleted but simplify onShouldAutoScrollChanged to only reset delayCompleted when auto-scroll stops.
  • Retain the hover timers but remove now-redundant comments in the QML to streamline the component.
  • Trigger a single performScroll() when the button is clicked via a new onClicked handler, enabling one-step scrolls without hover.
qt6/src/qml/private/ArrowListViewButton.qml
Plumb item-height-based step size from ArrowListView into its up/down arrow buttons.
  • Set stepSize: control.itemHeight on the top ArrowListViewButton instance so its scroll step matches one item height.
  • Set stepSize: control.itemHeight on the bottom ArrowListViewButton instance for symmetric behavior with the top button.
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

deepin pr auto review

代码审查意见

我已对提供的 git diff 进行了详细审查,以下是针对语法逻辑、代码质量、代码性能和代码安全方面的改进建议:

1. 语法逻辑方面

问题1:performScroll()函数中的边界检查逻辑

function performScroll() {
    if (!view)
        return
    var maxY = Math.max(0, view.contentHeight - view.height)
    if (maxY <= 0)
        return
    var step = Math.max(1, stepSize)
    var nextY = direction === ArrowListViewButton.UpButton ? (view.contentY - step)
                                                           : (view.contentY + step)
    view.contentY = Math.max(0, Math.min(maxY, nextY))
}

改进建议

  • maxY <= 0 的检查可能过于严格,当 view.contentHeight 等于 view.height 时,maxY 为 0,此时按钮应该禁用而不是直接返回
  • 建议添加对 stepSize 有效性的检查,确保它是正整数

问题2:stepSize属性定义

property int stepSize: DS.Style.arrowListView.itemHeight

改进建议

  • 缺少对 DS.Style.arrowListView.itemHeight 存在性的检查,可能导致运行时错误
  • 建议添加验证逻辑:
property int stepSize: DS.Style.arrowListView && DS.Style.arrowListView.itemHeight || 1

2. 代码质量方面

问题1:代码注释

  • 原代码中有详细的注释说明各个部分的功能,但修改后的代码删除了这些注释
  • 建议:保留或添加适当的注释,特别是对于 performScroll() 函数的复杂逻辑

问题2:代码一致性

  • ArrowListView.qml 中添加了 stepSize: control.itemHeight,但 ArrowListViewButton.qml 中使用的是 DS.Style.arrowListView.itemHeight
  • 建议:统一使用一种方式设置 stepSize,避免不一致

问题3:错误处理

  • performScroll() 函数中只检查了 view 是否存在,但没有检查 view.contentY 是否有效
  • 建议:添加更全面的错误检查:
function performScroll() {
    if (!view || typeof view.contentY === 'undefined' || typeof view.contentHeight === 'undefined')
        return
    // 其余逻辑...
}

3. 代码性能方面

问题1:定时器管理

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

Timer {
    id: hoverScrollTimer
    interval: 100
    running: shouldAutoScroll && delayCompleted
    onTriggered: performScroll()
}

改进建议

  • shouldAutoScroll 频繁变化时,定时器会频繁启动/停止,可能影响性能
  • 考虑使用更高效的滚动机制,如 BehaviorNumberAnimation 来实现平滑滚动

问题2:滚动计算

  • 每次滚动都重新计算 maxY,即使这些值在视图生命周期内可能不变
  • 建议:缓存这些值,在视图尺寸变化时更新:
property real maxY: Math.max(0, view.contentHeight - view.height)
Connections {
    target: view
    function onContentHeightChanged() { maxY = Math.max(0, view.contentHeight - view.height) }
    function onHeightChanged() { maxY = Math.max(0, view.contentHeight - view.height) }
}

4. 代码安全方面

问题1:属性验证

  • stepSize 属性没有类型和范围验证
  • 建议:添加验证逻辑:
property int stepSize: {
    var size = DS.Style.arrowListView && DS.Style.arrowListView.itemHeight || 1
    return Math.max(1, Math.min(size, 100)) // 限制最大步长为100
}

问题2:边界值处理

  • view.contentY 被直接设置时,没有考虑可能的动画效果或绑定冲突
  • 建议:使用更安全的滚动方式:
function performScroll() {
    if (!view || !view.flickable)
        return
    // 使用flickable的API而不是直接设置contentY
    if (direction === ArrowListViewButton.UpButton) {
        view.flickable.flick(0, stepSize)
    } else {
        view.flickable.flick(0, -stepSize)
    }
}

综合改进建议代码

// ArrowListViewButton.qml 改进版
Loader {
    required property Item view
    property int direction
    property int stepSize: {
        var size = DS.Style.arrowListView && DS.Style.arrowListView.itemHeight || 1
        return Math.max(1, Math.min(size, 100))
    }
    active: view && view.interactive

    // 缓存最大滚动位置
    property real maxY: view ? Math.max(0, view.contentHeight - view.height) : 0

    Connections {
        target: view
        function onContentHeightChanged() { maxY = Math.max(0, view.contentHeight - view.height) }
        function onHeightChanged() { maxY = Math.max(0, view.contentHeight - view.height) }
    }

    sourceComponent: ActionButton {
        // 原有属性...
        
        // 执行滚动操作,带有边界检查
        function performScroll() {
            if (!view || typeof view.contentY === 'undefined' || maxY <= 0)
                return
            
            var step = Math.max(1, stepSize)
            var nextY = direction === ArrowListViewButton.UpButton 
                ? (view.contentY - step) 
                : (view.contentY + step)
            
            // 使用flickable的API进行更安全的滚动
            if (view.flickable) {
                if (direction === ArrowListViewButton.UpButton) {
                    view.flickable.flick(0, step)
                } else {
                    view.flickable.flick(0, -step)
                }
            } else {
                // 回退方案:直接设置contentY
                view.contentY = Math.max(0, Math.min(maxY, nextY))
            }
        }

        // 自动滚动控制
        property bool shouldAutoScroll: hovered && enabled
        property bool delayCompleted: false

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

        Timer {
            id: hoverScrollTimer
            interval: 100
            running: shouldAutoScroll && delayCompleted
            repeat: true
            onTriggered: performScroll()
        }
                
        onShouldAutoScrollChanged: {
            if (!shouldAutoScroll)
                delayCompleted = false
        }

        onClicked: performScroll()
    }
}

这些改进建议旨在提高代码的健壮性、可维护性和性能,同时增强错误处理和边界条件检查。

@deepin-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

@JWWTSL
Copy link
Contributor Author

JWWTSL commented Feb 5, 2026

/forcemerge

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Feb 5, 2026

This pr force merged! (status: unstable)

@deepin-bot deepin-bot bot merged commit 88b6373 into linuxdeepin:master Feb 5, 2026
18 of 20 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