Skip to content
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

Make Grid row/column definitions styleable #4397

Closed
wants to merge 31 commits into from

Conversation

jmacato
Copy link
Member

@jmacato jmacato commented Jul 29, 2020

What does the pull request do?

This is a PR for this concept i had in mind for a long time now. Unit tests seems to pass and ControlCatalog/DataGrid works too.

What is the current behavior?

Currently you can't set Grid's Row/ColumnDefinition property in Styles...

What is the updated/expected behavior with this PR?

This enables the user to set those properties in Styles. If combined with pseudoclasses, it can also enable responsive layout too.

Checklist

  • Added unit tests (if possible)?
  • Added XML documentation to any related classes?

@jmacato jmacato requested a review from a team July 29, 2020 08:22
@jmacato jmacato marked this pull request as ready for review July 29, 2020 08:30
@jmacato jmacato force-pushed the styleable-grid-rowcoldefs branch from 917cf94 to 6eaea94 Compare July 30, 2020 09:20
@MarchingCube
Copy link
Collaborator

Can you add unit tests for this?

Copy link
Member

@grokys grokys left a comment

Choose a reason for hiding this comment

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

Seems to work well, however there's a definite problem with the way the properties are implemented, and also we need some unit tests that actually test this working with styles. Close though!

CellCache cell = new CellCache();

var cell = new CellCache();
var control = (Control)child;

// Read indices from the corresponding properties:
// clamp to value < number_of_columns
Copy link
Member

Choose a reason for hiding this comment

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

This comment isn't really true any more is it? The value's no longer being clamped.

/// </summary>
public ColumnDefinitions ColumnDefinitions
{
get
{
if (_data == null) { _data = new ExtendedData(); }
if (_data.ColumnDefinitions == null) { _data.ColumnDefinitions = new ColumnDefinitions() { Parent = this }; }
if (_data is null)
Copy link
Member

@grokys grokys Aug 6, 2020

Choose a reason for hiding this comment

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

Putting initialization in the getter for a styled property isn't a good idea because it won't be run when calling e.g. GetValue or GetObservable, for example this fails:

        [Fact]
        public void ColumnDefinitions_Should_Not_Be_Null()
        {
            var grid = new Grid();

            Assert.NotNull(grid.GetValue(Grid.ColumnDefinitionsProperty));
        }

It's not really possible to have lazily-initialized styled properties; you should initialize the properties in the constructor. This is the same in WPF/UWP: https://wpf.2000things.com/2010/11/07/118-dont-add-code-to-dependency-property-gettersetter/

Copy link
Member

Choose a reason for hiding this comment

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

@grokys I don't remember about WPF, but UWP has default value factory in the DependencyProperty
See https://docs.microsoft.com/en-us/uwp/api/windows.ui.xaml.propertymetadata.createdefaultvaluecallback?view=winrt-19041

It also helps in cases, when default value is mutable - https://devblogs.microsoft.com/oldnewthing/20191002-00/?p=102950

Copy link
Member

Choose a reason for hiding this comment

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

Although it doesn't provide access to dependency object itself as I remember

@jmacato jmacato force-pushed the styleable-grid-rowcoldefs branch from d52dd89 to 9d14e0b Compare August 7, 2020 11:38
@grokys
Copy link
Member

grokys commented Aug 9, 2020

I've spoken to @jmacato about this PR in private chat and we're running into the problem that collection properties currently cannot be styled due to a limitation of how XAML works.

I've opened an issue to discuss a proposed solution here: #4464

@jmacato jmacato force-pushed the styleable-grid-rowcoldefs branch from 9389d9f to a346964 Compare August 14, 2020 11:11
@grokys
Copy link
Member

grokys commented Dec 6, 2021

Given that this is very out of date by now, and the feature will need changes in the XAML compiler first, I'm going to close this PR to try to reduce noise in our PR list. We should definitely implement this feature though!

@grokys grokys closed this Dec 6, 2021
@CollinAlpert
Copy link
Contributor

I hate to be that guy, but any updates on this? Is there anything I could help out with?

@wieslawsoltes
Copy link
Collaborator

I hate to be that guy, but any updates on this? Is there anything I could help out with?

If you really need that you can do this workaround using custom control based on Grid (bind to ColumnDefinitionsSource and/or RowDefinitionsSource:

public class BindableGrid : Grid
{
	public static readonly StyledProperty<string?> ColumnDefinitionsSourceProperty =
		AvaloniaProperty.Register<BindableGrid, string?>(nameof(ColumnDefinitionsSource));

	public static readonly StyledProperty<string?> RowDefinitionsSourceProperty =
		AvaloniaProperty.Register<BindableGrid, string?>(nameof(RowDefinitionsSource));

	public BindableGrid()
	{
		InvalidateDefinitions();
	}

	public string? ColumnDefinitionsSource
	{
		get => GetValue(ColumnDefinitionsSourceProperty);
		set => SetValue(ColumnDefinitionsSourceProperty, value);
	}

	public string? RowDefinitionsSource
	{
		get => GetValue(RowDefinitionsSourceProperty);
		set => SetValue(RowDefinitionsSourceProperty, value);
	}

	protected override void OnPropertyChanged<T>(AvaloniaPropertyChangedEventArgs<T> change)
	{
		base.OnPropertyChanged(change);

		if (change.Property == ColumnDefinitionsSourceProperty
			|| change.Property == RowDefinitionsSourceProperty)
		{
			InvalidateDefinitions();
		}
	}

	private void InvalidateDefinitions()
	{
		if (ColumnDefinitionsSource is not null && RowDefinitionsSource is not null)
		{
			var columns = GridLength.ParseLengths(ColumnDefinitionsSource).Select(x => new ColumnDefinition(x));
			ColumnDefinitions.Clear();
			ColumnDefinitions.AddRange(columns);

			var rows = GridLength.ParseLengths(RowDefinitionsSource).Select(x => new RowDefinition(x));
			RowDefinitions.Clear();
			RowDefinitions.AddRange(rows);

			InvalidateMeasure();
			InvalidateArrange();
		}
	}
}

@CollinAlpert
Copy link
Contributor

CollinAlpert commented May 30, 2022

@wieslawsoltes Thanks for the workaround, however I am trying to modify the ColumnDefinitions in a TimePicker to save some space.

@jmacato
Copy link
Member Author

jmacato commented May 30, 2022

@CollinAlpert i would really love to revive this but @grokys and I hasnt settled on a final version of this issue #4464 . until then this will not move forward. Which is quite a shame tbh :(

@amwx
Copy link
Contributor

amwx commented Oct 1, 2022

I was looking at some things in MAUI which reminded me of this PR and it seems they have a nice solution for this:

Their BindableProperty.Create function takes an optional delegate for creating a default value, which passes in the reference to the BindableObject for creation, ex from their Grid

public static readonly BindableProperty ColumnDefinitionsProperty = BindableProperty.Create("ColumnDefinitions",
	typeof(ColumnDefinitionCollection), typeof(Grid), null, validateValue: (bindable, value) => value != null,
	propertyChanged: UpdateSizeChangedHandlers, defaultValueCreator: bindable =>
	{
                var colDef = new ColumnDefinitionCollection();
                colDef.ItemSizeChanged += ((Grid)bindable).DefinitionsChanged;
                return colDef;
	});

This is then invoked in BindableObject.GetValue() if necessary. This seems like a pretty straightforward IMO, and shouldn't be to hard to implement as I don't think it would require any big changes to implement and should avoid any issues with priority.

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.

7 participants