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

Add close command #115

Conversation

iamdiluxedbutcooler
Copy link

@iamdiluxedbutcooler iamdiluxedbutcooler commented Oct 21, 2024

closes #113
close command to close the split view

please focus on changes in the 4 latest commits (after fixing EOF 2)

Copy link

@colinhia colinhia left a comment

Choose a reason for hiding this comment

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

Overall LGTM! Just left some possible suggestions

}

/**
* Constructs a {@code CommandResult} with the specified {@code feedbackToUser},
* and other fields set to their default value.
*/
public CommandResult(String feedbackToUser) {
this(feedbackToUser, false, false);
this(feedbackToUser, false, false, false, null);

Choose a reason for hiding this comment

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

Instead of using null as a place holder value, perhaps we could consider using a static constant EMPTY_PERSON to align with OOP principles?

Choose a reason for hiding this comment

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

Adding on, you can place this EMPTY_PERSON or DEFAULT_PERSON value in CommandCommons class.


// Add the tier-specific style class
String tier = person.getTier().toParsableString().toLowerCase();
tierLabel.getStyleClass().add(tier + "-tier");

Choose a reason for hiding this comment

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

Great idea of using the enum and appending -tier to get the corresponding strings!

} else if (commandText.trim().toLowerCase().equals("close")) {
handleCloseCommand();
}

Choose a reason for hiding this comment

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

Perhaps to align with the same levels of abstraction, the boolean evaluations (trim().toLowerCase...) can be moved to command result (similar to how below it is commandResult.isShowHelp); and abstracted as isShowView

Copy link

@ZShunRen ZShunRen left a comment

Choose a reason for hiding this comment

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

LGTM, great work in making the close command simple.

}

/**
* Constructs a {@code CommandResult} with the specified {@code feedbackToUser},
* and other fields set to their default value.
*/
public CommandResult(String feedbackToUser) {
this(feedbackToUser, false, false);
this(feedbackToUser, false, false, false, null);

Choose a reason for hiding this comment

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

Adding on, you can place this EMPTY_PERSON or DEFAULT_PERSON value in CommandCommons class.


public static final String COMMAND_WORD = "close";

public static final String MESSAGE_SUCCESS = "Detail view closed.";

Choose a reason for hiding this comment

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

Perhaps this should be changed to Detailed view closed.

Comment on lines +16 to +23
public void execute_close_success() {
CloseCommand closeCommand = new CloseCommand();
CommandResult commandResult = closeCommand.execute(model);

assertEquals(CloseCommand.MESSAGE_SUCCESS, commandResult.getFeedbackToUser());
assertFalse(commandResult.isShowHelp());
assertFalse(commandResult.isExit());
}

Choose a reason for hiding this comment

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

Perhaps you can add one more test to check if when close is used when the detailed panel is not shown, that nothing happens?

Choose a reason for hiding this comment

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

Testing on GUI quite a big change, have opened a new issue to further add that #120

itsme-zeix and others added 18 commits October 22, 2024 12:58
…r/add-view-and-close-command

Add view command
…d-close-split-command

# Conflicts:
#	src/main/java/seedu/address/logic/commands/CloseCommand.java
#	src/main/java/seedu/address/logic/commands/HelpCommand.java
#	src/main/java/seedu/address/ui/MainWindow.java
#	src/test/java/seedu/address/logic/commands/CommandResultTest.java
#	src/test/java/seedu/address/logic/commands/HelpCommandTest.java
@iamdiluxedbutcooler iamdiluxedbutcooler merged commit 9074ba2 into AY2425S1-CS2103T-T14-4:master Oct 22, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Close command implementation
5 participants