Create view command #138
Create view command #138
Conversation
Andrew22Teoh
assigned Andrew22Teoh and AbdulrahmanAlRammah and unassigned AbdulrahmanAlRammah
| @@ -13,6 +13,9 @@ public class CommandResult { | |||
|
|
|||
| private final String feedbackToUser; | |||
|
|
|||
| /** User wants to view all information about a contact. */ | |||
| private final boolean shouldView; | |||
There was a problem hiding this comment.
I suggest changing the name to showFullView or another more descriptive name.
There was a problem hiding this comment.
Sounds good, I've changed this to shouldOpenView
| public static final String COMMAND_WORD = "view"; | ||
|
|
||
| public static final String MESSAGE_USAGE = COMMAND_WORD | ||
| + ": Views all information about the person identified by the index number in the displayed person list.\n" |
There was a problem hiding this comment.
Message should be ": Shows all" instead of views.
There was a problem hiding this comment.
Sounds good to me, changed accordingly!
| + "Parameters: INDEX (must be a positive integer)\n" | ||
| + "Example: " + COMMAND_WORD + " 1"; | ||
|
|
||
| public static final String MESSAGE_VIEW_SUCCESS = "Showing view for %1$s."; |
There was a problem hiding this comment.
I think "Displaying information for" would be better.
There was a problem hiding this comment.
Good point, edited to "Showing information for..."!
| * | ||
| * @param personList The filtered {@code ObservableList} from {@code Logic}. Will only view 1 {@code Person}. | ||
| */ | ||
| public void view(ObservableList<Person> personList) { |
There was a problem hiding this comment.
Just wondering. Why does this method accept a list instead of just accepting the person straight up. Isn't just complicating things?
There was a problem hiding this comment.
This method is called in the MainWindow class, which is at quite a high level of abstraction. Within the UI elements, there are generally no mentions or references to any Logic objects like the Person class, and so this method is called as view(logic.getFilteredPersonList()). If I were to make this accept a Person, the method call would have to invoke the .get() within MainWindow, which I think makes the SLAP worse.
| public void parse_validArgsWithWhitespace_returnsViewCommand() { | ||
| assertParseSuccess(parser, " 1", new ViewCommand(INDEX_FIRST_PERSON)); | ||
| assertParseSuccess(parser, "3 ", new ViewCommand(INDEX_THIRD_PERSON)); | ||
| assertParseSuccess(parser, " \n 2 \t ", new ViewCommand(INDEX_SECOND_PERSON)); |
There was a problem hiding this comment.
How does this line succeed? Aren't the \n \t misinputs? When I tried it in the actual console it failed.
There was a problem hiding this comment.
The \n and \t are special characters here to indicate new line and tab respectively, and not the n/ t/ prefixes we use for specific types of inputs.
This test will never come to pass though, since the user cannot enter newline or tab characters within the command input box. It's meant to be a check to ensure that we never lose the functionality to ignore excess whitespace within the ViewCommandParser, in case we change the UI in the future and allow users to enter newline or tab characters!
Hope this helps, do let me know if more clarification is required :)
AbdulrahmanAlRammah
merged commit 26a143e
into
AY2425S1-CS2103-F10-2:master
Closes #121
This is a very barebones implementation of the View command by creating a new window with the user's information.
In future iterations, this should be changed such that the View does not pop up a new window and rather has a pre-allocated space within the main window GUI so as not to interrupt user typing.
The biggest problem with this is maintaining abstraction, since there is no way to pass a
Personout of theModeland into theUIfor the processing. This view command is implemented by filtering the list to only match the exactPersonindexed, and viewing the first item in the list.Some minor changes are also made:
PersonCardto reduce method lengthResultDisplayPersonCardas displayed byListCommandnow contains less information so as to prevent visually overwhelming users, motivating the creation of thisViewCommand.Tests have been updated and written, but CodeCov is low due to UI files not being tested.
User guide has not yet been updated, and will be done in the next PR.