Skip to content

Update StorageBar.xaml - Replace Storyboard with VisualState Setters#18531

Open
mdtauk wants to merge 1 commit into
files-community:mainfrom
mdtauk:mdta-CQ-StorageBar
Open

Update StorageBar.xaml - Replace Storyboard with VisualState Setters#18531
mdtauk wants to merge 1 commit into
files-community:mainfrom
mdtauk:mdta-CQ-StorageBar

Conversation

@mdtauk
Copy link
Copy Markdown
Contributor

@mdtauk mdtauk commented May 30, 2026

Screen.Recording.2026-05-30.162926.mp4

Based on what we are seeing with the WinUI Perf2026 changes, I thought I would start by updating our StorageBar to use VisualStateSetters, instead of Storyboards.

It does not resolve or close any existing issues

Steps used to test these changes
I ran the UITests app and the control behaves the same as it did before.

If possible, @0x5bfa could make some kind of test to ensure there is nothing that has been overlooked. It should just be 1:1 behaviour though, and I suppose if Microsoft felt moving from storyboards offered a performance improvement, there may be some gains from this change.

Based on what we are seeing with the WinUI Perf2026 changes, I thought I would start by updating our StorageBar to use VisualStateSetters, instead of Storyboards.

I ran the UITests app and the control behaves the same as it did before.

If possible, @0x5bfa could make some kind of test to ensure there is nothing that has been overlooked.  It should just be 1:1 behaviour though, and I suppose if Microsoft felt moving from storyboards offered a performance improvement, there may be some gains from this change.
Copy link
Copy Markdown
Member

@0x5bfa 0x5bfa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@yair100
Copy link
Copy Markdown
Member

yair100 commented May 31, 2026

Does this have a noticeable impact on performance? Have you run any benchmarks? Fyi @marcelwgn

@mdtauk
Copy link
Copy Markdown
Contributor Author

mdtauk commented May 31, 2026

Does this have a noticeable impact on performance? Have you run any benchmarks? Fyi @marcelwgn

I don't think we have enough testing in the app to benchmark it - but if Microsoft has decided it offers a performance improvement - and has no functional difference making the change - I see no reason not to make the change.

There are other uses of Storyboard within the app, such as the Sidebar, etc - but I only felt comfortable touching the code I wrote, and the other uses would be others.

@yair100
Copy link
Copy Markdown
Member

yair100 commented May 31, 2026

I don't think we have enough testing in the app to benchmark it - but if Microsoft has decided it offers a performance improvement - and has no functional difference making the change - I see no reason not to make the change.

VisualState Setters are easier to work with in general, but I don't see a reason to change something that's already working. That is unless we know that it makes a difference.

@mdtauk
Copy link
Copy Markdown
Contributor Author

mdtauk commented May 31, 2026

I don't think we have enough testing in the app to benchmark it - but if Microsoft has decided it offers a performance improvement - and has no functional difference making the change - I see no reason not to make the change.

VisualState Setters are easier to work with in general, but I don't see a reason to change something that's already working. That is unless we know that it makes a difference.

Microsoft are making the changes to all their default WinUI control templates. and list the new xaml resource dictionaries as Perf2026, and its part of their effort to improve WinUI performance.

The changes reduce lines of Xaml, and involve no behavioural changes. If it does boost performance as Microsoft seems to think, and reduces lines of code - it should be a no brainer to make these minimal changes.

@marcelwgn
Copy link
Copy Markdown
Contributor

I did some naive benchmarking, there is a slight performance gain. Question is though, if that matters for us. Its not like we are going to show dozens or hundreds of storagebars is it?

@yair100
Copy link
Copy Markdown
Member

yair100 commented May 31, 2026

I did some naive benchmarking, there is a slight performance gain. Question is though, if that matters for us. Its not like we are going to show dozens or hundreds of storagebars is it?

Thank you for checking. I don't expect this to have much impact in Files, but it's good to confirm that this does have a performance gain. It's also something we can keep in mind for future development.

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.

4 participants