[opensource-dev] Review Request: Local Bitmap Browser implementation.

[Vadim ProductEngine]

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


Briefly tested. Works well and looks like a great feature.

But I'm not a fan of the code design: each class knows too much about the others. I started describing specific design flaws but gave up: there are too many of them.
Please consider improving encapsulation and adding more comments.

Thanks.


indra/newview/llfloaterlocalbitmap.h
<http://codereview.secondlife.com/r/347/#comment749>

    Please move this to the .cpp.



indra/newview/llfloaterlocalbitmap.h
<http://codereview.secondlife.com/r/347/#comment751>

    Move to the .cpp file.



indra/newview/llfloaterlocalbitmap.h
<http://codereview.secondlife.com/r/347/#comment752>

    Consider using enums instead.



indra/newview/llfloaterlocalbitmap.h
<http://codereview.secondlife.com/r/347/#comment770>

    Please drop the "Bool" suffix: a method name should describe what it does, not what type it returns.



indra/newview/llfloaterlocalbitmap.h
<http://codereview.secondlife.com/r/347/#comment769>

    spelling



indra/newview/llfloaterlocalbitmap.h
<http://codereview.secondlife.com/r/347/#comment764>

    Does this class implement a browser?
    Looks more like some kind of helper/manager class.
    If so, I'd rename it.



indra/newview/llfloaterlocalbitmap.h
<http://codereview.secondlife.com/r/347/#comment763>

    What is this floater for?
    Isn't it enough to have the "Local" tab in the texture picker?



indra/newview/llfloaterlocalbitmap.h
<http://codereview.secondlife.com/r/347/#comment756>

    Inconsistent indentation.
    Use tabs everywhere in C++ code.



indra/newview/llfloaterlocalbitmap.h
<http://codereview.secondlife.com/r/347/#comment762>

    A better description would not hurt.



indra/newview/llfloaterlocalbitmap.cpp
<http://codereview.secondlife.com/r/347/#comment754>

    Using LLSingleton would be safer.



indra/newview/llfloaterlocalbitmap.cpp
<http://codereview.secondlife.com/r/347/#comment760>

    No need to to use this for accessing members: this is why they have a distinctive naming style.



indra/newview/llfloaterlocalbitmap.cpp
<http://codereview.secondlife.com/r/347/#comment757>

    Inconsistent spaces here and there.
    Please use the following style: "if (cond)".
    This is how most of our code is written.



indra/newview/llfloaterlocalbitmap.cpp
<http://codereview.secondlife.com/r/347/#comment758>

    Am I right assuming that viewer_texture manages life time of raw_image?



indra/newview/llfloaterlocalbitmap.cpp
<http://codereview.secondlife.com/r/347/#comment759>

    delete raw_image if decoding fails?



indra/newview/llfloaterlocalbitmap.cpp
<http://codereview.secondlife.com/r/347/#comment761>

    Please follow the coding standard:
    http://wiki.secondlife.com/wiki/Coding_standard#Braces



indra/newview/llfloaterlocalbitmap.cpp
<http://codereview.secondlife.com/r/347/#comment771>

    Memory leak?
    Shouldn't we delete the "unit" if it's not valid?



indra/newview/llfloaterlocalbitmap.cpp
<http://codereview.secondlife.com/r/347/#comment772>

    1. Any reason not to pass vector by reference?
    2. Why should LLLocalBitmapBrowser know about some external scroll lists and their columns? I think it should just be passed a list of image IDs.



indra/newview/llfloaterlocalbitmap.cpp
<http://codereview.secondlife.com/r/347/#comment773>

    I think this check should be moved to LLLocalBitmapBrowserTimer::start() for better encapsulation.



indra/newview/llfloaterlocalbitmap.cpp
<http://codereview.secondlife.com/r/347/#comment774>

    same here



indra/newview/llfloaterlocalbitmap.cpp
<http://codereview.secondlife.com/r/347/#comment767>

    Looks like this code was written a long time ago.
    We now avoid using void* in favor of boost::bind().



indra/newview/llfloaterlocalbitmap.cpp
<http://codereview.secondlife.com/r/347/#comment768>

    same here and below



indra/newview/llfloaterlocalbitmap.cpp
<http://codereview.secondlife.com/r/347/#comment765>

    This naming style is reserved for static members.



indra/newview/llfloaterlocalbitmap.cpp
<http://codereview.secondlife.com/r/347/#comment766>

    same here



indra/newview/lltexturectrl.cpp
<http://codereview.secondlife.com/r/347/#comment744>

    You can just omit the second argument or pass LLSD().
    NULL leads to gcc compilation error.



indra/newview/lltexturectrl.cpp
<http://codereview.secondlife.com/r/347/#comment746>

    Looks like too much copy&paste.
    Please try reusing this code in both overridden methods.



indra/newview/skins/default/xui/en/floater_texture_ctrl.xml
<http://codereview.secondlife.com/r/347/#comment755>

    Mixed tabs and spaces (here and below).


- Vadim


On June 18, 2011, 8:04 a.m., Vaalith Jinn wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/347/
> -----------------------------------------------------------
> 
> (Updated June 18, 2011, 8:04 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> -------
> 
> Local Bitmap Browser is a mechanism to locally load images into the viewer, track them and optionally (per each image)
> have it check if the image has been overwritten locally and if so - update it in the viewer and inworld.
> 
> This change affects build menu by adding a way to open the floater there.
> This change also affects the texture picker - adding tabs that lets the user choose between the "Server" (regular inventory) and "Local" tabs (list of locally added files).
> 
> 
> This addresses bug STORM-64.
>     http://jira.secondlife.com/browse/STORM-64
> 
> 
> Diffs
> -----
> 
>   doc/contributions.txt 6a3e7e403bd1 
>   indra/llmath/llvolumemgr.h 6a3e7e403bd1 
>   indra/llmath/llvolumemgr.cpp 6a3e7e403bd1 
>   indra/newview/CMakeLists.txt 6a3e7e403bd1 
>   indra/newview/llfloaterlocalbitmap.h PRE-CREATION 
>   indra/newview/llfloaterlocalbitmap.cpp PRE-CREATION 
>   indra/newview/lltexturectrl.h 6a3e7e403bd1 
>   indra/newview/lltexturectrl.cpp 6a3e7e403bd1 
>   indra/newview/llviewerfloaterreg.cpp 6a3e7e403bd1 
>   indra/newview/llviewerobjectlist.h 6a3e7e403bd1 
>   indra/newview/llviewertexturelist.h 6a3e7e403bd1 
>   indra/newview/llvovolume.h 6a3e7e403bd1 
>   indra/newview/skins/default/xui/en/floater_local_bitmap.xml PRE-CREATION 
>   indra/newview/skins/default/xui/en/floater_texture_ctrl.xml 6a3e7e403bd1 
>   indra/newview/skins/default/xui/en/menu_viewer.xml 6a3e7e403bd1 
> 
> Diff: http://codereview.secondlife.com/r/347/diff
> 
> 
> Testing
> -------
> 
> Texture/Sculptmap/Avatar Layer show.
> Texture/Sculptmap/Avatar Layer update.
> Multiple sculpties update.
> Opening multiple browser floaters works correctly. (two possible since one can be opened from menu above, one from texture picker. all of them + texture picker play nice.)
> Tested adding over 200 images at the same time.
> 
> 
> Thanks,
> 
> Vaalith
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.secondlife.com/pipermail/opensource-dev/attachments/20110620/f2458166/attachment-0001.htm