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.

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

Set CommandResult, Exception & save EntryBook independently of Command #31

Merged
merged 11 commits into from
Mar 1, 2019

Conversation

rlrh
Copy link

@rlrh rlrh commented Feb 26, 2019

Modify design to enable getting and setting of CommandResult and Exception, and EntryBook auto-saving, independently of any Commands, with UI updating accordingly

Resolves #34

Supports future commands that need to return an initial result first, do some long-running work, and then return another final result or exception, and also internal tasks that modify the model and need to return a user-facing result or exception

  • Decouples saving address book from command execution - makes address book save itself to storage whenever it is modified, even if not through command execution
  • Makes exception observable and enables it to be manually set (currently used for data-saving exception only)
  • Makes manually set command result observable and enables it to be manually set (currently unused)
  • Makes UI observe command result and exception and update appropriately, in addition to currently updating on command execution
  • Modifies existing tests to support new implementation, adds tests for new capabilities

New:

A command can now return an initial command result first, do some long-running task that involves modifying the model, which will save itself to storage, and then update the observable command result or exception, which can then be handled appropriately by the UI.

Old:

A command can only return an initial command result, and then do some long-running task that involves modifying the model, but it will not save itself to storage, and a final command result or exception cannot be propagated.

How to Use:

In a Command's execute method, return an initial CommandResult, and within an asynchronous task, set the Model's commandResultProperty or exceptionProperty.
Example:

Task<Void> task = new Task<>() {
    @Override
    public Void call() throws Exception {
        try {
            Thread.sleep(5000); // some long-running work
            Platform.runLater(() -> {
                model.setCommandResult(new CommandResult(MESSAGE_SUCCESS));
            });
        } catch (Exception e) {
            Platform.runLater(() -> {
                model.setException(new Exception(MESSAGE_FAILURE));
            });
        } finally {
            return null;
        }
    }
};
new Thread(task).start();
return new CommandResult(String.format(MESSAGE_IN_PROGRESS, toAdd));

@rlrh rlrh marked this pull request as ready for review February 28, 2019 07:36
@rlrh rlrh added this to the v1.1 milestone Feb 28, 2019
@rlrh rlrh changed the title Support asynchronous commands Get and set CommandResult, Exception independently of Command Mar 1, 2019
@rlrh rlrh changed the title Get and set CommandResult, Exception independently of Command Set CommandResult, Exception & save EntryBook independently of Command Mar 1, 2019
@rlrh rlrh self-assigned this Mar 1, 2019
@rlrh rlrh added this to In progress in Automated kanban via automation Mar 1, 2019
@rlrh rlrh added the type.Task Something that needs to be done, but not story, bug or epic. label Mar 1, 2019
@rlrh rlrh merged commit 2750928 into CS2103-AY1819S2-W10-1:master Mar 1, 2019
Automated kanban automation moved this from In progress to Done Mar 1, 2019
@rlrh rlrh deleted the decouple-design branch March 1, 2019 16:15
initHistory();
historySnapshot.next();
commandTextField.setText("");
processCommandSuccess();

Choose a reason for hiding this comment

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

Note to selves that this will be the second time this function is called. The first time is in the line above, within the MainWindow's executeCommand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type.Task Something that needs to be done, but not story, bug or epic.
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants