[opensource-dev] Review Request: STORM-829 Viewer 2 does not parse /me in object Instant Messages

[Aleric Inglewood]

> On Jan. 7, 2011, 9:36 a.m., Joshua Linden wrote:
> > 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.
> 
> Jonathan Yap wrote:
>     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.

@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.


- Aleric


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


On Jan. 5, 2011, 6:14 p.m., Jonathan Yap wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/71/
> -----------------------------------------------------------
> 
> (Updated Jan. 5, 2011, 6:14 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> -------
> 
> The "/me" in the lsl code below would be displayed rather than being translated to a name:
> llInstantMessage(llGetOwner(),"/me Hello, Avatar!");
> 
> 
> This addresses bug STORM-829.
>     http://jira.secondlife.com/browse/STORM-829
> 
> 
> Diffs
> -----
> 
>   indra/newview/llviewermessage.cpp 845cab866155 
> 
> Diff: http://codereview.secondlife.com/r/71/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jonathan
> 
>

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