Create an OverlayManager class

[GoogleCodeExporter]

The overlays are an important part of the maps, and they are only going to get 
more sophisticated. We should probably move much of the overlay functionality 
to an OverlayManager class. This would handle:

1. Synchronized access to the overlays - Adding, removing, drawing are all 
performed in a probably unsafe way now. By controlling the overlay collection, 
we can synchronize access to the overlays.
2. Consolidated Overlay events - the MapView has its share of work to do 
responding to events, and we should keep any work for the overlays out of that 
class. So MapView would handle click/gesture events how it wants to and then 
pass them to the OverlayManager to let the overlays respond.
3. The effort would close issue 51, issue 116, and issue 125

Agreed?

Original issue reported on code.google.com by kurtzm...@gmail.com on 4 Feb 2011 at 4:43

[GoogleCodeExporter]

Makes sense.

Original comment by neilboyd on 4 Feb 2011 at 6:22

[GoogleCodeExporter]

See r776. First revision includes thread safety, and migration of existing 
functionality to new manager class.

Original comment by kurtzm...@gmail.com on 5 Feb 2011 at 9:56

[GoogleCodeExporter]

What about option menu handling in the overlays? I'd like to allow overlays to 
add option menu items. This would allow something like the MyLocationOverlay to 
provide the "Enable Compass" and "My Location" option menu items in 
OpenStreetMapViewer to all SDK users without requiring them to rewrite the code 
themselves.

The issue arises with resources and localization. We can provide default icons 
and strings in the jar, but people will want to provide localized strings and 
maybe even icons. I haven't really dug into the resource proxy classes yet, but 
I assume that is a solution to this problem.

Original comment by kurtzm...@gmail.com on 6 Feb 2011 at 2:59

[GoogleCodeExporter]

Okay - looked into the ResourceProxy class and it makes sense. I implemented 
"Compass" and "Location" option menu items in MyLocationOverlay, "Map mode" in 
TilesOverlay, and "Offline/Online mode" in the OverlayManager class (since it 
is a global setting rather than a particular overlay setting). Everything works 
nicely.

I just have to add a mechanism to turn on/off adding options menu items on a 
per-Overlay basis and I'll commit.

Original comment by kurtzm...@gmail.com on 6 Feb 2011 at 6:20

[GoogleCodeExporter]

Added option menu handling, GestureDetector handling, and 
GestureDetector.OnDoubleTapListener handling.

Rather than doing virtual functions to dispense events, maybe it would be wise 
to create a class that allows overlays to subscribe to only the specific events 
they want. We are pushing a lot of events now, and probably wasting CPU cycles. 
Is there a common pattern or class to use for this in Java?

Original comment by kurtzm...@gmail.com on 7 Feb 2011 at 4:13

[GoogleCodeExporter]

I notice that you removed MapView.getOverlays().  That makes it inconsistent 
with Google MapView.  How about you add it back, but just pass it through to 
the OverlayManager?

Or a bigger question: is it the intention that people do stuff directly with 
the manager?  Could you make it private (or remove MapView.getOverlayManager())?

Original comment by neilboyd on 9 Feb 2011 at 9:00

[GoogleCodeExporter]

Or how about making OverlayManager implement List<Overlay> and making 
MapView.getOverlays() return List<Overlay>, which actually returns the overlay 
controller?

Original comment by neilboyd on 9 Feb 2011 at 7:16

[GoogleCodeExporter]

Neil - Sorry about that. I'm actually not that happy with the way the manager 
class is implemented yet, and I'm going to try to follow the Google API closer 
as we move forward. For now, I've added a method back into MapView that simply 
returns the overlays so that the wrapper isn't broken.

Original comment by kurtzm...@gmail.com on 10 Feb 2011 at 6:25

[GoogleCodeExporter]

Marc - it's not actually for the wrapper, just for consistency. I didn't do 
getOverlays in the wrapper because osmdroid and Google have different overlays.

Original comment by neilboyd on 10 Feb 2011 at 8:14

[GoogleCodeExporter]

The MapView.getOverlays() that's there now isn't very useful because you can't 
add an overlay that way. The way it used to be was getOverlays().add(overlay), 
but if you do that you're just adding to a copy of the list.

Probably better to remove it again rather than having something that doesn't 
work.

Original comment by neilboyd on 10 Feb 2011 at 7:20

[GoogleCodeExporter]

This issue was updated by revision r790.

Added much more sensible OverlayManager implementation. The OverlayManager 
implements a List interface and the Overlays be directly added, removed, moved, 
etc... There is an optional TilesOverlay that is not a part of the regular List 
of Overlays, so it can't be inadvertently cleared.

Original comment by kurtzm...@gmail.com on 11 Feb 2011 at 3:59

[GoogleCodeExporter]

Marc, another idea:

How about changing OverlayManager to have a list of IOverlay instead of Overlay 
(IOverlay to be defined in org.osmdroid.api).

That will make it possible to define a class that extends Google's Overlay and 
implements osmdroid IOverlay. That class can then be usable as an overlay in 
Google *and* osmdroid.

It would also be a good idea to extract the menu stuff out of the Overlay base 
class.  That'll make it easier to implement an overlay without extending the 
base class.

Am I making sense or should I explain some more?  This idea is coming from the 
groups thread: https://groups.google.com/d/topic/osmdroid/HiIZuOLSjts/discussion

Original comment by neilboyd on 15 Feb 2011 at 11:39

[GoogleCodeExporter]

Neil - I think that's a good idea. In fact, we should probably rename some 
methods to match the Google API's Overlay.

Perhaps the approach we should take is to create the IOverlay interface that 
matches Google API, and then create an IOverlayExtended that will implement the 
rest of the stuff we implement in osmdroid. The overlay manager will keep a 
list of IOverlay and when an IOverlayExtended method is called it will check 
each overlay to see if it is an instanceof IOverlayExtended. The osmdroid base 
Overlay class will implement both interfaces.

How do we handle GeoPoints? Do we create an IGeoPoint class too? In the end, do 
we need to create interfaces for the entire Google API?

Original comment by kurtzm...@gmail.com on 16 Feb 2011 at 3:27

[GoogleCodeExporter]

There's already an IGeoPoint.

The extended interfaces sounds like a reasonable idea.

The reason I suggested extracting the menu stuff is because if I implement a 
Google+osmdroid overlay it won't be able to extend the osmdroid Overlay base 
class. If that's extracted then we can reference it rather than duplicate it.

I'll make a start on the IOverlay.

Original comment by neilboyd on 16 Feb 2011 at 10:21

[GoogleCodeExporter]

Actually I got a bit stuck with it. The methods in Overlay are not accessors - 
they're methods to be overridden - so I end up with an empty IOverlay.  I could 
just put all the "extended" methods into IOverlay, but then it's just a marker 
rather than a common implementation.

There's a lot of onManaged... methods that would be in IOverlay (because 
they're called by OverlayManager).  That seems a bit awkward.  Isn't the 
manager supposed to be managing anyway, so the overlays can just do their stuff 
without worrying about it.  And looking at the description of 
Overlay.onManagedDraw it seems to be doing the same thing as Google's Overlay 
is doing with the shadow parameter.

So I think a bit of refactoring of Overlay and OverlayManaged could be done 
first:
 - make Overlay a bit more similar to Google's
 - get the menu stuff out of Overlay.

What I'm aiming for is something like this: 

class TileOverlay extends google..Overlay implements osmdroid..IOverlay {

void draw(Canvas, Google mapView, shadow) {
   IProjection projection = mapView.getProjection();
   // probably need to use the wrapper here
   draw(canvas, projection)
}

void draw(Canvas, osmdroid mapView, shadow) {
   IProjection projection = mapView.getProjection();
   draw(canvas, projection)
}

and then all the real work can be in draw(canvas, projection) which will work 
the same for Google and osmdroid.

Original comment by neilboyd on 16 Feb 2011 at 12:46

[GoogleCodeExporter]

I can only judge this from the outside (as user of the osmdroid.jar), but this 
really sounds promising:
- class TileOverlay extends google..Overlay implements osmdroid..IOverlay

(Furthermore: I vote for doing it the Google way; the lower the learning curve 
the better)

Original comment by alex.vanderlinden on 16 Feb 2011 at 8:44

[GoogleCodeExporter]

Neil, I think I see what you're saying, but I'm wary of making all of the OSM 
overlays directly extend a Google API class. Now we're directly mixing Google 
API with the main osmdroid code (if I am understanding you correctly), rather 
than wrapping the osmdroid stuff to work in Google Maps. I think we'd have to 
link to the Google APIs and along with that comes all the licensing issues with 
the API.

Additionally, I think we may be using a different coordinate system than Google 
Maps in the sense that the center of the screen is 0,0 rather than the upper 
left corner (which I hate, but I think assists in implementing some gesture 
stuff), so directly using osm Overlays in Google Maps may not work. I'm not 
sure about that, but I'm bringing it up in case it is true.

Maybe we should write a wrapper for our Overlays that makes them Google Maps 
compatible. It could make any coordinate adjustments, and then call the 
appropriate functions in the Overlay class. I assume that if someone wanted to 
implement cross map compatibility for their app the way it would work is they 
would create the appropriate map class (org.osmdroid.MapView and/or 
com.google.MapView) and then create the osmdroid overlay classes and 
individually add each overlay to *both* map classes (or whatever map class was 
"active" at the time). When they add the overlay to the Google Maps, they would 
have to wrap it in the overlay wrapper first.

And I agree that we should refactor the Overlays (and probably other classes) 
to be closer to the Google API. You keep mentioning removing the menu stuff - 
we can put that in it's appropriate place, but I would really like to keep that 
(optional) ability for overlays to somehow be able to add menu items. Maybe 
that will be it's own interface (IOverlayMenuEnabled or whatever).

Original comment by kurtzm...@gmail.com on 17 Feb 2011 at 3:28

[GoogleCodeExporter]

Marc, I wasn't suggesting to make all the overlays extend Google Overlay. 
Although it does cause a bit of a dilemma. I imagined doing TileOverlay as I 
described, but that means it would be in the google-wrapper project, which is a 
bit odd. Or there could be the existing one in osmdroid-android and another one 
in google-wrapper, which is also not good.
What I was suggesting was using IOverlay to *allow the possibility* of doing 
what I described.

The coordinate system is different, but that's handled by the projection so 
it's transparent.

A wrapper for our overlays might work, but the idea was to keep the similar 
code together, and a wrapper would have passthrough code for osmdroid 
implementation and actual implementation for Google.

Yes, I keep mentioning the menu stuff. IOverlayMenuEnabled and an 
implementation in a separate class is what I had in mind. The static id stuff 
in Overlay can easily be somewhere else. Or maybe it doesn't matter that the 
static methods stay in Overlay and the implementation stays in the specific 
overlay that implements it. But having the separate IOverlayMenuEnabled 
interface means we can keep it out of the general overlay implementation.

The whole problem is that by not using a base class we don't get all the 
default implementations for free.  We could do a DefaultOverlayBase class for 
convenience, but for overlays that don't use it we want to make the 
implementation as simple as possible with only a few methods that are relevant 
to the overlay, which is what the Google Overlay looks like.

Original comment by neilboyd on 17 Feb 2011 at 7:35

[GoogleCodeExporter]

I think having a solid sample project for the wrapper would be a big help. That 
would give us something to play around with and give us a better idea how to 
proceed. I'm not sure when I'd be able to write something up, so feel free to 
take the lead if you have the time.

Original comment by kurtzm...@gmail.com on 21 Feb 2011 at 3:46

[GoogleCodeExporter]

Agreed, and ditto ;-)

Original comment by neilboyd on 21 Feb 2011 at 6:11

[GoogleCodeExporter]

I've attached a patch for IOverlay.

I haven't tested the Google wrapper Overlay yet and it almost certainly doesn't 
work.  I can probably add a gesture detector and that kind of stuff to 
implement some of the other event handlers.

IOverlay looks rather messy, but everything else looks harmless.  But I'm still 
not sure if this whole idea is going anywhere yet.

Original comment by neilboyd on 21 Feb 2011 at 3:02

Attachments:

• IOverlay.patch

[GoogleCodeExporter]

I've attached a patch to replace onManagedDraw with onDraw similar to Google 
Overlay.  Seems okay but I haven't tested it yet.  If you think the idea's okay 
I'll test it and commit it tomorrow.

Original comment by neilboyd on 21 Feb 2011 at 3:23

Attachments:

• onDraw.patch

[GoogleCodeExporter]

I've made a new issue 164 for the change to onDraw.

I'm beginning to think the IOverlay idea isn't going to achieve anything so 
I'll forget it for now.  In any case, I won't add any more comments issue about 
it to this.

Original comment by neilboyd on 22 Feb 2011 at 3:44

[GoogleCodeExporter]

This issue was updated by revision r839.

Extracted menu functionality into its own interface.
Updated OverlayManager to use new IOverlayMenuProvider

Original comment by kurtzm...@gmail.com on 27 Feb 2011 at 5:52

[GoogleCodeExporter]

I think this issue can be closed!

Original comment by kurtzm...@gmail.com on 16 Jan 2012 at 8:56

• Changed state: Fixed