Skip to content

Conversation

danwalmsley
Copy link
Member

@danwalmsley danwalmsley commented May 5, 2025

If they user changes the source… when horizontally scrolled to the right, we dont have realized elements before the viewport.

This means we have to fallback to estimating the anchor element position and index. however because the CellsPresenters in each row, make this calculation independently of each other and of the column header… we get different results (due to recycled presenters not resetting its _lastEstimatedElementSizeU field)

This PR fixes it by deferring the columnar presenters estimations and calculation to IColumns, ensuring consistent results.

…and maintain the measure information if the datasource appears to show the same data.

This prevents horizontal scrolling becoming disjointed... if the data is different we scroll to the top left again.
@danwalmsley danwalmsley requested review from grokys, maxkatz6 and Copilot May 5, 2025 11:06
Copy link

@Copilot 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 fixes a horizontal scroll issue by preserving column measure information when the underlying source changes.

  • Introduces a new method, TryToMaintainColumnLayouts, to restore column measurements if the columns match.
  • Posts a scroll-to-home action on the Dispatcher if the columns are different, ensuring the scroll offsets reset.
  • Adds new scroll utility methods (ScrollToHome and ScrollToEnd) in the presenter base class.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/Avalonia.Controls.TreeDataGrid/Primitives/TreeDataGridRowsPresenter.cs Introduces logic to maintain column layouts and conditionally reset the scroll position.
src/Avalonia.Controls.TreeDataGrid/Primitives/TreeDataGridPresenterBase.cs Adds helper methods for scrolling operations.

{
for (int i = 0; i < oldValue.Count; i++)
{
if (newValue[i].Header == oldValue[i].Header)
Copy link
Preview

Copilot AI May 5, 2025

Choose a reason for hiding this comment

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

[nitpick] Comparing columns solely by 'Header' could lead to issues if headers are not unique. Consider using a unique identifier or additional column properties to more reliably verify column equivalence.

Suggested change
if (newValue[i].Header == oldValue[i].Header)
if (newValue[i].Id == oldValue[i].Id) // Use a unique identifier for comparison

Copilot uses AI. Check for mistakes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is ok since the user sets this to either a viewmodel or a string, and we are essentially checking if all the columns are the same.

@danwalmsley danwalmsley changed the title Try and maintain the column measure state if the source changes - Fixes horizontal scroll issue Ensure all Rows layout its cells consistently with the Column headers May 7, 2025
@grokys grokys requested a review from Copilot May 8, 2025 07:11
Copy link

@Copilot 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 addresses inconsistent cell layout during horizontal scrolling by deferring column estimation and calculation to the IColumns interface to ensure uniform presenter behavior.

  • TreeDataGridPresenterBase now exposes StartU and FirstIndex properties and changes the accessibility of EstimateElementSizeU.
  • TreeDataGridColumnarPresenterBase uses a new pattern-matched call to IColumns.GetOrEstimateColumnAt and overrides EstimateElementSizeU to update the cached estimate.
  • IColumns and ColumnList are extended with GetOrEstimateColumnAt and EstimateElementSizeU methods, and ColumnList now uses MathUtilities.AreClose for viewport width comparisons.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
src/Avalonia.Controls.TreeDataGrid/Primitives/TreeDataGridPresenterBase.cs Added properties and updated element size estimation accessibility.
src/Avalonia.Controls.TreeDataGrid/Primitives/TreeDataGridColumnarPresenterBase.cs Updated anchor element and size estimation logic using new IColumns methods.
src/Avalonia.Controls.TreeDataGrid/Models/TreeDataGrid/IColumns.cs Extended interface with column estimation methods.
src/Avalonia.Controls.TreeDataGrid/Models/TreeDataGrid/ColumnList.cs Implemented estimation logic and adjusted viewport comparison methodology.

@danwalmsley
Copy link
Member Author

I attempted to unit test this:

the problem is that when this happens:

            target.Source = newSource;
            // Do a layout pass and check that the columns have been correctly sized.
            Dispatcher.UIThread.RunJobs();

during runjobs, cells presenters seem to start appearing with invalid viewports, causing it to just realize all the columns / cells.

This doesnt match the issue that this PR fixes.
I was unable to determine why that happens, so leaving this test here for future reference.

[AvaloniaFact(Timeout = 10000)]
        public void Should_Correctly_Align_Columns_When_Horizontally_Scrolled_To_Right_And_Source_Changed()
        {
            static void AssertRealizedCells(TreeDataGrid target, Action<List<TreeDataGridColumnHeader>, List<TreeDataGridCell>> assert)
            {
                var columnHeaders = target.ColumnHeadersPresenter!.GetRealizedElements()
                    .Cast<TreeDataGridColumnHeader>()
                    .OrderBy(x=>x.ColumnIndex)
                    .ToList();
                
                var rows = target.RowsPresenter!.GetRealizedElements().Cast<TreeDataGridRow>();

                foreach (var row in rows)
                {
                    var cells = row.CellsPresenter!.GetRealizedElements()
                        .Cast<TreeDataGridCell>()
                        .OrderBy(x => x.ColumnIndex)
                        .ToList();
                    
                    assert(columnHeaders, cells);
                }
            }

            var (target, items) = CreateTarget(columns:
            [
                new TextColumn<Model, int>("ID", x => x.Id, width: new GridLength(100, GridUnitType.Pixel)),
                new TextColumn<Model, string?>("Title1", x => x.Title),
                new TextColumn<Model, string?>("Title2", x => x.Title),
                new TextColumn<Model, string?>("Title3", x => x.Title),
                new TextColumn<Model, string?>("Title4", x => x.Title),
                new TextColumn<Model, string?>("Title5", x => x.Title),
                new TextColumn<Model, string?>("Title6", x => x.Title),
                new TextColumn<Model, string?>("Title7", x => x.Title),
                new TextColumn<Model, string?>("Title8", x => x.Title),
                new TextColumn<Model, string?>("Title9", x => x.Title),
                new TextColumn<Model, string?>("Title10", x => x.Title),
                new TextColumn<Model, string?>("Title11", x => x.Title),
                new TextColumn<Model, string?>("Title12", x => x.Title),
                new TextColumn<Model, string?>("Title13", x => x.Title),
                new TextColumn<Model, string?>("Title14", x => x.Title),
                new TextColumn<Model, string?>("Title15", x => x.Title),
            ]);

            // Scroll horizontally and check that the realized cells are positioned correctly.

            for (int i = 0; i < 20; i++)
            {
                target.Scroll!.Offset = target.Scroll.Offset.WithX(target.Scroll.Offset.X + 50);
                target.UpdateLayout();
                Dispatcher.UIThread.RunJobs();    
            }
            
            for (int i = 0; i < 100; i++)
            {
                target.Scroll!.Offset = target.Scroll.Offset.WithY(target.Scroll.Offset.Y + 50);
                target.UpdateLayout();
                Dispatcher.UIThread.RunJobs();    
            }
            
            for (int i = 0; i < 100; i++)
            {
                target.Scroll!.Offset = target.Scroll.Offset.WithY(target.Scroll.Offset.Y - 50);
                target.UpdateLayout();
                Dispatcher.UIThread.RunJobs();    
            }
            
            AssertRealizedCells(target, (columnHeaders, cells) =>
            {
                Assert.Equal(4, cells.Count);
                Assert.Equal(12, cells[0].ColumnIndex);
                Assert.Equal(13, cells[1].ColumnIndex);
                Assert.Equal(14, cells[2].ColumnIndex);
                Assert.Equal(15, cells[3].ColumnIndex);
                
                Assert.Equal(12, columnHeaders[0].ColumnIndex);
                Assert.Equal(13, columnHeaders[1].ColumnIndex);
                Assert.Equal(14, columnHeaders[2].ColumnIndex);
                Assert.Equal(15, columnHeaders[3].ColumnIndex);
                
                Assert.Equal(columnHeaders[0].Bounds.Left, cells[0].Bounds.Left);
                Assert.Equal(columnHeaders[1].Bounds.Left, cells[1].Bounds.Left);
                Assert.Equal(columnHeaders[2].Bounds.Left, cells[2].Bounds.Left);
                Assert.Equal(columnHeaders[3].Bounds.Left, cells[3].Bounds.Left);
            });
            
            var newSource = new FlatTreeDataGridSource<Model>(items)
            {
                Columns =
                {
                    new TextColumn<Model, int>("ID", x => x.Id, width: new GridLength(100, GridUnitType.Pixel)),
                    new TextColumn<Model, string?>("Title1", x => x.Title),
                    new TextColumn<Model, string?>("Title2", x => x.Title),
                    new TextColumn<Model, string?>("Title3", x => x.Title),
                    new TextColumn<Model, string?>("Title4", x => x.Title),
                    new TextColumn<Model, string?>("Title5", x => x.Title),
                    new TextColumn<Model, string?>("Title6", x => x.Title),
                    new TextColumn<Model, string?>("Title7", x => x.Title),
                    new TextColumn<Model, string?>("Title8", x => x.Title),
                    new TextColumn<Model, string?>("Title9", x => x.Title),
                    new TextColumn<Model, string?>("Title10", x => x.Title),
                    new TextColumn<Model, string?>("Title11", x => x.Title),
                    new TextColumn<Model, string?>("Title12", x => x.Title),
                    new TextColumn<Model, string?>("Title13", x => x.Title),
                    new TextColumn<Model, string?>("Title14", x => x.Title),
                    new TextColumn<Model, string?>("Title15", x => x.Title),
                }
            };
            
            target.Source = newSource;
            
            Assert.True(double.IsNaN(newSource.Columns[1].ActualWidth));
            Assert.True(double.IsNaN(newSource.Columns[2].ActualWidth));
            Assert.True(double.IsNaN(newSource.Columns[3].ActualWidth));

            // Do a layout pass and check that the columns have been correctly sized.
            Dispatcher.UIThread.RunJobs();
            
            AssertRealizedCells(target, (columnHeaders, cells) =>
            {
                Assert.Equal(4, cells.Count);
                Assert.Equal(12, cells[0].ColumnIndex);
                Assert.Equal(13, cells[1].ColumnIndex);
                Assert.Equal(14, cells[2].ColumnIndex);
                Assert.Equal(15, cells[3].ColumnIndex);
                
                Assert.Equal(12, columnHeaders[0].ColumnIndex);
                Assert.Equal(13, columnHeaders[1].ColumnIndex);
                Assert.Equal(14, columnHeaders[2].ColumnIndex);
                Assert.Equal(15, columnHeaders[3].ColumnIndex);
                
                Assert.Equal(columnHeaders[0].Bounds.Left, cells[0].Bounds.Left);
                Assert.Equal(columnHeaders[1].Bounds.Left, cells[1].Bounds.Left);
                Assert.Equal(columnHeaders[2].Bounds.Left, cells[2].Bounds.Left);
                Assert.Equal(columnHeaders[3].Bounds.Left, cells[3].Bounds.Left);
            });
        }

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.

1 participant