Ui: Make the ResultDisplay red on command failure #799

yamgent · 2018-01-19T05:44:09Z

Part of #784. Rebased, and added tests to verify the result display.

Propsed commit message:

ResultDisplay shows an appropriate error message when the user made a 
mistake in his command. The color of the error message is the same as 
the color of a successful one.

Using a different color for error messages makes it easier for a user
to realise that it is communicating the mistake made by him.

Let's make the ResultDisplay red on command failure.

Note: For demo purpose, don't merge this to master!

CanIHasReview-bot · 2018-01-19T10:48:56Z

v1

@yamgent submitted v1 for review.

[v1 1/3] CommandBox: Send result's success status to ResultDisplay
[v1 2/3] ResultDisplay: Display message in red when command fails
[v1 3/3] AddressBookSystemTest: Verify that ResultDisplay style changes

(📚 Archive)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/799/1/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

Zhiyuan-Amos

[1/3]: communicates -> passes. That's the term we usually use right? XD

Zhiyuan-Amos · 2018-01-22T01:04:36Z

src/test/java/seedu/address/ui/CommandBoxTest.java

 * - the text remains <br>
 * - the command box's style is the same as {@code errorStyleOfCommandBox}.
 */
 private void assertBehaviorForFailedCommand() {
+ eventsCollectorRule.eventsCollector.reset();


maybe we should clear after the verifications requiring eventsCollectorRule#eventsCollector have completed i.e. at line 150, for the purpose of segmenting eventsCollector related things together.

Zhiyuan-Amos

[2/3] 1st para: executed by the user succeeded or failed.

3rd para: Let's modify ResultDisplay to display the message in red when the
command execution fails. The message will still display white when
the command execution succeeds. (since you've already mentioned that we are only modifying ResultDisplay to turn red when it fails, so it's clear that we aren't modifying the success case)

Zhiyuan-Amos · 2018-01-22T01:07:20Z

src/main/java/seedu/address/ui/ResultDisplay.java

+ Platform.runLater(() -> {
+ displayed.setValue(event.message);
+
+ if (event.isSuccessful) {


can we ternary this? :P

Ternary operators are only meant for assigning values though.

Zhiyuan-Amos · 2018-01-22T01:12:21Z

src/main/java/seedu/address/ui/ResultDisplay.java

+ /**
+ * Sets the {@code ResultDisplay} style to use the default style.
+ */
+ private void setStyleToDefault() {


since we have setStyleToIndicateCommandFailure, it seems that this method should be named setStyleToIndicateCommandSuccess?

Zhiyuan-Amos · 2018-01-22T01:13:11Z

src/test/java/guitests/guihandles/ResultDisplayHandle.java

@@ -19,4 +20,11 @@ public ResultDisplayHandle(TextArea resultDisplayNode) {
 public String getText() {
 return getRootNode().getText();
 }
+
+ /**
+ * Returns the list of style classes present in the command box.


~~command box~~ result display :P

Zhiyuan-Amos · 2018-01-22T01:13:44Z

src/test/java/guitests/guihandles/ResultDisplayHandle.java

+ /**
+ * Returns the list of style classes present in the command box.
+ */
+ public ObservableList<String> getStyleClass() {


I guess we can return this simply as a List<String>, instead of an Observable<List>, since we aren't observing it.

Zhiyuan-Amos · 2018-01-22T01:25:46Z

src/test/java/seedu/address/ui/ResultDisplayTest.java

+ new NewResultAvailableEvent("failure", false);
+
+ private ArrayList<String> defaultStyleOfResultDisplay;
+ private ArrayList<String> errorStyleOfResultDisplay;


can these 2 simply be List<String>?

Zhiyuan-Amos · 2018-01-22T01:29:17Z

src/test/java/seedu/address/ui/ResultDisplayTest.java

+
+ /**
+ * Posts the {@code event} to the {@code EventsCenter}, then verifies that <br>
+ * - the text matches the event message <br>


the text on the result display matches the {@code event}'s message

Zhiyuan-Amos · 2018-01-22T01:31:03Z

src/test/java/seedu/address/ui/ResultDisplayTest.java

+ /**
+ * Posts the {@code event} to the {@code EventsCenter}, then verifies that <br>
+ * - the text matches the event message <br>
+ * - the result display's style is the same as {@code defaultStyleOfResultDisplay} for successful message,


for successful message sounds weird :P

Zhiyuan-Amos

[3/3] Should there be a commit message for this? :P

Zhiyuan-Amos · 2018-01-22T01:33:51Z

src/test/java/systemtests/AddressBookSystemTest.java

+ private static final List<String> RESULT_DISPLAY_DEFAULT_STYLE =
+ Arrays.asList("text-input", "text-area", "result-display");
+ private static final List<String> RESULT_DISPLAY_ERROR_STYLE =
+ Arrays.asList("text-input", "text-area", "result-display", ResultDisplay.ERROR_STYLE_CLASS);


I realise that we have inconsistencies in our code base; for ResultDisplayTest, we use private ArrayList<String> defaultStyleOfResultDisplay and then we call defaultStyleOfResultDisplay = new ArrayList<>(resultDisplayHandle.getStyleClass()) to add the style classes. :P

CanIHasReview-bot · 2018-01-27T16:38:25Z

v2

@yamgent submitted v2 for review.

[v2 1/3] CommandBox: Send result's success status to ResultDisplay
[v2 2/3] ResultDisplay: Display message in red when command fails
[v2 3/3] AddressBookSystemTest: Verify that ResultDisplay style changes

(📚 Archive) (📈 Interdiff between v1 and v2)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/799/2/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

Zhiyuan-Amos

[1/3]: The commit message mentions about ResultDisplay, but there's no changes made in the code concerning ResultDisplay :P Perhaps:

1st para: NewResultAvailableEvent only stores the message of the command execution. It does not store the success status.

2nd para: This is bad cos event subscribers like ResultDisplay may require this information (can phrase better)

3rd para: As part of teaching ResultDisplay to use the success status to update the display accordingly, let's modify NewResultAvailableEvent to store the success status.

Zhiyuan-Amos

[3/3] there should be a 2nd para mentioning that this isn't current being tested :P

damithc · 2018-01-28T04:19:45Z

Just a general comment: after adding tests, this PR is a lot more complicated than before although it is a small UI change. This might overwhelm students. Is it possible to split into even smaller commits so that students can learn one commit at a time. e.g., add functionality first and then add the tests ?

CanIHasReview-bot · 2018-01-28T09:34:41Z

v3

@yamgent submitted v3 for review.

(📚 Archive) (📈 Interdiff between v2 and v3)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/799/3/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

yamgent · 2018-01-28T09:35:39Z

Is it possible to split into even smaller commits so that students can learn one commit at a time. e.g., add functionality first and then add the tests ?

I split up the commits, so now the unit tests are in [v3 2/5] and [v3 4/5] instead.

damithc

Looks good now. On a related note, in the dev guide we can order the suggested UI-component improvements in the order of difficulty so that this one (which seems to be the most difficult one) comes last and also leave a comment there to recommend the reader to learn one commit at a time.

NewResultAvailableEvent only stores the message of the command execution. It does not store the success status. Subscribers such as ResultDisplay requires the success status to work. As part of teaching ResultDisplay to update the display according to the success status, let's modify NewResultAvailableEvent to store the success status.

CommandBox stores the success status of command execution in NewResultAvailableEvent. This behavior is not verified by any of the unit tests for CommandBox. Let's update the unit tests in CommandBoxTest to verify that behavior.

The ResultDisplay shows the same color (white) regardless of whether the command executed by the user succeeded or failed. Changing the color of the message when the command execution failed gives a clear visual signal to the user that he made a mistake, and that the message shown is an error, rather than a normal message. Let's modify ResultDisplay to display the message in red when the command execution fails.

ResultDisplay will turn red when command execution fails. This behavior is not verified by any of the unit tests for ResultDisplay. Let's update the unit tests in ResultDisplayTest to verify that behavior.

The ResultDisplay style changes if there is an error in command execution. This behavior is not verified by any system tests. Let's update the system tests to verify that.

CanIHasReview-bot · 2018-08-09T05:31:59Z

v4

@yamgent submitted v4 for review.

(📚 Archive) (📈 Interdiff between v3 and v4) (📈 Range-Diff between v3 and v4)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/799/4/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

yamgent · 2018-08-09T06:52:34Z

Changelog:

fc363ad: Updated HelpCommandSystemTest.java, because it now uses assertCommandBoxShowsDefaultStyle(), but in this PR we renamed this method to assertCommandBoxAndResultDisplayShowsDefaultStyle().

CanIHasReview-bot added the s.ToReview label Jan 19, 2018

Zhiyuan-Amos added the pr.Answer Pull request is answer for a tutorial in developer guide. Do not actually merge. label Jan 20, 2018

Zhiyuan-Amos reviewed Jan 22, 2018

View reviewed changes

yamgent added s.Ongoing and removed s.ToReview labels Jan 22, 2018

Zhiyuan-Amos mentioned this pull request Jan 24, 2018

Rebase PRs used in suggested programming tasks #784

Closed

yamgent force-pushed the getting-started-ui-result branch from 0357510 to 825f2ce Compare January 27, 2018 16:30

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Jan 27, 2018

Zhiyuan-Amos reviewed Jan 27, 2018

View reviewed changes

Zhiyuan-Amos requested a review from damithc January 27, 2018 23:19

damithc approved these changes Jan 28, 2018

View reviewed changes

yamgent added s.Ongoing and removed s.ToReview labels Jan 28, 2018

yamgent force-pushed the getting-started-ui-result branch from 825f2ce to 9b4657a Compare January 28, 2018 09:25

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Jan 28, 2018

Zhiyuan-Amos approved these changes Jan 28, 2018

View reviewed changes

Zhiyuan-Amos removed the s.ToReview label Jan 28, 2018

damithc approved these changes Jan 28, 2018

View reviewed changes

yamgent mentioned this pull request Aug 4, 2018

Rebase all answer PRs #891

Closed

7 tasks

yamgent added 3 commits August 9, 2018 11:03

ResultDisplayTest: Verify display turns red on error

ca028a4

ResultDisplay will turn red when command execution fails. This behavior is not verified by any of the unit tests for ResultDisplay. Let's update the unit tests in ResultDisplayTest to verify that behavior.

yamgent force-pushed the getting-started-ui-result branch from 9b4657a to d5dd735 Compare August 9, 2018 03:04

yamgent added the s.Ongoing label Aug 9, 2018

AddressBookSystemTest: Verify that ResultDisplay style changes

fc363ad

The ResultDisplay style changes if there is an error in command execution. This behavior is not verified by any system tests. Let's update the system tests to verify that.

yamgent force-pushed the getting-started-ui-result branch from d5dd735 to fc363ad Compare August 9, 2018 05:26

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Aug 9, 2018

yamgent requested a review from a team August 9, 2018 05:32

Zhiyuan-Amos approved these changes Aug 9, 2018

View reviewed changes

pyokagan removed the s.ToReview label Aug 16, 2018

damithc closed this Dec 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ui: Make the ResultDisplay red on command failure #799

Ui: Make the ResultDisplay red on command failure #799

yamgent commented Jan 19, 2018

CanIHasReview-bot commented Jan 19, 2018

Zhiyuan-Amos left a comment

Zhiyuan-Amos Jan 22, 2018

Zhiyuan-Amos left a comment

Zhiyuan-Amos Jan 22, 2018

yamgent Jan 27, 2018 •

edited

Loading

Zhiyuan-Amos Jan 22, 2018

Zhiyuan-Amos Jan 22, 2018

Zhiyuan-Amos Jan 22, 2018

Zhiyuan-Amos Jan 22, 2018

Zhiyuan-Amos Jan 22, 2018

Zhiyuan-Amos Jan 22, 2018

Zhiyuan-Amos left a comment

Zhiyuan-Amos Jan 22, 2018

CanIHasReview-bot commented Jan 27, 2018

Zhiyuan-Amos left a comment

Zhiyuan-Amos left a comment

damithc commented Jan 28, 2018

CanIHasReview-bot commented Jan 28, 2018

yamgent commented Jan 28, 2018

damithc left a comment

CanIHasReview-bot commented Aug 9, 2018

yamgent commented Aug 9, 2018

Ui: Make the ResultDisplay red on command failure #799

Ui: Make the ResultDisplay red on command failure #799

Conversation

yamgent commented Jan 19, 2018

CanIHasReview-bot commented Jan 19, 2018

v1

Zhiyuan-Amos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zhiyuan-Amos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yamgent Jan 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zhiyuan-Amos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Jan 27, 2018

v2

Zhiyuan-Amos left a comment

Choose a reason for hiding this comment

Zhiyuan-Amos left a comment

Choose a reason for hiding this comment

damithc commented Jan 28, 2018

CanIHasReview-bot commented Jan 28, 2018

v3

yamgent commented Jan 28, 2018

damithc left a comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Aug 9, 2018

v4

yamgent commented Aug 9, 2018

yamgent Jan 27, 2018 •

edited

Loading