[opensource-dev] [META][WEB] Registering a SL Review Board account: Jira email address vs. mailing list address (was: Re: Review Request: Restructure loops that use breaks without need (reviewboard te

[Boroondas Gupte]

https://codereview.secondlife.com/account/register/ advises:
> Use the email address that you have associated with that Jira account.
However, as mails automatically generated from my actions on Review
Board get sent to the mailing list, I now changed the address in my SL
Review Board profile to my mailing list address, so that future messages
won't get rejected.

Cheers,
Boroondas


On 12/02/2010 12:40 PM, opensource-dev-ow...@lists.secondlife.com wrote:
> You are not allowed to post to this mailing list, and your message has
> been automatically rejected.  If you think that your messages are
> being rejected in error, contact the mailing list owner at
> opensource-dev-ow...@lists.secondlife.com.
>
>  Attached Email 
> Subject:  Re: Review Request: Restructure loops that use breaks
> without need (reviewboard test)
> Date: Thu, 02 Dec 2010 11:40:12 -0000
> From: Boroondas Gupte <*[The address I use for jira* (different from
> the one I use for the mailing list and not intended for public
> visibility)*]*>
> To:   Boroondas Gupte <*[...]*>, Oz Linden , Viewer
> 
>
>
>
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/2/
>
>
> indra/newview/llappviewer.cpp
> <http://codereview.secondlife.com/r/2/diff/1/?file=8#file8line1188>
> (Diff revision 1)
> bool LLAppViewer::mainLoop()
> 1188  
>   S32 total_io_pending = 0;   
> 
>   1188
>   S32 total_io_pending = 0;   
> 
>
> While you're at it, please fix the whitespace errors in the touched (and 
> nearby) code. (In a separate changeset, though, if possible)
>
> The Review-Board diff view conveniently highlights them in red, so they are 
> hard to miss ;-)
>
> indra/newview/llappviewer.cpp
> <http://codereview.secondlife.com/r/2/diff/1/?file=8#file8line1199>
> (Diff revision 1)
> bool LLAppViewer::mainLoop()
> 1195  
>   work_pending += 
> LLAppViewer::getTextureCache()->update(1); // unpauses the texture cache 
> thread
>   1199
>   work_pending += 
> LLAppViewer::getTextureCache()->update(1); // unpauses the texture cache 
> thread
>
> e.g. here: spaces before tabs
>
> indra/newview/llappviewer.cpp
> <http://codereview.secondlife.com/r/2/diff/1/?file=8#file8line1631>
> (Diff revision 1)
> bool LLAppViewer::cleanup()
> 1624  
>   if(!pending)
>   1623
> } while ( pending && idle_time < max_idle_time) 
>
> Your change even introduces new whitespace errors (trailing whitespace here)
>
> - Boroondas
>
>
> On December 1st, 2010, 7:57 p.m., Oz Linden wrote:
>
> Review request for Viewer.
> By Oz Linden.
>
> /Updated 2010-12-01 19:57:24/
>
>
>   Description
>
> This review is mostly a first test of reviewboard.
>
> I do have an esthetic dislike for the 'break' statement anywhere but as the 
> end of a case, so I chose to change some instances of break usage that were 
> not justified by any extreme need.
>
>
>   Testing
>
> None at all... have not even compiled it yet.
>
> *Bugs: * storm-606 <http://jira.secondlife.com/browse/storm-606>
>
>
>   Diffs
>
> * indra/newview/llappviewer.cpp (bf98b026bcb1)
>
> View Diff <http://codereview.secondlife.com/r/2/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

Re: [opensource-dev] Review Request: Restructure loops that use breaks without need (reviewboard test)

[Boroondas Gupte]

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



indra/newview/llappviewer.cpp


e.g. here, some lines are indented by spaces and others by tabs


- Boroondas


On 2010-12-01 19:57:24, Oz Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/2/
> ---
> 
> (Updated 2010-12-01 19:57:24)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> This review is mostly a first test of reviewboard.
> 
> I do have an esthetic dislike for the 'break' statement anywhere but as the 
> end of a case, so I chose to change some instances of break usage that were 
> not justified by any extreme need.
> 
> 
> This addresses bug storm-606.
> http://jira.secondlife.com/browse/storm-606
> 
> 
> Diffs
> -
> 
>   indra/newview/llappviewer.cpp bf98b026bcb1 
> 
> Diff: http://codereview.secondlife.com/r/2/diff
> 
> 
> Testing
> ---
> 
> None at all... have not even compiled it yet.
> 
> 
> 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: Restructure loops that use breaks without need (reviewboard test)

[Boroondas Gupte]

> On 2010-12-02 04:52:10, Boroondas Gupte wrote:
> >

EDITED TO ADD: (apparently Review Board can't handle comments on comments on 
lines (sic!) and new comments on lines at the same time)
"The Review-Board diff view conveniently highlights them in red, so they are 
hard to miss ;-)"
Of course, Review Board can't detect all whitespace mistakes:


- Boroondas


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


On 2010-12-01 19:57:24, Oz Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/2/
> ---
> 
> (Updated 2010-12-01 19:57:24)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> This review is mostly a first test of reviewboard.
> 
> I do have an esthetic dislike for the 'break' statement anywhere but as the 
> end of a case, so I chose to change some instances of break usage that were 
> not justified by any extreme need.
> 
> 
> This addresses bug storm-606.
> http://jira.secondlife.com/browse/storm-606
> 
> 
> Diffs
> -
> 
>   indra/newview/llappviewer.cpp bf98b026bcb1 
> 
> Diff: http://codereview.secondlife.com/r/2/diff
> 
> 
> Testing
> ---
> 
> None at all... have not even compiled it yet.
> 
> 
> 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: Restructure loops that use breaks without need (reviewboard test)

[Boroondas Gupte]

> On 2010-12-02 04:52:10, Boroondas Gupte wrote:
> >
> 
> Boroondas Gupte wrote:
> EDITED TO ADD: (apparently Review Board can't handle comments on comments 
> on lines (sic!) and new comments on lines at the same time)
> "The Review-Board diff view conveniently highlights them in red, so they 
> are hard to miss ;-)"
> Of course, Review Board can't detect all whitespace mistakes:

Ah, I guess I should have read 
http://www.reviewboard.org/docs/manual/1.5/users/reviews/reviewing-diffs/#reading-existing-comments
 , first: "It’s important to note that this is meant to be used as a reference 
to see if other people have already said what you plan to say. The comment box 
is not the place to reply to those comments. Instead, you can click the Reply 
link next to the particular comment, which will take you back to the review 
request page and open a reply box."


- Boroondas


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


On 2010-12-01 19:57:24, Oz Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/2/
> ---
> 
> (Updated 2010-12-01 19:57:24)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> This review is mostly a first test of reviewboard.
> 
> I do have an esthetic dislike for the 'break' statement anywhere but as the 
> end of a case, so I chose to change some instances of break usage that were 
> not justified by any extreme need.
> 
> 
> This addresses bug storm-606.
> http://jira.secondlife.com/browse/storm-606
> 
> 
> Diffs
> -
> 
>   indra/newview/llappviewer.cpp bf98b026bcb1 
> 
> Diff: http://codereview.secondlife.com/r/2/diff
> 
> 
> Testing
> ---
> 
> None at all... have not even compiled it yet.
> 
> 
> 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] ReviewBoard email to this list?

[Boroondas Gupte]

On 12/02/2010 02:52 PM, Oz Linden (Scott Lawrence) wrote:
> Note that these emails are easy to set up filters for - not only will
> all of them have 'Review Request' in the Subject lines, there are also
> a couple of custom headers in each message that make them easy to
> recognize.
On 12/02/2010 02:58 PM, WolfPup Lowenhar wrote:
>
> I would be nice if the review board system had an address that was
> something that people that use email programs( I.E. Outlook,
> Thunderbird, Incredimail) could much easier filter. Like
> nore...@codereview.secondlife.com
>  or something similar that
> way if they set up a filter it would not interfere with any other
> filters they have set up that are for SL.
>

Even easier (for subscribers) would be to have a separate mailing list
for the automatic Review Board emails. Then, everyone can decide
themselves whether to subscribe to that, too (or even only to that).
Non-subscribers would still have the option of seeing the threads on the
list's archive website.

This would be consistent with existing and previous single-purpose lists
like Jira-notify
,
viewer-development-builds
,
viewer-development-commits

and sldev-commits (or whatever the SVN commit notification list was
called back then.)

Cheers,
Boroondas

PS: Is there any reason why the viewer-development-builds Archives

are 'private' (i.e. only available to list members)?
**


___
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] nametag opacity

[Boroondas Gupte]

On 12/03/2010 05:36 AM, Chuck Baggett wrote:
> Are the nametags fixed at 100 per cent opaque or are they of variable
> opacity?
>
> By nametags I mean avatar names and group titles.

The opacity is variable, but the control for the setting isn't obvious,
as it's labeled "bubble chat opacity", although it also applies to
nametags while not chatting and even when the bubble chat option is off.


In Viewer 1.23:

   1. Open Preferences (*Ctrl-P*)
   2. Go to tab *Text Chat*
   3. Section *Bubble Chat* has an *Opacity* slider


In Viewer 2.3 (current release):

   1. Open Preferences (*Ctrl-P*)
   2. Go to tab *Advaced*
   3. In the section with the *walking person* icon (sic!), below the
  *Bubble chat* checkbox, there is an *Opacity* slider


In current viewer-development code:

Since the recent preferences overhaul (see STORM-31
 and in particular
STORM-573 )

   1. Open Preferences (*Ctrl-P*)
   2. Go to tab *Colors*
   3. Section *Bubble Chat* has an *Opacity* slider


The section should probably relabeled to reflect that the setting
applies to both, nametags and bubble chat. There were several issues
with the nametags and their transparency and color, recently:

* STORM-584  The
  settings didn't have any effect. The text color was just white.
* Fixing this made tags unreadable for everyone who didn't set a
  custom color, because the default of the color setting was still
  black. (So you had black text on black background.)
* STORM-667  Even
  after the above was fixed, contrast was poor due to the text in
  the tag being semitrasparent.
  o The slider did only influence the background opacity. That's
probably what it was supposed to do, but the
(non-customizable) text opacity should have been fully opaque.
  o As far as I can tell by eye, this is fixed in latest
viewer-development code.
* On Mac, there were
  

  (and maybe still are) issues that nametags would appear unreadable
  or not at all on ATi cards.
  o Is there a separate issue for this, yet?
* VWR-24017  text on
  name tags is visible, even if the background of the nametag gets
  occluded by objects. I guess the underlying reason is the same as
  for why floating text is not occluded anymore (SH-489
  )

STORM-584  will probably
lead to further changes
,
which aren't in viewer-development, yet. (Text in the tag being
determined by the same settings as for the chat window, the "bubble
chat" color setting affecting the tag's background instead (hopefully
with yet again changed default, so we keep the contrast).)

Cheers,
Boroondas
___
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] nametag opacity

[Boroondas Gupte]

On 12/03/2010 12:28 PM, Boroondas Gupte wrote:
> Since the recent preferences overhaul (see STORM-31
> <https://jira.secondlife.com/browse/STORM-31> and in particular
> STORM-573 <https://jira.secondlife.com/browse/STORM-573>)
err, STORM-583 <https://jira.secondlife.com/browse/STORM-583>, not STORM-573
___
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-524 : Refresh L$ balance

[Boroondas Gupte]

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

Ship it!


Didn't test, but code looks good.


indra/newview/llstatusbar.cpp


consider to break long comments onto several lines


Other then that, go for it!

- Boroondas


On 2010-12-08 21:59:58, Merov Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/6/
> ---
> 
> (Updated 2010-12-08 21:59:58)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Since the L$ balance can be updated by events outside the viewer entirely 
> (purchase of L$ on web) and since we do not pool or receive notifications for 
> this, I implemented a simple "click to refresh" event on the LLTextBox 
> holding the L$ balance value (proposed by Q in JIRA).
> I modified the tooltip to reflect this new functionality.
> 
> 
> This addresses bug STORM-524.
> http://jira.secondlife.com/browse/STORM-524
> 
> 
> Diffs
> -
> 
>   indra/newview/llstatusbar.h 094773a5a314 
>   indra/newview/llstatusbar.cpp 094773a5a314 
>   indra/newview/skins/default/xui/en/panel_status_bar.xml 094773a5a314 
> 
> Diff: http://codereview.secondlife.com/r/6/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Merov
> 
>

___
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] Linux 64bit and gstreamer

[Boroondas Gupte]

Hi Mike,

On 12/10/2010 06:51 PM, Mike Chase wrote:
> Can someone point me to a summary of 64bit support for Linux 
> for that series of viewers?  I know in the past I was able to run a 
> 32bit version but with no streaming media.
See the forum thread Linux 64 bits and medias
.

On 12/10/2010 10:29 PM, Marc Adored wrote:
> I've been hoping for linden
> to really work on simplifying the 64bit building because 64bit systems
> are being more and more popular with the need for more and more
> memory. I do believe someone was working on fixing this in the
> opensource community but I don't recall who
For the viewer-development branch, that'd be me. Although I mostly just
ported others' fixes that already existed for the 1.x code base and/or
Snowglobe 2.

>  or how far they got.
Should be fully working now (see below), if you start form
viewer-development (or viewer-beta or viewer-release). About the
mesh-development branch ... well, that's another animal entirely when it
comes to building 64bit (due to new dependencies and whatnot).


On 12/11/2010 04:46 AM, Mike Chase wrote:
> On 12/10/2010 08:06 PM, Carlo Wood wrote:
>> Huh huh... No need to install 32bit libs! Just compile the viewer
>> yourself! I've been running native 64 bit since day one.
> Carlo, do you have a script or a pointer to the steps to do the build?  
All necessary steps should be on the wiki at Compiling the viewer
(Linux) .

> Is it simply the standard steps or something special.
Because Linden Lab does not provide pre-built libraries for 64bit linux,
you'll have to build "standalone", i.e., using system libraries. (See
What does 'Standalone' mean?
)
Thus you'll need to have all build time dependencies installed into your
system. The wiki article linked above lists debian and ubuntu package
names for most dependencies. Standalone-specific steps are included and
marked as such (e.g. through section naming).

The build documentation tends to get out-of-date quickly. If you decide
to build yourself and run into troubles or have further questions,
please do ask on this mailing list or on IRC (channel #opensl on
freenode ) so that you can be helped and
the documentation improved/corrected.

Good luck & cheers,
Boroondas
___
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: Settings.xml: redundant entries and unnecessary tag

[Boroondas Gupte]

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

Ship it!


looks good

- Boroondas


On 2010-12-14 12:39:34, Jonathan Yap wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/18/
> ---
> 
> (Updated 2010-12-14 12:39:34)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> I wrote two programs that use settings.xml to produce this massive table:
> http://wiki.secondlife.com/wiki/Debug_Settings
> 
> While doing this I found 4 places with duplicate entries and 1 entry that is 
> repeated 4 times.  There is also a pair of unnecessary tags.
> 
> Having these cleaned out would make running my program, and thus updating the 
> wiki table, easier.
> 
> I've erased all but the last entry for these redundant debug settings.
> 
> 
> This addresses bug vwr-24100.
> http://jira.secondlife.com/browse/vwr-24100
> 
> 
> Diffs
> -
> 
>   indra/newview/app_settings/settings.xml 46a990f8296f 
> 
> Diff: http://codereview.secondlife.com/r/18/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: Crash in LLRemoteParcelInfoProcessor::processParcelInfoReply()

[Boroondas Gupte]

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



indra/newview/llremoteparcelrequest.cpp


post-increments are subtle ... please include some comment in the code that 
points out what's going on. (Can be short)



indra/newview/llremoteparcelrequest.cpp


Please also comment in the code why cur_oi may not be oi when calling this, 
so we don't risk someone reverting your fix by accident.


- Boroondas


On 2010-12-15 11:27:50, Kitty Barnett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/24/
> ---
> 
> (Updated 2010-12-15 11:27:50)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> erase() on a multimap will only invalidate iterators that point to the 
> element being erased so pre-incrementing the loop iterator should prevent it 
> from getting invalidated when an observer calls removeObserver() as part of 
> its processParcelInfo() implementation.
> 
> 
> This addresses bug VWR-24207.
> http://jira.secondlife.com/browse/VWR-24207
> 
> 
> Diffs
> -
> 
>   indra/newview/llremoteparcelrequest.cpp UNKNOWN 
> 
> Diff: http://codereview.secondlife.com/r/24/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kitty
> 
>

___
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: scripts/install.py --uninstall does not always remove symbolic links.

[Boroondas Gupte]

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

Ship it!


Your explanation is consistent with python's documentation and the change looks 
simple and sane. The change won't have any effect on platforms that don't have 
os.lstat(), but those seem to be the ones that don't have symlinks in the first 
place.

- Boroondas


On 2010-12-16 07:34:22, Aleric Inglewood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/29/
> ---
> 
> (Updated 2010-12-16 07:34:22)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Packages (tar balls) installed with scripts/install.py do contain symbolic 
> links.
> Everything that they contain is written to the file installed.xml, and upon
> uninstall attempted to remove.
> 
> However, the python script first tests if a file exists before it removes it
> and uses os.path.exists for this, which only returns true when the target
> is a file, or a symbolic link *pointing* to an existing file.
> 
> Since the removal of the tar ball elements is arbitrary, it is possible (and
> often the case) that the file the symbolic link points to is removed before
> the symbolic link itself is removed, causing the test to fail and the symbolic
> link not to be removed.
> 
> This patch solves this bug by using os.path.lexists which returns true for
> normal files when they exist, and true for symbolic links if they exist,
> whether or not the file they point to exists, exactly what we want.
> 
> os.path.lexists was added to python 2.4, so that should not be problem.
> 
> 
> This addresses bug SNOW-744.
> http://jira.secondlife.com/browse/SNOW-744
> 
> 
> Diffs
> -
> 
>   scripts/install.py b0689af42a71 
> 
> Diff: http://codereview.secondlife.com/r/29/diff
> 
> 
> Testing
> ---
> 
> This patch was originally tested to work several months ago, and has been in
> use by the Imprudence TPV for a while now.
> 
> 
> Thanks,
> 
> Aleric
> 
>

___
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] VWR-24172 (tab key not working)

[Boroondas Gupte]

On 12/18/2010 04:55 PM, Trilo Byte wrote:
> [...] tab key stopped working to jump between fields (in the build window, 
> debugged settings window, etc).  [...] anybody else experiencing this?
Yes, me.

Boroondas
___
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-24247: develop.py configure still searches for the wrong header file when checking for Tut

[Boroondas Gupte]

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



indra/cmake/FindTut.cmake


According to the CMake 2.8.1 man page, NO_SYSTEM_ENVIRONMENT_PATH makes 
find_path not only skip $PATH but also $INCLUDE. I think we should respect the 
latter when looking for headers.


- Boroondas


On 2010-12-18 09:01:35, Aleric Inglewood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/39/
> ---
> 
> (Updated 2010-12-18 09:01:35)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> There is no #include "tut.h" anywhere.
> The only includes are #include "tut/tut.hpp" (which is correct).
> Tut installs in /include/tut/* among which the headerfile tut.hpp.
> The changed file, indra/cmake/FindTut.cmake, is only included when configured 
> with --standalone (and thus only on linux).
> find_path searches in /usr/include and /usr/local/include anyway, so there is 
> no way to add that.
> Adding the NO_SYSTEM_ENVIRONMENT_PATH stops it from reading the PATH 
> environment variable and looking
> for tut/tut.hpp, which probably won't harm, but it simply makes no sense: 
> we're looking for a headerfile, not an executable.
> 
> Without this patch (and without creating a fake /usr/local/include/tut.h), I 
> get the error:
> CMake Error at cmake/FindTut.cmake:26 (message):
>   Could not find Tut
> Call Stack (most recent call first):
>   cmake/Tut.cmake:8 (include)
>   llmessage/CMakeLists.txt:13 (include)
> 
> With the patch, it finds /usr/local/include/tut/tut.hpp as expected, setting 
> TUT_INCLUDE_DIR to "/usr/local/include",
> and it also finds (in my case) 
> /usr/src/secondlife/viewers/snowstorm/viewer-development/include/tut/tut.hpp, 
> setting
> TUT_INCLUDE_DIR to 
> "/usr/src/secondlife/viewers/snowstorm/viewer-development/include", since I 
> have a symbolic
> link from 
> /usr/src/secondlife/viewers/snowstorm/viewer-development/include/tut to 
> ../linden/libraries/include/tut
> (which was installed manually with ./scripts/install.py tut) and I have the 
> environment variable CMAKE_INCLUDE_PATH
> set to 
> "/usr/src/secondlife/llqtwebkit/install-imprudence/include:/usr/src/secondlife/viewers/snowstorm/viewer-development/include:/sl/usr/include".
> In other words, this allows developers to install headers whereever they want 
> and use the API of cmake as it is
> intended, to find those headers.
> 
> 
> This addresses bug VWR-24247.
> http://jira.secondlife.com/browse/VWR-24247
> 
> 
> Diffs
> -
> 
>   indra/cmake/FindTut.cmake b0689af42a71 
> 
> Diff: http://codereview.secondlife.com/r/39/diff
> 
> 
> Testing
> ---
> 
> Tested to see if it finds the header when installed in /usr/local/include as 
> well in a path specified
> with CMAKE_INCLUDE_PATH.
> 
> Note that you need to configure with --standalone in order to test this.
> Also note that unless tut is installed in /usr/local, and when you configure 
> with -DLL_TESTS:BOOL=ON,
> it still won't compile because of another bug. I will submit a patch for that 
> separately.
> 
> 
> Thanks,
> 
> Aleric
> 
>

___
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-24247: develop.py configure still searches for the wrong header file when checking for Tut

[Boroondas Gupte]

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

Ship it!


Ah, I wasn't aware gcc ignores INCLUDE. In that case, the change is good as-is.

- Boroondas


On 2010-12-18 09:01:35, Aleric Inglewood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/39/
> ---
> 
> (Updated 2010-12-18 09:01:35)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> There is no #include "tut.h" anywhere.
> The only includes are #include "tut/tut.hpp" (which is correct).
> Tut installs in /include/tut/* among which the headerfile tut.hpp.
> The changed file, indra/cmake/FindTut.cmake, is only included when configured 
> with --standalone (and thus only on linux).
> find_path searches in /usr/include and /usr/local/include anyway, so there is 
> no way to add that.
> Adding the NO_SYSTEM_ENVIRONMENT_PATH stops it from reading the PATH 
> environment variable and looking
> for tut/tut.hpp, which probably won't harm, but it simply makes no sense: 
> we're looking for a headerfile, not an executable.
> 
> Without this patch (and without creating a fake /usr/local/include/tut.h), I 
> get the error:
> CMake Error at cmake/FindTut.cmake:26 (message):
>   Could not find Tut
> Call Stack (most recent call first):
>   cmake/Tut.cmake:8 (include)
>   llmessage/CMakeLists.txt:13 (include)
> 
> With the patch, it finds /usr/local/include/tut/tut.hpp as expected, setting 
> TUT_INCLUDE_DIR to "/usr/local/include",
> and it also finds (in my case) 
> /usr/src/secondlife/viewers/snowstorm/viewer-development/include/tut/tut.hpp, 
> setting
> TUT_INCLUDE_DIR to 
> "/usr/src/secondlife/viewers/snowstorm/viewer-development/include", since I 
> have a symbolic
> link from 
> /usr/src/secondlife/viewers/snowstorm/viewer-development/include/tut to 
> ../linden/libraries/include/tut
> (which was installed manually with ./scripts/install.py tut) and I have the 
> environment variable CMAKE_INCLUDE_PATH
> set to 
> "/usr/src/secondlife/llqtwebkit/install-imprudence/include:/usr/src/secondlife/viewers/snowstorm/viewer-development/include:/sl/usr/include".
> In other words, this allows developers to install headers whereever they want 
> and use the API of cmake as it is
> intended, to find those headers.
> 
> 
> This addresses bug VWR-24247.
> http://jira.secondlife.com/browse/VWR-24247
> 
> 
> Diffs
> -
> 
>   indra/cmake/FindTut.cmake b0689af42a71 
> 
> Diff: http://codereview.secondlife.com/r/39/diff
> 
> 
> Testing
> ---
> 
> Tested to see if it finds the header when installed in /usr/local/include as 
> well in a path specified
> with CMAKE_INCLUDE_PATH.
> 
> Note that you need to configure with --standalone in order to test this.
> Also note that unless tut is installed in /usr/local, and when you configure 
> with -DLL_TESTS:BOOL=ON,
> it still won't compile because of another bug. I will submit a patch for that 
> separately.
> 
> 
> Thanks,
> 
> Aleric
> 
>

___
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-24254: Add support for using ld.gold on linux.

[Boroondas Gupte]

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



indra/cmake/LLCommon.cmake


Is this removal related to ld.gold, too, or just general cleanup?


- Boroondas


On 2010-12-19 09:42:53, Aleric Inglewood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/48/
> ---
> 
> (Updated 2010-12-19 09:42:53)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> To use ld.gold configure with:
>   -DCMAKE_EXE_LINKER_FLAGS:STRING="-Wl,-use-gold".
> 
> See VWR-24254 for more details.
> 
> 
> This addresses bug VWR-24254.
> http://jira.secondlife.com/browse/VWR-24254
> 
> 
> Diffs
> -
> 
>   indra/cmake/BerkeleyDB.cmake b0689af42a71 
>   indra/cmake/LLCommon.cmake b0689af42a71 
>   indra/cmake/LLPlugin.cmake b0689af42a71 
>   indra/llwindow/CMakeLists.txt b0689af42a71 
> 
> Diff: http://codereview.secondlife.com/r/48/diff
> 
> 
> Testing
> ---
> 
> ld.gold links the viewer on my machine in 8 seconds, as
> opposed to 19 seconds with ld.bfd. Moreover, it uses a
> LOT less memory during linking (about 750 MB instead of
> 2.5 GB! (for viewer 1)).
> 
> Since my machine locked up hard when I run out of memory
> (something with using an encrypted RAID for my swap),
> and compiling viewer 2 uses more than 3 GB, I couldn't
> compile viewer 2 at all anymore without this patch (and
> using ld.gold). And OMG this is fast!
> 
> 
> Thanks,
> 
> Aleric
> 
>

___
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-10579: Fix NDOF.cmake to do the right thing on standalone.

[Boroondas Gupte]

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


I'd prefer the following behavior: Let the user specify whether to use 
libndofdev or not. (Of course we'd have to agree on a default if nothing was 
specified). Then ...

... if libndofdev use has been enabled and libndofdev can be found,
  --> build with libndofdev support, thus with the joystick/spacenavigator 
features.

... if libndofdev use has been enabled but libndofdev can not be found,
  --> error out. (With some meaningful message)

... if libndofdev use has been disabled,
  --> don't build with libndofdev support, even if libndofdev is present.

Rationale:
If I *want* spacenavigator or joystick support, but libndofdev cannot be found 
(e.g. because I forgot to install it or because it was installed to a location 
that isn't searched), I'll get a heads-up at build time rather than having to 
figure out why stuff doesn't behave the way I want at runtime.

Note:
This would be analogous to how we currently handle FMOD, even although 
disabling FMOD -- other than disabling libndofdev -- doesn't result in 
unavailable viewer features.

- Boroondas


On 2010-12-19 07:54:19, Aleric Inglewood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/45/
> ---
> 
> (Updated 2010-12-19 07:54:19)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Only define -DLIB_NDOF=1 when NDOF is actually found, on standalone. While 
> FindNDOF.cmake was added to the repository, it wasn't used.
> 
> 
> This addresses bug VWR-10579.
> http://jira.secondlife.com/browse/VWR-10579
> 
> 
> Diffs
> -
> 
>   indra/cmake/NDOF.cmake b0689af42a71 
> 
> Diff: http://codereview.secondlife.com/r/45/diff
> 
> 
> Testing
> ---
> 
> I can't remember when I started using this, but it was long ago. I don't have 
> libndofdev installed.
> I just installed Michelle's debian package libndofdev-dev and then it found 
> it:
> -- Found NDOF: Library in '/usr/lib/libndofdev.so' and header in 
> '/usr/include' 
> 
> 
> Thanks,
> 
> Aleric
> 
>

___
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: SNOW-240: Fix libjson naming madness, for standalone.

[Boroondas Gupte]

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

Ship it!


Looks good.

Like this, it should continue working even if a system-wide installed libjson 
uses the compiler-version-specific filename, like the gentoo ebuild from 
Techwolf's portage overlay currently does. I guess all other distributions use 
the plain name, so once this ebuild has been updated, we can even drop looking 
for the compiler version specific filename in the standalone case completely.

- Boroondas


On 2010-12-19 09:19:37, Aleric Inglewood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/47/
> ---
> 
> (Updated 2010-12-19 09:19:37)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> On linux (and remember this is about standalone)
> the libjson packages of distributions don't have this
> complex compiler version baked into their name.
> 
> This patch fixes this issue by first searching for
> libjson_linux-gcc-${_gcc_COMPILER_VERSION}_libmt.so
> and when that fails search for the system package
> library file libjson.so.
> 
> 
> This addresses bug SNOW-240.
> http://jira.secondlife.com/browse/SNOW-240
> 
> 
> Diffs
> -
> 
>   indra/cmake/FindJsonCpp.cmake b0689af42a71 
> 
> Diff: http://codereview.secondlife.com/r/47/diff
> 
> 
> Testing
> ---
> 
> It works :p (I have Michelle's debian package libjsoncpp0 installed, which 
> provides /usr/lib/libjson.so).
> 
> 
> Thanks,
> 
> Aleric
> 
>

___
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: SNOW-240: Fix libjson naming madness, for standalone.

[Boroondas Gupte]

> On 2010-12-19 12:48:35, Boroondas Gupte wrote:
> > Looks good.
> > 
> > Like this, it should continue working even if a system-wide installed 
> > libjson uses the compiler-version-specific filename, like the gentoo ebuild 
> > from Techwolf's portage overlay currently does. I guess all other 
> > distributions use the plain name, so once this ebuild has been updated, we 
> > can even drop looking for the compiler version specific filename in the 
> > standalone case completely.

Just wondering: Is there some reason behind the search order 
(libjson_linux-gcc-${_gcc_COMPILER_VERSION}_libmt.so first, then libjson.so) or 
is that arbitrary? (I guess the systems where both are present are rare.)


- Boroondas


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


On 2010-12-19 09:19:37, Aleric Inglewood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/47/
> ---
> 
> (Updated 2010-12-19 09:19:37)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> On linux (and remember this is about standalone)
> the libjson packages of distributions don't have this
> complex compiler version baked into their name.
> 
> This patch fixes this issue by first searching for
> libjson_linux-gcc-${_gcc_COMPILER_VERSION}_libmt.so
> and when that fails search for the system package
> library file libjson.so.
> 
> 
> This addresses bug SNOW-240.
> http://jira.secondlife.com/browse/SNOW-240
> 
> 
> Diffs
> -
> 
>   indra/cmake/FindJsonCpp.cmake b0689af42a71 
> 
> Diff: http://codereview.secondlife.com/r/47/diff
> 
> 
> Testing
> ---
> 
> It works :p (I have Michelle's debian package libjsoncpp0 installed, which 
> provides /usr/lib/libjson.so).
> 
> 
> Thanks,
> 
> Aleric
> 
>

___
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] Indicating privacy implications of preference settings (was: STORM-34 Test Binaries)

[Boroondas Gupte]

On 12/28/2010 09:40 PM, Celierra Darling wrote:
> It seems a little unclear to try to communicate "this can have privacy
> implications" by putting the setting on the privacy tab.  It might be
> better to write the setting label so it's more explicit (i.e.
> something like "Show my favorites to anyone under 'Start At' before
> logging in" or "Show my favorites under 'Start At' on login (before
> authenticating)" or etc.).
I have to agree that placing settings on the "privacy" tab is not a good
way to indicate the fact that they have privacy implications. It goes
against the principle that a setting should be placed where users expect
it. I.e., if you know (or suspect) a setting exists, but don't know
where to find it, where would you first look for it? Education about the
consequences of changing the setting can be postponed until after you
found it.

Of course, besides looking at tab labels, placing similar or related
settings together can help in finding them. But the common property
"affects privacy" seems to me too week a reason for placing settings on
the same tab, when that property is not the goal of a setting but rather
a side effect (even although a side effect inherent to the goal itself,
not just to the setting's implementation).

Further, when a user finds a setting on the "privacy" tab *and* realizes
this placement means the setting has privacy implications, how would she
or he know whether enabling or disabling the setting grants higher
privacy? Of course, if one can derive from the description of a setting
what it actually does and then thinks it all through, one gets the
answer as well as how one's privacy would be affected. While the users
are probably well capable to do that, do they want to do that---when
they could just as well be told openly about the setting's effect, e.g.
like in Celierra's labeling proposals above?

Boroondas
___
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: Reenable the LLMatrix3::orthogonalize test in llmath/tests/m3math_test.cpp

[Boroondas Gupte]

> On 2010-12-28 10:10:54, Aleric Inglewood wrote:
> > I think this line should be deleted, not commented out. If there still is a 
> > reason to think that it might need to be uncommented later than apparently 
> > we aren't sure the test really works, in which case I think it should be 
> > skipped until we are certain and this is fixed on all platforms.

I agree: Delete the line, don't just comment it out. The fact that it was once 
there will be preserved in the revision history, which should be sufficient.


- Boroondas


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


On 2010-12-28 10:05:04, Wolfpup Lowenhar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/67/
> ---
> 
> (Updated 2010-12-28 10:05:04)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> This patch is to fix this test is working in Windows environments
> 
> 
> This addresses bug https://jira.secondlife.com/browse/VWR-24332.
> 
> http://jira.secondlife.com/browse/https://jira.secondlife.com/browse/VWR-24332
> 
> 
> Diffs
> -
> 
>   indra/llmath/tests/m3math_test.cpp 940cd25d4b78 
> 
> Diff: http://codereview.secondlife.com/r/67/diff
> 
> 
> Testing
> ---
> 
> Re-enabled the test and built the viewer including all tests and have no 
> errors and all of the test done in m3math_test are reporting that they all 
> succeed. 
> 
> 
> Thanks,
> 
> Wolfpup
> 
>

___
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: Constraints in XUI files don't match the constraints imposed elsewhere in the viewer/server code.

[Boroondas Gupte]

> On 2010-12-24 11:06:43, Vadim ProductEngine wrote:
> > How do you know the server constraints?

Signpost already answered a similar question on jira: 
https://jira.secondlife.com/browse/VWR-24197?focusedCommentId=228070&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-228070


- Boroondas


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


On 2010-12-22 12:17:10, SignpostMarv Martin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/57/
> ---
> 
> (Updated 2010-12-22 12:17:10)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Constraints in XUI files don't match the constraints imposed elsewhere in the 
> viewer/server code.
> 
> Don't know where the constraints are specified, but this XUI mod lets the 
> user play within the full range of the actual constraints.
> 
> JIRA page has screenshots of before & after.
> 
> 
> This addresses bug VWR-24197.
> http://jira.secondlife.com/browse/VWR-24197
> 
> 
> Diffs
> -
> 
>   indra/newview/skins/default/xui/en/floater_tools.xml UNKNOWN 
> 
> Diff: http://codereview.secondlife.com/r/57/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> SignpostMarv
> 
>

___
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] Tab button no longer works

[Boroondas Gupte]

On 12/30/2010 04:54 AM, tinselsilv...@frontiernet.net wrote:
> My tab button has not worked in the last two Dev versions 217861 & 217920. 
> Did I miss a post or Jira regarding this issue?
STORM-823 

> The lack of a tab button makes building and teleporting to exact coordinates 
> more difficult.
Indeed.

> I tend to tweak my Debug Settings for personal preferences. Did I 
> inadvertently disable the tab button?
Unlikely. I wouldn't know of any setting that would do that.

> Any advice on how to correct would be most appreciated. Thanks.
I guess there isn't much you can do besides waiting until it's fixed
and/or use a version that isn't affected. (Except of course if you're
able to fix it yourself, in which case a changeset would be most welcome.)

Cheers,
Boroondas
___
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] FMOD or not (was: Debian Squeeze compiling)

[Boroondas Gupte]

On 12/29/2010 04:21 PM, Duncan Bradders wrote:
> Hello, I'm trying to compile the latest viewer development on my
> distro, the envoronment it's the same for other viewers and usually I
> can compile TPV V1 viewers.
>
> dbradd...@casa:~/Src/viewer-development/indra$ ./develop.py
> --type=Release configure 2>&1 | tee build.log
>
> then:
>
> dbradd...@casa:~/Src/viewer-development/indra$ ./develop.py
> --type=Release build 2>&1 | tee -a build.log
>
> It tell me fmod.h is missing but it's not. Where I'm wrong?
Maybe that fmod.h is just somewhere else than the build system expects
it? STORM-406  probably
changed the location where the FMOD files have to be placed.

> many thanks in advance for any hint...
Since quite some time, you can build the Viewer without FMOD and have
sound anyway (OpenAL will be used instead). That's why the instructions
for where to place the FMOD files have been removed from the linux build
documentation
.
However, since STORM-406 
one has to explicitly disable the FMOD option (e.g. by configuring with
-DFMOD=OFF) to be able to build when FMOD cannot be found, which still
isn't properly documented, yet.

Cheers,
Boroondas
___
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...

[Boroondas Gupte]

On 01/04/2011 07:07 AM, Anya Kanevsky wrote:
> Ponzu, I'm understandably very interested in finding out which xml
> files ... Thanks!
> Getting IMs from people telling me to lay off other people I don't
> know and have never talked to is getting kind of old. 
> [...]
>
> 2010/12/31 Ponzu mailto:lee.po...@gmail.com>>
>
> I just thought of looking in the xml files, and indeed "Grumpity
> Productengine" shows up in a few of them, I assume as some sort of
> place holder left by a programmer.
>
> Maybe such "magic words" should be replace by something like
> "Unkown Resident" or some such.
>
*indra/newview/skins/default/xui/en/inspect_avatar.xml*
46: value="Grumpity ProductEngine with a long name"
57:value="Grumpity ProductEngine"

*indra/newview/skins/default/xui/en/panel_activeim_row.xml*
79:Grumpity ProductEngine

*indra/newview/skins/default/xui/en/inspect_group.xml*
34:Grumpity's Grumpy Group of Moose

*indra/newview/skins/default/xui/fr/inspect_avatar.xml*
13: 
14: 

*indra/newview/skins/default/xui/fr/panel_activeim_row.xml*
4:  Grumpity ProductEngine

*indra/newview/skins/default/xui/fr/inspect_group.xml*
20: Groupe grognon des Orignaux Grumpity

*indra/newview/skins/default/xui/de/inspect_avatar.xml*
14: 

*indra/newview/skins/default/xui/de/panel_activeim_row.xml*
4:  Grumpity ProductEngine

*indra/newview/skins/default/xui/de/inspect_group.xml*
20: Grumpitys schlecht gelaunte Elche

*indra/newview/skins/default/xui/es/inspect_avatar.xml*
13: 

*indra/newview/skins/default/xui/da/inspect_avatar.xml*
13: 

*indra/newview/skins/default/xui/pt/inspect_avatar.xml*
13: 

*indra/newview/skins/default/xui/ja/inspect_avatar.xml*
13: 

*indra/newview/skins/default/xui/ja/panel_activeim_row.xml*
4:  Grumpity ProductEngine

*indra/newview/skins/default/xui/ja/inspect_group.xml*
20: Grumpity's Grumpy Group of Moose

*indra/newview/skins/default/xui/pl/inspect_avatar.xml*
13: 

Extracted from current viewer-development

by ack  --xml "Grumpity".

Cheers,
Boroondas
___
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

[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

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

[Boroondas Gupte]

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


Still CRLFs in indra/newview/skins/default/xui/en/floater_web_content.xml , 
which isn't windows specific, I think.
Otherwise https://bitbucket.org/oz_linden/storm-826/changeset/6e6d1de23cce 
looks good.

I'm wondering whether we should also fix indentation on lines that are touched 
by this change anyway. (E.g. floater_web_content.xml mixes spaces and tabs.)

- Boroondas


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-844 "More" should be "Less" when Media Control is open

[Boroondas Gupte]

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



doc/contributions.txt


Sorting issue numbers in doc/contributions.txt is a Good Thing(TM), but 
unrelated to the issue at hand, thus should happen in a separate commit.


- Boroondas


On Jan. 12, 2011, 6:02 a.m., Jonathan Yap wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/78/
> ---
> 
> (Updated Jan. 12, 2011, 6:02 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> "More" should be "Less" when Media Control is open
> 
> This is a trivial text change in an xml file.  The reason I am posting this 
> here is due to some confusion using label_selected.  In this case having it 
> set to a different value than when label is set to seems to have no effect, 
> so I have made them identical.
> 
> I scanned all the xml files and there are only about 5 places where 
> label_selected is different from the preceding label= value.
> 
> Is there any reason to revert back to having them set to different values?
> i.e. label="More" and label_selected="Less"
> 
> 
> This addresses bug storm-844.
> http://jira.secondlife.com/browse/storm-844
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt 179e0754a27d 
>   indra/newview/skins/default/xui/en/panel_nearby_media.xml 179e0754a27d 
> 
> Diff: http://codereview.secondlife.com/r/78/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] Clarification on contributions.txt

[Boroondas Gupte]

[Sent from wrong address, so once again for the list. Sorry Oz for the
duplicate message.]

On 01/13/2011 06:05 PM, Oz Linden (Scott Lawrence) wrote:
> On 2011-01-13 11:37, Jonathan Welch wrote:
>> Hi Oz,
>>
>> I updated two wiki entries dealing with adding to contributions.txt
>> and would appreciate it if you could look over what I wrote and tell
>> me and the mailing list if this is what you would like to have done.
>>
>> http://wiki.secondlife.com/w/index.php?title=Preparing_Code&diff=1131640&oldid=1028212
> Thank you, Jonathan... you were essentially correct and that's a good 
> addition.  I tweaked what you wrote slightly.
Should new entries really always be added below existing ones of the
same person, or could they be added at the 'right' position (if any), as
long as existing entries aren't moved around nor removed?

E.g., if we have

Example Resident
FOO-1000
FOO-3000

and Example Resident implements FOO-2000, should that become

Example Resident
FOO-1000
*   FOO-2000*
FOO-3000

(i.e. added at "right" position) or

Example Resident
FOO-1000
FOO-3000
*   FOO-2000*

(i.e. added below existing entries)?

Also, could you make your metric-generating code public? Maybe someone
can adapt it to not be mislead by re-sorting of issue entries.

Cheers,
Boroondas
___
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-24311: Uninstall packages that are renewed.

[Boroondas Gupte]

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



scripts/install.py


I think for multiple return values, packaging in tuples is preferred over 
packaging in lists.[citation needed] I.e., use

return (to_install, to_uninstall)



scripts/install.py


This can be written more pythonesque as

(to_install, to_uninstall) = self._build_ifiles(platform, cache_dir)



scripts/install.py


Does to_install need filtering at all? (Can it contain non-installables?)


- Boroondas


On Jan. 14, 2011, 12:26 p.m., Aleric Inglewood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/80/
> ---
> 
> (Updated Jan. 14, 2011, 12:26 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> See https://jira.secondlife.com/browse/VWR-24311
> 
> Basically, this fixes the TODO comment in install.py but with the difference 
> that we really want to uninstall any old package with the same name, 
> different md5 or not.
> 
> 
> This addresses bug VWR-24311.
> http://jira.secondlife.com/browse/VWR-24311
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt b0bd26c5638a 
>   scripts/install.py b0bd26c5638a 
> 
> Diff: http://codereview.secondlife.com/r/80/diff
> 
> 
> Testing
> ---
> 
> Loads of testing on linux... Installing new packages now cleanly removes the 
> old one first.
> 
> 
> Thanks,
> 
> Aleric
> 
>

___
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-24312: Massively duplicated objects (part 2)

[Boroondas Gupte]

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



indra/llcharacter/llanimationstates.cpp


If all these lines are touched anyway, (didn't select all, to avoid an 
unnecessary long review), please either remove the alignment or use spaces 
instead of tabs for aligning, so they will look nice independent of tab display 
length.



indra/llcommon/lllslconstants.h


Yay for having type modifiers after the core type name. Makes them much 
easier to understand, especially when there are several cascaded ones. :-)


- Boroondas


On Jan. 14, 2011, 12:34 p.m., Aleric Inglewood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/81/
> ---
> 
> (Updated Jan. 14, 2011, 12:34 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Turns out that most of my SNOW-800 patch was included in Viewer 2 (albeit 
> without crediting me).
> However, not everything was used and some more cleaning up was possible.
> 
> After this patch, and when compiling with optimization, there are no 
> duplicates left
> anymore that shouldn't be there in the first place: apart from the debug 
> stream
> iostream guard variable, there are several static variables with the same 
> name (r, r1,
> r2, etc) but that indeed actually different symbol objects. Then there are a 
> few
> constant POD arrays that are duplicated a hand full of times because they are
> accessed with a variable index (so optimizing them away is not possible). I 
> left them
> like that (although defining those as extern as well would have been more 
> consistent
> and not slower; in fact it would be faster theoretically because those arrays 
> could
> share the same cache page then). 
> 
> 
> This addresses bug VWR-24312.
> http://jira.secondlife.com/browse/VWR-24312
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt b0bd26c5638a 
>   indra/llcharacter/llanimationstates.cpp b0bd26c5638a 
>   indra/llcommon/llavatarconstants.h b0bd26c5638a 
>   indra/llcommon/lllslconstants.h b0bd26c5638a 
>   indra/llcommon/llmetricperformancetester.h b0bd26c5638a 
>   indra/llmath/llcamera.h b0bd26c5638a 
>   indra/llmath/llcamera.cpp b0bd26c5638a 
>   indra/newview/llviewerobject.cpp b0bd26c5638a 
>   indra/newview/llvoavatar.cpp b0bd26c5638a 
>   indra/newview/llvosky.h b0bd26c5638a 
>   indra/newview/llvosky.cpp b0bd26c5638a 
> 
> Diff: http://codereview.secondlife.com/r/81/diff
> 
> 
> Testing
> ---
> 
> Compiled with optimization and then running readelf on the executable to find 
> duplicated symbols.
> 
> 
> Thanks,
> 
> Aleric
> 
>

___
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-24317: Incorrect start up warnings: WARNING: ll_apr_warn_status: APR: No such file or directory

[Boroondas Gupte]

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

Ship it!


Looks good. Files should only be removed when they actually exists, which this 
change accomplishes.


Just wondering:


indra/newview/llappviewer.cpp






indra/newview/llappviewer.cpp






indra/newview/llappviewer.cpp


what's the reason for moving the LL_INFOS around?


- Boroondas


On Jan. 14, 2011, 12:48 p.m., Aleric Inglewood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/83/
> ---
> 
> (Updated Jan. 14, 2011, 12:48 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> At start up one can get the following “warnings”:
> 
> 2010-10-23T12:22:44Z WARNING: ll_apr_warn_status: APR: No such file or 
> directory
> 2010-10-23T12:22:44Z WARNING: remove: Attempting to remove filename: 
> /home/aleric/.imprudence/logs/Imprudence.logout_marker
> 2010-10-23T12:22:44Z WARNING: ll_apr_warn_status: APR: No such file or 
> directory
> 2010-10-23T12:22:44Z WARNING: remove: Attempting to remove filename: 
> /home/aleric/.imprudence/logs/Imprudence.llerror_marker
> 2010-10-23T12:22:44Z WARNING: ll_apr_warn_status: APR: No such file or 
> directory
> 2010-10-23T12:22:44Z WARNING: remove: Attempting to remove filename: 
> /home/aleric/.imprudence/logs/Imprudence.error_marker
> 
> This is nonsense since it is perfectly ok when those files don’t exist. 
> 
> 
> This addresses bug VWR-24317.
> http://jira.secondlife.com/browse/VWR-24317
> 
> 
> Diffs
> -
> 
>   indra/newview/llappviewer.cpp b0bd26c5638a 
> 
> Diff: http://codereview.secondlife.com/r/83/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Aleric
> 
>

___
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-24317: Incorrect start up warnings: WARNING: remove: Attempting to remove filename: /ramdisk/imprudence/cache/textures/*/*.texture

[Boroondas Gupte]

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



indra/newview/lltexturecache.cpp


As this will sometimes stay true, even when the file doesn't exist, 
file_must_be_removed or delete_the_file might be better names. (Maybe you can 
think off even better ones.)



indra/newview/lltexturecache.cpp


Make this
  LL_WARNS("TextureCache") << "Entry has a body size of 
zero but file " << filename << " exists. This file will be deleted, too." << 
LL_ENDL;
or similar.


- Boroondas


On Jan. 14, 2011, 12:50 p.m., Aleric Inglewood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/84/
> ---
> 
> (Updated Jan. 14, 2011, 12:50 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> This turned out to be a simple matter of trying to remove non-existant files:
> A texture with a 'body size' of 0 doesn't have a texture body file.
> 
> 
> This addresses bug VWR-24317.
> http://jira.secondlife.com/browse/VWR-24317
> 
> 
> Diffs
> -
> 
>   indra/newview/lltexturecache.cpp b0bd26c5638a 
> 
> Diff: http://codereview.secondlife.com/r/84/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Aleric
> 
>

___
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-24321: Validate textures starting with 00 too.

[Boroondas Gupte]

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

Ship it!


Correct.

- Boroondas


On Jan. 14, 2011, 1:02 p.m., Aleric Inglewood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/90/
> ---
> 
> (Updated Jan. 14, 2011, 1:02 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Trivial patch, just removes stupidity.
> 
> 
> This addresses bug VWR-24321.
> http://jira.secondlife.com/browse/VWR-24321
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt b0bd26c5638a 
>   indra/newview/lltexturecache.cpp b0bd26c5638a 
> 
> Diff: http://codereview.secondlife.com/r/90/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Aleric
> 
>

___
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-24366: CMAKE_EXE_LINKER_FLAGS not honored when linking the viewer binary if -DLL_TESTS:BOOL=ON

[Boroondas Gupte]

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

Ship it!


Looks sane and your explanation makes sense.

- Boroondas


On Jan. 14, 2011, 1:15 p.m., Aleric Inglewood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/95/
> ---
> 
> (Updated Jan. 14, 2011, 1:15 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Setting CMAKE_EXE_LINKER_FLAGS to "" because the tests "need" that is pretty
> hard measure. Not only is it not necessary to do so, it also changes how
> the viewer is linked depending on a whether or not the tests are compiled
> and that is not good.
> 
> The reason that this was needed is that libgmock is underlinked
> (see http://wiki.mandriva.com/en/Underlinking), which is not compatible
> with -Wl,--as-needed that is being used on linux. libgmock.so.0 needs
> a symbol that is defined in libgtest.so.o, but -lgtest was not passed
> to the linker when creating libgmock.so.0:
> 
> Underlinked (no libgtest.so.o):
> $ objdump -p /usr/lib/libgmock.so.0 | grep NEEDED
> NEEDED libstdc++.so.6
> NEEDED libm.so.6
> NEEDED libc.so.6
> NEEDED libgcc_s.so.1
> 
> The solution is to wrap between -Wl,--no-as-needed -lgtest -Wl,--as-needed
> causing it to be added again. This is only needed on linux, since that
> the only platform that we use -Wl,--as-needed on. Moreover, we can just
> set GOOGLEMOCK_LIBRARIES to "gmock -Wl,--no-as-needed gtest -Wl,--as-needed"
> since that is only passed to TARGET_LINK_LIBRARIES which only adds -l
> in front of 'things' that don't start with '-', to allow you do pass
> special flags like this.
> 
> 
> This addresses bug VWR-24366.
> http://jira.secondlife.com/browse/VWR-24366
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt 422f636c3343 
>   indra/cmake/GoogleMock.cmake 422f636c3343 
>   indra/cmake/LLAddBuildTest.cmake 422f636c3343 
> 
> Diff: http://codereview.secondlife.com/r/95/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Aleric
> 
>

___
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] Link times

[Boroondas Gupte]

On 01/14/2011 06:04 PM, Jonathan Welch wrote:
> I just did a quick study on link times for various viewers on my 2Gb XP system
>
> Viewer  1st link  2nd link
> CV 1.220:53  0:24
> SG 1.5  3:30  2:07
> V2.5 6:18  6:01
The long time for 2.5 be due to VWR-24366
. Dunno why linking Cool
Viewer (that's what "CV" stands for, isn't it?) is so much quicker than
SG 1.5, though.

Good news: A fix is being reviewed at
https://codereview.secondlife.com/r/95/

Cheers,
Boroondas
___
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-24311: Uninstall packages that are renewed.

[Boroondas Gupte]

> On Jan. 14, 2011, 1:16 p.m., Boroondas Gupte wrote:
> > scripts/install.py, line 598
> > <http://codereview.secondlife.com/r/80/diff/1/?file=388#file388line598>
> >
> > Does to_install need filtering at all? (Can it contain 
> > non-installables?)

Reading the docstring of install() , I realized this filtering *is* necessary, 
or all out-of-date installables would be uninstalled (and only the requested 
ones re-installed in new versions).

I originally was mistaken on what "installables" is. To make the code less 
misleading, I suggest renaming that function argument to something like 
"requested_installables".


- Boroondas


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


On Jan. 14, 2011, 12:26 p.m., Aleric Inglewood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/80/
> ---
> 
> (Updated Jan. 14, 2011, 12:26 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> See https://jira.secondlife.com/browse/VWR-24311
> 
> Basically, this fixes the TODO comment in install.py but with the difference 
> that we really want to uninstall any old package with the same name, 
> different md5 or not.
> 
> 
> This addresses bug VWR-24311.
> http://jira.secondlife.com/browse/VWR-24311
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt b0bd26c5638a 
>   scripts/install.py b0bd26c5638a 
> 
> Diff: http://codereview.secondlife.com/r/80/diff
> 
> 
> Testing
> ---
> 
> Loads of testing on linux... Installing new packages now cleanly removes the 
> old one first.
> 
> 
> Thanks,
> 
> Aleric
> 
>

___
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-24311: Uninstall packages that are renewed.

[Boroondas Gupte]

> On Jan. 14, 2011, 1:16 p.m., Boroondas Gupte wrote:
> > scripts/install.py, lines 592-594
> > <http://codereview.secondlife.com/r/80/diff/1/?file=388#file388line592>
> >
> > This can be written more pythonesque as
> > 
> > (to_install, to_uninstall) = self._build_ifiles(platform, 
> > cache_dir)

Here, just
to_install, to_uninstall = self._build_ifiles(platform, cache_dir)

which again constructs a tuple (now on the left hand side to receive the values 
of the returned tuple). Again, omitting the parentheses stresses that the 
returned tuple just serves as a temporary vehicle for the two values. This is 
also the prevalent style for unpacking multiple return values in the existing 
python code from the viewer codebase. (The style in functions for returning 
multiple values is less homogeneous. But the use of tuples for that purpose, 
with or without parentheses, seems established.)


> On Jan. 14, 2011, 1:16 p.m., Boroondas Gupte wrote:
> > scripts/install.py, line 549
> > <http://codereview.secondlife.com/r/80/diff/1/?file=388#file388line549>
> >
> > I think for multiple return values, packaging in tuples is preferred 
> > over packaging in lists.[citation needed] I.e., use
> > 
> > return (to_install, to_uninstall)

Actually, make that just
return to_install, to_uninstall

The comma is enough for constructing the tuple, the parentheses aren't needed 
(in this context), and leaving them away IMHO makes more clear that we aren't 
really interested in the tuple as container, but just use it to return multiple 
values at once.


- Boroondas


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


On Jan. 14, 2011, 12:26 p.m., Aleric Inglewood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/80/
> ---
> 
> (Updated Jan. 14, 2011, 12:26 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> See https://jira.secondlife.com/browse/VWR-24311
> 
> Basically, this fixes the TODO comment in install.py but with the difference 
> that we really want to uninstall any old package with the same name, 
> different md5 or not.
> 
> 
> This addresses bug VWR-24311.
> http://jira.secondlife.com/browse/VWR-24311
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt b0bd26c5638a 
>   scripts/install.py b0bd26c5638a 
> 
> Diff: http://codereview.secondlife.com/r/80/diff
> 
> 
> Testing
> ---
> 
> Loads of testing on linux... Installing new packages now cleanly removes the 
> old one first.
> 
> 
> Thanks,
> 
> Aleric
> 
>

___
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-24311: Uninstall packages that are renewed.

[Boroondas Gupte]

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

Ship it!



scripts/install.py


Please add a comment "regular directories (no symlinks)" ...



scripts/install.py


... and here a comment "regular files and symlinks", so that the intention 
behind these conditionals is clear to people unfamiliar with the used os.path 
functions.


Other than that, I'd say it's ready. Great work!

- Boroondas


On Jan. 16, 2011, 5:35 a.m., Aleric Inglewood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/80/
> ---
> 
> (Updated Jan. 16, 2011, 5:35 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> See https://jira.secondlife.com/browse/VWR-24311
> 
> Basically, this fixes the TODO comment in install.py but with the difference 
> that we really want to uninstall any old package with the same name, 
> different md5 or not.
> 
> 
> This addresses bug VWR-24311.
> http://jira.secondlife.com/browse/VWR-24311
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt 422f636c3343 
>   scripts/install.py 422f636c3343 
> 
> Diff: http://codereview.secondlife.com/r/80/diff
> 
> 
> Testing
> ---
> 
> Loads of testing on linux... Installing new packages now cleanly removes the 
> old one first.
> 
> 
> Thanks,
> 
> Aleric
> 
>

___
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-24317: Incorrect start up warnings: WARNING: ll_apr_warn_status: APR: No such file or directory

[Boroondas Gupte]

> On Jan. 14, 2011, 1:47 p.m., Boroondas Gupte wrote:
> > indra/newview/llappviewer.cpp, lines 3091-3094
> > <http://codereview.secondlife.com/r/83/diff/1/?file=402#file402line3091>
> >
> > what's the reason for moving the LL_INFOS around?
> 
> Aleric Inglewood wrote:
> The last two, in order to print the correct value that gLastExecEvent is 
> being set to: depending on the conditional, the value is set to what was 
> printed, or to another value. The first hunk to have more symmetric code and 
> treat that part the same as the others: first set the variable and then print 
> it's contents.

Makes sense.


- Boroondas


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


On Jan. 14, 2011, 12:48 p.m., Aleric Inglewood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/83/
> ---
> 
> (Updated Jan. 14, 2011, 12:48 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> At start up one can get the following “warnings”:
> 
> 2010-10-23T12:22:44Z WARNING: ll_apr_warn_status: APR: No such file or 
> directory
> 2010-10-23T12:22:44Z WARNING: remove: Attempting to remove filename: 
> /home/aleric/.imprudence/logs/Imprudence.logout_marker
> 2010-10-23T12:22:44Z WARNING: ll_apr_warn_status: APR: No such file or 
> directory
> 2010-10-23T12:22:44Z WARNING: remove: Attempting to remove filename: 
> /home/aleric/.imprudence/logs/Imprudence.llerror_marker
> 2010-10-23T12:22:44Z WARNING: ll_apr_warn_status: APR: No such file or 
> directory
> 2010-10-23T12:22:44Z WARNING: remove: Attempting to remove filename: 
> /home/aleric/.imprudence/logs/Imprudence.error_marker
> 
> This is nonsense since it is perfectly ok when those files don’t exist. 
> 
> 
> This addresses bug VWR-24317.
> http://jira.secondlife.com/browse/VWR-24317
> 
> 
> Diffs
> -
> 
>   indra/newview/llappviewer.cpp b0bd26c5638a 
> 
> Diff: http://codereview.secondlife.com/r/83/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Aleric
> 
>

___
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-24311: Uninstall packages that are renewed.

[Boroondas Gupte]

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

Ship it!


- Boroondas


On Jan. 17, 2011, 5:24 a.m., Aleric Inglewood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/80/
> ---
> 
> (Updated Jan. 17, 2011, 5:24 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> See https://jira.secondlife.com/browse/VWR-24311
> 
> Basically, this fixes the TODO comment in install.py but with the difference 
> that we really want to uninstall any old package with the same name, 
> different md5 or not.
> 
> 
> This addresses bug VWR-24311.
> http://jira.secondlife.com/browse/VWR-24311
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt 422f636c3343 
>   scripts/install.py 422f636c3343 
> 
> Diff: http://codereview.secondlife.com/r/80/diff
> 
> 
> Testing
> ---
> 
> Loads of testing on linux... Installing new packages now cleanly removes the 
> old one first.
> 
> 
> Thanks,
> 
> Aleric
> 
>

___
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: VWR-24520: Don't use pkg_check_modules( ... QUIET ) on CMake < 2.8.2

[Boroondas Gupte]

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

Review request for Viewer.


Summary
---

Only use QUIET in pkg_check_modules() on CMake >=2.8.2 (where it's supported) 
rather than already on CMake >=2.8.


This addresses bug VWR-24520.
http://jira.secondlife.com/browse/VWR-24520


Diffs
-

  doc/contributions.txt 9e99b2c8fb28 
  indra/cmake/FindLLQtWebkit.cmake 9e99b2c8fb28 

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


Testing
---

Configured (standalone) without a .pgk file for libllqtwebkit on Linux with 
CMake 2.8.1 and CMake 2.8.3. Output as expected.

Not tested:
* CMake 2.8.2
* system with a .pgk file for libllqtwebkit
* non-standalone
* Mac, Win


Thanks,

Boroondas

___
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: make PREHASH variables char const* const

[Boroondas Gupte]

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

Review request for Viewer.


Summary
---

For the reason for this change, see 
https://jira.secondlife.com/browse/VWR-24487 and 
https://jira.secondlife.com/browse/VWR-24522

What I did:
In indra/llmessage/message_prehash.(h|cpp), I turned everything into constant 
pointers to constants by search/replace. Then I tried to compile and added 
const qualifiers in dependent code as needed to stop the compiler complaining.

Note that comments in indra/llmessage/message_prehash.(h|cpp) say these files 
have been generated from the message template. If this generation wasn't a 
one-off thing, the generating code must be changed, too, so it won't override 
this change here when the generation happens the next time.


This addresses bug VWR-24487.
http://jira.secondlife.com/browse/VWR-24487


Diffs
-

  doc/contributions.txt fc7e5dcf3059 
  indra/llmessage/message_prehash.h fc7e5dcf3059 
  indra/llmessage/message_prehash.cpp fc7e5dcf3059 
  indra/llprimitive/llprimitive.h fc7e5dcf3059 
  indra/llprimitive/llprimitive.cpp fc7e5dcf3059 
  indra/llprimitive/llvolumemessage.h fc7e5dcf3059 
  indra/llprimitive/llvolumemessage.cpp fc7e5dcf3059 
  indra/llui/tests/llurlentry_stub.cpp fc7e5dcf3059 
  indra/newview/tests/llremoteparcelrequest_test.cpp fc7e5dcf3059 

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


Testing
---

Compiled (standalone, 64bit Linux) with and without LL_TESTS.
Started the viewer, logged in, walked and flew around a bit. Everything seems 
to work.


Thanks,

Boroondas

___
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-24321: Validate textures starting with 00 too.

[Boroondas Gupte]

> On Jan. 18, 2011, 12:09 p.m., Merov Linden wrote:
> > indra/newview/lltexturecache.cpp, line 1595
> > 
> >
> > validate_idx being used in a test later, it's not just for 
> > (validate_idx == 0) that the behavior will be different. I need to 
> > understand better what that idx is all about or you need to give a bit more 
> > explanation before I approve this diff.
> 
> Aleric Inglewood wrote:
> The debug setting CacheValidateCounter is set to 'next_id', which makes 
> clear what it's meaning is: namely, the id that we will check next time. 
> next_idx is a very local variable that is simply set to the value of 
> CacheValidateCounter plus 1, and then that value is stored to 
> CacheValidateCounter again for next time.
> 
> validate_idx is the ID that is actually being tested this time. Hence, it 
> should be the value of CacheValidateCounter before we increase that.
> 
> Obviously, those ID's should be in the range 0...255, which is garanteed 
> by doing a modulo 256 on next_id before writing it to CacheValidateCounter.
> 
> The old code also increases validate_idx *before* it is used. That means 
> that it will be in the range 1...256, and 0 is never tested (note that 
> validate_idx is just increased, there is no modulo applied, so it's obvious 
> that it shouldn't be increased). Even the wording ("next"_id) suggests that 
> validate_idx should just be the value of CacheValidateCounter, which is the 
> value of the previous next_id...
> 
> So, after this patch, we get to the following code with validate_idx in 
> the range 0...255, as it should be.
>
> 
> Cron Stardust wrote:
> Just double checking, as switching from pre-increment to addition can 
> change behavior: In both cases next_idx will have the same value after this 
> line is executed, however validate_idx will not.
> 
> Prediff start state: validate_idx == 0;
> Prediff stop state: validate_idx == 1; next_idx == 1;
> 
> Postdiff start state: validate_idx == 0;
> Postdiff stop state: validate_idx == 0; next_idx == 1;
> 
> And another round over at the other end:
> 
> Prediff start state: validate_idx == 255;
> Prediff stop state: validate_idx == 256; next_idx == 0;
> 
> Postdiff start state: validate_idx == 255;
> Postdiff stop state: validate_idx == 255; next_idx == 0;
> 
> So, yes, validate_idx will only have a 255 in it in this last case, 
> however the over-all behavior has changed: validate_idx isn't being 
> incremented at all in this line.
> 
> WARNING: I have NOT looked at the rest of the diff, only at this one 
> line.  Nor do I know if validate_idx should or shouldn't be incremented by 
> this line of code.
> 
> Twisted Laws wrote:
> Given the way this seems to work to me without changing the sequence 
> things are checked...  
> I'd change line 1619 to  if (uuididx == (validate_idx % 256))
> Otherwise it was checking for 1 thru 256 and never 0...  this does not 
> change what appears
> to be incorrect coding where Aleric pointed out and thus won't change the 
> current
> logic that it starts by checking 1 thru 255 before checking 0.  To retain 
> the current sequence
> that things are checked, you would have to compare uuididx to next_idx 
> along with the change
> Aleric provided.
> 
> However it seems to me that all is ok with using Aleric's correction and 
> leaving the remaining
> code untouched.  (I can't see where changing the sequence of checking 
> makes a difference.)

> Just double checking, as switching from pre-increment to addition can change 
> behavior:
> In both cases next_idx will have the same value after this line is executed, 
> however validate_idx will not.

That's the intention. Without the change suggested here, validate_idx ends up 
in the wrong range. See my more detailed review below.


- Boroondas


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


On Jan. 14, 2011, 1:02 p.m., Aleric Inglewood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/90/
> ---
> 
> (Updated Jan. 14, 2011, 1:02 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Trivial patch, just removes stupidity.
> 
> 
> This addresses bug VWR-24321.
> http://jira.secondlife.com/browse/VWR-24321
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt b0bd26c5638a 
>   indra/newview/lltexturecache.cpp b0bd26c5638a 
> 
> Diff: http://codereview.secondlife.com/r/90/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Aleric
> 
>

__

Re: [opensource-dev] Review Request: VWR-24321: Validate textures starting with 00 too.

[Boroondas Gupte]

> On Jan. 19, 2011, 2:44 p.m., Boroondas Gupte wrote:
> > > validate_idx being used in a test later, it's not just for (validate_idx 
> > > == 0)
> > > that the behavior will be different. I need to understand better what 
> > > that idx
> > > is all about or you need to give a bit more explanation before I approve 
> > > this diff.
> > 
> > The different intention is needed. Without the change Aleric suggests, 
> > files that have a specific byte of their UUID being 0 will never be 
> > validated. On the other hand, sometimes no files will be tested at all 
> > (when validate_idx is 256, as no byte can assume that value).

> The different intention is needed.
Err ... I mean "The different behavior is intentional (and needed)."


- Boroondas


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


On Jan. 14, 2011, 1:02 p.m., Aleric Inglewood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/90/
> ---
> 
> (Updated Jan. 14, 2011, 1:02 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Trivial patch, just removes stupidity.
> 
> 
> This addresses bug VWR-24321.
> http://jira.secondlife.com/browse/VWR-24321
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt b0bd26c5638a 
>   indra/newview/lltexturecache.cpp b0bd26c5638a 
> 
> Diff: http://codereview.secondlife.com/r/90/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Aleric
> 
>

___
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-24321: Validate textures starting with 00 too.

[Boroondas Gupte]

> On Jan. 19, 2011, 2:44 p.m., Boroondas Gupte wrote:
> > indra/newview/lltexturecache.cpp, line 1595
> > <http://codereview.secondlife.com/r/90/diff/1/?file=413#file413line1595>
> >
> > Old code:
> > validate_idx will now be in [1,256]. (one higher than before)
> > 
> > New code:
> > validate_idx will stay in [0,256]. (untouched)
> > 
> > Old and new code:
> > next_idx will be in [0,256], but different than the new-code 
> > validate_idx (one higher modulo 256).

ok, tripple checked the ranges, but still typed them wrong ... this should be

New code:
validate_idx will stay in [0,255]. (untouched)

Old and new code:
next_idx will be in [0,255], but different than the new-code validate_idx (one 
higher modulo 256).


> On Jan. 19, 2011, 2:44 p.m., Boroondas Gupte wrote:
> > indra/newview/lltexturecache.cpp, lines 1596-1598
> > <http://codereview.secondlife.com/r/90/diff/1/?file=413#file413line1596>
> >
> > next_idx (still in the range [0,256]) gets saved to setting. (Thus why 
> > the value we get from the setting above must be in that interval.)

make this [0,255], too ^_^


- Boroondas


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


On Jan. 14, 2011, 1:02 p.m., Aleric Inglewood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/90/
> ---
> 
> (Updated Jan. 14, 2011, 1:02 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Trivial patch, just removes stupidity.
> 
> 
> This addresses bug VWR-24321.
> http://jira.secondlife.com/browse/VWR-24321
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt b0bd26c5638a 
>   indra/newview/lltexturecache.cpp b0bd26c5638a 
> 
> Diff: http://codereview.secondlife.com/r/90/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Aleric
> 
>

___
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] Motto & Goals (was: STORM-243 - simulator version notifications)

[Boroondas Gupte]

On 01/19/2011 09:20 AM, Stickman wrote:
> Is there a mission statement, a motto, a creed, or a list of goals
> somewhere?
"Fast, Easy and Fun", wasn't it? Or has that motto, too, already been
antiquated? A list of long term goals (besides what is dictated by
server side changes anyway) would interest me, too, though.

Boroondas
___
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: make PREHASH variables char const* const

[Boroondas Gupte]

> On Jan. 18, 2011, 5:58 p.m., Aleric Inglewood wrote:
> > I love it! Thanks, this was stopped me from compiling the tests (since some 
> > commit not long ago I guess, because I could before).
> > Just one remark. In one test _PREHASH_AgentID is set to "AgentID" (no doubt 
> > a place holder), while in the other you set it to 0 (now needed because 
> > it's a const). Perhaps a more symmetric approach is better?  I'd opt for 
> > setting the other also to a random string, like "Grumpity Productengine", 
> > or "Aleric Inglewood" now I think about it. Other options are "All Your 
> > Base Are Belong To Us" and "Hi mom!". Got be SURE they aren't used though 
> > -- don't want any new 'Grumpity' embarrashments :p
> >

Because I don't know how to quote code in a comment, I'll answer in a 'review'. 
See below.


- Boroondas


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


On Jan. 18, 2011, 7:29 a.m., Boroondas Gupte wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/100/
> ---
> 
> (Updated Jan. 18, 2011, 7:29 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> For the reason for this change, see 
> https://jira.secondlife.com/browse/VWR-24487 and 
> https://jira.secondlife.com/browse/VWR-24522
> 
> What I did:
> In indra/llmessage/message_prehash.(h|cpp), I turned everything into constant 
> pointers to constants by search/replace. Then I tried to compile and added 
> const qualifiers in dependent code as needed to stop the compiler complaining.
> 
> Note that comments in indra/llmessage/message_prehash.(h|cpp) say these files 
> have been generated from the message template. If this generation wasn't a 
> one-off thing, the generating code must be changed, too, so it won't override 
> this change here when the generation happens the next time.
> 
> 
> This addresses bug VWR-24487.
> http://jira.secondlife.com/browse/VWR-24487
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt fc7e5dcf3059 
>   indra/llmessage/message_prehash.h fc7e5dcf3059 
>   indra/llmessage/message_prehash.cpp fc7e5dcf3059 
>   indra/llprimitive/llprimitive.h fc7e5dcf3059 
>   indra/llprimitive/llprimitive.cpp fc7e5dcf3059 
>   indra/llprimitive/llvolumemessage.h fc7e5dcf3059 
>   indra/llprimitive/llvolumemessage.cpp fc7e5dcf3059 
>   indra/llui/tests/llurlentry_stub.cpp fc7e5dcf3059 
>   indra/newview/tests/llremoteparcelrequest_test.cpp fc7e5dcf3059 
> 
> Diff: http://codereview.secondlife.com/r/100/diff
> 
> 
> Testing
> ---
> 
> Compiled (standalone, 64bit Linux) with and without LL_TESTS.
> Started the viewer, logged in, walked and flew around a bit. Everything seems 
> to work.
> 
> 
> Thanks,
> 
> Boroondas
> 
>

___
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: make PREHASH variables char const* const

[Boroondas Gupte]

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


> Just one remark. In one test _PREHASH_AgentID is set to "AgentID"
> (no doubt a place holder), while in the other you set it to 0
> (now needed because it's a const). Perhaps a more symmetric approach
> is better?


indra/llui/tests/llurlentry_stub.cpp
<http://codereview.secondlife.com/r/100/#comment152>

This test already contained these placeholder strings, so I just left it at 
that, only adapting the types.



indra/newview/tests/llremoteparcelrequest_test.cpp
<http://codereview.secondlife.com/r/100/#comment153>

In this test here, however, I think the pointers actually ended up 
undefined in the original code, so setting them to NULL was the closest I 
possibility could think off.

As building succeeded with this, I concluded that these pointers are passed 
around but never dereferenced during the test.[*] I feel setting nullpointers 
nicely signals that fact, though it should probably be augmented by a
// never used during this test
or
// never dereferenced in this test
comment to make that intent clear.


I agree though, that we should try to handle this similarly in both tests, if 
possible. So I tried setting the pointers in 
indra/llui/tests/llurlentry_stub.cpp to NULL, too, which works nicely.

However, I then realized that the tested code might have nullpointer guards 
around dereferencing code, so that if we pass nullpointers we are actually 
testing another scenario than when passing pointers to actual data and that 
this might be the reason why no null-pointer exceptions occur during the tests.

Of course, we could now examine the tested code to see whether this is the 
case. However, it would be a bad idea to adapt the test code based on that 
examination, as the tested code might be changed later without this question 
being re-evaluated and the test rewritten accordingly.

Is there another pointer value besides NULL (thus not usually tested for) that 
still fails reliably when tried to dereference?


[*] Remember that tests are executed when building and are hopefully completely 
deterministic, so that any potential null-pointer-exception would have shown up.

- Boroondas


On Jan. 18, 2011, 7:29 a.m., Boroondas Gupte wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/100/
> ---
> 
> (Updated Jan. 18, 2011, 7:29 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> For the reason for this change, see 
> https://jira.secondlife.com/browse/VWR-24487 and 
> https://jira.secondlife.com/browse/VWR-24522
> 
> What I did:
> In indra/llmessage/message_prehash.(h|cpp), I turned everything into constant 
> pointers to constants by search/replace. Then I tried to compile and added 
> const qualifiers in dependent code as needed to stop the compiler complaining.
> 
> Note that comments in indra/llmessage/message_prehash.(h|cpp) say these files 
> have been generated from the message template. If this generation wasn't a 
> one-off thing, the generating code must be changed, too, so it won't override 
> this change here when the generation happens the next time.
> 
> 
> This addresses bug VWR-24487.
> http://jira.secondlife.com/browse/VWR-24487
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt fc7e5dcf3059 
>   indra/llmessage/message_prehash.h fc7e5dcf3059 
>   indra/llmessage/message_prehash.cpp fc7e5dcf3059 
>   indra/llprimitive/llprimitive.h fc7e5dcf3059 
>   indra/llprimitive/llprimitive.cpp fc7e5dcf3059 
>   indra/llprimitive/llvolumemessage.h fc7e5dcf3059 
>   indra/llprimitive/llvolumemessage.cpp fc7e5dcf3059 
>   indra/llui/tests/llurlentry_stub.cpp fc7e5dcf3059 
>   indra/newview/tests/llremoteparcelrequest_test.cpp fc7e5dcf3059 
> 
> Diff: http://codereview.secondlife.com/r/100/diff
> 
> 
> Testing
> ---
> 
> Compiled (standalone, 64bit Linux) with and without LL_TESTS.
> Started the viewer, logged in, walked and flew around a bit. Everything seems 
> to work.
> 
> 
> Thanks,
> 
> Boroondas
> 
>

___
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: make PREHASH variables char const* const

[Boroondas Gupte]

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

(Updated Jan. 20, 2011, 2:57 p.m.)


Review request for Viewer and Seth ProductEngine.


Changes
---

Successfully tested with (char const*)0x1, so using NULL pointers, now. Adding 
Seth as reviewer, as he introduced the place holder data that would now get 
replaced by NULL.


Summary
---

For the reason for this change, see 
https://jira.secondlife.com/browse/VWR-24487 and 
https://jira.secondlife.com/browse/VWR-24522

What I did:
In indra/llmessage/message_prehash.(h|cpp), I turned everything into constant 
pointers to constants by search/replace. Then I tried to compile and added 
const qualifiers in dependent code as needed to stop the compiler complaining.

Note that comments in indra/llmessage/message_prehash.(h|cpp) say these files 
have been generated from the message template. If this generation wasn't a 
one-off thing, the generating code must be changed, too, so it won't override 
this change here when the generation happens the next time.


This addresses bug VWR-24487.
http://jira.secondlife.com/browse/VWR-24487


Diffs (updated)
-

  doc/contributions.txt fc7e5dcf3059 
  indra/llmessage/message_prehash.h fc7e5dcf3059 
  indra/llmessage/message_prehash.cpp fc7e5dcf3059 
  indra/llprimitive/llprimitive.h fc7e5dcf3059 
  indra/llprimitive/llprimitive.cpp fc7e5dcf3059 
  indra/llprimitive/llvolumemessage.h fc7e5dcf3059 
  indra/llprimitive/llvolumemessage.cpp fc7e5dcf3059 
  indra/llui/tests/llurlentry_stub.cpp fc7e5dcf3059 
  indra/newview/tests/llremoteparcelrequest_test.cpp fc7e5dcf3059 

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


Testing (updated)
---

Compiled (standalone, 64bit Linux) with and without LL_TESTS.
Started the viewer, logged in, walked and flew around a bit. Everything seems 
to work.


Locally set _PREHASH_AgentData and _PREHASH_AgentID to (char const*)0x1 in 
indra/llui/tests/llurlentry_stub.cpp and 
indra/newview/tests/llremoteparcelrequest_test.cpp to verify they actually are 
never dereferenced, even when not NULL, so that using NULL pointers instead of 
place holder data won't change what code paths gets tested. Both tests binaries 
executed without crashes and all the contained tests passed.


Thanks,

Boroondas

___
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-236 Actual Code Review

[Boroondas Gupte]

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



indra/newview/llbottomtray.h


The comment here says "Specifies buttons which can be hidden when bottom 
tray is shrunk."

Indeed, when you make the Viewer window narrower and narrower, after all 
buttons have reached their minimal size, the ones listed here (before your 
change, that's all bottom bar buttons except the speak button) will vanish one 
by one. (Making the window wider again will make them re-appear.)

Does listing the speak button here make it disappear in that case, too? If 
so, is that intended?



indra/newview/llbottomtray.cpp


Comments should be written for those reading the final code, not for those 
reading the diff. I.e. comment on what is done, maybe on how it is done and 
most important (if not obvious) why it's done. Do not comment on how it's done 
differently from earlier code or why the change was made. That would later only 
confuse those who aren't reading the old and new code side-by-side. A better 
place for comments on changes than the code are the jira issues, the 
"Description" field here on RB and last but no least the commit messages.

An exception is to be made when whole subsystems are changed in either very 
subtle or very fundamental ways (or both), so in-code comments pointing out the 
differences would help people already closely familiar with the old code to 
find their way around in the new one.

This comment here can probably just be removed.



indra/newview/llspeakbutton.cpp


Simplify your comments! No need to point out the obvious. (Here, that the 
"if" is some sort of check, that you added this check here (where else?) and 
that you do something dependent on the result of the check.)

// Only draw the speak button when voice is enabled

would capture the intent of the code perfectly and be quicker to grasp than

// Adding check here to see if ... if so then ...



indra/newview/llspeakbutton.cpp


Please don't remove the single empty line between the end of one method and 
the beginning of the next one.



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


Begin XML comments with just "", not "".



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


Indentation of name attribute is wrong. (Should be same as label and layout 
attributes.)



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


Make that
 label="Speak button (enables voice chat)"



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


More indentation strangeness.



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


Indentation of  is wrong. (Should be same level as 
preceding and following )


- Boroondas


On Jan. 20, 2011, 6:37 p.m., Wolfpup Lowenhar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/113/
> ---
> 
> (Updated Jan. 20, 2011, 6:37 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> This allows the Speak Button to auto-hide for those that do not use Voice at 
> all.
> 
> 
> This addresses bug STORM-236.
> http://jira.secondlife.com/browse/STORM-236
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt 9c7d543fd15d 
>   indra/newview/llbottomtray.h 9c7d543fd15d 
>   indra/newview/llbottomtray.cpp 9c7d543fd15d 
>   indra/newview/llspeakbutton.cpp 9c7d543fd15d 
>   indra/newview/skins/default/xui/en/menu_bottomtray.xml 9c7d543fd15d 
> 
> Diff: http://codereview.secondlife.com/r/113/diff
> 
> 
> Testing
> ---
> 
> Built locally and did the following:
> 1 Verified that when Voice is toggled via the preference panel the Speak 
> Button auto hid/showed.
> 2 Verified that drag and drop functionality of the Speak Button was not 
> affected.
> 3 Went to a non-Voice area with Voice active and verified that button was 
> still there but grayed out.
> 
> 
> Thanks,
> 
> Wolfpup
> 
>

___
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-236 Actual Code Review

[Boroondas Gupte]

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


Looks like RB ate some of my comments in the review above. (Maybe because the 
quoted code sections overlapped.)


indra/newview/llspeakbutton.cpp


The code here should be indented the same as the comment.



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


"section"? "selection"? Please check the spelling.

Also, the comment here confused me. I think what you meant to say is: "The 
Speak Button is visible if and only if Voice Chat is enabled. Thus, to toggle 
the button's visibility, we enable or disable Voice Chat accordingly."



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


Trailing whitespace.
(Didn't RB highlight those in red, before? Doesn't seem to do that anymore 
in the diff view, only in quoted code on the review.)


- Boroondas


On Jan. 20, 2011, 6:37 p.m., Wolfpup Lowenhar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/113/
> ---
> 
> (Updated Jan. 20, 2011, 6:37 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> This allows the Speak Button to auto-hide for those that do not use Voice at 
> all.
> 
> 
> This addresses bug STORM-236.
> http://jira.secondlife.com/browse/STORM-236
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt 9c7d543fd15d 
>   indra/newview/llbottomtray.h 9c7d543fd15d 
>   indra/newview/llbottomtray.cpp 9c7d543fd15d 
>   indra/newview/llspeakbutton.cpp 9c7d543fd15d 
>   indra/newview/skins/default/xui/en/menu_bottomtray.xml 9c7d543fd15d 
> 
> Diff: http://codereview.secondlife.com/r/113/diff
> 
> 
> Testing
> ---
> 
> Built locally and did the following:
> 1 Verified that when Voice is toggled via the preference panel the Speak 
> Button auto hid/showed.
> 2 Verified that drag and drop functionality of the Speak Button was not 
> affected.
> 3 Went to a non-Voice area with Voice active and verified that button was 
> still there but grayed out.
> 
> 
> Thanks,
> 
> Wolfpup
> 
>

___
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-24520: Don't use pkg_check_modules( ... QUIET ) on CMake < 2.8.2

[Boroondas Gupte]

> On Jan. 20, 2011, 11:33 p.m., Merov Linden wrote:
> > I'm advising the MM to merge in a test repo and do a full TC cycle on all 
> > platforms before merging though...

As far as I can see indra/cmake/FindLLQtWebkit.cmake only gets called by 
indra/cmake/WebKitLibPlugin.cmake (through find_package(LLQtWebkit REQUIRED 
QUIET)), and there the call only happens for STANDALONE. So I would be very 
surprised if this affects TC builds.


- Boroondas


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


On Jan. 17, 2011, 10:03 a.m., Boroondas Gupte wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/97/
> ---
> 
> (Updated Jan. 17, 2011, 10:03 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Only use QUIET in pkg_check_modules() on CMake >=2.8.2 (where it's supported) 
> rather than already on CMake >=2.8.
> 
> 
> This addresses bug VWR-24520.
> http://jira.secondlife.com/browse/VWR-24520
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt 9e99b2c8fb28 
>   indra/cmake/FindLLQtWebkit.cmake 9e99b2c8fb28 
> 
> Diff: http://codereview.secondlife.com/r/97/diff
> 
> 
> Testing
> ---
> 
> Configured (standalone) without a .pgk file for libllqtwebkit on Linux with 
> CMake 2.8.1 and CMake 2.8.3. Output as expected.
> 
> Not tested:
> * CMake 2.8.2
> * system with a .pgk file for libllqtwebkit
> * non-standalone
> * Mac, Win
> 
> 
> Thanks,
> 
> Boroondas
> 
>

___
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-236 Actual Code Review

[Boroondas Gupte]

> On Jan. 21, 2011, 3:44 a.m., Boroondas Gupte wrote:
> > indra/newview/llspeakbutton.cpp, lines 67-70
> > <http://codereview.secondlife.com/r/113/diff/1/?file=619#file619line67>
> >
> > Please don't remove the single empty line between the end of one method 
> > and the beginning of the next one.

Oops, you didn't remove the line, it wasn't there before, just looks as if it 
was due to the diff view ... but can't hurt to add it.


- Boroondas


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


On Jan. 20, 2011, 6:37 p.m., Wolfpup Lowenhar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/113/
> ---
> 
> (Updated Jan. 20, 2011, 6:37 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> This allows the Speak Button to auto-hide for those that do not use Voice at 
> all.
> 
> 
> This addresses bug STORM-236.
> http://jira.secondlife.com/browse/STORM-236
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt 9c7d543fd15d 
>   indra/newview/llbottomtray.h 9c7d543fd15d 
>   indra/newview/llbottomtray.cpp 9c7d543fd15d 
>   indra/newview/llspeakbutton.cpp 9c7d543fd15d 
>   indra/newview/skins/default/xui/en/menu_bottomtray.xml 9c7d543fd15d 
> 
> Diff: http://codereview.secondlife.com/r/113/diff
> 
> 
> Testing
> ---
> 
> Built locally and did the following:
> 1 Verified that when Voice is toggled via the preference panel the Speak 
> Button auto hid/showed.
> 2 Verified that drag and drop functionality of the Speak Button was not 
> affected.
> 3 Went to a non-Voice area with Voice active and verified that button was 
> still there but grayed out.
> 
> 
> Thanks,
> 
> Wolfpup
> 
>

___
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: make PREHASH variables char const* const

[Boroondas Gupte]

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


As already mentioned in the review request description, comments in 
message_prehash.h and message_prehash.cpp claim that these files have been 
generated:


indra/llmessage/message_prehash.h
<http://codereview.secondlife.com/r/100/#comment181>





indra/llmessage/message_prehash.cpp
<http://codereview.secondlife.com/r/100/#comment182>




Do we have to expect them to be re-generated again in the future? If so, the 
code for generating them should be changed alongside the change I propose here, 
so that future generations (pun intended) won't  inadvertently override and 
revert the change.

At first, I thought the code for generating them was not part of the source 
tree, but now I think I have located it in indra/llmessage/message.cpp. (I 
would have expected it to be a script, not a compiled program.) I will 
investigate this further and augment the review request with a change to the 
generating code, too, if applicable.

- Boroondas


On Jan. 20, 2011, 2:57 p.m., Boroondas Gupte wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/100/
> ---
> 
> (Updated Jan. 20, 2011, 2:57 p.m.)
> 
> 
> Review request for Viewer and Seth ProductEngine.
> 
> 
> Summary
> ---
> 
> For the reason for this change, see 
> https://jira.secondlife.com/browse/VWR-24487 and 
> https://jira.secondlife.com/browse/VWR-24522
> 
> What I did:
> In indra/llmessage/message_prehash.(h|cpp), I turned everything into constant 
> pointers to constants by search/replace. Then I tried to compile and added 
> const qualifiers in dependent code as needed to stop the compiler complaining.
> 
> Note that comments in indra/llmessage/message_prehash.(h|cpp) say these files 
> have been generated from the message template. If this generation wasn't a 
> one-off thing, the generating code must be changed, too, so it won't override 
> this change here when the generation happens the next time.
> 
> 
> This addresses bug VWR-24487.
> http://jira.secondlife.com/browse/VWR-24487
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt fc7e5dcf3059 
>   indra/llmessage/message_prehash.h fc7e5dcf3059 
>   indra/llmessage/message_prehash.cpp fc7e5dcf3059 
>   indra/llprimitive/llprimitive.h fc7e5dcf3059 
>   indra/llprimitive/llprimitive.cpp fc7e5dcf3059 
>   indra/llprimitive/llvolumemessage.h fc7e5dcf3059 
>   indra/llprimitive/llvolumemessage.cpp fc7e5dcf3059 
>   indra/llui/tests/llurlentry_stub.cpp fc7e5dcf3059 
>   indra/newview/tests/llremoteparcelrequest_test.cpp fc7e5dcf3059 
> 
> Diff: http://codereview.secondlife.com/r/100/diff
> 
> 
> Testing
> ---
> 
> Compiled (standalone, 64bit Linux) with and without LL_TESTS.
> Started the viewer, logged in, walked and flew around a bit. Everything seems 
> to work.
> 
> 
> Locally set _PREHASH_AgentData and _PREHASH_AgentID to (char const*)0x1 in 
> indra/llui/tests/llurlentry_stub.cpp and 
> indra/newview/tests/llremoteparcelrequest_test.cpp to verify they actually 
> are never dereferenced, even when not NULL, so that using NULL pointers 
> instead of place holder data won't change what code paths gets tested. Both 
> tests binaries executed without crashes and all the contained tests passed.
> 
> 
> Thanks,
> 
> Boroondas
> 
>

___
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: make PREHASH variables char const* const

[Boroondas Gupte]

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

(Updated Jan. 22, 2011, 7:40 a.m.)


Review request for Viewer and Seth ProductEngine.


Changes
---

Found and changed the generating code for message_prehash.(h|cpp), too.


Summary (updated)
---

For the reason for this change, see 
https://jira.secondlife.com/browse/VWR-24487 and 
https://jira.secondlife.com/browse/VWR-24522

What I did:
In indra/llmessage/message_prehash.(h|cpp), I turned everything into constant 
pointers to constants by search/replace. Then I tried to compile and added 
const qualifiers in dependent code as needed to stop the compiler complaining.

Note that comments in indra/llmessage/message_prehash.(h|cpp) say these files 
have been generated from the message template. Because this generation might 
not have been a one-off thing, I changed the generating code, too, so it won't 
override this change here when the generation happens the next time. However, 
that part of the code is not called by Viewer, although the relevant function — 
dump_prehash_files() — ends up in the Viewer binary. That function probably 
gets called by the simulator, when one runs the latter with -prehash. (See 
https://bitbucket.org/lindenlab/viewer-development/src/fc7e5dcf3059/indra/llmessage/message.cpp#cl-2532
 )


This addresses bug VWR-24487.
http://jira.secondlife.com/browse/VWR-24487


Diffs (updated)
-

  doc/contributions.txt fc7e5dcf3059 
  indra/llmessage/message.cpp fc7e5dcf3059 
  indra/llmessage/message_prehash.h fc7e5dcf3059 
  indra/llmessage/message_prehash.cpp fc7e5dcf3059 
  indra/llprimitive/llprimitive.h fc7e5dcf3059 
  indra/llprimitive/llprimitive.cpp fc7e5dcf3059 
  indra/llprimitive/llvolumemessage.h fc7e5dcf3059 
  indra/llprimitive/llvolumemessage.cpp fc7e5dcf3059 
  indra/llui/tests/llurlentry_stub.cpp fc7e5dcf3059 
  indra/newview/tests/llremoteparcelrequest_test.cpp fc7e5dcf3059 

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


Testing (updated)
---

Compiled (standalone, 64bit Linux) with and without LL_TESTS.
Started the viewer, logged in, walked and flew around a bit. Everything seems 
to work.


Locally set _PREHASH_AgentData and _PREHASH_AgentID to (char const*)0x1 in 
indra/llui/tests/llurlentry_stub.cpp and 
indra/newview/tests/llremoteparcelrequest_test.cpp to verify they actually are 
never dereferenced, even when not NULL, so that using NULL pointers instead of 
place holder data won't change what code paths gets tested. Both tests binaries 
executed without crashes and all the contained tests passed.

Locally invoked start_messaging_system() with b_dump_prehash_file == true 
instead of FALSE, to see what would be generated after my change to 
dump_prehash_files().
The message_prehash.(h|cpp) generated by that had the correct type qualifiers 
and formatting, but some lines were removed or added compared to the modified 
files from the source tree. That probably means that the files aren't fully 
synchronized with the message template file in the source tree. Because the 
"added" constants are spread all over the file, while the "removed" ones were 
at the end, I'd wager that message_prehash.(h|cpp) in the viewer source tree 
are actually more up-to-date than the message template file in the source tree.


Thanks,

Boroondas

___
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-24612 lscript_compile warnings treated as errors -- workaround

[Boroondas Gupte]

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


I'm not a Windows person, but ...


indra/llcommon/llpreprocessor.h


Isn't the 'pop' sufficient for restoring the previous warning settings? 
What purpose do the 'disable', 'push' and 'default' after that serve?

See also OlafvdSpek's comment about the proposed workaround on the page you 
linked.


- Boroondas


On Jan. 25, 2011, 11:12 a.m., Nicky Perian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/122/
> ---
> 
> (Updated Jan. 25, 2011, 11:12 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Correct marco redefinition warnings introduced in Visual Studio 10. Patch 
> applies workaround documented at:
> http://connect.microsoft.com/VisualStudio/feedback/details/621653/including-stdint-after-intsafe-generates-warnings
> 
> Depends on vwr-24610 which should be applied first.
> 
> 
> This addresses bug vwr-24612.
> http://jira.secondlife.com/browse/vwr-24612
> 
> 
> Diffs
> -
> 
>   indra/llcommon/llpreprocessor.h 26c09ad4293e 
> 
> Diff: http://codereview.secondlife.com/r/122/diff
> 
> 
> Testing
> ---
> 
> Before (snip)
> 1>C:\Program Files (x86)\Microsoft Visual Studio 
> 10.0\VC\include\stdint.h(81): warning C4005: 'UINT32_MAX' : macro redefinition
> 1>  indra.l.cpp(85) : see previous definition of 'UINT32_MAX'
> == Build: 0 succeeded, 1 failed, 0 up-to-date, 0 skipped ==
> After patch applied:
> 1>-- Build started: Project: lscript_compile, Configuration: Release 
> Win32 --
> 1>  lscript_bytecode.cpp
> 1>  lscript_error.cpp
> 1>  lscript_resource.cpp
> 1>  lscript_scope.cpp
> 1>  lscript_tree.cpp
> 1>  lscript_typecheck.cpp
> 1>  indra.y.cpp
> 1>  indra.l.cpp
> 1>  lscript_compile.vcxproj -> 
> C:\lindenhg\vcexpress2010build\indra\build-vc100\lscript\lscript_compile\Release\lscript_compile.lib
> == Build: 1 succeeded, 0 failed, 0 up-to-date, 0 skipped ==
> 
> 
> Thanks,
> 
> Nicky
> 
>

___
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-24612 lscript_compile warnings treated as errors -- workaround

[Boroondas Gupte]

> On Jan. 25, 2011, 11:30 a.m., Boroondas Gupte wrote:
> > indra/llcommon/llpreprocessor.h, lines 164-167
> > <http://codereview.secondlife.com/r/122/diff/1/?file=635#file635line164>
> >
> > Isn't the 'pop' sufficient for restoring the previous warning settings? 
> > What purpose do the 'disable', 'push' and 'default' after that serve?
> > 
> > See also OlafvdSpek's comment about the proposed workaround on the page 
> > you linked.
> 
> Nicky Perian wrote:
> I thought I followed the second comment. I could not tell from 
> OlafvdSpek's comments if that was the complete solution or a added to 
> solution.
> 
> Nicky Perian wrote:
> I think the 'disable" 'push", 'default' are to correct later includes 
> causing warnings. But, I can take them out and test it.

As I read it, OlafvdSpek's comment is neither a complete solution, nor does it 
just add something to Maxime Chamley's workaround. It *replaces* a part of 
Maxime's solution:
Instead of
push, disable, (do something for which the warning needs to be 
disabled,) push, default
one should do
push, disable, (do something for which the warning needs to be 
disabled,) pop
so the second 'push' and the 'default' are replaced by just 'pop'.

This makes sense under the assumption that (an educated guess) 'push' records 
the current state of what warnings are enabled and disabled and places it on a 
stack (a FIFO data structure) and 'pop' takes such a record from the stack 
(i.e., reads it and removes it from the stack) and restores the respective 
state. 'default' probably just restores the default value for the specified 
warning type.

So the original workaround has two problems:
* It leaves two recorded states on the stack which might never be used. (Or 
worse, they might not be the ones expected to be on the stack where they'll get 
used.)
* If the enable/disable warning 4005 setting was at its default value before 
this part of the code, everything is fine, as the default will be restored. But 
if the value wasn't the default one, that previous value will be lost and the 
default restored instead anyway.


- Boroondas


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


On Jan. 25, 2011, 11:12 a.m., Nicky Perian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/122/
> ---
> 
> (Updated Jan. 25, 2011, 11:12 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Correct marco redefinition warnings introduced in Visual Studio 10. Patch 
> applies workaround documented at:
> http://connect.microsoft.com/VisualStudio/feedback/details/621653/including-stdint-after-intsafe-generates-warnings
> 
> Depends on vwr-24610 which should be applied first.
> 
> 
> This addresses bug vwr-24612.
> http://jira.secondlife.com/browse/vwr-24612
> 
> 
> Diffs
> -
> 
>   indra/llcommon/llpreprocessor.h 26c09ad4293e 
> 
> Diff: http://codereview.secondlife.com/r/122/diff
> 
> 
> Testing
> ---
> 
> Before (snip)
> 1>C:\Program Files (x86)\Microsoft Visual Studio 
> 10.0\VC\include\stdint.h(81): warning C4005: 'UINT32_MAX' : macro redefinition
> 1>  indra.l.cpp(85) : see previous definition of 'UINT32_MAX'
> == Build: 0 succeeded, 1 failed, 0 up-to-date, 0 skipped ==
> After patch applied:
> 1>-- Build started: Project: lscript_compile, Configuration: Release 
> Win32 --
> 1>  lscript_bytecode.cpp
> 1>  lscript_error.cpp
> 1>  lscript_resource.cpp
> 1>  lscript_scope.cpp
> 1>  lscript_tree.cpp
> 1>  lscript_typecheck.cpp
> 1>  indra.y.cpp
> 1>  indra.l.cpp
> 1>  lscript_compile.vcxproj -> 
> C:\lindenhg\vcexpress2010build\indra\build-vc100\lscript\lscript_compile\Release\lscript_compile.lib
> == Build: 1 succeeded, 0 failed, 0 up-to-date, 0 skipped ==
> 
> 
> Thanks,
> 
> Nicky
> 
>

___
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: Use consistent path for all *.py scripts

[Boroondas Gupte]

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

Ship it!


Covers all occurrences of the variant with env, so should be fine.

As currently both variants are present, choosing only one of them (no matter 
which) should not hinder anyone who can already use the scripts today (except 
if they only used scripts with one of the variants, up to now.) If we later 
discover that using the other variant would bring any advantage (like, allowing 
to call the script directly rather than prepending the interpreter command) we 
can always make that change later.

- Boroondas


On Jan. 28, 2011, 5:56 p.m., Merov Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/129/
> ---
> 
> (Updated Jan. 28, 2011, 5:56 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Simple consistency change, using "#!/usr/bin/python" in all python script.
> 
> 
> This addresses bug STORM-937.
> http://jira.secondlife.com/browse/STORM-937
> 
> 
> Diffs
> -
> 
>   indra/copy_win_scripts/start-client.py b542f8134a2b 
>   indra/develop.py b542f8134a2b 
>   indra/lib/python/indra/util/simperf_host_xml_parser.py b542f8134a2b 
>   indra/lib/python/indra/util/simperf_oprof_interface.py b542f8134a2b 
>   indra/lib/python/indra/util/test_win32_manifest.py b542f8134a2b 
>   indra/newview/generate_breakpad_symbols.py b542f8134a2b 
>   scripts/build_version.py b542f8134a2b 
>   scripts/install.py b542f8134a2b 
> 
> Diff: http://codereview.secondlife.com/r/129/diff
> 
> 
> Testing
> ---
> 
> Pulled into a test repo and build successfully on all platforms on TC so I 
> guess no bad surprise here.
> 
> 
> Thanks,
> 
> Merov
> 
>

___
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: Do not fail when no scp command is found, unless it is actually needed to fetch something

[Boroondas Gupte]

> On Jan. 28, 2011, 2:46 p.m., Alain Linden wrote:
> > autobuild/common.py, line 415
> > 
> >
> > ...and test for None here.

Good idea. This would avoid bugs that could occur e.g. when the string is 
changed and not spelled the same in both places.


- Boroondas


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


On Jan. 28, 2011, 5:20 a.m., Oz Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/127/
> ---
> 
> (Updated Jan. 28, 2011, 5:20 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> During initialization, if there is no scp or pscp command found then 
> autobuild fails immediately.  This is true whether or not any scp urls need 
> to be used.
> 
> This change modifies the behavior so that a warning is printed if no command 
> is found, but execution proceeds until an scp command is needed, at which 
> time execution fails with an explanatory message.
> 
> This patch can print the warning multiple times - I didn't attempt to 
> suppress the extras.
> 
> 
> Diffs
> -
> 
>   autobuild/common.py f49808fe3c07 
> 
> Diff: http://codereview.secondlife.com/r/127/diff
> 
> 
> Testing
> ---
> 
> I've tested this locally, simulating the error by temporarily modifying the 
> names of the commands to be found for scp.
> 
> I have not checked it on Windows, where the original problem was found.
> 
> 
> 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: Do not fail when no scp command is found, unless it is actually needed to fetch something

[Boroondas Gupte]

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



autobuild/common.py


The docstring of find_executable should probably indicate that ...



autobuild/common.py


... the return value is None when no matching executable can be found, as 
we (somewhat) rely on what the return value is in that case. (Not your fault, 
the old code already relied on that, too.)



autobuild/common.py


Usually one would test for None with
if self._scp is None:
as testing for `not self._scp` will also trigger when the value is False 
(obviously) or (maybe less obvious) becomes False when casted to bool.

However, because we don't want self._scp to be an empty string when 
building the command below ...



autobuild/common.py


... testing for `not self._scp` here might actually be better, I'm not sure.


- Boroondas


On Jan. 29, 2011, 5:01 a.m., Oz Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/127/
> ---
> 
> (Updated Jan. 29, 2011, 5:01 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> During initialization, if there is no scp or pscp command found then 
> autobuild fails immediately.  This is true whether or not any scp urls need 
> to be used.
> 
> This change modifies the behavior so that a warning is printed if no command 
> is found, but execution proceeds until an scp command is needed, at which 
> time execution fails with an explanatory message.
> 
> This patch can print the warning multiple times - I didn't attempt to 
> suppress the extras.
> 
> 
> Diffs
> -
> 
>   autobuild/common.py 9ae505976dfa 
> 
> Diff: http://codereview.secondlife.com/r/127/diff
> 
> 
> Testing
> ---
> 
> I've tested this locally, simulating the error by temporarily modifying the 
> names of the commands to be found for scp.
> 
> I have not checked it on Windows, where the original problem was found.
> 
> 
> 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] Python 2.5 implementations that come without ElementTree? (was: attempting to compile a viewer)

[Boroondas Gupte]

Hi Andro

On 01/30/2011 10:35 AM, Andromeda Quonset wrote:
> CMake Error at cmake/Prebuilt.cmake:39 (message):
>   Failed to download or unpack prebuilt 'ogg-vorbis'.  Process returned 1.
>
> [...]
>
> I have gane back through the archives for this list, and I've found a
> few other instances of the same error message:
>
> Failed to download or unpack prebuilt 'ogg-vorbis'.  Process returned 1.
>
> and one person solved it by manually downloading it, and saving it in
> a temp directory.  That doesn't seem to have worked for me.  Either
> the problem here is something else, or I am putting it in the wrong
> temp directory.
I guess this error message indicates an effect of your problem, not its
cause. The lines directly above that might come closer:
> from xml.etree.ElementTree import *
> ImportError: No module named etree.ElementTree
Although, according to the documentation
,
xml.etree.ElementTree should be part of python since version 2.5, not
all interpreters seem to ship with it
.

> I am using Python 2.5 and CMake 2.8.3  Should I be using other
> versions instead?
The python version should be ok, but you might need to either use a
different python /implementation/ or to separately install ElementTree.
Which of these, I'm not sure, as I'm not building on Windows myself.

I hope this helps.

Cheers,
Boroondas
___
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] "Nearby" people tab

[Boroondas Gupte]

Hi Dave

On 02/01/2011 01:57 AM, Dave Booth wrote:
> On 1/31/2011 07:52, Oz Linden (Scott Lawrence) wrote:
>> It would be interesting to see what was in the log file for the period 
>> of time that the window was not properly filled in.
> I intend to capture some more specific data in the next few days - will 
> involve laying out precisely positioned prims in 3 dimensions for an av 
> to sit on and probing what "NearMeRange" distances they appear and 
> disappear from the nearby tab to an alt sat on the zero-ref prim. If 
> properly designed, that study will establish any issues with height AGL, 
> "flat" (2d) vector magnitude and "true" (3d) vector magnitude. Will 
> include logfile capture and will advise.
While broad testing is a very good idea, also to find yet unknown bugs,
you might want to test the "interesting" values for this specific issue
(and related ones) first, which are heights around 4 * (2^8 - 1) m ==
1020 m, if the underlying cause of VWR-9326
 and VWR-17050
 are the same. Aimee and
Henry provide some technical explanation of that value in comments on
VWR-6484
.

Note that a fix for VWR-17050 is being reviewed at
https://codereview.secondlife.com/r/132/. If you document your test
well, it could probably also be used to QA that fix.

Cheers,
Boroondas
___
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-864: As as developer, I would like an object oriented wrapper to make safe use of memory pools easier

[Boroondas Gupte]

> On Feb. 2, 2011, 6:30 p.m., Merov Linden wrote:
> > indra/llmessage/llpumpio.cpp, line 359
> > 
> >
> > An example where the use of operator() is particularly unsightly...
> 
> Aleric Inglewood wrote:
> And rightfully so! It should be "extremely unpleasant" for the user to 
> get to the underlaying apr_pool_t*.
> That this code hacks access is exactly that: a hack. One has to be very 
> careful when doing this.
> The reason I did it here is because 1) I know what I'm doing, so it's ok 
> in this case, 2) for this
> first introduction of LLAPRPool I tried to make minimal changes to the 
> actual functionality of
> existing code using apr pools (with the exception of LLAPRFile, the 
> rewrite of its API actually
> coming from SNOW-103 which already proved itself in snowglobe, imprudence 
> and other TPV's).
> The fact that this access here is needed actually signifies that this 
> code is not handling
> pools in a very safe way, but I consider(ed) it better to keep the code 
> "as-is" and hack around the
> *safity* of LLAPRPool (and as a result not changing anything!) than to 
> rewrite the interface at this
> point for this particular part of the code; changing things is more risk, 
> in this case, than not
> using the API of LLAPRPool as intended. It would be harder to find if 
> that rewrite would introduce
> some kind of bug we made lots of changes at the same time. I propose to 
> leave this as it is now
> and look at changing it later once everyone feels secure about the 
> stability of the current patch.
> 
> Okay, that sounded nice -- but the truth is that I have no idea (at this 
> point) if it is even
> possible to do what that code does without accessing the underlaying 
> apr_pool_t* like that (meaning,
> not passing it directly to an APR function, but storing it in a 
> structure). I just know that it
> should feel dangerous to do so, so it seems OK that code that does it 
> doesn't look very nice.
>

Then it should maybe be made even more obvious with a super-ugly name like 
getAPRPool_DO_NOT_CALL_THIS_DIRECTLY_use_LLAPRPool_instead()


- Boroondas


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


On Jan. 29, 2011, 9:10 a.m., Aleric Inglewood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/99/
> ---
> 
> (Updated Jan. 29, 2011, 9:10 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Please see http://jira.secondlife.com/browse/STORM-864
> 
> I made a mercurial repository available for testing here:
> 
> hg clone https://bitbucket.org/aleric/viewer-development-storm-864
> 
> From the commit message:
> 
> Introduces a LLThreadLocalData class that can be
> accessed through the static LLThread::tldata().
> Currently this object contains two (public) thread-local
> objects: a LLAPRRootPool and a LLVolatileAPRPool.
> 
> The first is the general memory pool used by this thread
> (and this thread alone), while the second is intended
> for short lived memory allocations (needed for APR).
> The advantages of not mixing those two is that the latter
> is used most frequently, and as a result of it's nature
> can be destroyed and reconstructed on a "regular" basis.
> 
> This patch adds LLAPRPool (completely replacing the old one),
> which is a wrapper around apr_pool_t* and has complete
> thread-safity checking.
> 
> Whenever an apr call requires memory for some resource,
> a memory pool in the form of an LLAPRPool object can
> be created with the same life-time as this resource;
> assuring clean up of the memory no sooner, but also
> not much later than the life-time of the resource
> that needs the memory.
> 
> Many, many function calls and constructors had the
> pool parameter simply removed (it is no longer the
> concern of the developer, if you don't write code
> that actually does an libapr call then you are no
> longer bothered with memory pools at all).
> 
> However, I kept the notion of short-lived and
> long-lived allocations alive (see my remark in
> the jira here: 
> https://jira.secondlife.com/browse/STORM-864?focusedCommentId=235356&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-235356
> which requires that the LLAPRFile API needs
> to allow the user to specify how long they
> think a file will stay open. By choosing
> 'short_lived' as default for the constructor
> that immediately opens a file, the number of
> instances where this needs to be specified is
> drastically reduced however (obviously, any
> automatic LLAPRFile is short lived).

Re: [opensource-dev] streaming media on 64bit?

[Boroondas Gupte]

Hi Marc

On 02/05/2011 05:03 AM, Marc Adored wrote:
> I am on 64bit ubuntu and noticed that media on a prim works now even
> though I am using a 32bit viewer. I am assuming this is because it is
> a SLPlugin and not part of the viewer?
That's not the reason: The SLPlugin binary, while a separate process
than the viewer main binary, is also shipped only compiled for 32bit in
the official downloads and thus faces the same problem as the main
binary: it can only load 32bit libraries. The reason why it can display
web content anyway, is that the library used for rendering html is also
included (also in 32bit) in the download.

> I was wondering if the
> streaming audio/video that has not worked for 32bit viewers on 64bit
> systems could be reworked to work the same way?
For audio/video stream display, the GStreamer libraries are used, but
those aren't included in the download. So SLPlugin tries to load the
system-installed version (if any), which (if SLPlugin is compiled for
32bit) can only succeed if those libs are compiled for 32bit, too.

So the possibilities would be:

* Include 32bit GStreamer libraries in the official download (or in
  a separate download)
  o Around 17 MB including common plugins, but still excluding
any dependency libraries that might have to be added for
this to work, too
* Offer SPLugin (and maybe the whole viewer) compiled for 64bit, too
  o This would mean additional QA work for LL.
* Have Linux 64bit users install a 32bit GStreamer
  o Probably difficult on many distributions (because GStreamer
is usually not part of the 32bit compatibility bundles, such
as ia32-libs)

(I've gone into more detail about the options from the users' point of
view in past explanations
.)

> Is there plans for this?
I don't know of any specific one. There's a feature request for offering
official 64bit downloads: VWR-13793
.

>  I think that with the refusal to work on a 64bit viewer this
> would be a good compromise because that is the only reason I require a
> 64bit built viewer.
That would indeed make life much easier for non-technical Linux 64bit users.

> I just thought I would probe here first before messing with jiras :)
Although this has been discussed in the past, it's good that someone
brings it up again.

Cheers,
Boroondas
___
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] Copying GStreamer from a 32bit system to a 64bit one (was: streaming media on 64bit?)

[Boroondas Gupte]

On 02/05/2011 04:15 PM, Marc Adored wrote:
> (:5205): GStreamer-WARNING **: Failed to load plugin
> '/usr/lib32/gstreamer-0.10/libgstspc.so': libopenspc.so.0: cannot open
> shared object file: No such file or directory
>
> [...]
>
> I have copied the gstreamer-0.10 from my wifes 32bit ubuntu to
> /usr/lib32/gstreamer-0.10 but it still cant find the file
> /usr/lib32/gstreamer-0.10/libgstspc.so.
GStreamer is not failing to find libgstspc.so ... ratherlibgstspc.so is
failing to find libopenspc.so.0 ...

GStreamer is not self-contained: It (or its plugins) uses a lot of
shared libraries that aren't part of GStreamer itself (and thus not in
its directory but somewhere else on the system).

Cheers,
Boroondas
___
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] Friends Permissions

[Boroondas Gupte]

On 02/06/2011 02:55 PM, Trilo Byte wrote:
> [...] permissions the people on my friends list have (see me online, locate 
> me on the map, move my objects, etc), but I can't figure out how to change 
> those settings.  It used to be on the Notes & Privacy tab of the Profile, but 
> the web profile doesn't have any such option.  Any ideas?
On the web profile, button *Action* > *Set Privacy*.

Though, I must say I'd prefer if this was possible directly from the
friends list.

Boroondas
___
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-24520: Don't use pkg_check_modules( ... QUIET ) on CMake < 2.8.2

[Boroondas Gupte]

> On Jan. 20, 2011, 11:33 p.m., Merov Linden wrote:
> > I'm advising the MM to merge in a test repo and do a full TC cycle on all 
> > platforms before merging though...
> 
> Boroondas Gupte wrote:
> As far as I can see indra/cmake/FindLLQtWebkit.cmake only gets called by 
> indra/cmake/WebKitLibPlugin.cmake (through find_package(LLQtWebkit REQUIRED 
> QUIET)), and there the call only happens for STANDALONE. So I would be very 
> surprised if this affects TC builds.

Any results from that TC cycle?


- Boroondas


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


On Jan. 17, 2011, 10:03 a.m., Boroondas Gupte wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/97/
> ---
> 
> (Updated Jan. 17, 2011, 10:03 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Only use QUIET in pkg_check_modules() on CMake >=2.8.2 (where it's supported) 
> rather than already on CMake >=2.8.
> 
> 
> This addresses bug VWR-24520.
> http://jira.secondlife.com/browse/VWR-24520
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt 9e99b2c8fb28 
>   indra/cmake/FindLLQtWebkit.cmake 9e99b2c8fb28 
> 
> Diff: http://codereview.secondlife.com/r/97/diff
> 
> 
> Testing
> ---
> 
> Configured (standalone) without a .pgk file for libllqtwebkit on Linux with 
> CMake 2.8.1 and CMake 2.8.3. Output as expected.
> 
> Not tested:
> * CMake 2.8.2
> * system with a .pgk file for libllqtwebkit
> * non-standalone
> * Mac, Win
> 
> 
> Thanks,
> 
> Boroondas
> 
>

___
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] autobuild on Linux 64-bit standalone: setup & configuring issues (was: Using 'autobuild' to build the viewer)

[Boroondas Gupte]

Ok, I've finally taken some time to test autobuild, too.

On 01/28/2011 12:41 AM, Oz Linden (Scott Lawrence) wrote:
>
> To begin experimenting with this new way of doing things, you'll need
> to check out autobuild itself (it's written in python, and is open
> source) from:
>
> https://bitbucket.org/lindenlab/autobuild
>
> You can either run it directly from the checkout, or install it as a
> normal python package using the command:
>
> setup.py install
>
With the current (9ee2db08d677
)
autobuild, this command won't work on UNIX-like systems for two reasons
(see OPEN-10 ). Workaround
is to prepend python, i.e. call

python setup.py install

or, because this tries to write into system directories

sudo python setup.py install


> A "standalone" non-Linden developer build:
>
> autobuild configure -c OpenSourceStandaloneRelWithDebInfo
>
This won't work with the current (00453191c1b9
)
viewer-autobuild code: "standalone" has to be CamelCased, too:

autobuild configure -c OpenSourceStand*A*loneRelWithDebInfo


Then, I had to merge in my VWR-24520
 fix or I'd hit that bug,
just as I would in unmodified viewer-development. After that,
configuring yields:

$ autobuild configure -c OpenSourceStandAloneRelWithDebInfo
-- The C compiler identification is GNU
-- The CXX compiler identification is GNU
-- Check for working C compiler: /usr/bin/gcc
-- Check for working C compiler: /usr/bin/gcc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Using autobuild at: /usr/bin/autobuild
-- checking for module 'ogg'
--   found ogg, version 1.2.0
-- checking for module 'vorbis'
--   found vorbis, version 1.3.1
-- checking for module 'vorbisenc'
--   found vorbisenc, version 1.3.1
-- checking for module 'vorbisfile'
--   found vorbisfile, version 1.3.1
-- checking for module 'openal'
--   found openal, version 1.11.753
-- checking for module 'freealut'
--   found freealut, version 1.1.0
-- Building with OpenAL audio support
-- Found Google BreakPad: /usr/lib64/libbreakpad_client.so
*CMake Error at cmake/Copy3rdPartyLibs.cmake:263 (copy_if_different):
  COPY_IF_DIFFERENT Macro invoked with incorrect arguments for macro named:
  COPY_IF_DIFFERENT
Call Stack (most recent call first):
  llcommon/CMakeLists.txt:14 (include)*


-- Looking for XOpenDisplay in /usr/lib64/libX11.so;/usr/lib64/libXext.so
-- Looking for XOpenDisplay in /usr/lib64/libX11.so;/usr/lib64/libXext.so - 
found
-- Looking for gethostbyname
-- Looking for gethostbyname - found
-- Looking for connect
-- Looking for connect - found
-- Looking for remove
-- Looking for remove - found
-- Looking for shmat
-- Looking for shmat - found
-- Looking for IceConnectionNumber in ICE
-- Looking for IceConnectionNumber in ICE - found
-- Found X11: /usr/lib64/libX11.so
-- checking for module 'freetype2'
--   found freetype2, version 12.1.6
-- Looking for include files CMAKE_HAVE_PTHREAD_H
-- Looking for include files CMAKE_HAVE_PTHREAD_H - found
-- Looking for pthread_create in pthreads
-- Looking for pthread_create in pthreads - not found
-- Looking for pthread_create in pthread
-- Looking for pthread_create in pthread - found
-- Found Threads: TRUE
-- checking for module 'atk'
--   found atk, version 1.30.0
-- checking for module 'cairo'
--   found cairo, version 1.8.10
-- checking for module 'gdk-2.0'
--   found gdk-2.0, version 2.20.1
-- checking for module 'gdk-pixbuf-2.0'
--   found gdk-pixbuf-2.0, version 2.20.1
-- checking for module 'glib-2.0'
--   found glib-2.0, version 2.24.2
-- checking for module 'gmodule-2.0'
--   found gmodule-2.0, version 2.24.2
-- checking for module 'gtk+-2.0'
--   found gtk+-2.0, version 2.20.1
-- checking for module 'gthread-2.0'
--   found gthread-2.0, version 2.24.2
-- checking for module 'libpng'
--   found libpng, version 1.4.5
-- checking for module 'pango'
--   found pango, version 1.28.3
-- checking for module 'pangoft2'
--   found pangoft2, version 1.28.3
-- checking for module 'pangox'
--   found pangox, version 1.28.3
-- checking for module 'pangoxft'
--   found pangoxft, version 1.28.3
-- checking for module 'sdl'
--   found sdl, version 1.2.13
-- Looking for Q_WS_X11
-- Looking for Q_WS_X11 - found
-- Looking for Q_WS_WIN
-- Lookin

Re: [opensource-dev] Review Request: make PREHASH variables char const* const

[Boroondas Gupte]

> On Feb. 3, 2011, 6:35 p.m., Aleric Inglewood wrote:
> >
> 
> Aleric Inglewood wrote:
> Can this patch please be added to viewer-development? It's getting really 
> annoying that I have to apply patches to the soruce tree before it even can 
> compile cleanly :(.

Maybe the hack from 
https://bitbucket.org/mani_linden/viewer-autobuild/changeset/57d9f93978a7 could 
be cherry-picked into viewer-development to give some relief for the time until 
a decision on this more invasive fix can be made?


- Boroondas


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


On Jan. 22, 2011, 7:40 a.m., Boroondas Gupte wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/100/
> ---
> 
> (Updated Jan. 22, 2011, 7:40 a.m.)
> 
> 
> Review request for Viewer and Seth ProductEngine.
> 
> 
> Summary
> ---
> 
> For the reason for this change, see 
> https://jira.secondlife.com/browse/VWR-24487 and 
> https://jira.secondlife.com/browse/VWR-24522
> 
> What I did:
> In indra/llmessage/message_prehash.(h|cpp), I turned everything into constant 
> pointers to constants by search/replace. Then I tried to compile and added 
> const qualifiers in dependent code as needed to stop the compiler complaining.
> 
> Note that comments in indra/llmessage/message_prehash.(h|cpp) say these files 
> have been generated from the message template. Because this generation might 
> not have been a one-off thing, I changed the generating code, too, so it 
> won't override this change here when the generation happens the next time. 
> However, that part of the code is not called by Viewer, although the relevant 
> function — dump_prehash_files() — ends up in the Viewer binary. That function 
> probably gets called by the simulator, when one runs the latter with 
> -prehash. (See 
> https://bitbucket.org/lindenlab/viewer-development/src/fc7e5dcf3059/indra/llmessage/message.cpp#cl-2532
>  )
> 
> 
> This addresses bug VWR-24487.
> http://jira.secondlife.com/browse/VWR-24487
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt fc7e5dcf3059 
>   indra/llmessage/message.cpp fc7e5dcf3059 
>   indra/llmessage/message_prehash.h fc7e5dcf3059 
>   indra/llmessage/message_prehash.cpp fc7e5dcf3059 
>   indra/llprimitive/llprimitive.h fc7e5dcf3059 
>   indra/llprimitive/llprimitive.cpp fc7e5dcf3059 
>   indra/llprimitive/llvolumemessage.h fc7e5dcf3059 
>   indra/llprimitive/llvolumemessage.cpp fc7e5dcf3059 
>   indra/llui/tests/llurlentry_stub.cpp fc7e5dcf3059 
>   indra/newview/tests/llremoteparcelrequest_test.cpp fc7e5dcf3059 
> 
> Diff: http://codereview.secondlife.com/r/100/diff
> 
> 
> Testing
> ---
> 
> Compiled (standalone, 64bit Linux) with and without LL_TESTS.
> Started the viewer, logged in, walked and flew around a bit. Everything seems 
> to work.
> 
> 
> Locally set _PREHASH_AgentData and _PREHASH_AgentID to (char const*)0x1 in 
> indra/llui/tests/llurlentry_stub.cpp and 
> indra/newview/tests/llremoteparcelrequest_test.cpp to verify they actually 
> are never dereferenced, even when not NULL, so that using NULL pointers 
> instead of place holder data won't change what code paths gets tested. Both 
> tests binaries executed without crashes and all the contained tests passed.
> 
> Locally invoked start_messaging_system() with b_dump_prehash_file == true 
> instead of FALSE, to see what would be generated after my change to 
> dump_prehash_files().
> The message_prehash.(h|cpp) generated by that had the correct type qualifiers 
> and formatting, but some lines were removed or added compared to the modified 
> files from the source tree. That probably means that the files aren't fully 
> synchronized with the message template file in the source tree. Because the 
> "added" constants are spread all over the file, while the "removed" ones were 
> at the end, I'd wager that message_prehash.(h|cpp) in the viewer source tree 
> are actually more up-to-date than the message template file in the source 
> tree.
> 
> 
> Thanks,
> 
> Boroondas
> 
>

___
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-977 llmediaplugintest shows up even though -DLL_TESTS:BOOL=OFF has been used

[Boroondas Gupte]

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


> If there is some better way to more exactly target these two items please 
> point it out.

You should be able to get the same effect when wrapping the only place where 
this file is referenced in a LL_TESTS condition, i.e., change 
https://bitbucket.org/lindenlab/viewer-development/src/b0bceb572090/indra/CMakeLists.txt#cl-74
 from
if (NOT LINUX)
to
if (LL_TESTS AND NOT LINUX)
(and the same for the endif, of course)

Whether that's a better place, I don't know.

Though, I think LL_TESTS is the wrong conditional here, anyway. LL_TESTS is for 
enabling unit and integration tests. llplugintest however, I have learned on 
IRC today, seems to be a fully separate program based on and similar to 
uBrowser that could be used to load and test individual llmediaplugins, would 
it communicate with them in the same way the viewer does. (Which, according to 
MichelleZ, it doesn't, thus potentially misleading developers of new plugins.)

It should probably not be built unless explicitly requested, thus a new 
variable, defaulting to OFF and different from LL_TESTS would suit this much 
better. Or, just delete the referencing at 
https://bitbucket.org/lindenlab/viewer-development/src/b0bceb572090/indra/CMakeLists.txt#cl-75
 completely, thus unconditionally removing it from the dependencies of the 
viewer itself, and have those that want to build it explicitly state it as a 
build target.

- Boroondas


On Feb. 8, 2011, 8:46 a.m., Jonathan Yap wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/144/
> ---
> 
> (Updated Feb. 8, 2011, 8:46 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> llmediaplugintest shows up to be compiled even though -DLL_TESTS:BOOL=OFF has 
> been used on the command line.
> 
> This cmake file does not use the call to LL_ADD_PROJECT_UNIT_TESTS that other 
> cmake files do.  
> 
> LL_ADD_PROJECT_UNIT_TESTS is usually wrapped with if(LL_TESTS)
> 
> I could not figure out which lines suppress the inclusion of 
> copy_plugintest_libs and llmediaplugintest into the list of what is to be 
> built so wrapped the entire file around if(LL_TESTS).  
> 
> If there is some better way to more exactly target these two items please 
> point it out.
> 
> 
> This addresses bug STORM-977.
> http://jira.secondlife.com/browse/STORM-977
> 
> 
> Diffs
> -
> 
>   indra/test_apps/llplugintest/CMakeLists.txt b0bceb572090 
> 
> Diff: http://codereview.secondlife.com/r/144/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] Test build

[Boroondas Gupte]

Downloaded the Linux build.

On 02/08/2011 09:15 PM, Philippe (Merov) Bossut wrote:
> - STORM-955 : Massively duplicated objects
Tested that with the long command line Aleric gave on RB. Much less
multiple occurrences than in build 219680. Remaining multiple ones are:

  2 COLLAPSED_BY_USER
  2 FTM_CREATE_OBJECT
  2 FTM_CULL_REBOUND
  2 FTM_GEO_SKY
  2 FTM_PROCESS_OBJECTS
  2 FTM_REBUILD_VBO
  2 FTM_UPDATE_WLPARAM
  2 LSCRIPTDataSize
  2 LSCRIPTStateBitField
  2 LSCRIPTTypeNames
  2 MANIPULATOR_IDS
  2 NEW_LINE
  2 PANEL_PICKS
  2 PANEL_PROFILE
  2 PREVIEW_HPAD
  2 PREVIEW_TEXTURE_HEIGHT
  2 WEARABLE_NAME_COMPARATOR
  2 simd_w97_rem
  2 simd_w97_preoff
  2 initialize_transition_table()::C.36
  2 aanscalefactor.106
  2 aanscales.105
  2 deferred_render
  2 extend_offset
  2 extend_test
  2 icon_m
  2 icon_pg
  2 icon_r
  2 sTesterName
  2 shader
  2 t_panel_group_general
  2 t_places
  3 empty_string
  3 gDirOpposite
  3 t2
  3 t_inventory
  4 r3
  5 t1
  8 OGL_TO_CFR_ROTATION
  8 r2
  8 rcsid
 10 boost::tuples::ignore
 12 r1
 52 r
804 std::__ioinit

Except for rcsid and boost::tuples::ignore, all the ones with a count of
3 or above also show in the list of remaining duplicate names /after/
the patch in Aleric's comment on RB
, so I guess
they are OK. I don't know if something should be done about rcsid and
boost::tuples::ignore.

Cheers,
Boroondas
___
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: CMAKE_EXE_LINKER_FLAGS not honored when linking the viewer binary if -DLL_TESTS:BOOL=ON

[Boroondas Gupte]

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


A good change to avoid heisenbugs.


indra/cmake/GoogleMock.cmake


Don't mix spaces and tabs. (Use spaces here, like in the rest of the file.)



indra/cmake/GoogleMock.cmake


I'd put the
-Wl,--no-as-needed
down on gtest's line, so that the "wrapping" is more obvious. Or maybe even 
use some creative indentation like:
# ...
if (LINUX)
set(GOOGLEMOCK_LIBRARIES 
gmock
-Wl,--no-as-needed # VWR-24366: gmock is underlinked, it needs 
gtest.
gtest
-Wl,--as-needed) # continue to use "as-needed" for other libs
elseif(WINDOWS)
# ...


- Boroondas


On Feb. 9, 2011, 10:20 p.m., Merov Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/147/
> ---
> 
> (Updated Feb. 9, 2011, 10:20 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> From Aleric's patch:
> 
> Setting CMAKE_EXE_LINKER_FLAGS to "" because the tests "need" that is pretty 
> hard measure.  Not only is it not necessary to do so, it also changes how the 
> viewer is linked depending on a whether or not the tests are compiled and 
> that is not good.
> 
> The reason that this was needed is that libgmock is underlinked (see 
> http://wiki.mandriva.com/en/Underlinking), which is not compatible with 
> -Wl,--as-needed that is being used on linux. libgmock.so.0 needs a symbol 
> that is defined in libgtest.so.o, but -lgtest was not passed to the linker 
> when creating libgmock.so.0:
> 
> Underlinked (no libgtest.so.o):
> $ objdump -p /usr/lib/libgmock.so.0 | grep NEEDED
>   NEEDED   libstdc++.so.6
>   NEEDED   libm.so.6
>   NEEDED   libc.so.6
>   NEEDED   libgcc_s.so.1
> 
> The solution is to wrap between -Wl,--no-as-needed -lgtest -Wl,--as-needed 
> causing it to be added again. This is only needed on linux, since that the 
> only platform that we use -Wl,--as-needed on. Moreover, we can just set 
> GOOGLEMOCK_LIBRARIES to "gmock -Wl,--no-as-needed gtest -Wl,--as-needed" 
> since that is only passed to TARGET_LINK_LIBRARIES which only adds -l in 
> front of 'things' that don't start with '-', to allow you do pass special 
> flags like this.
> 
> 
> This addresses bug STORM-981.
> http://jira.secondlife.com/browse/STORM-981
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt 27dae7b01a81 
>   indra/cmake/GoogleMock.cmake 27dae7b01a81 
>   indra/cmake/LLAddBuildTest.cmake 27dae7b01a81 
> 
> Diff: http://codereview.secondlife.com/r/147/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Merov
> 
>

___
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: OPEN-29: Error out if lldir_.h is included when building for a different platform.

[Boroondas Gupte]

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

Review request for Viewer.


Summary
---

Rather than relying on (probably anyway accidental) on syntax 
incompatibilities, added explicit #error to the indra/llvfs/lldir_*.h headers.

In separate commits, but also in the diff for review here, removed one such 
syntax incompatibility 
(https://bitbucket.org/boroondas/open-29/changeset/19ab94811717) and did some 
cleanup (superfluous "public:"s, trailing whitespace).


This addresses bug OPEN-29.
http://jira.secondlife.com/browse/OPEN-29


Diffs
-

  doc/contributions.txt ec4ad7e3ecca 
  indra/llvfs/lldir_linux.h ec4ad7e3ecca 
  indra/llvfs/lldir_mac.h ec4ad7e3ecca 
  indra/llvfs/lldir_solaris.h ec4ad7e3ecca 
  indra/llvfs/lldir_win32.h ec4ad7e3ecca 

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


Testing
---

Built on Linux before pulling 
https://bitbucket.org/lindenlab/viewer-development/changeset/7a1440277911
* Errors out as wanted.

Build on Linux after pulling 7a1440277911
* Builds like normal.

Note: These tests were done with other build fixes present
* for VWR-24520 https://codereview.secondlife.com/r/97/
* for VWR-24487 https://codereview.secondlife.com/r/100/
not on a pristine source tree.


Thanks,

Boroondas

___
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: OPEN-29: Error out if lldir_.h is included when building for a different platform.

[Boroondas Gupte]

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

(Updated Feb. 11, 2011, 6:07 a.m.)


Review request for Viewer.


Changes
---

(just edited description, so that the link there works)


Summary (updated)
---

Rather than relying on (probably anyway accidental) on syntax 
incompatibilities, added explicit #error to the indra/llvfs/lldir_*.h headers.

In separate commits, but also in the diff for review here, removed one such 
syntax incompatibility 
(https://bitbucket.org/boroondas/open-29/changeset/19ab94811717 ) and did some 
cleanup (superfluous "public:"s, trailing whitespace).


This addresses bug OPEN-29.
http://jira.secondlife.com/browse/OPEN-29


Diffs
-

  doc/contributions.txt ec4ad7e3ecca 
  indra/llvfs/lldir_linux.h ec4ad7e3ecca 
  indra/llvfs/lldir_mac.h ec4ad7e3ecca 
  indra/llvfs/lldir_solaris.h ec4ad7e3ecca 
  indra/llvfs/lldir_win32.h ec4ad7e3ecca 

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


Testing
---

Built on Linux before pulling 
https://bitbucket.org/lindenlab/viewer-development/changeset/7a1440277911
* Errors out as wanted.

Build on Linux after pulling 7a1440277911
* Builds like normal.

Note: These tests were done with other build fixes present
* for VWR-24520 https://codereview.secondlife.com/r/97/
* for VWR-24487 https://codereview.secondlife.com/r/100/
not on a pristine source tree.


Thanks,

Boroondas

___
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: open-2 (improved progress messages during dependency checks) and open-31 (more standardized short form option switches)

[Boroondas Gupte]

On 02/12/2011 10:13 PM, Oz Linden wrote:
> (there is a failure displaying the diff for autobuild.configfile.py because 
> it is a one word change of the logging level for a log message added by one 
> of my other changes that has not yet been applied to the trunk)
You should be able use the 'Parent Diff' feature to let RB know about
your other changes that have "not yet been applied to the trunk",
without having it mix them into the diff to review.

Here's how I think the procedure will work in detail:

   1. Identify a revision that is in the repo you chose when creating
  the review request^[1] <#footnote1> *and* present in your local
  repository. I will call that revision 'common_base' from here on.
  * If in doubt, pull from the chosen remote repository, so
you'll have the tip in common and can use that as
common_base. (I don't think common_base actually has to be a
common ancestor of the remote's tip and the revision you
want reviewed.)
   2. Identify the revision that the change to be reviewed is based on.
  Let's call it 'begin'.
   3. Save the diff from common_base to begin as parent.diff:

  hg diff -r common_base -r begin > parent.diff

  * Of course, substitute the revisions identified above for
common_base and begin, respectively.
   4. I'll assume you still have the diff you uploaded when creating the
  review. I not, get it from
  https://codereview.secondlife.com/r/150/diff/raw/ (or click the
  *Download Diff* button)
  * This diff should apply cleanly on the begin revision
   5. On https://codereview.secondlife.com/r/150/, click *Update Diff*
   6. In the *Diff* field, choose the diff you already uploaded when
  creating the review
   7. In the *Parent Diff* field, choose the new parent.diff file
   8. Click *Upload*



^[1] <#footnote1_back> In your case, the one referenced by
'03_autobuild', so probably
https://bitbucket.org/mani_linden/viewer-autobuild

Cheers,
Boroondas
___
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-977 llmediaplugintest shows up even though -DLL_TESTS:BOOL=OFF has been used

[Boroondas Gupte]

> On Feb. 8, 2011, 10:41 a.m., Boroondas Gupte wrote:
> > > If there is some better way to more exactly target these two items please 
> > > point it out.
> > 
> > You should be able to get the same effect when wrapping the only place 
> > where this file is referenced in a LL_TESTS condition, i.e., change 
> > https://bitbucket.org/lindenlab/viewer-development/src/b0bceb572090/indra/CMakeLists.txt#cl-74
> >  from
> > if (NOT LINUX)
> > to
> > if (LL_TESTS AND NOT LINUX)
> > (and the same for the endif, of course)
> > 
> > Whether that's a better place, I don't know.
> > 
> > Though, I think LL_TESTS is the wrong conditional here, anyway. LL_TESTS is 
> > for enabling unit and integration tests. llplugintest however, I have 
> > learned on IRC today, seems to be a fully separate program based on and 
> > similar to uBrowser that could be used to load and test individual 
> > llmediaplugins, would it communicate with them in the same way the viewer 
> > does. (Which, according to MichelleZ, it doesn't, thus potentially 
> > misleading developers of new plugins.)
> > 
> > It should probably not be built unless explicitly requested, thus a new 
> > variable, defaulting to OFF and different from LL_TESTS would suit this 
> > much better. Or, just delete the referencing at 
> > https://bitbucket.org/lindenlab/viewer-development/src/b0bceb572090/indra/CMakeLists.txt#cl-75
> >  completely, thus unconditionally removing it from the dependencies of the 
> > viewer itself, and have those that want to build it explicitly state it as 
> > a build target.
> 
> Merov Linden wrote:
> I'm afraid that, if that app is not built by default, it will rapidly rot 
> and become unmaintainable. There is value having it built (even if not used) 
> by LL devs and TC at least (who all build with LL_TESTS on): make sure we 
> don't add viewer dependencies where we don't need any and maintain a clean 
> separation of concerns between plugins and hosts.

I can see that point. Though, I'd still like a separate variable, if alone to 
avoid people from mistaking llplugintest for a unit or integration test, like I 
initially did. Just have that (new) variable default on ON, then, rather than 
OFF.


- Boroondas


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


On Feb. 16, 2011, 11:32 a.m., Jonathan Yap wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/144/
> ---
> 
> (Updated Feb. 16, 2011, 11:32 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> llmediaplugintest shows up to be compiled even though -DLL_TESTS:BOOL=OFF has 
> been used on the command line.
> 
> This cmake file does not use the call to LL_ADD_PROJECT_UNIT_TESTS that other 
> cmake files do.  
> 
> LL_ADD_PROJECT_UNIT_TESTS is usually wrapped with if(LL_TESTS)
> 
> I could not figure out which lines suppress the inclusion of 
> copy_plugintest_libs and llmediaplugintest into the list of what is to be 
> built so wrapped the entire file around if(LL_TESTS).  
> 
> If there is some better way to more exactly target these two items please 
> point it out.
> 
> 
> This addresses bug STORM-977.
> http://jira.secondlife.com/browse/STORM-977
> 
> 
> Diffs
> -
> 
>   indra/CMakeLists.txt b0bceb572090 
>   indra/test_apps/llplugintest/CMakeLists.txt b0bceb572090 
> 
> Diff: http://codereview.secondlife.com/r/144/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-977 llmediaplugintest shows up even though -DLL_TESTS:BOOL=OFF has been used

[Boroondas Gupte]

> On Feb. 8, 2011, 10:41 a.m., Boroondas Gupte wrote:
> > > If there is some better way to more exactly target these two items please 
> > > point it out.
> > 
> > You should be able to get the same effect when wrapping the only place 
> > where this file is referenced in a LL_TESTS condition, i.e., change 
> > https://bitbucket.org/lindenlab/viewer-development/src/b0bceb572090/indra/CMakeLists.txt#cl-74
> >  from
> > if (NOT LINUX)
> > to
> > if (LL_TESTS AND NOT LINUX)
> > (and the same for the endif, of course)
> > 
> > Whether that's a better place, I don't know.
> > 
> > Though, I think LL_TESTS is the wrong conditional here, anyway. LL_TESTS is 
> > for enabling unit and integration tests. llplugintest however, I have 
> > learned on IRC today, seems to be a fully separate program based on and 
> > similar to uBrowser that could be used to load and test individual 
> > llmediaplugins, would it communicate with them in the same way the viewer 
> > does. (Which, according to MichelleZ, it doesn't, thus potentially 
> > misleading developers of new plugins.)
> > 
> > It should probably not be built unless explicitly requested, thus a new 
> > variable, defaulting to OFF and different from LL_TESTS would suit this 
> > much better. Or, just delete the referencing at 
> > https://bitbucket.org/lindenlab/viewer-development/src/b0bceb572090/indra/CMakeLists.txt#cl-75
> >  completely, thus unconditionally removing it from the dependencies of the 
> > viewer itself, and have those that want to build it explicitly state it as 
> > a build target.
> 
> Merov Linden wrote:
> I'm afraid that, if that app is not built by default, it will rapidly rot 
> and become unmaintainable. There is value having it built (even if not used) 
> by LL devs and TC at least (who all build with LL_TESTS on): make sure we 
> don't add viewer dependencies where we don't need any and maintain a clean 
> separation of concerns between plugins and hosts.
> 
> Boroondas Gupte wrote:
> I can see that point. Though, I'd still like a separate variable, if 
> alone to avoid people from mistaking llplugintest for a unit or integration 
> test, like I initially did. Just have that (new) variable default on ON, 
> then, rather than OFF.
> 
> Jonathan Yap wrote:
> New variable name aside, would it make it clearer what this module does 
> if it were renamed to something more obvious?  One person pointed out to me 
> that lltestplugin might be a better choice.

> New variable name aside, would it make it clearer what this module does if it 
> were renamed to something more obvious?
Yes! Though to what?

> One person pointed out to me that lltestplugin might be a better choice.
That might have been me, back when I thought it was a test plugin, which it 
apparently isn't, either.


- Boroondas


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


On Feb. 16, 2011, 11:32 a.m., Jonathan Yap wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/144/
> ---
> 
> (Updated Feb. 16, 2011, 11:32 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> llmediaplugintest shows up to be compiled even though -DLL_TESTS:BOOL=OFF has 
> been used on the command line.
> 
> This cmake file does not use the call to LL_ADD_PROJECT_UNIT_TESTS that other 
> cmake files do.  
> 
> LL_ADD_PROJECT_UNIT_TESTS is usually wrapped with if(LL_TESTS)
> 
> I could not figure out which lines suppress the inclusion of 
> copy_plugintest_libs and llmediaplugintest into the list of what is to be 
> built so wrapped the entire file around if(LL_TESTS).  
> 
> If there is some better way to more exactly target these two items please 
> point it out.
> 
> 
> This addresses bug STORM-977.
> http://jira.secondlife.com/browse/STORM-977
> 
> 
> Diffs
> -
> 
>   indra/CMakeLists.txt b0bceb572090 
>   indra/test_apps/llplugintest/CMakeLists.txt b0bceb572090 
> 
> Diff: http://codereview.secondlife.com/r/144/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: When a bake texture upload fails, retry instead of giving up.

[Boroondas Gupte]

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



indra/newview/lltexlayer.h


Is there any reason why you use BOOL instead of bool for this new data 
member? If not, prefer bool. (See 
https://wiki.secondlife.com/wiki/Coding_standard#Linden_Variable_Types )



indra/newview/lltexlayer.cpp


Is "res" for "resolution" or "result"?


- Boroondas


On Feb. 17, 2011, 9 a.m., Thickbrick Sleaford wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/152/
> ---
> 
> (Updated Feb. 17, 2011, 9 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> When a bake upload fails, the viewer doesn't retry it, and subsequently 
> doesn't send a AgentSetAppearance message. This can happen without the user 
> being aware, leaving the avatar looking good on their screen, but not updated 
> to the same outfit on other people's screens. The avatar will remain in that 
> state until the user does something that causes a rebake (manually rebake or 
> change outfit.) The solution here is to retry the upload after a small delay.
> 
> What this diff changes: when a full-res upload fails, retry to upload it 
> after a 5s delay, up to 5 times (in case the cap is available, last attempt 
> is via the old asset store.) Also, some clearer log messages. This implements 
> an old *FIX: comment:
> // *FIX: retry upload after n seconds, asset server could be busy
> 
> This isn't needed for low res uploads, because they don't block subsequent 
> full-res uploads (mNeedsUpload isn't set to FALSE in 
> LLTexLayerSetBuffer::doUpload in low-res uploads.)
> 
> 
> This addresses bug VWR-24889.
> http://jira.secondlife.com/browse/VWR-24889
> 
> 
> Diffs
> -
> 
>   indra/newview/llassetuploadresponders.h 379da6bd50a5 
>   indra/newview/llassetuploadresponders.cpp 379da6bd50a5 
>   indra/newview/lltexlayer.h 379da6bd50a5 
>   indra/newview/lltexlayer.cpp 379da6bd50a5 
> 
> Diff: http://codereview.secondlife.com/r/152/diff
> 
> 
> Testing
> ---
> 
> Attempted outfit changes using a problematic connection (not recently used 
> outfits to avoid using cached bakes). Looked for "Baked full res texture 
> upload for  failed" log messages, observed the subsequent 
> retries and successful upload for that region. Observed that eventually the 
> fully-baked avatar is visible to other users.
> 
> 
> Thanks,
> 
> Thickbrick
> 
>

___
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] Hacking up to Visual Studio 2010 ...

[Boroondas Gupte]

On 02/18/2011 01:56 AM, Oz Linden (Scott Lawrence) wrote:
> [...]
> Then check out:
>
> https://bitbucket.org/oz_linden/viewer-autobuild2010
>
> cd into the top level of that directory, and run:
>
> autobuild configure -c OpenSourceRelWithDebInfo
>
> autobuild configure -c OpenSourceRelWithDebInfo
>
Shouldn't the second command be

autobuild *build* -c OpenSourceRelWithDebInfo

?
___
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] Linux build error: missing binary operator before token "(" (was: Hacking up to Visual Studio 2010 ...)

[Boroondas Gupte]

On 02/18/2011 01:56 AM, Oz Linden (Scott Lawrence) wrote:
> [...]
>
> Then check out:
>
> https://bitbucket.org/oz_linden/viewer-autobuild2010
>
> cd into the top level of that directory, and run:
>
> autobuild configure -c OpenSourceRelWithDebInfo
>
> autobuild configure -c OpenSourceRelWithDebInfo
>
> and let me know if it works (and if not, see if you can figure out why
> not).  On our build farm, I'm getting an error:
>
> _[19:13:30]:_ //LogScan//_(1s)_
> _[19:13:30]:_ /[LogScan] / from /usr/include/c++/4.1.3/cmath:53,
> _[19:13:30]:_ /[LogScan] / from
> /var/opt/teamcity/checkout/L-oz_viewer-autobuild2010/latest/indra/llcommon/linden_common.h:48,
> _[19:13:30]:_ /[LogScan] / from
> /var/opt/teamcity/checkout/L-oz_viewer-autobuild2010/latest/indra/newview/tests/lldateutil_test.cpp:26:
> _[19:13:30]:_ /[LogScan] //usr/include/bits/huge_val.h:28:18: error:
> missing binary operator before token "("
> _[19:13:30]:_ /[LogScan] //usr/include/bits/huge_val.h:30:20: error:
> missing binary operator before token "("
With "build" in the second command instead of "configure", I'm getting
the same error, though not just for /usr/include/bits/huge_val.h but
many more system headers, too.

Boroondas

___
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-22220 Chat preferences > font size should increase size of input text as well

[Boroondas Gupte]

> On Feb. 18, 2011, 7:16 a.m., Jonathan Yap wrote:
> > My internet connection went dead while in the middle of updating the diffs. 
> >  They are corrupted and I don't know how to fix them.

Have you tried uploading them again? That won't remove the bogus intermediary 
revision, but if we know about that, we can easily ignore it.

Btw., after uploading, you can review the new diff before publishing it, I 
think: While on the page where you can describe the update, click the "View 
Diff" button.


- Boroondas


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


On Feb. 18, 2011, 7:11 a.m., Jonathan Yap wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/139/
> ---
> 
> (Updated Feb. 18, 2011, 7:11 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> This is a request for help.  I am trying to learn more about c++ and how 
> variables in one class are accessed from another.  For someone who knows what 
> they are doing this is probably a pretty easy question.
> 
> I have been able to set the font size on the chat input box when it is 
> created in llbottomtray.cpp.  I would like to do the same thing when someone 
> clicks in that box to input text; it is possible they have changed the font 
> setting and I would like to apply the size there as well, but I am stuck on 
> how to do this.  I think the right place to do this is in 
> llchatbar.cpp/LLChatBar::onInputEditorGainFocus().
> 
> I have tried all kinds of wrong ways but at this point am stymied.
> 
> Exact steps on how to proceed would be appreciated.
> 
> 
> This addresses bug vwr-0.
> http://jira.secondlife.com/browse/vwr-0
> 
> 
> Diffs
> -
> 
>   indra/newview/llbottomtray.h 3d2e71443c58 
>   indra/newview/llbottomtray.cpp 3d2e71443c58 
>   indra/newview/llchatbar.h 3d2e71443c58 
>   indra/newview/llchatbar.cpp 3d2e71443c58 
> 
> Diff: http://codereview.secondlife.com/r/139/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] Linux build error: missing binary operator before token "("

[Boroondas Gupte]

On 02/19/2011 08:30 PM, Nicky D. wrote:
>> [...]
>> [19:13:30]: [LogScan] /usr/include/bits/huge_val.h:30:20: error: missing
>> binary operator before token "("
>> [...]
> Tried it today, getting that too. Huge slew of errors.
>
> Even though this looks intimidating, the reason is really simple.
>
> In OZ's version of json there is a file features.h in
> ../include/json/. Metaphorical
> speaking there he laid the bomb.
> It is then trigged in cmake/JsonCpp.cmake and newview/CMakeList.txt
>
> JsonCpp.cmake  sets JSONCPP_INCLUDE_DIRS to ${LIBS_PREBUILD_DIR)/include/json.
> newview/CMakeList.txt adds JSONCPP_INCLUDE_DIRS to the system include dirs.
>
> Now the the problem with gcc is, that adding include dirs with -I
> makes them be searched
> before the system include dirs.
> And there our little bomb goes off. Because now the compiler findes
> the features.h file
> first in ../include/json. When it fact it needs the system one from
> /usr/include/features.h.
That analysis looks correct, so go ask Oz about that Eternal Glory he
offered on IRC. :-)

> One solution might be to use the -I- switch or -iquote for new gcc
> versions. But lucky
> enough there is a trivially simple fix, just use
> ${LIBS_PREBUILD_DIR)/include for
> JSONCPP_INCLUDE_DIRS.
>
> I attached a patch that does just this.
I just tested, and reverting eeb812d81330

(the changeset that switched to the new json download) works as a
workaround, too. Though, I guess your change is the preferred way to fix
this issue, because

   1. there probably was a reason for updating jsoncpp
   2. the other jsoncpp headers in the package also have very generic
  names, so using the containing dir as a way of namespacing will
  probably avoid further conflicts in the future


> Standalone builds might need
> some extra hackery,
> I did not try one of those yet.
Since 7690f4cb5e81
, the
jsoncpp include was probably broken for standalone anyway. (Can't test,
as standalone fails due to other (unrelated) errors.)

Cheers,
Boroondas
___
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] Is 'STANDALONE' confusing?

[Boroondas Gupte]

On 02/21/2011 03:28 PM, Oz Linden (Scott Lawrence) wrote:
> If we are going to change it, the replacement term should, in addition 
> to being more accurately descriptive of what it does, be an affirmative 
> term - don't suggest any 'NO_*" replacements.

Would it be acceptable to invert the setting's semantic in order to
avoid a negation? I.e., STANDALONE=OFF would become NEW_SETTING=ON and
vice versa. That'd allow for easy-to-understand names like
USE_PREBUILD_LIBS or DOWNLOAD_NEEDED_DEPENDENCIES.

Off course, the default value should be inverted together with the
setting's semantic, such that the default behavior does not change.

Cheers,
Boroondas
___
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] Is 'STANDALONE' confusing?

[Boroondas Gupte]

On 02/21/2011 04:41 PM, Aleric Inglewood wrote:
> A LOT worse, but still better than 'standalone' would be USE_SYSTEM_LIBS.
Why would you consider USE_SYSTEM_LIBS worse than other suggestions?

Cheers,
Boroondas
___
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] Is 'STANDALONE' confusing?

[Boroondas Gupte]

On 02/23/2011 10:57 AM, leliel wrote:
> On Tue, Feb 22, 2011 at 6:56 PM, Oz Linden (Scott Lawrence)
>  wrote:
>> The real distinction, I think, is whether or not you are using
>> _installed_ libraries.  Normally, the viewer build has only minimal
>> reliance on the libs that are installed on the system itself
> In other words the viewer is normally built as a standalone
> application, i.e. the STANDALONE option does the exact opposite of
> what you'd expect given the definition of the word. So why not just
> invert the meaning of STANDALONE?
I guess (and that's really just a guess) today's "STANDALONE" refers to
not relying on network connectivity at /build/ time. Whether the shipped
application package is self-contained (i.e. has minimal reliance on
libraries installed on the end user's system) depends mainly on what
libraries get packaged with it (or linked statically, so that they're
included anyway).

Boroondas
___
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-24333: Hardening against use of getLindenUserDir() before logging in.

[Boroondas Gupte]

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



indra/llvfs/lldir.cpp


I know we already have a lot of methods taking boolean arguments, but it's 
probably worth mentioning 
http://doc.qt.nokia.com/qq/qq13-apis.html#thebooleanparametertrap here. (I.e., 
boolean arguments make the calls of the method harder to read, when the method 
name doesn't already imply the semantics of the argument. You can give the 
reader a hint towards the semantics by using nicely named enums instead.)



indra/newview/llappviewer.cpp


E.g. here, it's impossible to guess what the "true" indicates. Either you 
already know, or you have to look it up.


- Boroondas


On Jan. 14, 2011, 1:05 p.m., Aleric Inglewood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/91/
> ---
> 
> (Updated Jan. 14, 2011, 1:05 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Without this patch, getLindenUserDir() is sometimes used without
> checking if it returns an empty value, resulting in trying to open
> an empty file and then gracefully recovering from that. So, this
> patch doesn't really fix a bug. However it might prevent one in the
> future. Note that this DID lead to a bug in Viewer 1 code, so it's
> possible. The main threat is that some singleton class that uses
> getLindenUserDir (indirectly) is instantiated before logging in:
> A singleton is only created once and when it is initialized with
> an empty getLindenUserDir() then that can remain.
> 
> This patch aborts when the viewer is compiled in debug mode (not
> in release mode, when it will do what it did before this patch)
> and when getLindenUserDir() is called before it was initialized,
> unless the developer explicitely passes 'true' (empty_ok) to
> getLindenUserDir, signaling that they considered the possibility
> that the function is being called before the user logged in.
> 
> 
> This addresses bug VWR-24333.
> http://jira.secondlife.com/browse/VWR-24333
> 
> 
> Diffs
> -
> 
>   indra/llvfs/lldir.h 422f636c3343 
>   indra/llvfs/lldir.cpp 422f636c3343 
>   indra/newview/llappviewer.cpp 422f636c3343 
>   indra/newview/llbottomtray.cpp 422f636c3343 
>   indra/newview/llfilepicker.cpp 422f636c3343 
>   indra/newview/llnavigationbar.cpp 422f636c3343 
>   indra/newview/llsearchhistory.h 422f636c3343 
>   indra/newview/llurlhistory.cpp 422f636c3343 
>   indra/newview/llviewerinventory.cpp 422f636c3343 
>   indra/newview/llviewermedia.cpp 422f636c3343 
>   indra/newview/llvoiceclient.cpp 422f636c3343 
> 
> Diff: http://codereview.secondlife.com/r/91/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Aleric
> 
>

___
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: on-line fix for OPEN-39: (standalone) bitpack_test.o: No such file or directory

[Boroondas Gupte]

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

Review request for Viewer and Aleric Inglewood.


Summary
---

On standalone, -I"${TUT_INCLUDE_DIR}" was added to the compile flags for 
integration tests of llcommon when TUT_INCLUDE_DIR might not have been set yet. 
This lead to a flag -I"", which would confuse g++ and lead to the tests' object 
files not being found.

This change makes sure that TUT_INCLUDE_DIR is set by including Tut.cmake, 
which is responsible for setting it.


This addresses bug OPEN-39.
http://jira.secondlife.com/browse/OPEN-39


Diffs
-

  doc/contributions.txt 1013caf84c2e 
  indra/cmake/LLAddBuildTest.cmake 1013caf84c2e 

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


Testing
---

Clean rebuild with LL_TESTS ON (see also repro instructions on jira)


Thanks,

Boroondas

___
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-1001: Viewer needlessly hits the "ObjectMedia" cap with thousands of requests

[Boroondas Gupte]

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



indra/llprimitive/lltextureentry.cpp


Is this creation/destruction of mMediaEntry taken care off elsewhere 
already?


- Boroondas


On Feb. 22, 2011, 11:12 a.m., Kitty Barnett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/162/
> ---
> 
> (Updated Feb. 22, 2011, 11:12 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> S32 LLTextureEntry::setMediaTexGen(U8 media) would appear to be the root 
> cause of the bug:
> 
> The header file suggests that:
> 
> // The Media Tex Gen values are bits in a bit field:
> // +--+
> // | .TTM | M = Media Flags (web page), T = LLTextureEntry::eTexGen, . = 
> unused
> // | 76543210 |
> // +--+
> const S32 TEM_MEDIA_MASK  = 0x01;
> const S32 TEM_TEX_GEN_MASK= 0x06;
> 
> and while LLTextureEntry::setTexGen() and LLTextureEntry::setMediaFlags() 
> each properly mask off the supplied parameter with their respective bit mask, 
> setMediaTexGen() will always return TEM_CHANGE_MEDIA even if only texgen has 
> changed while the media flag hasn't (causing 
> LLVOVolume::processUpdateMessage() to queue a request to the cap when it 
> shouldn't).
> 
> Changing it to:
> 
> S32 LLTextureEntry::setMediaTexGen(U8 media)
> {
>   S32 result = setTexGen(media & TEM_TEX_GEN_MASK);
>   result |= setMediaFlags(media & TEM_MEDIA_MASK);
>   return result;
> }
> 
> appears to resolve the issue completely (the cap isn't hit unless an object 
> nearby has media on it, or changes its URL)
> 
> 
> This addresses bug STORM-1001.
> http://jira.secondlife.com/browse/STORM-1001
> 
> 
> Diffs
> -
> 
>   indra/llprimitive/lltextureentry.cpp UNKNOWN 
> 
> Diff: http://codereview.secondlife.com/r/162/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kitty
> 
>

___
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: OPEN-38: autobuild support for StandAlone

[Boroondas Gupte]

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


Can you give some instructions on how to do a standalone build with these 
changes? I tried the following:

patch -p1 < /home/das-g/slsrc/patches/autobuild-standalone.diff
autobuild install glh_linear
autobuild configure -c OpenSourceStandAloneRelWithDebInfo

but this fails with:

CMake Error at cmake/FindGLH.cmake:26 (message):
  Could not find GLH


Thanks

- Boroondas


On Feb. 26, 2011, 6:56 a.m., Aleric Inglewood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/167/
> ---
> 
> (Updated Feb. 26, 2011, 6:56 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Oz: please merge into your repository.
> 
> 
> This addresses bug OPEN-38.
> 
> 
> Diffs
> -
> 
>   indra/cmake/FindGLH.cmake PRE-CREATION 
>   indra/cmake/GLH.cmake PRE-CREATION 
>   indra/cmake/LLRender.cmake 5f0ab9443ece 
>   indra/cmake/LLSharedLibs.cmake 5f0ab9443ece 
>   indra/cmake/Linking.cmake 5f0ab9443ece 
>   indra/linux_crash_logger/CMakeLists.txt 5f0ab9443ece 
> 
> Diff: http://codereview.secondlife.com/r/167/diff
> 
> 
> Testing
> ---
> 
> configures, compiles, runs.
> 
> 
> Thanks,
> 
> Aleric
> 
>

___
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: OPEN-38: autobuild support for StandAlone

[Boroondas Gupte]

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

Ship it!


Ah, with that, configuration succeeds and GLH_INCLUDE_DIR gets correctly set to 
the dir with the symlinks. Could the need for that symlink dir be avoided by 
having the user directly set the GLH_INCLUDE_DIR CMake variable?


indra/cmake/Linking.cmake


I thought the removal of the NOT STANDALONE condition here would give 
standalone the same (repository-local) include directories that non-standalone 
has, so that it'd find autobuild-installed dependencies. But it seems those are 
just for the library binaries? Or is this just about the creation of those 
directories, not their addition to the search path?


Building still fails for me, with

Linking CXX shared library libllcommon.so
/usr/lib/gcc/x86_64-pc-linux-gnu/4.4.5/../../../../x86_64-pc-linux-gnu/bin/ld: 
skipping incompatible 
/usr/lib/gcc/x86_64-pc-linux-gnu/4.4.5/../../../libbreakpad_client.so when 
searching for -lbreakpad_client
/usr/lib/gcc/x86_64-pc-linux-gnu/4.4.5/../../../../x86_64-pc-linux-gnu/bin/ld: 
skipping incompatible 
/usr/lib/gcc/x86_64-pc-linux-gnu/4.4.5/../../../libbreakpad_client.a when 
searching for -lbreakpad_client
/usr/lib/gcc/x86_64-pc-linux-gnu/4.4.5/../../../../x86_64-pc-linux-gnu/bin/ld: 
skipping incompatible /usr/lib/libbreakpad_client.so when searching for 
-lbreakpad_client
/usr/lib/gcc/x86_64-pc-linux-gnu/4.4.5/../../../../x86_64-pc-linux-gnu/bin/ld: 
skipping incompatible /usr/lib/libbreakpad_client.a when searching for 
-lbreakpad_client
/usr/lib/gcc/x86_64-pc-linux-gnu/4.4.5/../../../../x86_64-pc-linux-gnu/bin/ld: 
cannot find -lbreakpad_client
collect2: ld returned 1 exit status
make[2]: *** [llcommon/libllcommon.so] Error 1
make[1]: *** [llcommon/CMakeFiles/llcommon.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs

and

In file included from /usr/include/curl/curl.h:36,
 from [...]/indra/llmessage/llcurl.h:39,
 from [...]/indra/llmessage/llurlrequest.h:42,
 from [...]/indra/llmessage/llhttpclient.h:38,
 from [...]/indra/llmessage/message.h:53,
 from [...]/indra/llinventory/llcategory.cpp:30:
/usr/include/curl/curlrules.h:144: error: size of array ‘__curl_rule_01__’ is 
negative
/usr/include/curl/curlrules.h:154: error: size of array ‘__curl_rule_02__’ is 
negative

but I guess those are both 32bit/64bit issues and not related to your changes 
here, so "Ship it!".

- Boroondas


On Feb. 26, 2011, 6:56 a.m., Aleric Inglewood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/167/
> ---
> 
> (Updated Feb. 26, 2011, 6:56 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Oz: please merge into your repository.
> 
> 
> This addresses bug OPEN-38.
> 
> 
> Diffs
> -
> 
>   indra/cmake/FindGLH.cmake PRE-CREATION 
>   indra/cmake/GLH.cmake PRE-CREATION 
>   indra/cmake/LLRender.cmake 5f0ab9443ece 
>   indra/cmake/LLSharedLibs.cmake 5f0ab9443ece 
>   indra/cmake/Linking.cmake 5f0ab9443ece 
>   indra/linux_crash_logger/CMakeLists.txt 5f0ab9443ece 
> 
> Diff: http://codereview.secondlife.com/r/167/diff
> 
> 
> Testing
> ---
> 
> configures, compiles, runs.
> 
> 
> Thanks,
> 
> Aleric
> 
>

___
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: Nearby chat history is displaying both Display Names and user.names when the Display Name is not changed from default.

[Boroondas Gupte]

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

Ship it!



indra/llcommon/llavatarname.cpp


This could be shortened to

if (!mUsername.empty() && !mIsDisplayNameDefault)
{
name = mDisplayName + " (" + mUsername + ")";
}
else
{
// Display names are off, so legacy name is in mDisplayName,
// or Display Name is not changed from (user name based)
// default, so also showing the user name would be redundant.
name = mDisplayName;
}

but I guess the nested ifs make the intention more clear, so go for it.


- Boroondas


On Feb. 19, 2011, 9:32 a.m., ardy.lay wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/153/
> ---
> 
> (Updated Feb. 19, 2011, 9:32 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> https://jira.secondlife.com/browse/VWR-24917
> I have been finding the redundent display of functionally equivalent names in 
> nearby chat history and IM history quite tiresome.
> Simple proposal: If the resident's Display Name is at the default then do not 
> display their user.name.
> https://bitbucket.org/ArdyLay/viewer-development-vwr-24917
> 
> Change is to: LLAvatarName::getCompleteName
> 
> I find the following Callers:
> LLAvatarActions::requestFriendshipDialog
> LLAvatarActions::startIM
> LLAvatarActions::startCall
> LLIMModel::LLIMSession
> LLIMModel::logToFile
> LLPostponedNotification::onAvatarNameCache
> LLUrlEntryAgent::onAvatarNameCache
> LLUrlEntryAgent::getLabel
> LLUrlEntryAgentCompleteName::getName
> 
> // Callback for name resolution of a god/estate message
> llviewermessage.cpp(2149): args["NAME"] = av_name.getCompleteName();
> llviewermessage.cpp(2154): chat.mText = av_name.getCompleteName() + ": " + 
> message;
> 
> static void on_avatar_name_cache_toast ...
> llimview.cpp(108): args["FROM"] = av_name.getCompleteName();
> 
> Some of these make me wonder if this change will cause some defects and 
> should be implimented as a seperate function.
> 
> 
> This addresses bug VWR-24917.
> http://jira.secondlife.com/browse/VWR-24917
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt c10d5e37db1e 
>   indra/llcommon/llavatarname.cpp c10d5e37db1e 
> 
> Diff: http://codereview.secondlife.com/r/153/diff
> 
> 
> Testing
> ---
> 
> I have been using this trivial change and have shared it with a friend, via 
> bitbucket.  We have both built the viewer on Windows 7 and find the resulting 
> reduction in redundent text in chat and IM history on screen to be very 
> helpful.
> 
> 
> Thanks,
> 
> ardy.lay
> 
>

___
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-24889: When a bake texture upload fails, retry instead of giving up.

[Boroondas Gupte]

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



indra/newview/lltexlayer.h


"intermediary" is still misspelled


- Boroondas


On March 1, 2011, 8:28 a.m., Thickbrick Sleaford wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/152/
> ---
> 
> (Updated March 1, 2011, 8:28 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> When a bake upload fails, the viewer doesn't retry it, and subsequently 
> doesn't send a AgentSetAppearance message. This can happen without the user 
> being aware, leaving the avatar looking good on their screen, but not updated 
> to the same outfit on other people's screens. The avatar will remain in that 
> state until the user does something that causes a rebake (manually rebake or 
> change outfit.) The solution here is to retry the upload after a small delay.
> 
> What this diff changes: when a full-res upload fails, retry to upload it 
> after a 5s delay, up to 5 times (in case the cap is available, last attempt 
> is via the old asset store.) Also, some clearer log messages. This implements 
> an old *FIX: comment:
> // *FIX: retry upload after n seconds, asset server could be busy
> 
> This isn't needed for low res uploads, because they don't block subsequent 
> full-res uploads (mNeedsUpload isn't set to FALSE in 
> LLTexLayerSetBuffer::doUpload in low-res uploads.)
> 
> 
> This addresses bug VWR-24889.
> http://jira.secondlife.com/browse/VWR-24889
> 
> 
> Diffs
> -
> 
>   indra/newview/llassetuploadresponders.h 767feb16f05f 
>   indra/newview/llassetuploadresponders.cpp 767feb16f05f 
>   indra/newview/lltexlayer.h 767feb16f05f 
>   indra/newview/lltexlayer.cpp 767feb16f05f 
> 
> Diff: http://codereview.secondlife.com/r/152/diff
> 
> 
> Testing
> ---
> 
> Attempted outfit changes using a problematic connection (not recently used 
> outfits to avoid using cached bakes). Looked for "Baked full res texture 
> upload for  failed" log messages, observed the subsequent 
> retries and successful upload for that region. Observed that eventually the 
> fully-baked avatar is visible to other users.
> 
> 
> Thanks,
> 
> Thickbrick
> 
>

___
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-721) Information about resident is displayed incorrectly in mini-inspector if there are any resident or group SLURLs

[Boroondas Gupte]

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



indra/llui/lltextbase.cpp


Only tangent to your code, but is the scoping (the "{" and "}") here doing 
anything useful? (It causes the "clip" object to be destructed one line 
earlier, but is that the intention?)

Also, it seems the "clip" object is never used. Does it do all its work in 
its constructor?


- Boroondas


On March 1, 2011, 1:53 p.m., Seth ProductEngine wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/169/
> ---
> 
> (Updated March 1, 2011, 1:53 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Fixed text editor to display the embedded widgets only if they are in the 
> currently visible area of a text document.
> 
> 
> This addresses bug STORM-721.
> http://jira.secondlife.com/browse/STORM-721
> 
> 
> Diffs
> -
> 
>   indra/llui/lltextbase.cpp 767feb16f05f 
> 
> Diff: http://codereview.secondlife.com/r/169/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Seth
> 
>

___
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: one-line fix for OPEN-39: (standalone) bitpack_test.o: No such file or directory

[Boroondas Gupte]

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

(Updated March 2, 2011, 5:32 a.m.)


Review request for Viewer and Aleric Inglewood.


Changes
---

(fixed typo in summary)


Summary (updated)
---

On standalone, -I"${TUT_INCLUDE_DIR}" was added to the compile flags for 
integration tests of llcommon when TUT_INCLUDE_DIR might not have been set yet. 
This lead to a flag -I"", which would confuse g++ and lead to the tests' object 
files not being found.

This change makes sure that TUT_INCLUDE_DIR is set by including Tut.cmake, 
which is responsible for setting it.


This addresses bug OPEN-39.
http://jira.secondlife.com/browse/OPEN-39


Diffs
-

  doc/contributions.txt 1013caf84c2e 
  indra/cmake/LLAddBuildTest.cmake 1013caf84c2e 

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


Testing
---

Clean rebuild with LL_TESTS ON (see also repro instructions on jira)


Thanks,

Boroondas

___
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: Nearby chat history is displaying both Display Names and user.names when the Display Name is not changed from default.

[Boroondas Gupte]

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

Ship it!


Aleric's right, this is even cleaner :-)


indra/llcommon/llavatarname.cpp
<http://codereview.secondlife.com/r/153/#comment306>

Nitpicking now, but it should be "i.e.", not "ie". Also, the defaulted 
display name is NOT the same as the user name. (Mine would be "Boroondas Gupte" 
and "boroondas.gupte".) Actually, while one can derive the user name from the 
default display name, one cannot derive the default display name from the user 
name (because the casing is unknown). So maybe make the comment

// If the display name feature is off
// OR this particular display name is defaulted (i.e. based on user 
name),
// then display only the easier to read instance of the person's name.

(I know "based on" is a bit fuzzy, so if someone can think of a more 
specific formulation that is still technically correct and not too compicated, 
go for it.)


- Boroondas


On March 3, 2011, 9:25 p.m., ardy.lay wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/153/
> ---
> 
> (Updated March 3, 2011, 9:25 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> https://jira.secondlife.com/browse/VWR-24917
> I have been finding the redundent display of functionally equivalent names in 
> nearby chat history and IM history quite tiresome.
> Simple proposal: If the resident's Display Name is at the default then do not 
> display their user.name.
> https://bitbucket.org/ArdyLay/viewer-development-vwr-24917
> 
> Change is to: LLAvatarName::getCompleteName
> 
> I find the following Callers:
> LLAvatarActions::requestFriendshipDialog
> LLAvatarActions::startIM
> LLAvatarActions::startCall
> LLIMModel::LLIMSession
> LLIMModel::logToFile
> LLPostponedNotification::onAvatarNameCache
> LLUrlEntryAgent::onAvatarNameCache
> LLUrlEntryAgent::getLabel
> LLUrlEntryAgentCompleteName::getName
> 
> // Callback for name resolution of a god/estate message
> llviewermessage.cpp(2149): args["NAME"] = av_name.getCompleteName();
> llviewermessage.cpp(2154): chat.mText = av_name.getCompleteName() + ": " + 
> message;
> 
> static void on_avatar_name_cache_toast ...
> llimview.cpp(108): args["FROM"] = av_name.getCompleteName();
> 
> Some of these make me wonder if this change will cause some defects and 
> should be implimented as a seperate function.
> 
> 
> This addresses bug VWR-24917.
> http://jira.secondlife.com/browse/VWR-24917
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt c10d5e37db1e 
>   indra/llcommon/llavatarname.cpp c10d5e37db1e 
> 
> Diff: http://codereview.secondlife.com/r/153/diff
> 
> 
> Testing
> ---
> 
> I have been using this trivial change and have shared it with a friend, via 
> bitbucket.  We have both built the viewer on Windows 7 and find the resulting 
> reduction in redundent text in chat and IM history on screen to be very 
> helpful.
> 
> 
> Thanks,
> 
> ardy.lay
> 
>

___
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: Nearby chat history is displaying both Display Names and user.names when the Display Name is not changed from default.

[Boroondas Gupte]

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

Ship it!


Perfect :-)

- Boroondas


On March 4, 2011, 5:11 a.m., ardy.lay wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/153/
> ---
> 
> (Updated March 4, 2011, 5:11 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> https://jira.secondlife.com/browse/VWR-24917
> I have been finding the redundent display of functionally equivalent names in 
> nearby chat history and IM history quite tiresome.
> Simple proposal: If the resident's Display Name is at the default then do not 
> display their user.name.
> https://bitbucket.org/ArdyLay/viewer-development-vwr-24917
> 
> Change is to: LLAvatarName::getCompleteName
> 
> I find the following Callers:
> LLAvatarActions::requestFriendshipDialog
> LLAvatarActions::startIM
> LLAvatarActions::startCall
> LLIMModel::LLIMSession
> LLIMModel::logToFile
> LLPostponedNotification::onAvatarNameCache
> LLUrlEntryAgent::onAvatarNameCache
> LLUrlEntryAgent::getLabel
> LLUrlEntryAgentCompleteName::getName
> 
> // Callback for name resolution of a god/estate message
> llviewermessage.cpp(2149): args["NAME"] = av_name.getCompleteName();
> llviewermessage.cpp(2154): chat.mText = av_name.getCompleteName() + ": " + 
> message;
> 
> static void on_avatar_name_cache_toast ...
> llimview.cpp(108): args["FROM"] = av_name.getCompleteName();
> 
> Some of these make me wonder if this change will cause some defects and 
> should be implimented as a seperate function.
> 
> 
> This addresses bug VWR-24917.
> http://jira.secondlife.com/browse/VWR-24917
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt ef2df52563bb 
>   indra/llcommon/llavatarname.cpp ef2df52563bb 
> 
> Diff: http://codereview.secondlife.com/r/153/diff
> 
> 
> Testing
> ---
> 
> I have been using this trivial change and have shared it with a friend, via 
> bitbucket.  We have both built the viewer on Windows 7 and find the resulting 
> reduction in redundent text in chat and IM history on screen to be very 
> helpful.
> 
> 
> Thanks,
> 
> ardy.lay
> 
>

___
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: Implementation of new testing hooks to enable the creation of a headless viewer for testing.

[Boroondas Gupte]

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


Any chance this big change could be split into several smaller (hopefully 
self-consistent) changes that could be reviewed separately? Dunno if that's 
feasible. If not, please at least push the hg changesets included in this 
change to some publicly readable repo, so we can look at those individually.

- Boroondas


On March 7, 2011, 3:22 p.m., Merov Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/181/
> ---
> 
> (Updated March 7, 2011, 3:22 p.m.)
> 
> 
> Review request for Viewer and Oz Linden.
> 
> 
> Summary
> ---
> 
> Massive changes implementing a set of new testing hooks so to enable the 
> creation of a headless viewer for testing.
> 
> 
> This addresses bug STORM-1051.
> http://jira.secondlife.com/browse/STORM-1051
> 
> 
> Diffs
> -
> 
>   .hgtags 8b9f50878cc7 
>   indra/llcommon/CMakeLists.txt 8b9f50878cc7 
>   indra/llcommon/lleventdispatcher.h 8b9f50878cc7 
>   indra/llcommon/lleventdispatcher.cpp 8b9f50878cc7 
>   indra/llcommon/llevents.h 8b9f50878cc7 
>   indra/llcommon/llevents.cpp 8b9f50878cc7 
>   indra/llcommon/llsdutil.h 8b9f50878cc7 
>   indra/llcommon/llsdutil.cpp 8b9f50878cc7 
>   indra/llcommon/tests/lleventdispatcher_test.cpp PRE-CREATION 
>   indra/llrender/llgl.h 8b9f50878cc7 
>   indra/llrender/llgl.cpp 8b9f50878cc7 
>   indra/llrender/llimagegl.cpp 8b9f50878cc7 
>   indra/llui/llfloaterreglistener.h 8b9f50878cc7 
>   indra/llui/llfloaterreglistener.cpp 8b9f50878cc7 
>   indra/llui/llui.h 8b9f50878cc7 
>   indra/llui/llui.cpp 8b9f50878cc7 
>   indra/llui/tests/llurlentry_stub.cpp 8b9f50878cc7 
>   indra/llwindow/CMakeLists.txt 8b9f50878cc7 
>   indra/llwindow/llkeyboardheadless.h PRE-CREATION 
>   indra/llwindow/llkeyboardheadless.cpp PRE-CREATION 
>   indra/llwindow/llwindow.h 8b9f50878cc7 
>   indra/llwindow/llwindow.cpp 8b9f50878cc7 
>   indra/llwindow/llwindowheadless.cpp 8b9f50878cc7 
>   indra/llwindow/llwindowlistener.h PRE-CREATION 
>   indra/llwindow/llwindowlistener.cpp PRE-CREATION 
>   indra/newview/CMakeLists.txt 8b9f50878cc7 
>   indra/newview/app_settings/settings.xml 8b9f50878cc7 
>   indra/newview/llagent.cpp 8b9f50878cc7 
>   indra/newview/llagentcamera.cpp 8b9f50878cc7 
>   indra/newview/llagentlistener.h 8b9f50878cc7 
>   indra/newview/llagentlistener.cpp 8b9f50878cc7 
>   indra/newview/llappviewer.cpp 8b9f50878cc7 
>   indra/newview/llfloaterbump.cpp 8b9f50878cc7 
>   indra/newview/llhudeffectlookat.cpp 8b9f50878cc7 
>   indra/newview/llhudmanager.cpp 8b9f50878cc7 
>   indra/newview/llimview.cpp 8b9f50878cc7 
>   indra/newview/lllogininstance.cpp 8b9f50878cc7 
>   indra/newview/llselectmgr.cpp 8b9f50878cc7 
>   indra/newview/llsidetray.h 8b9f50878cc7 
>   indra/newview/llsidetray.cpp 8b9f50878cc7 
>   indra/newview/llsidetraylistener.h PRE-CREATION 
>   indra/newview/llsidetraylistener.cpp PRE-CREATION 
>   indra/newview/llstartup.cpp 8b9f50878cc7 
>   indra/newview/llsurface.cpp 8b9f50878cc7 
>   indra/newview/lltexturestats.cpp 8b9f50878cc7 
>   indra/newview/lluilistener.h 8b9f50878cc7 
>   indra/newview/lluilistener.cpp 8b9f50878cc7 
>   indra/newview/llviewerdisplay.cpp 8b9f50878cc7 
>   indra/newview/llviewermessage.cpp 8b9f50878cc7 
>   indra/newview/llviewerobject.cpp 8b9f50878cc7 
>   indra/newview/llviewerobjectlist.cpp 8b9f50878cc7 
>   indra/newview/llviewerparcelmgr.cpp 8b9f50878cc7 
>   indra/newview/llviewerregion.cpp 8b9f50878cc7 
>   indra/newview/llviewerstats.cpp 8b9f50878cc7 
>   indra/newview/llviewertexture.cpp 8b9f50878cc7 
>   indra/newview/llviewertexturelist.cpp 8b9f50878cc7 
>   indra/newview/llviewerwindow.cpp 8b9f50878cc7 
>   indra/newview/llviewerwindowlistener.cpp 8b9f50878cc7 
>   indra/newview/llvoavatar.cpp 8b9f50878cc7 
>   indra/newview/llvoavatarself.cpp 8b9f50878cc7 
>   indra/newview/llworld.cpp 8b9f50878cc7 
>   indra/newview/pipeline.cpp 8b9f50878cc7 
> 
> Diff: http://codereview.secondlife.com/r/181/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Merov
> 
>

___
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: make PREHASH variables char const* const (fixes VWR-24487: llurlentry_stub.cpp:196: error: deprecated conversion from string constant to 'char*')

[Boroondas Gupte]

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

(Updated March 7, 2011, 3:51 p.m.)


Review request for Viewer and Seth ProductEngine.


Changes
---

(just mentioning the jira issue in the summary)


Summary (updated)
---

For the reason for this change, see 
https://jira.secondlife.com/browse/VWR-24487 and 
https://jira.secondlife.com/browse/VWR-24522

What I did:
In indra/llmessage/message_prehash.(h|cpp), I turned everything into constant 
pointers to constants by search/replace. Then I tried to compile and added 
const qualifiers in dependent code as needed to stop the compiler complaining.

Note that comments in indra/llmessage/message_prehash.(h|cpp) say these files 
have been generated from the message template. Because this generation might 
not have been a one-off thing, I changed the generating code, too, so it won't 
override this change here when the generation happens the next time. However, 
that part of the code is not called by Viewer, although the relevant function — 
dump_prehash_files() — ends up in the Viewer binary. That function probably 
gets called by the simulator, when one runs the latter with -prehash. (See 
https://bitbucket.org/lindenlab/viewer-development/src/fc7e5dcf3059/indra/llmessage/message.cpp#cl-2532
 )


This addresses bug VWR-24487.
http://jira.secondlife.com/browse/VWR-24487


Diffs
-

  doc/contributions.txt fc7e5dcf3059 
  indra/llmessage/message.cpp fc7e5dcf3059 
  indra/llmessage/message_prehash.h fc7e5dcf3059 
  indra/llmessage/message_prehash.cpp fc7e5dcf3059 
  indra/llprimitive/llprimitive.h fc7e5dcf3059 
  indra/llprimitive/llprimitive.cpp fc7e5dcf3059 
  indra/llprimitive/llvolumemessage.h fc7e5dcf3059 
  indra/llprimitive/llvolumemessage.cpp fc7e5dcf3059 
  indra/llui/tests/llurlentry_stub.cpp fc7e5dcf3059 
  indra/newview/tests/llremoteparcelrequest_test.cpp fc7e5dcf3059 

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


Testing
---

Compiled (standalone, 64bit Linux) with and without LL_TESTS.
Started the viewer, logged in, walked and flew around a bit. Everything seems 
to work.


Locally set _PREHASH_AgentData and _PREHASH_AgentID to (char const*)0x1 in 
indra/llui/tests/llurlentry_stub.cpp and 
indra/newview/tests/llremoteparcelrequest_test.cpp to verify they actually are 
never dereferenced, even when not NULL, so that using NULL pointers instead of 
place holder data won't change what code paths gets tested. Both tests binaries 
executed without crashes and all the contained tests passed.

Locally invoked start_messaging_system() with b_dump_prehash_file == true 
instead of FALSE, to see what would be generated after my change to 
dump_prehash_files().
The message_prehash.(h|cpp) generated by that had the correct type qualifiers 
and formatting, but some lines were removed or added compared to the modified 
files from the source tree. That probably means that the files aren't fully 
synchronized with the message template file in the source tree. Because the 
"added" constants are spread all over the file, while the "removed" ones were 
at the end, I'd wager that message_prehash.(h|cpp) in the viewer source tree 
are actually more up-to-date than the message template file in the source tree.


Thanks,

Boroondas

___
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