Skip to content

Refactor InitProgressbarAnimation & avoid style cast issue #4499

Open
DavidGBrett wants to merge 8 commits into
devfrom
fix-InitProgressbarAnimation-avoid-style-creation
Open

Refactor InitProgressbarAnimation & avoid style cast issue #4499
DavidGBrett wants to merge 8 commits into
devfrom
fix-InitProgressbarAnimation-avoid-style-creation

Conversation

@DavidGBrett
Copy link
Copy Markdown
Contributor

@DavidGBrett DavidGBrett commented May 29, 2026

Should close #4348

Bug Fix

Avoids the creation of a new style based on PendingLineStyle as there seems to be a WPF bug in handling cloning of styles containing SystemAccentColors - seemingly the trigger for #4348 and other related issues.
That new style was created to add a trigger to play an animation if the progress bar is visible, so instead I moved that to an event handler.

Refactor

  • Generally improved the maintainability and readability of the method by being more explicit and using named variables where possible.
  • The length of the progress bar is now calculated based on the existing rect values in the xaml instead of a hardcoded value matching them.
  • While effectively the same right now, used the progress bar's actual width instead of the windows for the distance it needs to travel, more explicit and will work better if we want to change that.
  • Only start the animation if the progress bar is actually visible on the screen, instead of just when its visibility is set

Summary by cubic

Refactors the progress bar animation to start/stop based on true visibility and moves style usage to XAML to avoid the style/brush cloning cast issue. Fixes the progress bar styling problem (see #4348) and simplifies the animation logic.

Summary of changes

  • Changed: Build a single _progressBarStoryboard with explicit targets; use ProgressBar.ActualWidth and dynamic length; keep animations Freeze()d; set PendingLineStyle in XAML via DynamicResource; use ProgressBar.IsVisible instead of Visibility.
  • Added: IsVisibleChanged handler to begin/stop the storyboard only when the bar is truly visible.
  • Removed: On-the-fly Style creation with visibility Trigger, BeginStoryboard/StopStoryboard, and storyboard name registration.
  • Memory impact: Lower allocations by avoiding per-instance style cloning; one storyboard field retained; frozen animations.
  • Security risks: None.
  • Unit tests: No new tests (UI animation change).

Release Note
The progress bar animates more reliably and displays correctly, fixing a visual glitch.

Written for commit 238162a. Summary will update on new commits.

Review in cubic

…ler in InitProgressbarAnimation

This avoids the creation of a new style - so the problematic brush stroke with its SystemAccentColour resource in PendingLineStyle is not cloned
…tions

Clearer and more consistent with how its done elsewhere in the file.
@github-actions github-actions Bot added this to the 2.2.0 milestone May 29, 2026
@DavidGBrett DavidGBrett marked this pull request as ready for review May 29, 2026 12:24
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 29, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

Refactors progress-bar animation in MainWindow to use a stored Storyboard started/stopped on visibility changes and adds a dynamic PendingLineStyle binding to the ProgressBar Line.

Changes

Progress Bar Animation Control

Layer / File(s) Summary
Animation initialization and visibility wiring
Flow.Launcher/MainWindow.xaml.cs
Adds a private _progressBarStoryboard field; InitProgressbarAnimation now constructs and freezes two DoubleAnimations targeting Line.X1/Line.X2, stores the Storyboard, re-subscribes ProgressBar.IsVisibleChanged, and sets the VM ProgressBarVisibility to Hidden. ProgressBar_IsVisibleChanged begins or stops the stored Storyboard.
ProgressBar style binding
Flow.Launcher/MainWindow.xaml
Sets the ProgressBar Line element Style to {DynamicResource PendingLineStyle}.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

Possibly related PRs

Suggested reviewers

  • jjw24
  • Jack251970
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main changes: refactoring InitProgressbarAnimation and avoiding a style cast issue, which directly matches the PR objectives.
Linked Issues check ✅ Passed The PR addresses the linked issue #4348 by eliminating runtime style/brush cloning that triggered the InvalidCastException. The changes implement XAML-based style definition and visibility-driven animation control as required.
Out of Scope Changes check ✅ Passed All changes are focused on refactoring the progress bar animation in MainWindow.xaml and MainWindow.xaml.cs to address the specific casting issue, with no unrelated modifications detected.
Description check ✅ Passed The PR description clearly describes the bug fix addressing issue #4348 (WPF style cloning issue with SystemAccentColors) and the refactoring changes to the progress bar animation logic, directly relating to the changeset.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-InitProgressbarAnimation-avoid-style-creation

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-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.

1 issue found across 2 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread Flow.Launcher/MainWindow.xaml.cs Outdated
Calculated version that waits for true visibility

Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
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.

BUG: Unable to cast object of type 'System.Windows.Media.Color' to type 'System.Windows.Expression'

1 participant