> On March 29, 2012, 5:28 a.m., Vaalith Jinn wrote: > > indra/newview/lllocalbitmaps.h, line 42 > > <http://codereview.secondlife.com/r/566/diff/1/?file=7720#file7720line42> > > > > For clarity's sake i kept private enums block below where it's > > currently at. > > > > If i were to add an enum value in here i'd have to either move the > > whole block up to keep consistency or break consistency and write in a > > single enum below the main public block.. > > > > Neither is a really pretty solution as far as readability is concerned. > > > > Is it really worth it? > > Boroondas Gupte wrote: > I think it'd be worth it, as it'd make the calling code much more > readable. See > http://doc.qt.nokia.com/qq/qq13-apis.html#thebooleanparametertrap > > Note that in the example at hand, the function will only be called with > boolean literals, not boolean variables, so we don't have the problem of > forcing additional if-else code onto the caller. Also, half if not all of the > lengthy comment above the one place where you call the function with "true" > could be saved if a well-named constant or enum would be passed instead.
Um... several questions: A) Unless i am misreading, are you implying that an enum would make it easier to understand than the existing albeit lengthy comment? B) The way i designed it is actually the way one would see the mechanism: The updateself function updates by default, it's what it does no more no less. The bool is there to indicate a special case, special because in the life of a single local bitmap object it is only used once and that is during it's creation. So to me it seems that forcing the function to take a "run regularly" enum parameter during each regular, non first-run call would be something akin to stating the obvious, which would seem redundant to me to the point of causing confusion. - Vaalith ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/566/#review1199 ----------------------------------------------------------- On March 30, 2012, 4:06 p.m., Vaalith Jinn wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://codereview.secondlife.com/r/566/ > ----------------------------------------------------------- > > (Updated March 30, 2012, 4:06 p.m.) > > > Review request for Viewer, Callum Linden and Richard Nelson. > > > Description > ------- > > Local Bitmaps 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 the texture picker - adding radio checks that let the > user choose between the "Inventory" (regular inventory) and "Local" tabs > (list of locally added files). > > *** DO NOT USE THIS YET *** > do not implement for live viewers until fully reviewed. > > > This addresses bug STORM-64. > http://jira.secondlife.com/browse/STORM-64 > > > Diffs > ----- > > doc/contributions.txt 96a53bafcd1c > indra/newview/CMakeLists.txt 96a53bafcd1c > indra/newview/lllocalbitmaps.h PRE-CREATION > indra/newview/lllocalbitmaps.cpp PRE-CREATION > indra/newview/lltexturectrl.h 96a53bafcd1c > indra/newview/lltexturectrl.cpp 96a53bafcd1c > indra/newview/llviewertexturelist.h 96a53bafcd1c > indra/newview/llwearable.h 96a53bafcd1c > indra/newview/llwearable.cpp 96a53bafcd1c > indra/newview/skins/default/xui/en/floater_texture_ctrl.xml 96a53bafcd1c > > Diff: http://codereview.secondlife.com/r/566/diff/diff > > > Testing > ------- > > > Thanks, > > Vaalith Jinn > >
_______________________________________________ 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
