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

2011-01-06 Thread Liny Odell 
Ever done "/me's happy"? It comes out (in my case) "Liny Odell's happy".

On Thu, Jan 6, 2011 at 7:33 PM, Wolfpup Lowenhar wrote:

>This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/71/
>
> Ship it!
>
> indra/newview/llviewermessage.cpp
>  (Diff
> revision 1)
>
> void process_improved_im(LLMessageSystem *msg, void **user_data)
>
>2752
>
>   if (prefix == "/me " || prefix == "/me'")
>
>   Looks good to me, but just wondering why your checking for "/me " and 
> "/me'" .
>
>
> - Wolfpup
>
> 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-826 (partial): fix line endings in files that use a mix of CRLF and LF

2011-01-06 Thread Boroondas Gupte 

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


"View Diff" doesn't display anything and with "Download Diff", I get a patch 
file that doesn't change anything: It removes some lines and adds back the 
exact same lines, with the same line endings (just LF). The surrounding lines 
(in the diff context) seem also to be LF-ended, even though one of the actual 
original files (indra/newview/llfloaterwebcontent.h) has CRLF endings, except 
for the line touched by the patch.

I don't know how much of these are review-board and/or diffing issues. Please 
point us to a hg changeset so we can review the actual change.

- Boroondas


On Jan. 5, 2011, 7:19 a.m., Oz Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/70/
> ---
> 
> (Updated Jan. 5, 2011, 7:19 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> This is a simple change to correct existing line endings - I scanned all of 
> viewer-development to identify files that had a mixture of CRLF and LF 
> endings and converted them to just LF.
> 
> What to do about preventing future such will be dealt with separately...
> 
> 
> This addresses bug storm-826.
> http://jira.secondlife.com/browse/storm-826
> 
> 
> Diffs
> -
> 
>   indra/newview/llfloaterwebcontent.h 845cab866155 
>   indra/newview/llimview.h 845cab866155 
>   indra/newview/llimview.cpp 845cab866155 
>   indra/newview/lllogchat.cpp 845cab866155 
> 
> Diff: http://codereview.secondlife.com/r/70/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Oz
> 
>

___
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-826 (partial): fix line endings in files that use a mix of CRLF and LF

2011-01-06 Thread Boroondas Gupte 


> On Jan. 6, 2011, 4:01 a.m., Boroondas Gupte wrote:
> > "View Diff" doesn't display anything and with "Download Diff", I get a 
> > patch file that doesn't change anything: It removes some lines and adds 
> > back the exact same lines, with the same line endings (just LF). The 
> > surrounding lines (in the diff context) seem also to be LF-ended, even 
> > though one of the actual original files 
> > (indra/newview/llfloaterwebcontent.h) has CRLF endings, except for the line 
> > touched by the patch.
> > 
> > I don't know how much of these are review-board and/or diffing issues. 
> > Please point us to a hg changeset so we can review the actual change.

Hmm ... on the jira, I saw the link to 
https://bitbucket.org/oz_linden/storm-826/changeset/8037fad5dee4 but that 
changes many more files than the diff downloadable here even mentions.


- Boroondas


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


On Jan. 5, 2011, 7:19 a.m., Oz Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/70/
> ---
> 
> (Updated Jan. 5, 2011, 7:19 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> This is a simple change to correct existing line endings - I scanned all of 
> viewer-development to identify files that had a mixture of CRLF and LF 
> endings and converted them to just LF.
> 
> What to do about preventing future such will be dealt with separately...
> 
> 
> This addresses bug storm-826.
> http://jira.secondlife.com/browse/storm-826
> 
> 
> Diffs
> -
> 
>   indra/newview/llfloaterwebcontent.h 845cab866155 
>   indra/newview/llimview.h 845cab866155 
>   indra/newview/llimview.cpp 845cab866155 
>   indra/newview/lllogchat.cpp 845cab866155 
> 
> Diff: http://codereview.secondlife.com/r/70/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Oz
> 
>

___
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

2011-01-06 Thread Jonathan Yap 


> On Jan. 5, 2011, 7:33 p.m., Wolfpup Lowenhar wrote:
> > indra/newview/llviewermessage.cpp, line 2752
> > 
> >
> > Looks good to me, but just wondering why your checking for "/me " and 
> > "/me'" .

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


- Jonathan


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


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] Very Strange occurrence...

2011-01-06 Thread Carlo Wood 
On Wed, Jan 05, 2011 at 04:44:31PM -0500, Kent Quirk (Q Linden) wrote:
> So the reason that semi-plausible strings are used for these things is that 
> they're the only strings available when we use the test floater feature from 
> the login screen.
> 
> But we should probably try not to use real names.
> 
> And yes, Jonathan, these should mostly never be visible. But sometimes things 
> happen.

Nevertheless, it is a bug; if whatever failed is outside the influence of
the viewer itself, then still it should not have shown this. The viewer
should be fixed to show the account name, or at least the UUID, in
this -hopefully- rare case.

-- 
Carlo Wood 
___
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-826 (partial): fix line endings in files that use a mix of CRLF and LF

2011-01-06 Thread Aleric Inglewood 

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


You missed indra/newview/llfloaterwebcontent.cpp

Also, why not fix the only .xml with carriage returns in it:

indra/newview/skins/default/xui/en/floater_web_content.xml

Configuration files with carriage returns:

indra/cmake/GetPrerequisites_2_8.cmake
indra/cmake/LLAddBuildTest.cmake



- Aleric


On Jan. 5, 2011, 7:19 a.m., Oz Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/70/
> ---
> 
> (Updated Jan. 5, 2011, 7:19 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> This is a simple change to correct existing line endings - I scanned all of 
> viewer-development to identify files that had a mixture of CRLF and LF 
> endings and converted them to just LF.
> 
> What to do about preventing future such will be dealt with separately...
> 
> 
> This addresses bug storm-826.
> http://jira.secondlife.com/browse/storm-826
> 
> 
> Diffs
> -
> 
>   indra/newview/llfloaterwebcontent.h 845cab866155 
>   indra/newview/llimview.h 845cab866155 
>   indra/newview/llimview.cpp 845cab866155 
>   indra/newview/lllogchat.cpp 845cab866155 
> 
> Diff: http://codereview.secondlife.com/r/70/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Oz
> 
>

___
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

2011-01-06 Thread Aleric Inglewood 

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

2011-01-06 Thread Jonathan Yap 


> 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: VWR-24347 Reversion in Copy3rdPartyLibs.cmake -- cannot find msvc* files using VS 2005 Express

2011-01-06 Thread Jonathan Yap 


> On Jan. 4, 2011, 4:14 p.m., Merov Linden wrote:
> > Those lines have been missing since a long time: the first version in hg is 
> > already missing them:
> > https://bitbucket.org/lindenlab/viewer-development/changeset/07304583316d

They are not in that early version, but they are (plus code for VS2008) in a 
V2.1 copy I have of that file, but at some point were taken out again.


- Jonathan


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


On Dec. 29, 2010, 3:42 p.m., Jonathan Yap wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/68/
> ---
> 
> (Updated Dec. 29, 2010, 3:42 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Environment: Windows, VS 2005 Express
> 
> Back in the days of v2.1 I was able to supply the following option to 
> develop.py, which would allow me to specify the location of
> msvcr80.dll
> msvcp80.dll
> Microsoft.VC80.CRT.manifest
> 
> -DMSVC_REDIST_PATH:PATH=E:/some/path
> 
> There was a similar code path for the debug files
> -DMSVC_DEBUG_REDIST_PATH=E:/some/path
> 
> This files cannot be found by the Express compiler so without a way of 
> telling the compiler where to find them they have to be dropped into the 
> build tree manually.
> 
> At some point Copy3rdPartyLibs.cmake was rewritten and this option was 
> dropped.
> 
> 
> This addresses bug vwr-24347.
> http://jira.secondlife.com/browse/vwr-24347
> 
> 
> Diffs
> -
> 
>   indra/cmake/Copy3rdPartyLibs.cmake 27dae7b01a81 
> 
> Diff: http://codereview.secondlife.com/r/68/diff
> 
> 
> Testing
> ---
> 
> I tried using -DMSVC_REDIST_PATH:PATH=E:/some/path with my VS 2005 Express 
> compiler and obtained the desired result. 
> 
> I don't have the debug files to test -DMSVC_DEBUG_REDIST_PATH=E:/some/path
> 
> 
> 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-826 (partial): fix line endings in files that use a mix of CRLF and LF

2011-01-06 Thread Oz Linden 

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

(Updated Jan. 6, 2011, 10:20 a.m.)


Review request for Viewer.


Summary (updated)
---

This is a simple change to correct existing line endings - I scanned all of 
viewer-development to identify files that had a mixture of CRLF and LF endings 
and converted them to just LF.

The diff apparently won't show the change in line ending characters see the 
repo (in the issue) for the real change.
I expanded the scope of this to also convert some files that were all the same 
CRLF endings but did not obviously need to be.   I left files that were clearly 
windows specific alone on the theory that non-windows users probably won't need 
to touch them, and the windows tools might care.


This addresses bug storm-826.
http://jira.secondlife.com/browse/storm-826


Diffs (updated)
-

  indra/cmake/GetPrerequisites_2_8.cmake 6d44f0d85a80 
  indra/cmake/LLAddBuildTest.cmake 6d44f0d85a80 
  indra/newview/llfloaterwebcontent.h 6d44f0d85a80 
  indra/newview/llfloaterwebcontent.cpp 6d44f0d85a80 
  indra/newview/llimview.h 6d44f0d85a80 
  indra/newview/llimview.cpp 6d44f0d85a80 
  indra/newview/lllogchat.cpp 6d44f0d85a80 
  indra/newview/tests/llremoteparcelrequest_test.cpp 6d44f0d85a80 
  indra/viewer_components/updater/tests/llupdaterservice_test.cpp 6d44f0d85a80 

Diff: http://codereview.secondlife.com/r/70/diff


Testing
---


Thanks,

Oz

___
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] Snowstorm Sprint meeting cancellation info

2011-01-06 Thread Sarah (Esbee) Kuehnle 
Hi everybody,

Due to conflicting meetings, I've had to cancel the Snowstorm Sprint meeting
we had planned for 12pm PT today. We will still be having a Sprint Review
meeting, but it will be held at 1pm PT.

-- Esbee
___
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-826 (partial): fix line endings in files that use a mix of CRLF and LF

2011-01-06 Thread Aleric Inglewood 

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

Ship it!


Should be ok, I see no diff on the review board at all :p
I didn't actually apply the diff and check if all carriage returns are gone 
now, but I don't think that's the idea of the review board, is it?

- Aleric


On Jan. 6, 2011, 10:20 a.m., Oz Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/70/
> ---
> 
> (Updated Jan. 6, 2011, 10:20 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> This is a simple change to correct existing line endings - I scanned all of 
> viewer-development to identify files that had a mixture of CRLF and LF 
> endings and converted them to just LF.
> 
> The diff apparently won't show the change in line ending characters see 
> the repo (in the issue) for the real change.
> I expanded the scope of this to also convert some files that were all the 
> same CRLF endings but did not obviously need to be.   I left files that were 
> clearly windows specific alone on the theory that non-windows users probably 
> won't need to touch them, and the windows tools might care.
> 
> 
> This addresses bug storm-826.
> http://jira.secondlife.com/browse/storm-826
> 
> 
> Diffs
> -
> 
>   indra/cmake/GetPrerequisites_2_8.cmake 6d44f0d85a80 
>   indra/cmake/LLAddBuildTest.cmake 6d44f0d85a80 
>   indra/newview/llfloaterwebcontent.h 6d44f0d85a80 
>   indra/newview/llfloaterwebcontent.cpp 6d44f0d85a80 
>   indra/newview/llimview.h 6d44f0d85a80 
>   indra/newview/llimview.cpp 6d44f0d85a80 
>   indra/newview/lllogchat.cpp 6d44f0d85a80 
>   indra/newview/tests/llremoteparcelrequest_test.cpp 6d44f0d85a80 
>   indra/viewer_components/updater/tests/llupdaterservice_test.cpp 
> 6d44f0d85a80 
> 
> Diff: http://codereview.secondlife.com/r/70/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Oz
> 
>

___
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

2011-01-06 Thread Aleric Inglewood 


> 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

2011-01-06 Thread Twisted Laws 

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

[opensource-dev] Review Request: STORM-830 Volume slider isn't properly remembered if set to zero

2011-01-06 Thread Jonathan Yap 

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

Review request for Viewer.


Summary
---

There is an edge case in setMasterGain during startup which prevents 
setInternalGain from being called if the master volume setting and 
mInternalGain both equal 0.

Setting mInternalGain to a very low but non-zero value fixes this issue.


This addresses bug STORM-830.
http://jira.secondlife.com/browse/STORM-830


Diffs
-

  indra/llaudio/llaudioengine.cpp 6d44f0d85a80 

Diff: http://codereview.secondlife.com/r/72/diff


Testing
---

In Preferences / Sound & Media tested:
Buttons
Ambient
Sound Effects
Stream Music
Media
Voice Chat


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

2011-01-06 Thread Jonathan Yap 


> 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

Re: [opensource-dev] Review Request: STORM-830 Volume slider isn't properly remembered if set to zero

2011-01-06 Thread Aleric Inglewood 

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


This is really not how you want to deal with this bug :/.  It's a known fact 
that audio mixers are very bad with low volumes. Setting a volume to 0 (or 
something really small) can put a very high load on the CPU for the audio 
mixer, which causes severe problems. See 
https://jira.secondlife.com/browse/VWR-14914

So, on the contrary (as VWR-14914 fixed a horrible bug that made FPS drop 
drastically): when the volume is set to 0 (or even close to zero) the audio 
channel has to be muted and not mixed, ever. Assuming that VWR-14914 is in 
Viewer 2, and wasn't broken in the meantime, a volume of 0 would cause the 
channel to be muted, but setting it to 0.01 will not cause it to be muted, 
but result in a high CPU load.


- Aleric


On Jan. 6, 2011, 2:37 p.m., Jonathan Yap wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/72/
> ---
> 
> (Updated Jan. 6, 2011, 2:37 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> There is an edge case in setMasterGain during startup which prevents 
> setInternalGain from being called if the master volume setting and 
> mInternalGain both equal 0.
> 
> Setting mInternalGain to a very low but non-zero value fixes this issue.
> 
> 
> This addresses bug STORM-830.
> http://jira.secondlife.com/browse/STORM-830
> 
> 
> Diffs
> -
> 
>   indra/llaudio/llaudioengine.cpp 6d44f0d85a80 
> 
> Diff: http://codereview.secondlife.com/r/72/diff
> 
> 
> Testing
> ---
> 
> In Preferences / Sound & Media tested:
> Buttons
> Ambient
> Sound Effects
> Stream Music
> Media
> Voice Chat
> 
> 
> 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