Re: [opensource-dev] Review Request: VWR-24312: Massively duplicated objects (part 2)
On Jan. 14, 2011, 1:31 p.m., Boroondas Gupte wrote: indra/llcommon/lllslconstants.h, line 184 http://codereview.secondlife.com/r/81/diff/1/?file=392#file392line184 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
--- 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.
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/80/#review167 --- Ship it! scripts/install.py http://codereview.secondlife.com/r/80/#comment106 Please add a comment regular directories (no symlinks) ... scripts/install.py http://codereview.secondlife.com/r/80/#comment107 ... 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
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? 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
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: (STORM-383) Context menu cannot be open for Landmark that are located in the My inventory-Trash folder
--- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/77/#review170 --- indra/newview/llpanellandmarks.cpp http://codereview.secondlife.com/r/77/#comment110 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 http://codereview.secondlife.com/r/77/#comment111 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
I'm afraid that VWR-24366 won't reduce link times significantly. On Fri, Jan 14, 2011 at 11:49 PM, Boroondas Gupte slli...@boroon.dasgupta.ch 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
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() getChildLLUICtrl(more_btn)-setVisible(!is_more); getChildLLUICtrl(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 = getChildLLButton(more_btn)-getToggleState(); the two statements Jonathan shows are changed to just one getChildLLUICtrl(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
