Re: [opensource-dev] Review Request: STORM-829 Viewer 2 does not parse /me in object Instant Messages
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/71/#review132 --- 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. - Joshua 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
Re: [opensource-dev] Review Request: STORM-829 Viewer 2 does not parse /me in object Instant Messages
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. 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. - Jonathan --- 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
Re: [opensource-dev] Review Request: STORM-829 Viewer 2 does not parse /me in object Instant Messages
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
Re: [opensource-dev] Review Request: STORM-829 Viewer 2 does not parse /me in object Instant Messages
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. I'm... wondering why not test *ONCE* when the input is accepted, do the appropriate replacement and set the flag so it gets sent as an action instead of a statement, and be done with it? The IM input, chatbar inputs, and local history tab inputs all need pretty much the same validation, don't they? ___ 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
Re: [opensource-dev] Review Request: STORM-829 Viewer 2 does not parse /me in object Instant Messages
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/71/#review120 --- 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). - Aleric 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
Re: [opensource-dev] Review Request: STORM-829 Viewer 2 does not parse /me in object Instant Messages
On Jan. 6, 2011, 7 a.m., Aleric Inglewood wrote: 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). 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. - Jonathan --- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/71/#review120 --- 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
Re: [opensource-dev] Review Request: STORM-829 Viewer 2 does not parse /me in object Instant Messages
On Jan. 6, 2011, 7 a.m., Aleric Inglewood wrote:
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).
Jonathan Yap wrote:
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.
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.
- Aleric
---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/71/#review120
---
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
Re: [opensource-dev] Review Request: STORM-829 Viewer 2 does not parse /me in object Instant Messages
that would be bad to check for /me, what about /meow ?
From: aleric.inglew...@gmail.com
To: opensource-dev@lists.secondlife.com; aleric.inglew...@gmail.com;
jhwe...@gmail.com
Date: Thu, 6 Jan 2011 22:03:14 +
Subject: Re: [opensource-dev] Review Request: STORM-829 Viewer 2 does not parse
/me in object Instant Messages
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/71/
On January 6th, 2011, 7 a.m., Aleric Inglewood wrote:
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).
On January 6th, 2011, 7:45 a.m., Jonathan Yap wrote:
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. 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.
- Aleric
On January 5th, 2011, 6:14 p.m., Jonathan Yap wrote:
Review request for Viewer.
By Jonathan Yap.
Updated Jan. 5, 2011, 6:14 p.m.
Description
The /me in the lsl code below would be displayed rather than being translated
to a name:
llInstantMessage(llGetOwner(),/me Hello, Avatar!);
Bugs: STORM-829
Diffs
indra/newview/llviewermessage.cpp (845cab866155)
View Diff
___ 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
___
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
Re: [opensource-dev] Review Request: STORM-829 Viewer 2 does not parse /me in object Instant Messages
On Jan. 6, 2011, 7 a.m., Aleric Inglewood wrote:
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).
Jonathan Yap wrote:
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.
Aleric Inglewood wrote:
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.
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.
- Jonathan
---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/71/#review120
---
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
[opensource-dev] Review Request: STORM-829 Viewer 2 does not parse /me in object Instant Messages
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/71/ --- 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
Re: [opensource-dev] Review Request: STORM-829 Viewer 2 does not parse /me in object Instant Messages
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/71/#review114 --- Ship it! indra/newview/llviewermessage.cpp http://codereview.secondlife.com/r/71/#comment78 Looks good to me, but just wondering why your checking for /me and /me' . - Wolfpup 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
