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-829 Viewer 2 does not parse /me in object Instant Messages

Review Request #71 - Created Jan. 5, 2011 and submitted

Jonathan Yap Reviewers
2.5 viewer
STORM-829
None viewer-development 
The "/me" in the lsl code below would be displayed rather than being translated to a name:
llInstantMessage(llGetOwner(),"/me Hello, Avatar!");

 

 










  

 
















 

  




 

 

  
Ship it!

  

  
Wolfpup Lowenhar

  
Posted  (Jan. 5, 2011, 7:33 p.m.)

 

 

   

   

  

   

   
    
    
   


  






   







    

     
     

      

       
        

         
          indra/newview/llviewermessage.cpp
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    

     
Looks good to me, but just wondering why your checking for "/me " and "/me'" .


     

  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    That is the check used in other parts of the code and as was pointed out in the mailing list you can type /me's happy
    
    
    
   
    2. 


   
  2. 
    
    
     
    
      
     
    
     
    What other parts of the code use this check?  /me handling is server side.
    
    
    
   
    3. 

   


  





    

    

   


  
 




  

 

















 

  




 

 

  
  

  
Aleric Inglewood

  
Posted  (Jan. 6, 2011, 7 a.m.)

 

 

   
What about /Me, /ME or /me followed by another punctuation? Ie, "/me?", "/me!", etc...
Just asking because these comparisions with just "/me " and "/me'" seem very limited,
almost weird. More logical would be to not check anything at ALL - and either expand
things, or not. What happens if you just set a flag saying "whatever is in this
string, don't expand /me, /who, /whois, /kick etc" without at that point checking
for one specific case (missing possibly many other variations).


   

  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    If it was me writing the original code I would not have made it case-sensitive, but as this is a bug fix and not a new feature I am following the current design of having just /me work.  
    
    
    
   
    2. 


   
  2. 
    
    
     
    
      
     
    
     
    I didn't suggest to make it case insensitive, I wondered what happens
when you use /ME instead of /me with and without the patch.
And I wonder why it is necessary at all to compare a string with "/me ".
At the very least that indicates code duplication.

Let me clarify,

void do_it(std::string const& str)
{
  if (!flag && str == "/me")
    ...
  else
    ...
}

Bad code:

if (str == "/me")
  flag = 1;
do_it(str);

-------------------------------------------

Code that makes more sense:

flag = 1;
do_it(str);


But keep in mind that I didn't look at the actual code ;). I just looked at your patch.

    
    
    
   
    3. 


   
  3. 
    
    
     
    
      
     
    
     
    You have to check for /me with a blank after it as anything else would be processed as a potential gesture.  This "/me " and "/me'" testing is scattered throughout the chat handling code.
    
    
    
   
    4. 

   


  






  
 




  

 



















 

  




 

 

  
  

  
Joshua Linden

  
Posted  (Jan. 7, 2011, 9:36 a.m.)

 

 

   
I believe Aleric's comment is accurate. Logic testing for a prefix should be removed from the patch, and the flag should simply always be specified in this case. 

It is notable that the flag does trigger exactly the same test that is present in the patch (i.e. it's not case sensitive, it replicates prefix testing in several other places in the code base, etc). A more general fix might be to refactor all of the places that do prefix testing, but that wouldn't affect this specific issue. Again, the patch should be reduced to one line that simply adds the desired flag.

   

  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    Joshua, if the check for "/me " and "/me'" were not present and CHAT_STYLE_IRC was always passed in then all llInstantMessages from objects would be treated as if /me had been sent.  I don't think this is what is desired.
    
    
    
   
    2. 


   
  2. 
    
    
     
    
      
     
    
     
    @Joshua : Um yes - all of that seemed logical. But the existing code is far from logical, or clean. I just had a discussion with Jonathan on IRC and now understand that the meaning of the CHAT_STYLE_IRC is actually "we found this message to start with "/me", please BLINDLY replace the first 3 characters with the name of the object/avatar". The name of the flag is horribly wrong imho. Also, then I suggested to change the code in the what I had assumed it was: set the flag whenever replacement is necessary and just leave it to the replacement code to check for it. That would therefore require an additional change: make the code that now only tests if the flag is set ALSO test if the message indeed starts with "/me " or "/me'" (and having gotten rid of the code duplication, it then could easily be extended in the future). Unfortunately, not only the testing for "/me " is scattered all over the place, also the code that does the actual replacement of the first 3 characters exists in many places! ... So, without making this a much larger "project", I suppose adding one more duplication of code that checks for "/me " isn't the worst of things, and at least it is an immediate fix for the problem at hand :/. I have no objections anymore if the patch would be used as-is.

    
    
    
   
    3.