Decouple data EditDialogControllers from ModelManager

[AlexLMeow]

Currently all data-type (Person, ContactGroup) edit dialog controllers access ModelManager to perform changes on the model.

This is bad because:

---

1) Dialogs are reused by different operations

Edit dialogs can be used by different operations (eg. Add, Edit, any new extended actions). We cannot assume all such operations use the same logic.

For example, we do not allow duplicate group names, so our desired behaviour when the user performs an action that would violate uniquness is to warn the user and not proceed. With the current implementation of GroupEditDialogController (which mimics PersonEditDialogController):

If we try to add a new group with the same name as an existing group, we can detect the duplicate clash by checking the existing groups from ModelManager and warn the user..

However if we try to confirm an edit to an existing group without changing its name, we also get blocked, because the controller does not know that this is a special case and that we should not consider the original name when performing the duplicate check. The intent is different.

We can solve this by hacking an ugly solution (like remembering the initial name and excluding it from the duplicate check) but it's ugly, not easily extensible, and obfuscates intent.

We could also somehow inform the edit dialog controller of the specific type of operation and do different things depending on that type, but that bloats the class and violates the Single Responsibility Principle; at this point we would be better off making separate dialogs and controllers for each operation.

Which brings us to point 2:

---

2) Performing changes on the model is not the edit dialog's job

Classes should not be huge and do many things, it is better to modularise the logic into separate 'single responsibility' classes. This is also what we teach CS2103 students.

A dialog's job is only to collect and parse input from the user. See javafx.scene.control.Dialog.

The job of a PersonEditDialog is only to collect input from the user to define all relevant data in a Person object.

It should not change model state; it should not implement advanced input validation (checking the validity of a Person's fields is useful beyond input dialogs, for example saving and loading Persons from the database).

---

Solution? For now I can see 2 ways of dealing with these issues.

---

1) Push the extra logic implementation out of the dialog controller

The dialogs only provide parsed user input to the calling action handler. Stuff like duplicate checks and advanced input validation is left to the action handler method.

Example for an action handler that uses the person edit dialog:

@FXML
protected void handleNewPerson() {
    // launch edit dialog and return the parsed input Person when user confirms.
    Person p = mainController.getInputPersonDetails();
    if (isValid(p)) { // validate p including duplicate check
        model.addPerson(p); // if check passes
    } else {
        // inform user through alert or something else
    }
}

Pros

• Code is easier to reason about and understand
• Textbook use case for javafx.scene.control.Dialog
• Easier to code
• Less coupling

Cons

• Caller has no control over dialog's behaviour till after the user presses 'OK' and the final data is returned

---

2) Inversion of control: inject action-specific logic into the dialog controller

We could pass a callback(?) function to the dialog controller. Basically the caller tells the dialog: "Do this when you have the data needed".

In the example of the PersonEditDialogController, if the caller was the "add new person" handler, it would pass PersonEditDialogController a Consumer<Person> that performs all validation and updates the model with the new Person. PersonEditDialogController then calls that injected code when the user presses 'OK'.

Example for class PersonEditDialogController:

Consumer<Person> onConfirm;

public void setOnConfirm(Consumer<Person> callback) {
    onConfirm = callback;
}

@FXML
protected void handleOk() {
    Person p = parseInputFields();
    onConfirm.accept(p);
}

Example for "new person" action:

@FXML
protected void handleNewPerson() {
    PersonEditDialogController controller = setupDialogAndGetController();
    controller.setOnConfirm((p) -> {
           // implementation and dependencies hidden in callback
           if (isValid(p)) { 
                model.addPerson(p);
           }
    });
    controller.stage.showAndWait();
}

Note that model is also not exposed to PersonEditDialogController.

Pros

• Very very extensible
• Can use events to trigger the dialog since we do not need a return value from the dialog
• Dialog can know when an error occurs and perform corrective actions Example:

public abstract class EditDialogController<T> {
    // other common edit dialog logic
    // ....

    public enum Result {
        SUCCESS, FAIL
    }

    public void setOnConfirm(Function<T, Result> callback) {
        onConfirm = callback;
    }

    @FXML
    protected void handleOk() {
        switch (onConfirm.apply(getInputDataAsPerson())) {
            case SUCCESS :
                closeDialog(); // feedback success?
            case FAIL :
                showFailPopup(); // handle failure
        }
    }
}

• Less coupling

Cons

• Harder to understand for inexperienced students

  ---

Thoughts?

[m133225]

Strongly agree with solution 2. I think the details of the implementation can be optimized as we go along. Also, note that the group picker in PersonEditDialog will also need to be separately extracted into another type of dialog e.g. PickerDialog which we can then use for selection purposes.

[yl-coder]

I favor more towards solution 1. As I think that it teaches more important concepts like those that you mentioned more clearly and intuitively. Solution 2 is far better in terms of both User experience and better code though.

[damithc]

Let's use the simpler approach 1 wherever we can. We'll reserve approach 2 when the simpler approach is not good enough.

[damithc]

What's the status of this one?

[AlexLMeow]

Will do, but lower priority than user facing features like handling remote conflict and handling syncup failure