> 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 > >
_______________________________________________ Policies and (un)subscribe information available here: http://wiki.secondlife.com/wiki/OpenSource-Dev Please read the policies before posting to keep unmoderated posting privileges
