> 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? > > Merov Linden wrote: > 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.
Right, sounds reasonable. - Vadim ----------------------------------------------------------- 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 > >
_______________________________________________ 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
