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

Propagate VisualElement.IsEnabled to children #12488

Merged
merged 13 commits into from
Jan 17, 2023
Merged

Propagate VisualElement.IsEnabled to children #12488

merged 13 commits into from
Jan 17, 2023

Conversation

mattleibow
Copy link
Member

@mattleibow mattleibow commented Jan 7, 2023

Description of Change

There are several issues relating to the IsEnabled property because it is a relatively complex property that involves parent state and combinations of other properties.

Previous PRs:

This PR does a few things and tries to avoid major changes or new concepts.

IsEnabled

The main change to existing code is where previously if a command was "disabled" via the CanExecute method, the framework would actually SET the value of the IsEnabled property - overwriting any user value. This causes issues if you have both IsEnabled and Command set.

The new code rather takes advantage of the "coerce value" concept of the BindableProperty and "intercepts" the "user value" to make sure all the related properties are in agreement.

Previously the IButtonElement had a IsEnabledCore property that would be implemented to basically override user values when set - but that now is removed. The new version of that property is a getter-only and is virtual so that other elements can override.

The final IsEnabled value is now determined in the coerce value delegate of the property and is basically a check for IsEnabled = parent.IsEnabled && this.IsEnabledCore && userVlaue. Using the propagation system we already have, changing the parent element or the parent value triggers a re-evaluation of all the children - and thus consistency is preserved. Similarly, if the CanExecute value of a command is changed, then that also triggers a re-evaluation.

The IsEnabled property value is now propagated using the same system that Window and FlowDirection uses: IPropertyPropagationController.PropagatePropertyChanged().

Commands

Another (semi-related) change is to split the internal IButtonElement into ICommandElement and IButtonElement : ICommandEleent. The reason for this is that commands can happen with non-button controls (such as search bar, tool bar and menu bar). This separation now allows the same code to be shared instead of copied - as can be seen by the SearchBarchanges.

Tasks

  • Update IsEnabled to coerce values
  • Verify that it works with bindings and code
  • Add tests

Issues Fixed

@mattleibow mattleibow changed the title Propagate IsEnabled to children Propagate VisualElement.IsEnabled to children Jan 9, 2023
@nefen
Copy link

nefen commented Jan 10, 2023

Will anyone approve it, because the bug is still here!

@hartez
Copy link
Contributor

hartez commented Jan 10, 2023

Suggested changes: dev/is-enabled...dev/is-enabled-alt

@mattleibow
Copy link
Member Author

Very nice! Looking at that and will merge here

@williambohrmann3
Copy link

Very excited for this :)

Comment on lines -90 to +95
[EditorBrowsable(EditorBrowsableState.Never)]
set => SetValue(IsEnabledPropertyKey, value);
set => SetValue(IsEnabledProperty, value);
Copy link
Member Author

Choose a reason for hiding this comment

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

Bonus feature, this now works as a setter too!

Comment on lines +13 to +19
var ctx = self.GetContext(property);
if (ctx?.Binding is not null)
{
// support bound properties
if (!ctx.Attributes.HasFlag(BindableObject.BindableContextAttributes.IsBeingSet))
ctx.Binding.Apply(false);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

@StephaneDelcroix is this the correct way to ask a bindable property to "refresh" or "re-get" the value.

@williambohrmann3
Copy link

Which .NET 7 version is this fix expected in?

@hartez hartez added backport/suggested The PR author or issue review has suggested that the change should be backported. backport/NO This change should not be backported. It may break customers. labels Feb 15, 2023
@hosamyousof
Copy link

How to get this update?

@hosamyousof
Copy link

Hi @mattleibow @PureWeen
When this pull request #12488 will be released.
I checked the latest release 7.0.59 Service Release 3 and it's not there.

Please we need this update.

Thank you

@williambohrmann3
Copy link

@aidanjbailey @hosamyousof it seems like this bug is fixed in .NET 8 but won't be backported to .NET 7

@aidan-bailey
Copy link

Fair enough, thanks for the update. Looking forward to trading my 8 bindings for 1 haha.

@BaY1251
Copy link

BaY1251 commented Feb 28, 2023

非常实用,期待更新

@williambohrmann3
Copy link

This seems to be fixed in my .NET 7 app actually, at least button visual states #9753

@aidan-bailey
Copy link

The plot thickens...

@Sofiya-kumar
Copy link

Still its not working. Have to give separate binding for each item

@mattleibow
Copy link
Member Author

@Sofiya-kumar what version of maui and vs are you using? What platform? Maybe open a new issue and attach a repro so I can have a look.

@Sofiya-kumar
Copy link

Hi @mattleibow ,

Please check the below repo

ChildrenTesting.zip

Microsoft Visual Studio Professional 2022
Version 17.5.4
VisualStudio.17.Release/17.5.4+33530.505
Microsoft .NET Framework
Version 4.8.09032

Installed Version: Professional

Visual C++ 2022 00476-80000-00000-AA061
Microsoft Visual C++ 2022

ASP.NET and Web Tools 17.5.318.41597
ASP.NET and Web Tools

Azure App Service Tools v3.0.0 17.5.318.41597
Azure App Service Tools v3.0.0

C# Tools 4.5.2-3.23171.7+d17f741546fad2786cbd6394d08619544e53a36d
C# components used in the IDE. Depending on your project type and settings, a different version of the compiler may be used.

Extensibility Message Bus 1.4.3 (main@2a4517a)
Provides common messaging-based MEF services for loosely coupled Visual Studio extension components communication and integration.

Microsoft JVM Debugger 1.0
Provides support for connecting the Visual Studio debugger to JDWP compatible Java Virtual Machines

Mono Debugging for Visual Studio 17.5.9 (11975e6)
Support for debugging Mono processes with Visual Studio.

NuGet Package Manager 6.5.0
NuGet Package Manager in Visual Studio. For more information about NuGet, visit https://docs.nuget.org/

Razor (ASP.NET Core) 17.5.2.2316603+9f1b6856460af1e592d387ebef416eadddac453f
Provides languages services for ASP.NET Core Razor.

Syntax Visualizer 1.0
An extension for visualizing Roslyn SyntaxTrees.

TypeScript Tools 17.0.20105.2003
TypeScript Tools for Microsoft Visual Studio

Visual Basic Tools 4.5.2-3.23171.7+d17f741546fad2786cbd6394d08619544e53a36d
Visual Basic components used in the IDE. Depending on your project type and settings, a different version of the compiler may be used.

Visual F# Tools 17.5.0-beta.23053.5+794b7c259d9646a7eb685dad865aa27da7940a21
Microsoft Visual F# Tools

Visual Studio IntelliCode 2.2
AI-assisted development for Visual Studio.

VisualStudio.DeviceLog 1.0
Information about my package

VisualStudio.Mac 1.0
Mac Extension for Visual Studio

VSPackage Extension 1.0
VSPackage Visual Studio Extension Detailed Info

Xamarin 17.5.0.173 (d17-5@33e727c)
Visual Studio extension to enable development for Xamarin.iOS and Xamarin.Android.

Xamarin Designer 17.5.3.46 (remotes/origin/d17-5@e4dd80b2bb)
Visual Studio extension to enable Xamarin Designer tools in Visual Studio.

Xamarin Templates 17.5.41 (ba80d05)
Templates for building iOS, Android, and Windows apps with Xamarin and Xamarin.Forms.

Xamarin.Android SDK 13.2.0.0 (d17-5/797e2e1)
Xamarin.Android Reference Assemblies and MSBuild support.
Mono: 6dd9def
Java.Interop: xamarin/java.interop/main@149d70fe
SQLite: xamarin/sqlite@fdc1e34
Xamarin.Android Tools: xamarin/xamarin-android-tools/main@9f02d77

Xamarin.iOS and Xamarin.Mac SDK 16.2.0.5 (7738c90c9)
Xamarin.iOS and Xamarin.Mac Reference Assemblies and MSBuild support.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/NO This change should not be backported. It may break customers. backport/suggested The PR author or issue review has suggested that the change should be backported. fixed-in-8.0.0-preview.1.7762 Look for this fix in 8.0.0-preview.1.7762! needs-breaking-change-doc-created t/breaking 💥
Projects
None yet
10 participants