indra/newview/llbottomtray.h (Diff revision 1)

The comment here says "Specifies buttons which can be hidden when bottom tray is shrunk."

Indeed, when you make the Viewer window narrower and narrower, after all buttons have reached their minimal size, the ones listed here (before your change, that's all bottom bar buttons except the speak button) will vanish one by one. (Making the window wider again will make them re-appear.)

Does listing the speak button here make it disappear in that case, too? If so, is that intended?

indra/newview/llbottomtray.cpp (Diff revision 1)

Comments should be written for those reading the final code, not for those reading the diff. I.e. comment on what is done, maybe on how it is done and most important (if not obvious) why it's done. Do not comment on how it's done differently from earlier code or why the change was made. That would later only confuse those who aren't reading the old and new code side-by-side. A better place for comments on changes than the code are the jira issues, the "Description" field here on RB and last but no least the commit messages.

An exception is to be made when whole subsystems are changed in either very subtle or very fundamental ways (or both), so in-code comments pointing out the differences would help people already closely familiar with the old code to find their way around in the new one.

This comment here can probably just be removed.

indra/newview/llspeakbutton.cpp (Diff revision 1)

Simplify your comments! No need to point out the obvious. (Here, that the "if" is some sort of check, that you added this check here (where else?) and that you do something dependent on the result of the check.)

	// Only draw the speak button when voice is enabled

would capture the intent of the code perfectly and be quicker to grasp than

	// Adding check here to see if ... if so then ...

indra/newview/llspeakbutton.cpp (Diff revision 1)

Please don't remove the single empty line between the end of one method and the beginning of the next one.

1. Boroondas Gupte Jan. 21, 2011, 6:34 a.m. (Jan. 21, 2011, 6:34 a.m.)

   Oops, you didn't remove the line, it wasn't there before, just looks as if it was due to the diff view ... but can't hurt to add it.

indra/newview/skins/default/xui/en/menu_bottomtray.xml (Diff revision 1)

Begin XML comments with just "<!--" and end them with "-->", not "<!-->" and "<-->".

1. Cron Stardust Jan. 21, 2011, 2:50 p.m. (Jan. 21, 2011, 2:50 p.m.)

   Typically SGML/XML comments are supposed to start with "<!-- " and end with " -->" (note the post/pre-fixed whitespace) - however, http://www.w3.org/TR/2008/REC-xml-20081126/#sec-comments does not require the space in the syntax, it only demonstrates it.

indra/newview/skins/default/xui/en/menu_bottomtray.xml (Diff revision 1)

Indentation of name attribute is wrong. (Should be same as label and layout attributes.)

indra/newview/skins/default/xui/en/menu_bottomtray.xml (Diff revision 1)

Make that
         label="Speak button (enables voice chat)"

indra/newview/skins/default/xui/en/menu_bottomtray.xml (Diff revision 1)

More indentation strangeness.

indra/newview/skins/default/xui/en/menu_bottomtray.xml (Diff revision 1)

Indentation of <menu_item_separator/> is wrong. (Should be same level as preceding and following <menu_item_check />)

Looks like RB ate some of my comments in the review above. (Maybe because the quoted code sections overlapped.)

indra/newview/llspeakbutton.cpp (Diff revision 1)

The code here should be indented the same as the comment.

indra/newview/skins/default/xui/en/menu_bottomtray.xml (Diff revision 1)

"section"? "selection"? Please check the spelling.

Also, the comment here confused me. I think what you meant to say is: "The Speak Button is visible if and only if Voice Chat is enabled. Thus, to toggle the button's visibility, we enable or disable Voice Chat accordingly."

indra/newview/skins/default/xui/en/menu_bottomtray.xml (Diff revision 1)

Trailing whitespace.
(Didn't RB highlight those in red, before? Doesn't seem to do that anymore in the diff view, only in quoted code on the review.)

Perfect just the naming in menu should be better "Speak button (enables voice chat)" like boroondas sayd :) (and mayby the styling fixed or so but patch works as exspected)

tested patch and test build both worked as exspected

nice job

indra/newview/llbottomtray.cpp (Diff revision 1)

Unnecessary comment. Please delete.

indra/newview/llbottomtray.cpp (Diff revision 1)

Unnecessary comment. Please delete.

indra/newview/llbottomtray.cpp (Diff revision 1)

What Boroondas said. Please delete that comment.

indra/newview/llbottomtray.cpp (Diff revision 1)

Unnecessary comment.

indra/newview/llbottomtray.cpp (Diff revision 1)

Unnecessary comment.

indra/newview/skins/default/xui/en/menu_bottomtray.xml (Diff revision 1)

Typos: change "selction" to "selection", change "visable" to "visible"

This review request is obsolete. Wolfpup, please close it.