Skip to content

refactor: chapter control progress bar bindings#717

Open
United600 wants to merge 1 commit intomainfrom
United600/chapter-binding
Open

refactor: chapter control progress bar bindings#717
United600 wants to merge 1 commit intomainfrom
United600/chapter-binding

Conversation

@United600
Copy link
Collaborator

No description provided.

@United600 United600 requested a review from Copilot November 7, 2025 18:35
@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Nov 7, 2025
@United600 United600 changed the title refactor: update chapter control progress bar bindings refactor: chapter control progress bar bindings Nov 7, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR modernizes the XAML data binding in the ChapterProgressBar control by migrating from runtime Binding to compile-time x:Bind syntax, which provides better performance and compile-time type checking.

  • Converted all four property bindings (Width, Maximum, Minimum, Value) from Binding to x:Bind with explicit Mode=OneWay

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@@ -18,11 +18,11 @@

<DataTemplate x:Key="ItemTemplate" x:DataType="viewModels:ChapterViewModel">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Instead of spamming Mode=OneWay I could have just done:

<DataTemplate
    x:Key="ItemTemplate"
    x:DataType="viewModels:ChapterViewModel"
    x:DefaultBindMode="OneWay">

Copy link
Owner

Choose a reason for hiding this comment

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

Ooh I don't know about that. I think explicitly state binding mode still has its merits. I rely on the fact that x:Bind defaults to OneTime quite often. I don't know if implicitly changing the binding mode is a good idea here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, it still seems useful in certain situations (like in this one).

Copy link
Owner

@huynhsontung huynhsontung left a comment

Choose a reason for hiding this comment

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

If I remember correctly, I used Bindings here for a reason. When I last tested this, there were some edge cases where ItemRepeater and x:Bind don't play nice together. Did you encounter any issues in your testing?

@United600
Copy link
Collaborator Author

If I remember correctly, I used Bindings here for a reason. When I last tested this, there were some edge cases where ItemRepeater and x:Bind don't play nice together. Did you encounter any issues in your testing?

No, but I haven't tested it extensively. Just a couple of files with chapters. But now that I know you've run into issues, I'll test it with a few more.

@United600
Copy link
Collaborator Author

If I remember correctly, I used Bindings here for a reason. When I last tested this, there were some edge cases where ItemRepeater and x:Bind don't play nice together. Did you encounter any issues in your testing?

Can confirm. Works correctly for the first file in a session, but subsequent files incorrectly applies the Value to some chapters.

The ItemsRepeater also isn't properly clearing the list. While replacing it with an ItemsControl would resolve both this issue and the previous one, it introduces a regression (increasing the Spacing will cause value misalignment in some files).

<ItemsControl ItemTemplate="{StaticResource ItemTemplate}" ItemsSource="{x:Bind ProgressItems}">
    <ItemsControl.ItemsPanel>
        <ItemsPanelTemplate>
            <StackPanel Orientation="Horizontal" Spacing="1" />
        </ItemsPanelTemplate>
    </ItemsControl.ItemsPanel>
</ItemsControl>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants