All submissions to this site are governed by the Second Life Viewer Contribution Agreement. By submitting patches and other information using this site, you acknowledge that you have read, understood, and agreed to those terms.

Review Board 1.6.11

Welcome to the Second Life Viewer Code Review tool.
See the documentation on our wiki for how to use this site.

STORM-737 Add "+" menu to Inventory/Recent

Review Request #65 - Created Dec. 23, 2010 and submitted

Jonathan Yap Reviewers
2.5 viewer
storm-737
None viewer-development 
This change enables the "+" menu in Inventory/Recent
It grays out "New Folder" in this menu
It enables identical menu entries when you right click on an inventory item.

Question:
 Is graying out "New Folder" best done where I am doing it now -- in 
llpanelmaininventory.cpp / LLPanelMainInventory::onAddButtonClick()
I opened up Inventory/My Inventory and used all the "New xxx" options for both right clicking on an inventory item and also from the "+" menu.

I then changed to the Recent tab and performed the same steps.

New items were created as expected, except "New Folder" was not an option via either method when the Recent tab was active.
Vadim ProductEngine
Posted (Dec. 24, 2010, 10:55 a.m.)
Seems plausible.
indra/newview/llpanelmaininventory.cpp (Diff revision 1) 
 
 
 
This line can now be removed at all.
  1. Line 509 on the left is removed; it might be that the diff display is slightly confused about this matter.  Not sure what a lighter color of pink means.
  2. I meant the line on the right: no need to set visibility to true because it's the default value.
(I tried removing that line and nothing changed)
indra/newview/llpanelmaininventory.cpp (Diff revision 1) 
 
 
 
redundant line
Aleric Inglewood
Posted (Dec. 24, 2010, 1:25 p.m.)

   

  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    I'm going to respectfully disagree with Aleric on one minor style point.  When comparing equality between a literal or constant and a variable, putting the constant value has an advantage:  it avoids the "=" vs "==" error:

if (FOO_LIMIT == foo_counter) 

works just fine, but produces a syntax error if you mistype and leave off one of the "=", where:

if (foo_counter = FOO_LIMIT)

compiles just fine because = is an operator that returns the assigned value, and C/C++ treats any value as valid for true/false.  Occasionally, this is handy, but if what you wanted was comparison, it's a hard-to-spot bug.

    
    
    
   
    2. 

   


  






   







    

     
     

      

       
        

         
          indra/newview/llpanelmaininventory.cpp
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    

     
While you're removing that empty line anyway, I thought I'd help you to not learn the bad coding habbits of whoever wrote the original code ;).

operator== returns a bool, might as well do the conversions one step later (if at all) and use a bool here.

Using 'bool' always has the preference (BOOL is ugly window-ism).

Certainly in this case since !recent_active converts it to a bool again! (bool --> BOOL -> bool -> BOOL now).

Secondly, when testing if a variable is equal to a literal/constant, I think that's better readable to put the variable up front, thus: mActivePanel()->getName() == "Recent Items".

Finally, although you may choose to leave it like it is, be aware that the extra 'variable' recent_active here was only added as pseudo 'comment' and to because the monitor of the original coder wasn't wide enough. If you have a normal 22" inch like all devs, you might also consider the more professional looking:

// "New Folder" is broken for the Recent Items tab. Do not enable it for that case.
mMenuAdd->getChild<LLMenuItemGL>("New Folder")->setEnabled(mActivePanel->getName() != "Recent Items");



     

  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    I tend to agree regarding the BOOL, but the other two issues are rather a matter of taste, which, IMHO, is not a subject of code review.

Besides:
* Putting a constant on the left of a comparison operator makes sure you won't get a hard-to-find bug if you accidentally write "=" instead of "==" (I don't like this habit either, but at least it makes sense).
* Using reasonably named variables to make logic clearer is not a "bad coding habit" and doesn't look any less professional to me.
    
    
    
   
    2. 

   


  





    

    

   


  
 




  

 



















 

  




 

 

  
  

  
Jonathan Yap

  
Posted  (Dec. 24, 2010, 3:30 p.m.)

 

 

   

   

  

   

   
    
    
   


  






   







    

     
     

      

       
        

         
          indra/newview/llpanelmaininventory.cpp
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    

     
I updated the code per Aleric's suggestion but the 2nd diff failed to upload.  There must be some unusual hg command to generate follow-up diffs in a format RB will be happy with.  You can see the change on bitbucket though.


     

  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    This change breaks indentation.
    
    
    
   
    2.