[opensource-dev] Review Request: The world map can point to the wrong URL

[Merov Linden]

> On 2010-12-23 05:16:08, Vadim ProductEngine wrote:
> > indra/newview/llstartup.cpp, lines 3098-3099
> > <http://codereview.secondlife.com/r/61/diff/1/?file=230#file230line3098>
> >
> >     Frankly speaking, I'm not a fan of adding another setting to only use it as a global variable.
> >     
> >     I would search for a more proper way, maybe adding get/setMapServerURL() methods to LLWorldMap.
> >     Perhaps a person more familiar with the world map code than me would suggest a better approach.
> 
> Merov Linden wrote:
>     I don't like it either but, unfortunately, the LLWorldMap is lazy instantiated in LLFloaterWorldMap::createWorldMapView() and I can't guarantee that it exists at that point in llstartup.cpp. So I either store the map_server_url response in an ad-hoc global or, as I did, in a setting which a different sort of global when you think about it but somewhat cleaner (documented at least and with specific usage). I choose the second solution.
>     
>
> 
> Vadim ProductEngine wrote:
>     I see. Then what about storing the URL in a static member of LLWorldMap (and using static getter/setter), so we don't need it to be instantiated? Or even store in LLWorldMipmap?

Hmmm... That'll require to include llworldmipmap.h or llworldmap.h in llstartup.cpp, creating yet another dependency. It's possible but I don't like it. Hard coupling (explicit calls between object instances) can become issues later, with too many code snippets knowing too many objects by name just because they need one generic access to them. Here, llstartup shouldn't have any notion of the existence of a map. It just gets some data at login from the server and save them for whoever needs them later. I prefer soft coupling where a general context is created (here by settings) and have object instances checking this context when needed. That scheme has issues also but it's more flexible. Imagine for instance what we throw the map UI and use a web based panel to display the map in the viewer (likely scenario BTW). Whoever code that will simply get the root URL from the settings, ignoring when and how it got there and ignoring the old llworldmipmap. If we create a static in llworldmipmap, that devs will have to keep llworldmipmap around or relocate that static in a new object, therefore modifying llstartup.
The bottom line I think is that llstartup shouldn't be concerned by the existence of a map object. It receives some info at login and should save them in a global context. May be we should have a global "grid" context/global but I don't think we have one and the closest thing to it is a set of data saved in settings.


- Merov


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


On 2010-12-22 22:15:00, Merov Linden wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/61/
> -----------------------------------------------------------
> 
> (Updated 2010-12-22 22:15:00)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> -------
> 
> Implements the processing of map-server-url correctly so not to overwrite the default value (which can still be useful if a grid does not implement map-server-url).
> 
> 
> This addresses bug STORM-805.
>     http://jira.secondlife.com/browse/STORM-805
> 
> 
> Diffs
> -----
> 
>   indra/newview/app_settings/settings.xml 5d69e36a53ee 
>   indra/newview/llstartup.cpp 5d69e36a53ee 
>   indra/newview/llworldmipmap.cpp 5d69e36a53ee 
> 
> Diff: http://codereview.secondlife.com/r/61/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Merov
> 
>

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