Limitation in ObservableCommand

. So your UI would might bind a Guid to the Execute parameter. I find this a bit ugly since parameters are NOT used by CanExecute and it is inconsistent to use then in CanExecute but to use them in Execute. Also it is not strongly typed so your subscribe method has to cast it to a Guid or whatever you want.

My first solution here was to ignore ICommand parameters entirely and make it an ObservableCommand<T> : IObservable<T>, provide the T value during construction, and emit it on Execute. So each delete command might be a ObservableCommand<Guid>, and the actual Guid is provided when each command is created. This seemed like a nice improvement and works well in my sample app.

Another problem is that often you want a command to be disabled while it is executing an ansynchronous task. To make this happen, you need code like this...

_deleteCommands.Merge().Subscribe(async Guid id =>
{
// disable command, like cmd.OnNext(false);
// await delete the object
// re-enable the command, like cmd.OnNext(true);
});

But you can't disable the command because you don't have it. All you've got is the Guid of the command that was invoked, but not the command itself. To fix both these problems, I think it is better if...

ObservableCommand<T> : IObservable<IEventPattern<ObservableCommand<T>, T>>

Now subscribers can know which Guid is meant to be deleted, and can also enable/disable the command while the subscription logic is executing.

Some implementations of reactive commands have a Func<Task> in the constructor. When the command is executing, it automatically disables the command. Maybe the command exposes an IObservable IsExecuting so you can display progress indicators. CanExecute = IsExecuting==false AND IsEnabled==true. This is all quite nice and automatic. I got it working. But now the ObservableCommand code is much more complicated than what you've got because the BEHAVIOR of what happens on execute is bound up with the command itself.

I'd love to hear your thoughts on this.

jmagaram commented 7 years ago

Another issue is raising the event on the UI thread. I find that providing an Action<Action> invokeOnUIThread in the constructor works nicely; use it to raise the CanExecuteChanged event.

ibebbs commented 7 years ago

Hi @jmagaram,

Firstly, thanks for a thoughtful and thought-provoking comment. My answers below are based on a few initial thoughts regarding your comments but I'm certainly going to consider them further and perhaps look to include some examples in the sample.

Command parameter use when binding multiple controls to a single command

I can totally see your point here. As we've externalised the control of CanExecute away from the command we're no longer able to use the command parameter in the logic determining whether a specific control bound to the command should be enabled or not.

Your solution regarding passing the command parameter to the constructor of an ObservableCommand is neat but would require code that builds an internal list of ObservableCommand instances for each item in the source list. Furthermore it must keep this list of commands synchronised with the source list; i.e. if a "Delete" button is removed then an appropriate ObservableCommand must be removed also.

Alternative 1

Personally I feel that if we're instantiating "objects per item in order to more accurately control interaction with that item", then it might be better to instantiate a small view model which takes the parameter as part of its constructor and uses the basic ObservableCommand subscribed to an internal behaviour that uses this parameter. This would keep the code very clean and declarative.

Alternative 2

Alternatively, if we were to embrace the concept of an ObservableCommand being bound to multiple controls and respecting the command parameter when deciding whether the control should be enabled or not, then perhaps it would be possible to write an ObservableCommand : IObserver<Func<object>>. This command, instead of subscribing to a stream of bools, would subscribe to a stream of predicates. When the "CanExecute(object parameter)" call is made to the command it would return the result of the evaluation of the predicate with the supplied object. It may even be possible to keep backwards compatibility and write it as ObservableCommand : IObserver<bool>, IObserver<Func<object>> where the stream of bools is projected into a stream of Func<object> that simply ignores the parameter and returns the bool when CanExecute is called.

I hope this is clear as I'm not sure I've explained it too well. Plus I've not actually tried this yet, but think it should be doable.

Disabling the command while an asynchronous process runs

I've considered this in the past but never ended up implementing it. I always thought it could be achieved with some form of windowing operator in the canExecute observable. That said, using Alternative #2 above, perhaps it could be done with something like this [pseudo code - haven't tested!]:

private ObservableProperty<IEnumerable<Guid>> _performingAsyncOperation;
private ObservableCommand _performOperation;

private IDisposable ShouldDisablePerformOperationCommandWhenAsyncOperationIsInProgress()
{
  return _performingAsyncOperation
    .Select(guids => o => !((o is Guid) && guids.Contains((Guid)o)))
    .Subscribe(_performOperation);
}

Your subscription to the _performOperation command would need to maintain the collection of async operations in progress but this should be possible with some sort of scan operator. Again, writing without the aid of a compiler, something like:

private IDisposable ShouldPerformAsyncOperationWhenPeformOperationExecuted()
{
    return _performOperation
      .SelectMany(o => /* Wrap the async operation in an observable that emits Start<Guid> and Stop<Guid> items /* )
      .Scan(new ImmutableList<Guid>(), (acc,opp) => opp is Start<Guid> ? acc.Add((Start<Guid>).Value) : acc.Remove((Stop<Guid>).Value))
      .Subscribe(_performingAsyncOperation);
}

Again, this is greatly simplified if the items in the list are wrapped in their own view models.

Raising the event on the UI thread

I would certainly try to stay in the Rx domain as long as possible. Rather than pass an action that invokes the event onto the UI thread why not simply use .ObserveOn([DispatcherScheduler]) just before the subscription. For example, using the command subscription above:

private IDisposable ShouldDisablePerformOperationCommandWhenAsyncOperationIsInProgress()
{
  return _performingAsyncOperation
    .Select(guids => o => !((o is Guid) && guids.Contains((Guid)o)))
    .ObserveOn(_scheduler)
    .Subscribe(_performOperation);
}

Note that I recommend injecting the scheduler(s) your observables depend upon as this aids in testing, (i.e. you can use the TestScheduler or ImmediateScheduler in tests)

Anyway it's really great to hear your experiencing some succes with the behaviour pattern. Let me know how you proceed regarding the alternatives above.

jmagaram commented 7 years ago

Regarding “but would require code that builds an internal list of ObservableCommand instances for each item in the source list…”, I don’t understand the problem. I already create a view model for every item in the list. These view models have an ObservableProperty for every edited property so it is not a problem to have a corresponding ObservableCommand. In my code I also have a button at the top of the page bound to a DeleteSelectedItem command. It is straightforward to unify deleting the selected item, and deleting an item in the list by invoking its corresponding command. I do something like this…

    IDisposable WhenDeleteItem_UpdateDatabase() =>
        _rowsOfItems
        .Select(i => i.Select(j => j.DeleteCommand).Merge())
        .Switch()
        .Merge(_deleteSelectedItem.WithLatestFrom(_selectedItem,(_,t)=>t.Id))
        .Subscribe(id =>
        {
            _db.Items.Delete(id);
            _db.SaveChanges();
        });

Because my list of items keeps getting updated when the database changes I use a Switch to only subscribe to the most recent set of items in the list. The view model for each item in the list is pretty stupid; just a collection of properties and commands. The interesting logic and subscription happens outside that view model.

The simplest way to create the list of view models is to create them all from scratch each time the database is updated, and then stuff them into an ObservableCollection that is bound to the UI. But this causes a lot of flickering as all the old items are removed and all the new items are added. To work around this problem, I use Scan to generate an IObservable<IEnumerable<ViewModel>>. When an item with a new ID appears I add it to the enumerable. But if a view model with the same ID already exists I keep it in the results but update its properties. Sometimes I update the commands on it too. I allow the user to move items up and down in the list. So if an item has been moved to the top of the list, I do an OnNext(false) on the MoveUp command.

Regarding Alternative 1, it is a bit weird for the view model of an object to manage its own deletion because that object is not aware of the context in which it lives. That logic should probably reside outside the specific item’s view model. The most the view model should do is raise an event saying “the user requested I be deleted”.

Regarding Alternative 2 I’m not sure why you’d bind a single command to many objects. I understand your proposed solution here of how to get CanExecute to return a different answer for each object. It seems more complicated than what I’m proposing.

Regarding how to disable commands while an operation is in progress, if I squint I can understand what you are saying. It seems kind of complicated.

I understand the value of staying the in Rx domain for updating the UI thread. However adding some kind of scheduler to the ObservableCommand makes things more foolproof. If you get into the habit of initializing each command (and property) with the proper scheduler you don’t have to worry about how/when those objects are updated. Ideally the UI layer would only subscribe to these things on the UI thread and our view models could be completely oblivious to the issue.

I think for now I’m going to stick with my solution of changing IObservable<object> to IObservable<IEventPattern<ObservableCommand<T>, T>>. It adds almost no complexity to what you’ve got but adds more capability.

By the way I started using your ReadModel approach – that also works well.

ibebbs commented 7 years ago

Perhaps I don't totally see your point ;0)

I thought you were specifically asking about binding a single command to multiple controls. Do you have a repository or something you can share which illustrates the limitations you're describing. Everything I see in your latest comment makes sense but I can't see why you need to change ObservableCommand to achieve it.

jmagaram commented 7 years ago

I don’t have anything public that would make sense without a whole lot of explanation. It’s as simple as this. In the UI I display a list of objects and each object has a Delete button. I bind each button to the delete command on each item’s view model – separate command instance for each item in the list. There is also a Delete button at the top of the list whose meaning is “Delete the selected list item.”, and this is represented by a single delete command on the view model. My main view model needs to subscribe to invocations of the Delete command across ALL these objects. Unless the command is tagged somehow with the Guid of the object it pertains to, I can’t know which one to delete. Also, if I want to disable the Delete button while that specific object is being deleted, then I need to know which delete command instance is invoked. So that is why I like to make my commands IObservable<IEventPattern<ObservableCommand<T>, T>> rather than IObservable<object>. Here is a partial implementation of what is going on:

public class Person
{
    ObservableProperty<string> _firstName;
    ObservableProperty<string> _lastName;
    ObservableCommand<Guid> _delete;

    public Person(string firstName, string lastName, Guid id)
    {
        _firstName = new ObservableProperty<string>(nameof(FirstName), null, firstName);
        _lastName = new ObservableProperty<string>(nameof(LastName), null, lastName);
        _delete = new ObservableCommand<Guid>(id);
    }

    public ObservableCommand<Guid> Delete { get; }
    public string FirstName { get => _firstName.Value; set => _firstName.Value = value; }
    public string LastName { get => _lastName.Value; set => _lastName.Value = value; }
}

public class MainViewModel
{
    public MainViewModel()
    {
        // As my database is updated, I constantly generate a new version of
        // this from the ReadModel and stuff the contents into an ObservableCollection 
        // that is bound to the UI.
        IObservable<IEnumerable<Person>> _people = null;

        // This is bound to the SelectedItem property of the list box in my UI.
        IObservable<Person> _selectedPerson = null;

        // Just a simple button that gets enabled when something is selected in the
        // list box and disabled otherwise.
        ObservableCommand<Unit> _deleteSelectedCommand = null;

        // Here is the logic to listen for delete commands and delete objects from
        // the database.
        var deleteFromListCommand =
            _people
            .Select(i => i.Select(j => j.Delete).Merge())
            .Switch();
        var deleteSelectedCommand =
            _deleteSelectedCommand
            .WithLatestFrom(_selectedPerson.Where(i => i != null), (_, j) => j.Delete)
            .Switch();
        Observable.Merge(deleteSelectedCommand, deleteFromListCommand).Subscribe(i =>
        {
            i.Sender.OnNext(false); // disable command while deleting the object
            // delete object with ID i.EventArgs from the database
        });
    }
}

jmagaram commented 7 years ago

This approach is still working for me but I made a simplifying change for how to unify the DeleteSelectedItem command at the top of the list and the DeleteThisOne command that is on each item within the list. First, I expose the DeleteSelectedItem command through a property with INotifyPropertyChanged support. When the selection changes, I change this property to equal the DeleteThisOne command on the selected item. In other words, DeleteSelectedItem will always equal the same instance as DeleteThisOne on the selected item. The DeleteThisOne command for the selected item is bound to two buttons.

One advantage (other than simplicity) is that if I disable the command while deletion is in progress, both the Delete button on the item in the list AND the Delete button that pertains to the selection will be disabled simultaneously. If the selection changes while deletion is still in progress, the DeleteSelectedItem command gets enabled again.

ibebbs / Caliburn.Micro.Reactive.Extensions