Re: [opensource-dev] Review Request: VWR-24311: Uninstall packages that are renewed.

2011-01-16 Thread Aleric Inglewood 

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


Changes
---

This new diff addresses Boroondas' list/tuple remark.

I also added a fix to remove empty directories. Before this patch, install.py 
would only delete (empty) directories that were a part of the path of a deleted 
file. The problem with that is that if a tar ball contains an empty directory, 
it is never removed. I ran into this while running new tests. Fully tested (on 
linux). Thanks to Boroondas for assistance with the python syntax.


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 (updated)
-

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

2011-01-16 Thread Aleric Inglewood 

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

(Updated Jan. 16, 2011, 5:53 a.m.)


Review request for Viewer.


Changes
---

Changed tab's into spaces in the part that Boroondas pointed out (in 
indra/llcharacter/llanimationstates.cpp).


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 (updated)
-

  doc/contributions.txt 422f636c3343 
  indra/llcharacter/llanimationstates.cpp 422f636c3343 
  indra/llcommon/llavatarconstants.h 422f636c3343 
  indra/llcommon/lllslconstants.h 422f636c3343 
  indra/llcommon/llmetricperformancetester.h 422f636c3343 
  indra/llmath/llcamera.h 422f636c3343 
  indra/llmath/llcamera.cpp 422f636c3343 
  indra/newview/llviewerobject.cpp 422f636c3343 
  indra/newview/llvoavatar.cpp 422f636c3343 
  indra/newview/llvosky.h 422f636c3343 
  indra/newview/llvosky.cpp 422f636c3343 

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

2011-01-16 Thread Aleric Inglewood 


> On Jan. 14, 2011, 1:31 p.m., Boroondas Gupte wrote:
> > indra/llcommon/lllslconstants.h, line 184
> > 
> >
> > Yay for having type modifiers after the core type name. Makes them much 
> > easier to understand, especially when there are several cascaded ones. :-)

Yeah, I'm strongly convinced that TYPE const is superior in anyway over const 
TYPE.
See http://www.xs4all.nl/~carlo17/cpp/const.qualifier.html for the reasoning.
In one line: all type qualifiers work to the left, it's best to PUT them on the
left so the whole type is logically uniform in it's construction. The fact that
you can legally type 'const TYPE' is just a historically grown misfortune.
 


- Aleric


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


On Jan. 16, 2011, 5:53 a.m., Aleric Inglewood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/81/
> ---
> 
> (Updated Jan. 16, 2011, 5:53 a.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 422f636c3343 
>   indra/llcharacter/llanimationstates.cpp 422f636c3343 
>   indra/llcommon/llavatarconstants.h 422f636c3343 
>   indra/llcommon/lllslconstants.h 422f636c3343 
>   indra/llcommon/llmetricperformancetester.h 422f636c3343 
>   indra/llmath/llcamera.h 422f636c3343 
>   indra/llmath/llcamera.cpp 422f636c3343 
>   indra/newview/llviewerobject.cpp 422f636c3343 
>   indra/newview/llvoavatar.cpp 422f636c3343 
>   indra/newview/llvosky.h 422f636c3343 
>   indra/newview/llvosky.cpp 422f636c3343 
> 
> 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: remove: Attempting to remove filename: /ramdisk/imprudence/cache/textures/*/*.texture

2011-01-16 Thread Aleric Inglewood 

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

(Updated Jan. 16, 2011, 6:12 a.m.)


Review request for Viewer.


Changes
---

Renamed the variable to 'file_maybe_exists', which should cover its meaning 
more accurately, thanks!

Changed the warning into

LL_WARNS("TextureCache") << "Entry has body size of zero but file " << filename 
<< " exists. Deleting this file, too." << LL_ENDL;

The second part is the 'resolution' of the problem. Your proposal sounded a bit 
too much as if the warning was about the fact that the file was being removed...


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 (updated)
-

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

2011-01-16 Thread 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

2011-01-16 Thread Aleric Inglewood 


> On Jan. 14, 2011, 1:47 p.m., Boroondas Gupte wrote:
> > indra/newview/llappviewer.cpp, lines 3091-3094
> > 
> >
> > what's the reason for moving the LL_INFOS around?

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.


- Aleric


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

2011-01-16 Thread Boroondas Gupte 


> On Jan. 14, 2011, 1:47 p.m., Boroondas Gupte wrote:
> > indra/newview/llappviewer.cpp, lines 3091-3094
> > 
> >
> > 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: (STORM-383) Context menu cannot be open for Landmark that are located in the My inventory->Trash folder

2011-01-16 Thread Vadim ProductEngine 

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



indra/newview/llpanellandmarks.cpp


Why are the checks for are_all_items_in_trash and are_any_items_in_trash 
different?

I guess it should be something like:

  bool item_in_trash = listenerp->isItemInTrash() && *iter != trash_id;
  are_all_items_in_trash &= item_in_trash;
  are_any_items_in_trash |= item_in_trash;



indra/newview/llpanellandmarks.cpp


Although we use the same approach in the Appearance SP for 
enabling/displaying context menu items, it may be not what users expect. Please 
clearly state this behavior in the ticket comment for PO to notice.


- Vadim


On Jan. 14, 2011, 4:13 p.m., Seth ProductEngine wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/77/
> ---
> 
> (Updated Jan. 14, 2011, 4:13 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Added "Restore Item" context menu entry for landmarks and folders in Trash 
> category in Places->My Landmarks->My Inventory accordion tab.
> 
> 
> This addresses bug STORM-383.
> http://jira.secondlife.com/browse/STORM-383
> 
> 
> Diffs
> -
> 
>   indra/newview/llpanellandmarks.h 422f636c3343 
>   indra/newview/llpanellandmarks.cpp 422f636c3343 
>   indra/newview/skins/default/xui/en/menu_places_gear_folder.xml 422f636c3343 
>   indra/newview/skins/default/xui/en/menu_places_gear_landmark.xml 
> 422f636c3343 
> 
> Diff: http://codereview.secondlife.com/r/77/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] Link times

2011-01-16 Thread Aleric Inglewood 
I'm afraid that VWR-24366 won't reduce link times significantly.

On Fri, Jan 14, 2011 at 11:49 PM, Boroondas Gupte
 wrote:
> 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
>
___
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

2011-01-16 Thread Jonathan Yap 


> On Jan. 13, 2011, 8:05 a.m., Twisted Laws wrote:
> > To me, this is the wrong solution.  label_selected used to work to allow a 
> > button to display different text when it was selected, so you could have a 
> > button that said More until it was pressed or selected and displayed more 
> > informaiton.  At that time the text could change to Less indicating that 
> > pressing the button a second time would maybe reduce the information 
> > displayed.  If the button in question here doesn't change when pressing, 
> > that means that something down in the control code is wrong.
> 
> Jonathan Yap wrote:
> I did a test with the floater preview UI (one of the few places that 
> label is not the same as label_selected.  There is a >> button there that 
> does change to << when pressed.
> 
> However, in this case, there is some code that does the button swapping.  
> In llpanelnearbymedia.cpp / void LLPanelNearByMedia::onMoreLess()
> 
>   getChild("more_btn")->setVisible(!is_more);
>   getChild("less_btn")->setVisible(is_more);
> 
> If people think refactoring rather then just rewording is the way to go 
> please say so.  I always hesitate to "fix what ain't broke".
> 
> The vast majority of button definitions in the xml files have identical 
> entries for label and label_selected (is having label_selected present a 
> requirement in a button definition or are all these entires superfluous?).
> 
> Twisted Laws wrote:
> I looked into this and have a patch that corrects it.  What the patch 
> does in llPanelNearByMedia::onMoreLess(), the first statement is changed to 
> bool is_more = getChild("more_btn")->getToggleState();  the two 
> statements Jonathan shows are changed to just one 
> getChild("more_btn")->setVisible(true); (may just be removed and 
> not be necessary) and in the xml file, the first more_btn gets 
> is_toggle="true" added and the less_btn is removed.  My patch file is 
> attached to STORM-844.

I applied this patch and you can view it via the link to the repository in the 
jira.


- Jonathan


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


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