View initialization override lacks tests and ISupportInitialize.BeginInit/EndInit should be virtual

[kzu]

Since View is intended to be heavily inherited to create new types of views, it would be quite convenient to be able to override BeginInit and EndInit, so that additional automatic initialization can be performed.

This is the typical pattern for interfaces that can be of use to derived classes (i.e. IDisposable and the virtual Dispose impl.).

If desired, the interface implementation can be made explicit and a protected virtual member can be exposed instead, to preserve the base class behavior even if overriden. The former is simpler, but the latter might be more solid if the base View performs critical work on its BeginInit/EndInit pair.

(I can send a PR with whichever approach is deemed more appropriate)

[tznind]

public void BeginInit () and public void EndInit () in View look pretty complicated so I would definetly suggest exposing this functionality with the new protected virtual member (the second option you suggested).

So BeginInitImpl and EndInitImpl with the implementations in View being blank.

We could skip EndInitImpl though because there is an event called Initialized which is invoked as the last line of EndInit which we could expose through an OnEndInit method (which is typical for invoking events). So something like:

/// <summary>
/// This derived from <see cref="ISupportInitializeNotification"/> to allow notify all the views that are beginning initialized.
/// </summary>
public void BeginInit ()
{
	if (!IsInitialized) {
		oldCanFocus = CanFocus;
		oldTabIndex = tabIndex;
	}
	if (subviews?.Count > 0) {
		foreach (var view in subviews) {
			if (!view.IsInitialized) {
				view.BeginInit ();
			}
		}
	}
	
+	BeginInitImpl ();
}

+protected virtual void BeginInitImpl()
+{
+
+}
/// <summary>
/// This derived from <see cref="ISupportInitializeNotification"/> to allow notify all the views that are ending initialized.
/// </summary>
public void EndInit ()
{
	IsInitialized = true;
	if (subviews?.Count > 0) {
		foreach (var view in subviews) {
			if (!view.IsInitialized) {
				view.EndInit ();
			}
		}
	}
-	Initialized?.Invoke (this, EventArgs.Empty);
+	OnInitialized ();
}
+protected virtual void OnInitialized()
+{
+	Initialized?.Invoke (this, EventArgs.Empty);
+}

[BDisp]

If desired, the interface implementation can be made explicit and a protected virtual member can be exposed instead, to preserve the base class behavior even if overriden. The former is simpler, but the latter might be more solid if the base View performs critical work on its BeginInit/EndInit pair.

(I can send a PR with whichever approach is deemed more appropriate)

@kzu I agree allowing override these functions to perform additional initialization and invoke the event as desired. Is advised preserve some initial settings like, oldCanFocus and oldTabIndex, to not fail something. Thanks.

[kzu]

Ok, so I see two slightly different opinions on this :). To sumarize, I think they would be:

• Easiest, most typical way to extend classes (in .NET? everywhere?), just make both virtual:

public virtual void BeginInit() => ...
public virtual void EndInit() => ...

Benefits: clearly and intuitively convey these are supported extensibility hooks. It's the way a lot of .NET and UI frameworks work.
Cons: perhaps someone forgets to invoke base.BeginInit/EndInit? I'd say this is quite rare. Also, the finer control allowed to the extender by allowing their code to run either before or after the base impl. is perhaps a plus too.

• Not exposing this directly, but indirectly via another pair on On* methods, perhaps:

public void BeginInit() 
{
   OnInitializing();
   ...
}

public void EndInit()
{
   ....
   OnInitialized();
}

protected virtual void OnInitializing() { }
protected virtual void OnInitialized() { }

I think I'm fine with both. In the second case, I think it's slightly annoying that BeginInit/EndInit are part of the public API, yet not intended to be invoked by anyone other than the View itself. Seems like both could even be made explicit implementations if we were to go the second route. Reduces public API surface for View while making it clear you're not supposed to touch those. The view itself could downcast itself to ISupportInitialize to invoke BeginInit and EndInit as needed. And we'd have a clear extensibility point via the protected members.

Option 1 seems like it would be most natural given that both BeginInit and EndInit are already public. 🤔

[BDisp]

Should BeginInit be like this?

public void BeginInit() 
{
   ...
   OnInitializing(); 
}

The union of both options has no sense or have?

[kzu]

I think the union of both could work too, yeah. OnInitializing/OnInitialized would be a more deterministic way to run before everything in BeginInit and after everything in EndInit.

Given the current implementation, perhaps BeginInit should be:

public virtual void BeginInit ()
{
	if (!IsInitialized) {
		oldCanFocus = CanFocus;
		oldTabIndex = tabIndex;
	}
        OnInitializing();
	if (subviews?.Count > 0) {
		foreach (var view in subviews) {
			if (!view.IsInitialized) {
				view.BeginInit ();
			}
		}
	}
}

[BDisp]

Let's see the @tznind opinion.

[tznind]

These all sound like good solutions. I think I've been converted from my initial response and I'm now feeling that simplicity might be the best and simply marking them virtual as @kzu first suggested.

If someone forgets to call base.BeginInit then its not really our responsibility to complicate our API to prevent that. And it is the most flexible.

The OnInitializing bit seems like a nice addition too.

[kzu]

Yeah, that was my initial gut feeling too. Devs extending types via inheritance should know already how to properly handle overriding virtual members and invoking base as appropriate. Eventually the library could be made smarter and warn/fix this for users if they forget 😉

[BDisp]

Wow @kzu great API packages you have done and doing, fantastic. Don't give up creating useful libraries, please. Thanks.

[tig]

Related...

• Fixes #3192. Improve correctness / clarity of existing View.AutoSize functionality/unit tests #3202

I added BeginInit/EndInit specific unit tests. In doing so I noticed that BeginInit was virtual but EndInit was not.

I also noted that no existing code in the solution cares one way or another.

I also noted there's no code that exercises ISupportInitialize etc...

I've updated the title of this Issue as a result.