dotnet / dotnet/vblang

An update from the Design Safari on the `INotifyPropertyChanged` scenario

Open
#282 28 comments 11 reactions 0 assignees View on GitHub
Dominant language
No language data
Stars
328
Forks
71
PR merge metrics
No merged PRs in 30d

Description

* Four months ago (November, 2017), Visual Basic MVP Klaus Löffelmann (@KlausLoeffelmann) was invited to attend the VB LDM to present on the challenges of modern GUI programming. Klaus presented a few different patterns and important concerns from the field. Expectedly, the tedium of implementing `INotifyPropertyChanged` was chief among them. This left the LDM with a renewed interest in addressing this scenario.

* Klaus did a [prototype](https://twitter.com/loeffelmann/status/932972959081418752) he called `UserInterface` properties that used an attribute `` to generate `INotifyPropertyChanged` implementations.

* I proposed a slightly [more general approach](https://github.com/dotnet/vblang/issues/194) called `WithPropertyEvents` (temporary keyword choice).

* Klaus convinced me it was too messy, I adapted his prototype into a proposal specifically targeting `INotifyPropertyChanged` called [Bindable Classes and Properties](https://github.com/dotnet/vblang/issues/198) that was what Klaus designed but with a keyword instead of an attribute.

* Klaus updated his prototype to use a keyword instead of an attribute but chose a different keyword.

* The VB LDM revisited the iced proposal for compiler plug-in source generators and/or metaprogramming via [Replacable Members](https://github.com/dotnet/vblang/issues/107) for address this scenario.

* The VB looked at the three proposals on the spectrum from most general and expensive (meta-programming/generators) to most specific, self-descriptive, and cheapest (`Bindable`/`UserInterface`) and felt that meta-programming was too far out and too uncertain to block other ideas and that neither of the alternatives would dissuade the pursuit of meta-programming in the future. The middle option `WithPropertyEvents` was selected to move forward as it was cheap/fast and still not hard-coded to a particular UI pattern to bake into the compiler.

* Lots of comments were left on the proposal regarding possible keyword choices.

* I started working on a prototype of `WithPropertyEvents` with a working title/keyword choice of `Template` properties, as it changed the code-gen of auto-props to resemble an implementation of the classic OOP [Template Method Pattern](https://en.wikipedia.org/wiki/Template_method_pattern).

* I consulted with various friends and former colleagues for feedback. This design simplified based on feedback from them and from exploring implementation approaches which reduced the complexity of the code-gen from a property set having multiple pluging points via well-named methods to a vastly simplified single method passing arguments ByRef.

* More contention about keyword choice:
* "`Template` doesn't mean much, which is both not very meaningful, yet marketable--like Services and Workspaces".
* "It's more instructing the property to make a callback, what about `Callback` properties".

* I had several concerns about keyword choice:
* I believe that VB keywords should be somewhat self-descriptive for first-time and occasional readers. Even a business analyst who's pair-programming with the dev should have an idea what the keyword means.
* A single keyword can't capture all of the semantics VB devs might use the feature to implement. Maybe some special syntax that allows devs to name/tag the semantics.
* This feature would be used *a LOT* on ViewModels, so the ceremony can't be much as after you've used it 10 times in one class something like `Template:Binding` is going to get old. A keyword and an identifier gives too much visual space to the meaningless keyword.
* Went fishing through the AOP literature for terminology. Came back with terms like `Joinpoint`, `Pointcut`, and `Advice`.
* Whatever the keyword chosen will ultimately end up naming the feature. It would suck to market `Callback Properties` or `Advice Properties`. `Template Properties` sounds more marketable (though still vague).

* Alex (and others throughout history have proposed an attribute). I don't like the idea of an attribute for a baked-in pattern, but for an open-ended mechanism it seems good. The problem is the attribute isn't in the same class as the property and can't raise any events in another object.

# Today

* I ended up abandoning the whole `Template` property design entirely in favor of an extensible attribute based model I've tentatively titled "Property Handler Attributes". It's pretty simple. Given the existence of a well-known abstract attribute type, e.g. `PropertyHandlerAttribute`, if one or more attributes derived from said well-known type is applied to an auto-prop the code-gen for the setter (and potentially getter) of that auto-prop will:

* _First_ lookup a method *on the type containing the property* (including base types but not extension methods) based on some convention and the name of the attribute (e.g. `NotifyAttribute` -> `NotifyOnPropertySet`).
* If one or more such methods are found, the compiler will perform overload resolution and invoke the result passing the name of the property, a reference to the backing field, and the value parameter. This means that for patterns that depend on access to state or members of the type containing the property, they can call an instance method on that class.
* If no such method is found on the containing type, _then_ a `Shared` method `OnPropertySet` is looked up on the attribute class itself. If found, the compiler will perform overload resolution passing the `Me` parameter of the property (or null if the property is `Shared`, the name of the property, a reference to the backing field, and the value parameter, and invoke the method. This allows simpler handlers which don't depend on special access (like raising an event) or state and whose logic would be the same for any property on any class, e.g. `ThrowIfNullAttribute`.
* In both cases, performing overload resolution means one could use more efficient methods for certain types (e.g. value types) or potentially use generics and type inference in cool ways.
* In both cases, if the invoked method returns a `Boolean` and the result of the invocation is true, the setter will return immediately, ignoring subsequent handlers *AND* the setting of the backing field. This allows for a handler which prevents mutation (`FreezableAttribute`) or ignores duplicate sets (common in `INotifyPropertyChanged` implementations).
* In both cases, if one or more constructor arguments are passed in the attribute specification, those arguments will be appended to the argument list invoking the handler. This allows for attributes like ``.
* In both cases, if overload resolution fails the user gets a nice error message saying a method with the expected signature wasn't found, which means it's easy for "Generate From Usage" to spit out such a method.
* Handlers are emitted in the order the attributes are specified, so users can control what happens when.

This latest iteration addresses the original scenario, is general, has a marketable semi-descriptive name imho, dodges the keyword naming debate, doesn't significantly complicate the compiler and potentially simplifies a lot of programs. It takes repetitive situations that force you to give up the declarative benefits of auto-props to write boilerplate setters and allows you to keep it declarative, which I believe is more in keeping with the spirit of the language.

I implemented the prototype, played with it, good stuff. Now I want to share and get feedback. Here's some screenshots that illustrate the value (I'll build a shareable prototype when I get to a real computer. I've implemented this on a wholly inadequate laptop that's painful to code on):

Here are the property declarations involved:

![image](https://user-images.githubusercontent.com/10539978/37393197-a67ca3d2-272d-11e8-9d46-a7cb707e1547.png)

Here's what some instance handlers themselves look like:
![image](https://user-images.githubusercontent.com/10539978/37393310-fbd78734-272d-11e8-8dcc-5d20207e342b.png)

Here're some shared handlers (implementations in attributes themselves):
![image](https://user-images.githubusercontent.com/10539978/37393435-50cc2416-272e-11e8-8363-c40c8940baf1.png)

Here's a doc comment helping a reader understand what an attribute does:
![image](https://user-images.githubusercontent.com/10539978/37393465-67a4823c-272e-11e8-8fea-c111b5bdce5f.png)

Here's an example of the program running, showing off one of the validator attributes:
![image](https://user-images.githubusercontent.com/10539978/37393541-9a5ebde6-272e-11e8-9085-e99ec9d88a0c.png)

The rest of the attributes I need to record interaction for you to see them work but...

Here's the generated IL for one of the setters:

![image](https://user-images.githubusercontent.com/10539978/37393718-19fa8fe4-272f-11e8-89dd-42639e4727bf.png)

And here's all the actual code for the various demos. Obviously most of this wouldn't ever appear in user-code:
**Models**
``` VB.NET
Imports System
Imports System.Diagnostics
Imports System.Console
Imports System.ComponentModel

Public MustInherit Class PropertyHandlerAttribute
Inherits Attribute
End Class

#Region " Data-binding "

Public Class NotifyAttribute
Inherits PropertyHandlerAttribute
End Class
#End Region

#Region " Data hygene/transformation "
'''
''' Automatically trims leading and trailing whitespace from any value assigned to this property.
''' This trimming happens before subsequent handlers are executed.
'''

Public Class TrimAttribute
Inherits PropertyHandlerAttribute

Public Shared Sub OnPropertySet(sender As Object, propertyName As String, ByRef backingField As String, ByRef value As String)
value = If(value?.Trim(), "")
End Sub
End Class

' This only effects compilation. Attribute never needs to appear in metadata.

Public Class AutoRoundAttribute
Inherits PropertyHandlerAttribute

Sub New(digits As Integer)
' Because this constructor arguments are copied to the generated callsite,
' the 'digits' parameter doesn't actually have to be saved anywhere.
End Sub

Public Shared Sub OnPropertySet(sender As Object, propertyName As String, ByRef backingField As Decimal, ByRef value As Decimal, digits As Integer)
value = Math.Round(value, digits)
End Sub
End Class
#End Region

#Region " Validation "

Public Class ThrowOnNullAttribute
Inherits PropertyHandlerAttribute

Public Shared Sub OnPropertySet(sender As Object, propertyName As String, backingField As Object, value As Object)
If value Is Nothing Then Throw New ArgumentNullException(propertyName)
End Sub
End Class

#Region " Soft validation "

Public Class MaxLengthAttribute
Inherits PropertyHandlerAttribute

Sub New(value As Integer)

End Sub
End Class

Public Class RegexAttribute
Inherits PropertyHandlerAttribute

Sub New(pattern As String, message As String)

End Sub
End Class
#End Region
#End Region

#Region " Out there "

Public Class UndoableAttribute
Inherits PropertyHandlerAttribute

Public Shared Sub OnPropertySet(obj As Object, propertyName As String, previousValue As Object, ByRef newValue As Object)
UndoRedo.Remember(Sub() CallByName(obj, propertyName, CallType.Set, previousValue))
End Sub
End Class

Module UndoRedo
ReadOnly UndoStack As New Stack(Of Action)
ReadOnly RedoStack As New Stack(Of Action)

Private IsUndoPending As Boolean
Private IsRedoPending As Boolean

ReadOnly Property CanUndo As Boolean
Get
Return UndoStack.Count > 0
End Get
End Property

ReadOnly Property CanRedo As Boolean
Get
Return RedoStack.Count > 0
End Get
End Property

Event StackStateChanged()

Sub ForgetAll()
UndoStack.Clear()
RedoStack.Clear()
RaiseEvent StackStateChanged()
End Sub

Sub Remember(action As Action)
If Not IsUndoPending AndAlso Not IsRedoPending Then
UndoStack.Push(action)
RedoStack.Clear()
RaiseEvent StackStateChanged()
ElseIf IsUndoPending Then
RedoStack.Push(action)
ElseIf IsRedoPending Then
UndoStack.Push(action)
End If
End Sub

Sub Undo()
Try
IsUndoPending = True
UndoStack.Pop().Invoke()
Finally
IsUndoPending = False
RaiseEvent StackStateChanged()
End Try
End Sub

Sub Redo()
Try
IsRedoPending = True
RedoStack.Pop().Invoke()
Finally
IsRedoPending = False
RaiseEvent StackStateChanged()
End Try
End Sub

End Module
#End Region

Class ListingInfo
Implements INotifyPropertyChanged
Implements IDataErrorInfo

Private ReadOnly _Errors As New Dictionary(Of String, String) From {{NameOf(Title), ""}, {NameOf(Description), ""}}


Property Title As String = "New Listing"


Property Description As String = "No description."


Property LastSaved As Date


Property ListingCode As String = ""

Public Event PropertyChanged(sender As Object, e As PropertyChangedEventArgs) Implements INotifyPropertyChanged.PropertyChanged

#Disable Warning BC42353 ' Intentionally letting the function return 'False' by default.
Protected Function NotifyOnPropertySet(propertyName As String, ByRef backingField As String, value As String) As Boolean
If value = backingField Then Return True

RaiseEvent PropertyChanged(Me, New PropertyChangedEventArgs(propertyName))
End Function

Protected Function NotifyOnPropertySet(Of T)(propertyName As String, ByRef backingField As T, value As T) As Boolean
If Object.Equals(value, backingField) Then Return True

RaiseEvent PropertyChanged(Me, New PropertyChangedEventArgs(propertyName))
End Function

Protected Sub MaxLengthOnPropertySet(propertyName As String,
ByRef backingField As String,
value As String,
maxLength As Integer)
If value?.Length > maxLength Then
_Errors(propertyName) = $"Must be less than {maxLength} characters long."
Else
_Errors(propertyName) = ""
End If
End Sub

Protected Sub RegexOnPropertySet(propertyName As String, ByRef backingField As String, value As String, pattern As String, message As String)
If Text.RegularExpressions.Regex.IsMatch(value, pattern) Then
_Errors(propertyName) = ""
Else
_Errors(propertyName) = message
End If
End Sub

Private ReadOnly Property ValidationErrors(columnName As String) As String Implements IDataErrorInfo.Item
Get
Dim message As String = ""
If _Errors.TryGetValue(columnName, message) Then
Return message
Else
Return ""
End If
End Get
End Property

Private ReadOnly Property IDataErrorInfo_Error As String Implements IDataErrorInfo.Error
Get
Return ""
End Get
End Property
End Class
```

**UI Code-Behind**
``` VB.NET
Public Class Form1

Private Listings As New ComponentModel.BindingList(Of ListingInfo) From {New ListingInfo}

Private Sub Form1_Load(sender As Object, e As EventArgs) Handles MyBase.Load
ListingInfoBindingSource.DataSource = Listings
AddHandler UndoRedo.StackStateChanged, Sub()
UndoToolStripButton.Enabled = UndoRedo.CanUndo
RedoToolStripButton.Enabled = UndoRedo.CanRedo
End Sub
UndoRedo.ForgetAll()
End Sub

Private Sub ListingInfoBindingNavigatorSaveItem_Click(sender As Object, e As EventArgs) Handles ListingInfoBindingNavigatorSaveItem.Click
Dim newLastSavedDate = Date.Now
For Each item In Listings
item.LastSaved = newLastSavedDate
newLastSavedDate = newLastSavedDate.AddMinutes(1)
Next
End Sub

Private Sub UndoToolStripButton_Click(sender As Object, e As EventArgs) Handles UndoToolStripButton.Click
UndoRedo.Undo()
End Sub

Private Sub RedoToolStripButton_Click(sender As Object, e As EventArgs) Handles RedoToolStripButton.Click
UndoRedo.Redo()
End Sub
End Class
```

And here's [my prototype branch](https://github.com/AnthonyDGreen/roslyn/tree/features/vb-property-handlers).

Now I'm off to the Baltimore airport for another riveting day of flying across the country. Feedback welcomed!

**-ADG**

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.