(STORM-250) Unexpected "More" text appears in the About Landmark panel after minimizing the floater
Review Request #198 - Created March 10, 2011 and submitted
Seth ProductEngine | Reviewers | ||
viewer | |||
STORM-250 | |||
None | viewer-development |
Fixed "More" link being toggled in expandable textbox after reshaping.
Review request changed
Updated (March 15, 2011, 9:21 a.m.)
-
- added Diff r2
Fixed a bug in previous patch version.
Posted (March 16, 2011, 7:30 a.m.)
Apart from the bugfix (calling hideExpandText before evaluating (getTextPixelHeight() > getRect().getHeight()), if I'm not mistaken), the change between the first and second review request revision also changes an if-else construct to a ternary conditional operator (:?) :
-
indra/newview/llexpandabletextbox.cpp (Diff revisions 1 - 2) -
Here, if-else is in my opinion easier to read than the ternary conditional operator. Usage of the ternary conditional operator is nice when only a small part of a long expression depends on the condition and one doesn't want a temporary variable for storing that conditional part, but that really isn't the case here. About the complete method after the second review revision:
-
indra/newview/llexpandabletextbox.cpp (Diff revision 2) -
Should hideExpandText really be called a second time? Or would it be prudent to just do nothing when getTextPixelHeight() <= getRect().getHeight() after calling it the first time? Also, the new method's name is somewhat confusing. From a function named toggle<some boolean property> I'd expect to change <some boolean property> at each call, i.e., always make it true if it previously was false and vice versa. Name suggestions: toggleExpandTextAsNeeded toggleExpandTextIffNeeded hideOrShowExpandTextAsNeeded or maybe "TextExpander" instead of "ExpandText" in any of the above? (Feel free to come up with better ones.)
So the new method might become something like void LLExpandableTextBox::LLTextBoxEx::hideOrShowExpandTextAsNeeded() { // Restore the text box contents to calculate the text height properly, // otherwise if a part of the text is hidden under "More" link // getTextPixelHeight() returns only the height of currently visible text // including the "More" link. hideExpandText(); // Show the expander if we need it, depending on text // contents height. If not, keep it hidden. if (getTextPixelHeight() > getRect().getHeight()) { showExpandText(); } }
Review request changed
Updated (March 16, 2011, 5:19 p.m.)
-
- added Diff r3
Posted (March 16, 2011, 7:20 p.m.)
You're welcome, and thank you, too. :-)
-
indra/newview/llexpandabletextbox.h (Diff revisions 2 - 3) -
Even if the name of the method is now more descriptive, its doxygen documentation should provide at least as much or even more information. Maybe 'Shows the "More" link if the text is too high to be completely visible without expanding the text box. Hides that link otherwise.' ( Inspired by both versions of this comment: )
-
indra/newview/llexpandabletextbox.cpp (Diff revisions 2 - 3) -
-
indra/newview/llexpandabletextbox.cpp (Diff revision 3) -
The comments speak of a '"More" link' and a 'expander', but never mention that those terms actually mean the same thing. (Or that one is an example for the other, or whatever the case is.) Appending 'a.k.a. "More" link' after the word 'expander' on line 198 would probably be sufficient to hint at the relationship.
Looks good. Please fix the two minor doc issues and submit.
-
indra/newview/llexpandabletextbox.h (Diff revision 3) -
The description seems to be inappropriate.
-
indra/newview/llexpandabletextbox.cpp (Diff revision 3) -
Please add the ticket reference.
Review request changed
Updated (March 17, 2011, 4:19 p.m.)
-
- added Diff r4
Updated comments, added jira ticket reference.
Other reviews