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.

Sign up for GitHub

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

Jump to bottom

style: tidy up and expand comments in Activation #1268

Merged
merged 3 commits into from Feb 19, 2017
Merged

style: tidy up and expand comments in Activation #1268

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
69c8e98
style: tidy up and expand comments in Activation
olevett Feb 8, 2017
3300405
Merge branch 'develop' into tidying-comments-activation
olevett Feb 13, 2017
599a45a
Merge branch 'develop' into tidying-comments-activation
kentcb Feb 19, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
89 changes: 65 additions & 24 deletions src/ReactiveUI/Activation.cs
View file
Open in desktop
Expand Up @@ -54,6 +54,9 @@ public sealed class ViewModelActivator
/// <value>The deactivated.</value>
public IObservable<Unit> Deactivated { get { return deactivated; } }

/// <summary>
Copy link
Member Author

Choose a reason for hiding this comment

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

Comments like this always feel...pointless, happy to remove it if wanted but thought since I'm here I may as well fix the compile time warning...

Copy link
Member

Choose a reason for hiding this comment

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

Voted pointless, the return type is Unit

image

See above for another line that can be 🔥

Copy link
Member Author

Choose a reason for hiding this comment

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

In terms of compiler warnings (can't remember exactly what you get for missing a returns...), would you prefer empty comments or nothing at all?

/// Constructs a new ViewModelActivator
/// </summary>
public ViewModelActivator()
{
blocks = new List<Func<IEnumerable<IDisposable>>>();
Expand Down Expand Up @@ -87,8 +90,10 @@ public IDisposable Activate()
/// This method is called by the framework when the corresponding View
/// is deactivated.
/// </summary>
/// <param name="ignoreRefCount">Force the VM to be deactivated, even
/// if more than one person called Activate.</param>
/// <param name="ignoreRefCount">
/// Force the VM to be deactivated, even
/// if more than one person called Activate.
/// </param>
public void Deactivate(bool ignoreRefCount = false)
{
if (Interlocked.Decrement(ref refCount) == 0 || ignoreRefCount) {
Expand All @@ -98,6 +103,9 @@ public void Deactivate(bool ignoreRefCount = false)
}
}

/// <summary>
/// A set of extension methods to help wire up View and ViewModel activation
/// </summary>
public static class ViewForMixins
{
static ViewForMixins()
Expand All @@ -110,9 +118,11 @@ static ViewForMixins()
/// ViewModel's View is Activated.
/// </summary>
/// <param name="This">Object that supports activation.</param>
/// <param name="block">The method to be called when the corresponding
/// <param name="block">
/// The method to be called when the corresponding
/// View is activated. It returns a list of Disposables that will be
/// cleaned up when the View is deactivated.</param>
/// cleaned up when the View is deactivated.
/// </param>
public static void WhenActivated(this ISupportsActivation This, Func<IEnumerable<IDisposable>> block)
{
This.Activator.addActivationBlock(block);
Expand All @@ -123,10 +133,12 @@ public static void WhenActivated(this ISupportsActivation This, Func<IEnumerable
/// ViewModel's View is Activated.
/// </summary>
/// <param name="This">Object that supports activation.</param>
/// <param name="block">The method to be called when the corresponding
/// <param name="block">
/// The method to be called when the corresponding
/// View is activated. The Action parameter (usually called 'd') allows
/// you to register Disposables to be cleaned up when the View is
/// deactivated (i.e. "d(someObservable.Subscribe());")</param>
/// deactivated (i.e. "d(someObservable.Subscribe());")
/// </param>
public static void WhenActivated(this ISupportsActivation This, Action<Action<IDisposable>> block)
{
This.Activator.addActivationBlock(() => {
Expand All @@ -141,9 +153,11 @@ public static void WhenActivated(this ISupportsActivation This, Action<Action<ID
/// ViewModel's View is Activated.
/// </summary>
/// <param name="This">Object that supports activation.</param>
/// <param name="block">The method to be called when the corresponding
/// <param name="block">
/// The method to be called when the corresponding
/// View is activated. The Action parameter (usually called 'disposables') allows
/// you to collate all the disposables to be cleaned up during deactivation.</param>
/// you to collate all the disposables to be cleaned up during deactivation.
/// </param>
public static void WhenActivated(this ISupportsActivation This, Action<CompositeDisposable> block)
{
This.Activator.addActivationBlock(() => {
Expand All @@ -158,9 +172,11 @@ public static void WhenActivated(this ISupportsActivation This, Action<Composite
/// View is Activated.
/// </summary>
/// <param name="This">Object that supports activation.</param>
/// <param name="block">The method to be called when the corresponding
/// <param name="block">
/// The method to be called when the corresponding
/// View is activated. It returns a list of Disposables that will be
/// cleaned up when the View is deactivated.</param>
/// cleaned up when the View is deactivated.
/// </param>
/// <returns>A Disposable that deactivates this registration.</returns>
public static IDisposable WhenActivated(this IActivatable This, Func<IEnumerable<IDisposable>> block)
{
Expand All @@ -172,12 +188,16 @@ public static IDisposable WhenActivated(this IActivatable This, Func<IEnumerable
/// View is Activated.
/// </summary>
/// <param name="This">Object that supports activation.</param>
/// <param name="block">The method to be called when the corresponding
/// <param name="block">
/// The method to be called when the corresponding
/// View is activated. It returns a list of Disposables that will be
/// cleaned up when the View is deactivated.</param>
/// <param name="view">The IActivatable will ordinarily also host the View
/// cleaned up when the View is deactivated.
/// </param>
/// <param name="view">
/// The IActivatable will ordinarily also host the View
/// Model, but in the event it is not, a class implementing <see cref="IViewFor" />
/// can be supplied here.
/// </param>
/// <returns>A Disposable that deactivates this registration.</returns>
public static IDisposable WhenActivated(this IActivatable This, Func<IEnumerable<IDisposable>> block, IViewFor view)
{
Expand All @@ -204,10 +224,12 @@ public static IDisposable WhenActivated(this IActivatable This, Func<IEnumerable
/// View is Activated.
/// </summary>
/// <param name="This">Object that supports activation.</param>
/// <param name="block">The method to be called when the corresponding
/// <param name="block">
/// The method to be called when the corresponding
/// View is activated. The Action parameter (usually called 'd') allows
/// you to register Disposables to be cleaned up when the View is
/// deactivated (i.e. "d(someObservable.Subscribe());")</param>
/// deactivated (i.e. "d(someObservable.Subscribe());")
/// </param>
/// <returns>A Disposable that deactivates this registration.</returns>
public static IDisposable WhenActivated(this IActivatable This, Action<Action<IDisposable>> block)
{
Expand All @@ -219,13 +241,17 @@ public static IDisposable WhenActivated(this IActivatable This, Action<Action<ID
/// View is Activated.
/// </summary>
/// <param name="This">Object that supports activation.</param>
/// <param name="block">The method to be called when the corresponding
/// <param name="block">
/// The method to be called when the corresponding
/// View is activated. The Action parameter (usually called 'd') allows
/// you to register Disposables to be cleaned up when the View is
/// deactivated (i.e. "d(someObservable.Subscribe());")</param>
/// <param name="view">The IActivatable will ordinarily also host the View
/// deactivated (i.e. "d(someObservable.Subscribe());")
/// </param>
/// <param name="view">
/// The IActivatable will ordinarily also host the View
/// Model, but in the event it is not, a class implementing <see cref="IViewFor" />
/// can be supplied here.
/// </param>
/// <returns>A Disposable that deactivates this registration.</returns>
public static IDisposable WhenActivated(this IActivatable This, Action<Action<IDisposable>> block, IViewFor view)
{
Expand All @@ -241,12 +267,16 @@ public static IDisposable WhenActivated(this IActivatable This, Action<Action<ID
/// View is Activated.
/// </summary>
/// <param name="This">Object that supports activation.</param>
/// <param name="block">The method to be called when the corresponding
/// <param name="block">
/// The method to be called when the corresponding
/// View is activated. The Action parameter (usually called 'disposables') allows
/// you to collate all disposables that should be cleaned up during deactivation.</param>
/// <param name="view">The IActivatable will ordinarily also host the View
/// you to collate all disposables that should be cleaned up during deactivation.
/// </param>
/// <param name="view">
/// The IActivatable will ordinarily also host the View
/// Model, but in the event it is not, a class implementing <see cref="IViewFor" />
/// can be supplied here.
/// </param>
/// <returns>A Disposable that deactivates this registration.</returns>
public static IDisposable WhenActivated(this IActivatable This, Action<CompositeDisposable> block, IViewFor view = null)
{
Expand All @@ -262,7 +292,6 @@ static IDisposable handleViewActivation(Func<IEnumerable<IDisposable>> block, IO
var viewDisposable = new SerialDisposable();

return new CompositeDisposable(
// Activation
activation.Subscribe(activated => {
// NB: We need to make sure to respect ordering so that the cleanup
// happens before we invoke block again
Expand All @@ -280,7 +309,6 @@ static IDisposable handleViewModelActivation(IViewFor view, IObservable<bool> ac
var viewVmDisposable = new SerialDisposable();

return new CompositeDisposable(
// Activation
activation.Subscribe(activated => {
if (activated) {
viewVmDisposable.Disposable = view.WhenAnyValue(x => x.ViewModel)
Expand Down Expand Up @@ -315,16 +343,29 @@ static IDisposable handleViewModelActivation(IViewFor view, IObservable<bool> ac

/// <summary>
/// This class implements View Activation for classes that explicitly describe
/// their activation via ICanActivate. This class is used by the framework.
/// their activation via <see cref="ICanActivate"/>. This class is used by the framework.
/// </summary>
public class CanActivateViewFetcher : IActivationForViewFetcher
Copy link
Member Author

Choose a reason for hiding this comment

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

This feels like it should be in internal class, it's whole purpose in life is to be something used by the framework rather than end consumers, but that's definitely a breaking change/needs some discussion...

Copy link
Member

@ghuntley ghuntley Feb 8, 2017 •
edited

Choose a reason for hiding this comment

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

Hmm, I don't know.

My perspective on breaking stuff has changed since starting work on video courseware; there has to be an incredibly good reason to do it as it wastes the time of consumers and my time if the courseware needs to be updated. @kentcb probably has similar feelings as he's going through this process right now with authoring a book.

I had a quick look at the GitHub for Desktop aka Visual Studio plugin source code and it has taken no references to this class. It's one of the larger open-source projects that uses ReactiveUI. https://github.com/github/VisualStudio/search?utf8=%E2%9C%93&q=CanActivateViewFetcher

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm wary of breaking things, but the pre-existing comment is along the lines of "used by the framework" and it's explicitly registered by ReactiveUI, so think its both fairly low risk and fairly low impact....

{
/// <summary>
/// Returns a positive integer for derivates of the <see cref="ICanActivate"/> interface.
/// </summary>
/// <param name="view">The source type to check</param>
/// <returns>
/// A positive integer if <see cref="GetActivationForView(IActivatable)"/> is supported,
/// zero otherwise
/// </returns>
public int GetAffinityForView(Type view)
{
return (typeof(ICanActivate).GetTypeInfo().IsAssignableFrom(view.GetTypeInfo())) ?
10 : 0;
}

/// <summary>
/// Get an observable defining whether the view is active
/// </summary>
/// <param name="view">The view to observe</param>
/// <returns>An observable tracking whether the view is active</returns>
public IObservable<bool> GetActivationForView(IActivatable view)
{
var ca = view as ICanActivate;
Expand Down