sialcasa / mvvmFX

an Application Framework for implementing the MVVM Pattern with JavaFX
Apache License 2.0
489 stars 104 forks source link

Improvement: provide a convenient way for controllers to connect actions to existing ViewModel DelegateCommand #537

Closed ntherrien closed 6 years ago

ntherrien commented 6 years ago

Use case: A command is defined in the ViewModel (DelegateCommand), with a condition also defined in ViewModel.

View binds to executable property and since ViewModel and View are created at the same time, the Command must exist or else the binding will cause a NullPointerException.

Action is known to a controller class only. ViewModel does not know of controller class. Action cannot be defined in ViewModel class. It will be only set later during initialization.

Controller <---/?set command?/---> ViewModel <---binds to command --- View

Current Solution is to allow to set the Action later by using a custom Supplier:

DelegateCommandActionSupplier class created to allow to set Action later:
public class DelegateCommandActionSupplier implements Supplier<Action> {
    private Action action;

    @Override
    public Action get() {
        return action;
    }

    public void set(Action action) {
        this.action = action;
    }
}

Command is created with DelegateCommandActionSupplier. ViewModel has a method to set the command action on supplier. Controller can set Action on DelegateCommandActionSupplier during initialization.

Controller ---set action on command--> ViewModel <--binds to command-- View

Improvement: This ticket is to suggest an improvement to the API to either add to or to create a new kind of DelegateCommand which allows the user to set the action directly on the command object instead of going through the intermediate custom supplier class.

In other words, it would be nice if the DelegateCommand interface would allow for a setAction() function so we don't have to manage suppliers...

manuel-mauky commented 6 years ago

Hi, can you explain the use-case where you need to define an action from the controller? Typically, the ViewModel defines the UI-Logic and the View only connects to the ViewModel. Allowing the View to define ui-logic in the ViewModel sounds like a violation againts the MVVM pattern. But maybe I'm missing some valid use-cases?

ntherrien commented 6 years ago

Hi! The View only connects to the ViewModel (ie: viewModel.getCommand().execute()) The ViewModel defines the DelegateCommand, or else the view would hit a null. ie: private Command command = new DelegateCommand( action ); public Command getCommand();

But my ViewModel does not know about the Action logic for the command. My Controller contains the Action logic and knows about the ViewModel.

Right now, I am resolving this problem by defining a custom supplier on the command and allowing the controller to set the Action on the supplier. The supplier is also defined on the ViewModel, but has a setDelegatedAction(Action) method to set the action after the ViewModel is created but before the View will call it.

In the use case I am suggesting, the Controller sets an Action on the DelegateCommand directly. The improvement is that the viewModel does not need to explicitly provide means for setting an Action on a supplier and provide that supplier to the command.

Example: In ViewModel: private DelegateCommand command = new DelegateCommand();

In the controller's initialization code: viewModel.getCommand().setDelegatedAction(action);

manuel-mauky commented 6 years ago

One thing to keep in mind is that Action is a specialization of JavaFXs [Task](https://docs.oracle.com/javafx/2/api/javafx/concurrent/Task.html) andTaskcan only be used a single time. TheDelegateCommandon the other hand is meant to be used multiple times. This is the reason why you have to provide aSupplierinstead of justAction`. This way we can create a new Action instance for each invokation.

Therefore introducting a method setDelegateAction is problematic. So to be correct your Controller should also provide new instances of Actions for every invokation.

By using a Supplier you can achive your goal like this:

public class MyViewModel implements ViewModel {
    private Supplier<Action> actionSupplier = () -> new Action() {
        protected void action() throws Exception {
            // default implementation, can be no-op
        }
    };

    private DelegateCommand command = new DelegateCommand(() -> actionSupplier.get()); 

    public void setActionSupplier(Supplier<Action> supplier) {
        this.actionSupplier = supplier;
    }
}

Here I have a field for the action supplier that can be replaced from outside. But instead of passing this action supplier directly to the DelegateCommand I create a new supplier that, when invoked will invoke the currently set actionSupplier.

ntherrien commented 6 years ago

I understand. Thank you for explaining this to me. I agree with you there needs to be a new Action for every execute(). I will rewrite my code to use a model similar to your example. Closing topic.