-
Notifications
You must be signed in to change notification settings - Fork 43
fix: refactor progress bar animation and visual effects #554
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Synchronize source files from linuxdeepin/dtkdeclarative. Source-pull-request: linuxdeepin/dtkdeclarative#554
Reviewer's GuideRefactors the progress bar’s visual effects by replacing timer-driven light sweep logic with a reusable animation component, fixing shadow anchoring, and improving corner-radius clipping for both determinate and indeterminate modes. Class diagram for refactored progress bar animation componentsclassDiagram
class ProgressBarImpl {
}
class ProgressBarVisualArea {
+BoxShadow boxShadow
+Item maskedItem
+Rectangle rect
+OpacityMask rectMask
}
class DeterminateRect {
+color gradientColor
+real lightPosition
+real width
+real height
}
class IndeterminateRect {
+color gradientColor
+real lightPosition
}
class BoxShadow {
+real shadowOffsetY
+real shadowBlur
+real cornerRadius
+color shadowColor
+bool visible
}
class MaskedItem {
+real x
+real width
+real height
}
class OpacityMaskEffect {
}
class LoaderLightAnimDeterminate {
+Component sourceComponent
}
class LoaderLightAnimIndeterminate {
+Component sourceComponent
}
class LightSweepAnimationComponent {
}
class LightSweepAnimationInstance {
+Item targetItem
+int loops
}
class SequentialAnimation {
+int loops
}
class ParallelAnimation {
}
class ColorAnimation {
+Item target
+string property
+color from
+color to
+int duration
}
class NumberAnimation {
+Item target
+string property
+real from
+real to
+int duration
}
class PauseAnimation {
+int duration
}
ProgressBarImpl *-- ProgressBarVisualArea : contains
ProgressBarVisualArea *-- BoxShadow : contains
ProgressBarVisualArea *-- MaskedItem : contains
MaskedItem *-- DeterminateRect : contains
DeterminateRect *-- OpacityMaskEffect : maskedBy
IndeterminateRect *-- OpacityMaskEffect : maskedBy
ProgressBarImpl *-- DeterminateRect : determinateFill
ProgressBarImpl *-- IndeterminateRect : indeterminateFill
ProgressBarImpl *-- LoaderLightAnimDeterminate : uses
ProgressBarImpl *-- LoaderLightAnimIndeterminate : uses
LoaderLightAnimDeterminate --> LightSweepAnimationComponent : loads
LoaderLightAnimIndeterminate --> LightSweepAnimationComponent : loads
LightSweepAnimationComponent o-- LightSweepAnimationInstance : instantiates
LightSweepAnimationInstance *-- SequentialAnimation : root
SequentialAnimation *-- ParallelAnimation : contains
ParallelAnimation *-- ColorAnimation : drivesGradientColor
ParallelAnimation *-- NumberAnimation : drivesLightPosition
SequentialAnimation *-- PauseAnimation : trailingPause
ColorAnimation --> DeterminateRect : updatesGradientColor
NumberAnimation --> DeterminateRect : updatesLightPosition
ColorAnimation --> IndeterminateRect : updatesGradientColor
NumberAnimation --> IndeterminateRect : updatesLightPosition
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've found 2 issues, and left some high level feedback:
- Both
Loaderinstances are declared with the sameid: lightAnimLoaderin this QML file, which will cause id conflicts; use distinct ids or move the animation loader into a shared component to keep ids unique. - The
runningflag for thelightSweepAnimationis only set imperatively inonLoaded(especially for the indeterminate case), so it won’t react whenprogressBar.indeterminateorcontrol.animationStopchange; consider bindingrunningdirectly to these properties (e.g.running: progressBar.indeterminate && !control.animationStop) instead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Both `Loader` instances are declared with the same `id: lightAnimLoader` in this QML file, which will cause id conflicts; use distinct ids or move the animation loader into a shared component to keep ids unique.
- The `running` flag for the `lightSweepAnimation` is only set imperatively in `onLoaded` (especially for the indeterminate case), so it won’t react when `progressBar.indeterminate` or `control.animationStop` change; consider binding `running` directly to these properties (e.g. `running: progressBar.indeterminate && !control.animationStop`) instead.
## Individual Comments
### Comment 1
<location> `qt6/src/qml/private/ProgressBarImpl.qml:58-59` </location>
<code_context>
- moveTimer.interval = 2000
- return;
+
+ Loader {
+ id: lightAnimLoader
+ sourceComponent: lightSweepAnimation
+ onLoaded: {
</code_context>
<issue_to_address>
**issue (bug_risk):** Duplicate `id: lightAnimLoader` in the same QML document will cause an id collision.
There are two `Loader` elements in this file using `id: lightAnimLoader` (one in the determinate section and one in the indeterminate section). Since QML ids must be unique within a document, this can cause load failures or incorrect bindings. Please either rename one of the ids or refactor to share a single loader/component if that’s the intended behavior.
</issue_to_address>
### Comment 2
<location> `qt6/src/qml/private/ProgressBarImpl.qml:61-70` </location>
<code_context>
+ Loader {
+ id: lightAnimLoader
+ sourceComponent: lightSweepAnimation
+ onLoaded: {
+ lightAnimLoader.item.targetItem = rect
+ lightAnimLoader.item.running = true
</code_context>
<issue_to_address>
**issue (bug_risk):** `running` is set only once on load, so the animation won't respond to later state changes.
Because `running` is only set in `onLoaded`, changes to `progressBar.indeterminate` or `control.animationStop` later won’t start/stop the animation. Consider binding `running` to a property that reflects these conditions, or adding handlers that update `running` whenever those properties change so the animation state stays in sync.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Synchronize source files from linuxdeepin/dtkdeclarative. Source-pull-request: linuxdeepin/dtkdeclarative#554
deepin pr auto review我来分析一下这个 QML 代码的改进:
这些改进使得代码更加健壮、高效,同时提供了更好的视觉效果和用户体验。 |
1. Replaced manual timer-based animation with a reusable lightSweepAnimation component 2. Fixed BoxShadow positioning to use anchors instead of manual calculations 3. Added OpacityMask layers to handle corner radius clipping properly 4. Restructured gradient animation to use ColorAnimation for smoother transitions 5. Fixed indeterminate mode animation to respect animationStop control 6. Improved visual consistency between determinate and indeterminate modes Log: Improved progress bar visual effects with smoother animations and better corner handling Influence: 1. Test progress bar in both determinate and indeterminate modes 2. Verify smooth gradient animation without visual glitches 3. Check corner radius rendering at different progress values 4. Test animation stop/start functionality in indeterminate mode 5. Verify BoxShadow positioning and visibility at progress boundaries 6. Test performance with multiple progress bars simultaneously fix: 重构进度条动画和视觉效果 1. 将基于定时器的手动动画替换为可复用的 lightSweepAnimation 组件 2. 修复 BoxShadow 定位,使用锚点替代手动计算 3. 添加 OpacityMask 层以正确处理圆角裁剪 4. 重构渐变动画,使用 ColorAnimation 实现更平滑的过渡 5. 修复不确定模式动画以遵守 animationStop 控制 6. 改进确定模式和不确定模式之间的视觉一致性 Log: 改进了进度条的视觉效果,动画更平滑,圆角处理更好 Influence: 1. 测试确定模式和不确定模式下的进度条 2. 验证平滑的渐变动画,无视觉故障 3. 检查不同进度值下的圆角渲染 4. 测试不确定模式下的动画停止/开始功能 5. 验证进度边界处的 BoxShadow 定位和可见性 6. 测试同时显示多个进度条时的性能表现 pms: BUG-341547
Synchronize source files from linuxdeepin/dtkdeclarative. Source-pull-request: linuxdeepin/dtkdeclarative#554
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, mhduiy The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Synchronize source files from linuxdeepin/dtkdeclarative. Source-pull-request: linuxdeepin/dtkdeclarative#554
Log: Improved progress bar visual effects with smoother animations and better corner handling
Influence:
fix: 重构进度条动画和视觉效果
Log: 改进了进度条的视觉效果,动画更平滑,圆角处理更好
Influence:
pms: BUG-341547
Summary by Sourcery
Refine the progress bar’s visual behavior by replacing custom timer-driven highlight logic with a reusable animation component and improving clipping and shadow handling for both determinate and indeterminate modes.
New Features:
Bug Fixes:
Enhancements: