[opensource-dev] Review Request: STORM-1220 Region / Estate > Covenant - Type of region is in EN: "Estate / Full Region"

[Boroondas Gupte]

> On July 25, 2011, 10:44 a.m., Boroondas Gupte wrote:
> > indra/newview/llviewerregion.h, line 195
> > <http://codereview.secondlife.com/r/414/diff/2/?file=6759#file6759line195>
> >
> >     Can (and maybe should) be made private now. I don't think any other class has a valid reason to use the unlocalized name.
> >     
> >     Or just remove it, and use directly use mProductName in getLocalizedSimProductName().
> 
> Vadim ProductEngine wrote:
>     I can do it if you insist. However, I'm not sure that the sim product name is completely useless without translation. There is lots of stuff in the viewer not directly related to UI.

I certainly don't insist, merely suggest. As far as I have seen, after your change getSimProductName will only be used by getLocalizedSimProductName, so there is currently no other code needing it. I believe that any new code is more likely to need getLocalizedSimProductName than getSimProductName, so hiding the latter could avoid oversights. If someone knows he needs the unlocalized name (e.g. for logging, which we always do in English), they can still make getSimProductName public again, but has to make a conscious decision for that.

If you considered this and decided getSimProductName should stay public for now, anyway, I'll trust your judgment on that. For avoiding people picking the wrong method, making getSimProductName private or removing it is of course not the only possibility. Renaming getSimProductName to getEnglishSimProductName or getUnlocalizedSimProductName and/or adding some doxigen documentation indicating what it will return would probably serve the same purpose well enough.


- Boroondas


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


On July 25, 2011, 7:36 a.m., Vadim ProductEngine wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/414/
> -----------------------------------------------------------
> 
> (Updated July 25, 2011, 7:36 a.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> -------
> 
> Localized sim product name (e.g. "Estate / Full Region") everywhere.
> 
> 
> This addresses bug STORM-1220.
>     http://jira.secondlife.com/browse/STORM-1220
> 
> 
> Diffs
> -----
> 
>   indra/newview/llfloaterbuyland.cpp UNKNOWN 
>   indra/newview/llfloaterland.cpp UNKNOWN 
>   indra/newview/llfloaterregioninfo.cpp UNKNOWN 
>   indra/newview/llpanelplaceprofile.cpp UNKNOWN 
>   indra/newview/llviewerregion.h UNKNOWN 
>   indra/newview/llviewerregion.cpp UNKNOWN 
> 
> Diff: http://codereview.secondlife.com/r/414/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Vadim
> 
>

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