Skip to content
This repository was archived by the owner on May 1, 2024. It is now read-only.

Remove Device dependency from command implementation (#825) #830

Merged
merged 10 commits into from
Feb 8, 2021
Merged

Remove Device dependency from command implementation (#825) #830

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9fbeffd
Remove `Device` dependency from command implementation (#804)
Feb 4, 2021
722b7b5
Merge branch 'main' into feature/825_asynccommand_without_device_depe…
TheCodeTraveler Feb 4, 2021
b85e3ac
Move Xamarin.CommunityToolkit.ObjectModel.Internals classes to `Inter…
TheCodeTraveler Feb 4, 2021
9057a97
Add Platform-Specific MainThread Implementations, Update Unit Tests t…
TheCodeTraveler Feb 5, 2021
754a962
Update BaseCommand.uwp.cs
TheCodeTraveler Feb 5, 2021
553ab66
Add BaseCommand.wpf.cs
TheCodeTraveler Feb 5, 2021
09339d7
Finish BaseCommand.gtk.cs
TheCodeTraveler Feb 5, 2021
871c71d
Update BaseCommand.shared.cs
TheCodeTraveler Feb 5, 2021
b3b68fe
Merge branch 'main' into feature/825_asynccommand_without_device_depe…
TheCodeTraveler Feb 5, 2021
73c08a5
Merge branch 'main' into feature/825_asynccommand_without_device_depe…
TheCodeTraveler Feb 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ...it/ObjectModel/BaseAsyncCommand.shared.cs → ...odel/Internals/BaseAsyncCommand.shared.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.ComponentModel;
using System.Reflection;
using System.Threading.Tasks;
using System.Windows.Input;
Expand All @@ -9,6 +10,7 @@ namespace Xamarin.CommunityToolkit.ObjectModel.Internals
/// <summary>
/// Abstract Base Class used by AsyncValueCommand
/// </summary>
[EditorBrowsable(EditorBrowsableState.Never)]
public class BaseAsyncCommand<TExecute, TCanExecute> : BaseCommand<TCanExecute>, ICommand
{
readonly Func<TExecute, Task> execute;
Expand Down
4 changes: 3 additions & 1 deletion ...jectModel/BaseAsyncValueCommand.shared.cs → ...Internals/BaseAsyncValueCommand.shared.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.ComponentModel;
using System.Reflection;
using System.Threading.Tasks;
using System.Windows.Input;
Expand All @@ -9,7 +10,8 @@ namespace Xamarin.CommunityToolkit.ObjectModel.Internals
/// <summary>
/// Abstract Base Class used by AsyncValueCommand
/// </summary>
public class BaseAsyncValueCommand<TExecute, TCanExecute> : BaseCommand<TCanExecute>, ICommand
[EditorBrowsable(EditorBrowsableState.Never)]
public abstract class BaseAsyncValueCommand<TExecute, TCanExecute> : BaseCommand<TCanExecute>, ICommand
{
readonly Func<TExecute, ValueTask> execute;
readonly Action<Exception> onException;
Expand Down
9 changes: 8 additions & 1 deletion ...Toolkit/ObjectModel/BaseCommand.shared.cs → ...jectModel/Internals/BaseCommand.shared.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using System.ComponentModel;
using System.Threading;
using Xamarin.CommunityToolkit.Helpers;
using Xamarin.Forms;

Expand All @@ -8,10 +9,12 @@ namespace Xamarin.CommunityToolkit.ObjectModel.Internals
/// <summary>
/// Abstract Base Class used by AsyncCommand and AsyncValueCommand
/// </summary>
[EditorBrowsable(EditorBrowsableState.Never)]
public abstract class BaseCommand<TCanExecute>
{
readonly Func<TCanExecute, bool> canExecute;
readonly WeakEventManager weakEventManager = new WeakEventManager();
readonly SynchronizationContext synchronizationContext;

int executionCount;

Expand All @@ -24,6 +27,7 @@ public BaseCommand(Func<TCanExecute, bool> canExecute, bool allowsMultipleExecut
{
this.canExecute = canExecute ?? (_ => true);
AllowsMultipleExecutions = allowsMultipleExecutions;
synchronizationContext = SynchronizationContext.Current;
}

/// <summary>
Expand Down Expand Up @@ -83,7 +87,10 @@ protected int ExecutionCount
public void RaiseCanExecuteChanged()
{
// Automatically marshall to the Main Thread to adhere to the way that Xamarin.Forms automatically marshalls binding events to Main Thread
Device.BeginInvokeOnMainThread(() => weakEventManager.RaiseEvent(this, EventArgs.Empty, nameof(CanExecuteChanged)));
if (synchronizationContext != null && synchronizationContext != SynchronizationContext.Current)
Copy link
Contributor

Choose a reason for hiding this comment

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

now it doesn't marshall to the Main Thread (cuz synchronizationContext can be not UI thread's context)

Copy link

Choose a reason for hiding this comment

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

well I think the Post method of SyncContext does run the task in the android message loop (aka UI thread).

https://github.com/xamarin/xamarin-android/blob/aad2c29e30e362b19bf526eb14075d8325a1f2d8/src/Mono.Android/Android.App/SyncContext.cs#L30

	public override void Post (SendOrPostCallback d, object state)
	{
		var looper = Application.Context?.MainLooper;
		if (!EnsureLooper (looper, d))
			return;
		using (var h = new Handler (looper)) {
			h.Post (() => d (state));
		}
	}

Copy link
Contributor

@TheCodeTraveler TheCodeTraveler Feb 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hey Gang! I'm going to update this PR to use the platform-specific code for both checking if the current thread is the Main Thread and invoking Main Thread.

It'll be inspired by Xamarin.Essentials.MainThread: https://github.com/xamarin/Essentials/tree/main/Xamarin.Essentials/MainThread

Copy link
Contributor

Choose a reason for hiding this comment

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

The bad part of it's now we have 3 implementations of the same thing, on Forms, essentials and here... I would say to mock the Device platform services as @brminnick said on issue and @AndreiMisiukevich here...

Copy link

Choose a reason for hiding this comment

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

Well to be honest the point raised by @WebDucer is valid: a command belongs to the view model layer and then should be UI independent. The synchronization context is the correct way to implement main thread Marshalling in the net standard fashion. Since Post method make sure the task is executed on the main thread, this code is valid and does not need any modification (in fact this code is taken from prism implementation of delegate command...). Essentials and Forms are UI frameworks so it's only natural they are using directly Xamarin.Forms Device helpers.

Copy link
Contributor

@TheCodeTraveler TheCodeTraveler Feb 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@pictos I agree that having 3 implementations of marshaling code to the Main Thread is not idea, but I see it as a short-term stop-gap:

Xamarin.Essentials is being folded into .NET MAUI, so once we migrate Xamarin.CommunityToolkit from Xamarin.Forms v5.0 to .NET MAUI, we can replace this duplicate implementation with Xamarin.Essentials.MainThread's implementation.

And to avoid this 3rd implementation surfacing to (and potential confusing) developers using Xamarin.CommunityToolkit , I plan on keeping our implementation private/internal.

Copy link

Choose a reason for hiding this comment

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

I'm afraid the net standard implementation of the MainThread of essentials will not work:

   static bool PlatformIsMainThread =>
        throw ExceptionUtils.NotSupportedOrImplementedException

So without SynchronizationContext object, it will force people unit testing view models to mock essentials or forms.

Copy link
Contributor

Choose a reason for hiding this comment

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

@brminnick if you think this is a valuable change, let's keep it then (:

Copy link
Contributor

Choose a reason for hiding this comment

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

@roubachof yeah, for forms people need to wrap the APIs inside interfaces if they want to test it

synchronizationContext.Post(o => weakEventManager.RaiseEvent(this, EventArgs.Empty, nameof(CanExecuteChanged)), null);
else
weakEventManager.RaiseEvent(this, EventArgs.Empty, nameof(CanExecuteChanged));
}

/// <summary>
Expand Down