Index is cut off when names are too long for PersonCard #323 #338

MightyCupcakes · 2017-03-15T13:55:09Z

Resolves #323

Before:

After: (Index modified)

mention-bot · 2017-03-15T13:55:10Z

@MightyCupcakes, thanks for your PR! By analyzing the history of the files in this pull request, we identified @damithc, @pyokagan and @edmundmok to be potential reviewers.

CanIHasReview-bot · 2017-03-15T14:02:28Z

v1

@MightyCupcakes submitted v1 for review.

[v1 1/1] PersonCard: Set id label to use preferred width

(📚 Archive)

Checkout this PR version locally

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

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

yamgent · 2017-03-16T14:18:24Z

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

+     * Ensures that the index is shown on the UI regardless of the length of the {@code Person}
+     * name.
+     */
+    private void setMinWidthForIdLabel() {


Feels like this method can be inlined? The name of the method and the header comment doesn't seem to match up anyway (the intent is not obvious in the name :P).

Well there are several reasons this is a method. The original solution involves the calculation of the length of text in the ID label and adjusting accordingly. Then I found this elegant solution that does it in one line.

However, setting minimum width for the label is not that obvious. So the method name can be renamed to better fit the intentions if you want.

More importantly, I seem to recall the minWidth property can be set via the FXML file? We should try not to encode too much style-related information in the code.

FXML file, or via the stylesheet.

CanIHasReview-bot · 2017-03-16T14:57:53Z

v2

@MightyCupcakes submitted v2 for review.

[v2 1/1] PersonListCard.fxml: Set id label to use preferred width

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

Checkout this PR version locally

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

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

yamgent · 2017-03-17T03:36:19Z

src/main/resources/view/PersonListCard.fxml

@@ -30,7 +30,7 @@
                        <HBox spacing="5" alignment="CENTER_LEFT">
                            <children>
                                <HBox>
-                                    <Label fx:id="id" styleClass="cell_big_label"></Label>
+                                    <Label fx:id="id" styleClass="cell_big_label" minWidth="-Infinity"/>


Might want to put a comment in the .fxml file for CS2103 students, to explain to them the purpose of doing minWidth="-Infinity".

No, I don't agree.

I don't think it is a good idea to have to comment on every attribute we add to fxml files.

What's the meaning of that attribute? 'Never truncate'? Is it a well documented JavaFx behavior students can find with a quick web search?

The class Region defines a constant USE_PREF_WIDTH as Double.NEGATIVE_INFINITY

This is also one reason why I want to set this in the code instead in the FXML files because there are already meaningful constants names (along with javadocs!) already defined.

I don't think it is a good idea to have to comment on every attribute we add to fxml files.

Agree. This case is a bit special though, because that attribute is not self explanatory nor common knowledge. Does this and this help to make the code more self-explanatory? i.e. somehow make the fxml code refer to the actual constant. Not sure if it is possible though.

That might be the solution. Will explore this later.

chao1995

No additional comment.

CanIHasReview-bot · 2017-03-17T15:04:18Z

v3

@MightyCupcakes submitted v3 for review.

[v3 1/1] PersonListCard.fxml: Set id label to use preferred width

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

Checkout this PR version locally

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

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

damithc · 2017-03-17T15:40:46Z

src/main/resources/view/PersonListCard.fxml

-                                    <Label fx:id="id" styleClass="cell_big_label"></Label>
+                                    <Label fx:id="id" styleClass="cell_big_label">
+                                        <minWidth>
+                                            <Region fx:constant="USE_PREF_SIZE" />


Kudos for figuring out the syntax for referring a constant from fxml. I guess we cannot say Region.USE_PREF_SIZE?
On a related note, I still feel this can benefit from an explanatory comment though. A reader, especially someone new to fxml, may not be able to understand what's going on here.
Also, any chance of adding a test case that fails if this attribute is removed? I guess we have to do that in PersonCard unit test which is WIP.

A test case that fails?

I guess it will be possible if the tests can detect the presence of an ellipsis character shown on the screen? I am not sure if this is possible with FxRobot. Or maybe we can check the width allocated to the label on a card and make some educated guess about the value being truncated or not?

The JavaFx API's Label does not expose a public method to indicate that it has been truncated. Any checked to the actual displayed text requires the use of reflection because that is not accessible publicly (Source).

I see. May be using label width as @MightyCupcakes has suggested?

That can be explored. In fact, we might not need to guess the width if we create a separate label control, that automatically resizes, to determine the minimum required for a particular text.

CanIHasReview-bot · 2017-03-17T16:22:26Z

v4

@MightyCupcakes submitted v4 for review.

[v4 1/1] PersonListCard.fxml: Set id label to use preferred width

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

Checkout this PR version locally

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

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

MightyCupcakes · 2017-03-17T16:23:11Z

V4

Added comments. Although I am not so sure what is our stand on multi-line XML comments.

damithc

Just a minor suggestion.
We don't have a convention for multi-line comments on fxml yet. We should probably set indentation of markup files to 2 spaces instead, given their tendency for deep nesting, but that's not related to this PR.

damithc · 2017-03-18T02:07:52Z

src/main/resources/view/PersonListCard.fxml

+                                        <minWidth>
+                                            <!--
+                                            Ensures that the index is shown on the UI regardless of the length of the
+                                            Person's name


Ensures the label text is never truncated?

I am looking into simplifying the FXML files.

The FXML files can be simplified and some parent elements are unneeded. Removing those will reduce the amount of nesting needed.

yamgent

No other comments.

CanIHasReview-bot · 2017-03-18T04:02:06Z

v5

@MightyCupcakes submitted v5 for review.

[v5 1/1] PersonListCard.fxml: Set id label to use preferred width

(📚 Archive) (📈 Interdiff between v4 and v5)

Checkout this PR version locally

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

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

damithc

Commit message:

This is not desirable
as the PersonCard id is important for the user as they need it to use
and execute some commands (like the edit command)

Tried to shorten and to get rid of one of the two as in the sentence:

This is not desirable as users need to know the index of a person
to perform some commands (e.g. edit command)

Let's modify the id label's minimum width to use its preferred width.
This sets it so that it tracks its preferred width as its minimum width
and therefore it will always be wide enough to display the containing
PersonCard id.

It's not exactly correct to say 'modify the id label's minimum width', right?
Also, try to shorten the sentence (feels a bit verbose).

yamgent · 2017-03-18T05:58:17Z

Minor nit: Also add a full stop to the second paragraph. :)

CanIHasReview-bot · 2017-03-18T06:22:19Z

v6

@MightyCupcakes submitted v6 for review.

[v6 1/1] PersonListCard.fxml: Set id label to use preferred width

(📚 Archive) (📈 Interdiff between v5 and v6)

Checkout this PR version locally

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

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

chao1995

[v6 1/1]

As a consequence, a very long name would possibly cause the id label to
overflow, replacing the number with an ellipsis. This is not desirable as
users need to know the index of a person to perform some commands
(e.g. edit command).

As a consequence, a very long person name could cause the number in id
label to be replaced with an ellipsis. This is not desirable as users need to
know the id number of a person to use some commands. (e.g. edit).

Let's set the id label's minimum width to its preferred width.
This is so that the id label will always be wide enough to display the
containing PersonCard id.

Let's set the id label's minimum width to its preferred width so that the
id label will always be wide enough to display the id number, regardless
of the length of person names.

yamgent · 2017-03-18T07:43:47Z

@chao1995:
label to be replaced with an ellipsis.

Or can say truncated.

The id text label does not define its minimum width on the PersonCard cell. As a consequence, a very long person name could cause the number in the label to be truncated. This is not desirable as users need to know the id number of a person to use some commands (e.g. edit). Let's set the id label's minimum width to its preferred width so that the id label will always be wide enough to display the id number, regardless of the length of the person's name.

CanIHasReview-bot · 2017-03-18T08:35:43Z

v7

@MightyCupcakes submitted v7 for review.

[v7 1/1] PersonListCard.fxml: Set id label to use preferred width

(📚 Archive) (📈 Interdiff between v6 and v7)

Checkout this PR version locally

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

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

damithc · 2017-03-19T11:24:12Z

Going to merge soon.

MightyCupcakes added the s.Ongoing label Mar 15, 2017

MightyCupcakes requested review from PierceAndy and chao1995 March 15, 2017 14:01

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Mar 15, 2017

chao1995 approved these changes Mar 16, 2017

View reviewed changes

yamgent reviewed Mar 16, 2017

View reviewed changes

MightyCupcakes force-pushed the 323-persconcard-id-cut-off branch 4 times, most recently from 1a2814d to 7603875 Compare March 16, 2017 14:43

MightyCupcakes requested a review from chao1995 March 16, 2017 14:58

yamgent reviewed Mar 17, 2017

View reviewed changes

chao1995 reviewed Mar 17, 2017

View reviewed changes

MightyCupcakes force-pushed the 323-persconcard-id-cut-off branch 2 times, most recently from fd24267 to 913991d Compare March 17, 2017 06:54

damithc suggested changes Mar 17, 2017

View reviewed changes

MightyCupcakes force-pushed the 323-persconcard-id-cut-off branch from 913991d to a10a947 Compare March 17, 2017 16:09

CanIHasReview-bot requested a review from damithc March 17, 2017 16:22

damithc suggested changes Mar 18, 2017

View reviewed changes

yamgent reviewed Mar 18, 2017

View reviewed changes

MightyCupcakes force-pushed the 323-persconcard-id-cut-off branch from a10a947 to 0cce888 Compare March 18, 2017 03:40

CanIHasReview-bot requested a review from damithc March 18, 2017 04:02

damithc suggested changes Mar 18, 2017

View reviewed changes

MightyCupcakes force-pushed the 323-persconcard-id-cut-off branch from 0cce888 to b54671b Compare March 18, 2017 05:46

MightyCupcakes force-pushed the 323-persconcard-id-cut-off branch from b54671b to 5857bf3 Compare March 18, 2017 06:14

CanIHasReview-bot requested a review from damithc March 18, 2017 06:22

chao1995 reviewed Mar 18, 2017

View reviewed changes

yamgent added s.Ongoing and removed s.ToReview labels Mar 18, 2017

MightyCupcakes force-pushed the 323-persconcard-id-cut-off branch 2 times, most recently from 02c244c to defe7e8 Compare March 18, 2017 07:58

MightyCupcakes force-pushed the 323-persconcard-id-cut-off branch from defe7e8 to b5662d8 Compare March 18, 2017 08:23

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Mar 18, 2017

chao1995 approved these changes Mar 18, 2017

View reviewed changes

damithc approved these changes Mar 18, 2017

View reviewed changes

yamgent approved these changes Mar 19, 2017

View reviewed changes

PierceAndy approved these changes Mar 19, 2017

View reviewed changes

damithc added s.ToMerge and removed s.ToReview labels Mar 19, 2017

damithc merged commit 4c4e1ed into se-edu:master Mar 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Index is cut off when names are too long for PersonCard #323 #338

Index is cut off when names are too long for PersonCard #323 #338

MightyCupcakes commented Mar 15, 2017

mention-bot commented Mar 15, 2017

CanIHasReview-bot commented Mar 15, 2017

yamgent Mar 16, 2017

MightyCupcakes Mar 16, 2017

pyokagan Mar 16, 2017

pyokagan Mar 16, 2017

CanIHasReview-bot commented Mar 16, 2017

yamgent Mar 17, 2017

MightyCupcakes Mar 17, 2017 •

edited

Loading

damithc Mar 17, 2017

MightyCupcakes Mar 17, 2017 •

edited

Loading

damithc Mar 17, 2017

MightyCupcakes Mar 17, 2017

chao1995 left a comment

CanIHasReview-bot commented Mar 17, 2017

damithc Mar 17, 2017

MightyCupcakes Mar 17, 2017

yamgent Mar 18, 2017

damithc Mar 18, 2017

yamgent Mar 18, 2017 •

edited

Loading

CanIHasReview-bot commented Mar 17, 2017

MightyCupcakes commented Mar 17, 2017

damithc left a comment

damithc Mar 18, 2017

MightyCupcakes Mar 18, 2017

yamgent left a comment

CanIHasReview-bot commented Mar 18, 2017

damithc left a comment

yamgent commented Mar 18, 2017

CanIHasReview-bot commented Mar 18, 2017

chao1995 left a comment •

edited

Loading

yamgent commented Mar 18, 2017

CanIHasReview-bot commented Mar 18, 2017

damithc commented Mar 19, 2017

Index is cut off when names are too long for PersonCard #323 #338

Index is cut off when names are too long for PersonCard #323 #338

Conversation

MightyCupcakes commented Mar 15, 2017

mention-bot commented Mar 15, 2017

CanIHasReview-bot commented Mar 15, 2017

v1

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

CanIHasReview-bot commented Mar 16, 2017

v2

Choose a reason for hiding this comment

MightyCupcakes Mar 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MightyCupcakes Mar 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chao1995 left a comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Mar 17, 2017

v3

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

yamgent Mar 18, 2017 • edited Loading

Choose a reason for hiding this comment

CanIHasReview-bot commented Mar 17, 2017

v4

MightyCupcakes commented Mar 17, 2017

V4

damithc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yamgent left a comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Mar 18, 2017

v5

damithc left a comment

Choose a reason for hiding this comment

yamgent commented Mar 18, 2017

CanIHasReview-bot commented Mar 18, 2017

v6

chao1995 left a comment • edited Loading

Choose a reason for hiding this comment

yamgent commented Mar 18, 2017

CanIHasReview-bot commented Mar 18, 2017

v7

damithc commented Mar 19, 2017

MightyCupcakes Mar 17, 2017 •

edited

Loading

MightyCupcakes Mar 17, 2017 •

edited

Loading

yamgent Mar 18, 2017 •

edited

Loading

chao1995 left a comment •

edited

Loading