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

Updated MAUI bindings to use new Compiled Bindings introduced in .NET 9 #630

Open
wants to merge 1 commit into
base: v.next
Choose a base branch
from

Conversation

prathameshnarkhede
Copy link
Contributor

  • This pull request updates the MAUI bindings in Toolkit to leverage the new Compiled Bindings feature introduced in .NET 9.
  • The changes include replacing existing bindings with compiled bindings for improved performance and type safety.

@prathameshnarkhede prathameshnarkhede self-assigned this Mar 21, 2025
@prathameshnarkhede prathameshnarkhede marked this pull request as ready for review March 21, 2025 22:34
@@ -152,7 +152,7 @@ private static void Attachment_Tapped(object? sender, EventArgs e)
return parent as PopupViewer;
}

private class AttachmentViewModel : System.ComponentModel.INotifyPropertyChanged
internal class AttachmentViewModel : System.ComponentModel.INotifyPropertyChanged
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to make this internally accessible to make use of compiled binding.

Copy link
Member

@dotMorten dotMorten left a comment

Choose a reason for hiding this comment

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

This is a partial review up until overviewmap, since it appears there are general issues with this PR that needs to be addressed and tested.

@@ -56,7 +56,7 @@ static FloorFilter()
VerticalOptions = LayoutOptions.Fill,
InputTransparent = false,
};
textLabel.SetBinding(Label.TextProperty, "ShortName");
textLabel.SetBinding(Label.TextProperty, static (FloorLevel level) => level.ShortName);
Copy link
Member

Choose a reason for hiding this comment

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

You can delete the dynamicdependency attributes above since you've replaced them all with coded bindings

@@ -125,8 +125,8 @@ protected override void OnApplyTemplate()
{
PART_LevelListView.ItemTemplate = LevelDataTemplate;
PART_LevelListView.BindingContext = this;
PART_LevelListView.SetBinding(CollectionView.ItemsSourceProperty, new Binding(nameof(DisplayLevels), BindingMode.OneWay));
PART_LevelListView.SetBinding(CollectionView.SelectedItemProperty, new Binding(nameof(SelectedLevel), BindingMode.TwoWay));
PART_LevelListView.SetBinding(CollectionView.ItemsSourceProperty, nameof(DisplayLevels), BindingMode.OneWay);
Copy link
Member

Choose a reason for hiding this comment

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

I don't quite get the purpose of this change? Where's the static method?

Copy link
Contributor Author

@prathameshnarkhede prathameshnarkhede Mar 21, 2025

Choose a reason for hiding this comment

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

I just moved the code out of new Binding. But It can not be made static expression due to DisplayLevels have private set method.

@@ -40,25 +40,25 @@ static Legend()
s_DefaultLayerItemTemplate = new DataTemplate(() =>
{
var nameLabel = new Label { FontSize = 18, VerticalOptions = LayoutOptions.Center };
nameLabel.SetBinding(Label.TextProperty, $"{nameof(LegendEntry.Content)}.{nameof(Layer.Name)}");
nameLabel.SetBinding(Label.TextProperty, static (Layer layer) => layer.Name);
Copy link
Member

Choose a reason for hiding this comment

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

Judging from the code this takes the LegendEntry as parameter, and returns the entry.Content.Name property if I'm not mistaken.
Testing the legend it doesn't render anything either.

return nameLabel;
});

s_DefaultSublayerItemTemplate = new DataTemplate(() =>
{
var nameLabel = new Label { FontSize = 14, VerticalOptions = LayoutOptions.Center };
nameLabel.SetBinding(Label.TextProperty, $"{nameof(LegendEntry.Content)}.{nameof(ILayerContent.Name)}");
nameLabel.SetBinding(Label.TextProperty, static (ILayerContent content) => content.Name);
Copy link
Member

Choose a reason for hiding this comment

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

same here - I think the datacontext is a LegendEntry

Comment on lines +58 to +61
symbol.SetBinding(SymbolDisplay.SymbolProperty, static (LegendInfo info) => info.Symbol);
sl.Children.Add(symbol);
var nameLabel = new Label { FontSize = 12, VerticalOptions = LayoutOptions.Center };
nameLabel.SetBinding(Label.TextProperty, $"{nameof(LegendEntry.Content)}.{nameof(LegendInfo.Name)}");
nameLabel.SetBinding(Label.TextProperty, static (LegendInfo info) => info.Name);
Copy link
Member

Choose a reason for hiding this comment

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

And here too.

Also remove dynamic dependency attributes at top of method if they are no longer required

@@ -50,15 +50,15 @@ static OverviewMap()
IsAttributionTextVisible = false
};
ActivityIndicator activity = new ActivityIndicator();
activity.SetBinding(ActivityIndicator.IsRunningProperty, new Binding("Map.LoadStatus", converter: converter, converterParameter: "Loading", source : mapView));
activity.SetBinding(ActivityIndicator.IsRunningProperty, static (Map map) => map.LoadStatus, converter: converter, converterParameter: "Loading", source: mapView);
Copy link
Member

Choose a reason for hiding this comment

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

If the source is the MapView, how can the parameter be a map? Setting a breakpoint here never gets hit, but if I change it to MapView it'll hit the delegate.
Same for the ones below

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.

2 participants