[opensource-dev] Review Request: STORM-236 Actual Code Review

Boroondas Gupte sllists at boroon.dasgupta.ch
Fri Jan 21 03:44:26 PST 2011


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/113/#review229
-----------------------------------------------------------



indra/newview/llbottomtray.h
<http://codereview.secondlife.com/r/113/#comment160>

    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
<http://codereview.secondlife.com/r/113/#comment155>

    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
<http://codereview.secondlife.com/r/113/#comment156>

    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
<http://codereview.secondlife.com/r/113/#comment157>

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



indra/newview/skins/default/xui/en/menu_bottomtray.xml
<http://codereview.secondlife.com/r/113/#comment162>

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



indra/newview/skins/default/xui/en/menu_bottomtray.xml
<http://codereview.secondlife.com/r/113/#comment163>

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



indra/newview/skins/default/xui/en/menu_bottomtray.xml
<http://codereview.secondlife.com/r/113/#comment161>

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



indra/newview/skins/default/xui/en/menu_bottomtray.xml
<http://codereview.secondlife.com/r/113/#comment164>

    More indentation strangeness.



indra/newview/skins/default/xui/en/menu_bottomtray.xml
<http://codereview.secondlife.com/r/113/#comment158>

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


- Boroondas


On Jan. 20, 2011, 6:37 p.m., Wolfpup Lowenhar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/113/
> -----------------------------------------------------------
> 
> (Updated Jan. 20, 2011, 6:37 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> -------
> 
> This allows the Speak Button to auto-hide for those that do not use Voice at all.
> 
> 
> This addresses bug STORM-236.
>     http://jira.secondlife.com/browse/STORM-236
> 
> 
> Diffs
> -----
> 
>   doc/contributions.txt 9c7d543fd15d 
>   indra/newview/llbottomtray.h 9c7d543fd15d 
>   indra/newview/llbottomtray.cpp 9c7d543fd15d 
>   indra/newview/llspeakbutton.cpp 9c7d543fd15d 
>   indra/newview/skins/default/xui/en/menu_bottomtray.xml 9c7d543fd15d 
> 
> Diff: http://codereview.secondlife.com/r/113/diff
> 
> 
> Testing
> -------
> 
> Built locally and did the following:
> 1 Verified that when Voice is toggled via the preference panel the Speak Button auto hid/showed.
> 2 Verified that drag and drop functionality of the Speak Button was not affected.
> 3 Went to a non-Voice area with Voice active and verified that button was still there but grayed out.
> 
> 
> Thanks,
> 
> Wolfpup
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.secondlife.com/pipermail/opensource-dev/attachments/20110121/bbcc9662/attachment-0001.htm 


More information about the opensource-dev mailing list