----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/198/#review465 -----------------------------------------------------------
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 <http://codereview.secondlife.com/r/198/#comment345> 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 <http://codereview.secondlife.com/r/198/#comment346> 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(); } } - Boroondas On March 15, 2011, 9:21 a.m., Seth ProductEngine wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://codereview.secondlife.com/r/198/ > ----------------------------------------------------------- > > (Updated March 15, 2011, 9:21 a.m.) > > > Review request for Viewer. > > > Summary > ------- > > Fixed "More" link being toggled in expandable textbox after reshaping. > > > This addresses bug STORM-250. > http://jira.secondlife.com/browse/STORM-250 > > > Diffs > ----- > > indra/newview/llexpandabletextbox.h b761ed94eb26 > indra/newview/llexpandabletextbox.cpp b761ed94eb26 > > Diff: http://codereview.secondlife.com/r/198/diff > > > Testing > ------- > > > Thanks, > > Seth > >
_______________________________________________ Policies and (un)subscribe information available here: http://wiki.secondlife.com/wiki/OpenSource-Dev Please read the policies before posting to keep unmoderated posting privileges
