Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A mix of updates: LatLngBounds, MVCObject, Overlays #188

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

dslh
Copy link

@dslh dslh commented Aug 8, 2013

Hi Brandon,

We're undertaking an upgrade of our GWT project from Google Maps v2 to v3 and we've decided to use your project as our Maps-to-GWT bridge. Our project is fairly large and I expect we'll need to make a few more changes to your project before we're done but I thought it would be good to submit a pull request early and get a dialogue going.

The changes to LatLngBounds should be fairly uncontroversial, Google's API allows for bounds objects to be constructed without supplying full extents and handles the case gracefully. I do see though that you have a fairly exhaustive set of tests for the class, we'd be willing to add to those tests but we don't use Maven here and I'm having trouble getting the tests to run. I can compile and package the source code and when I execute 'mvn test' I'm told that it was successful, but I'm also told that 0 tests were executed. Is there anything obvious I might be missing or is there anything I can read to help me get up to speed on your setup?

We've started to change the way generics are used in MVCObject but so far just the methods that we're using. Our changes reflect that the Maps API allows MVCObjects to be bound to arbitrary MVCObjects of different types. It seems like other uses of generics in this class should also be corrected:

  • The signature of set(String, T) should probably be changed to set(String, Object)
  • The signature of T get(String) should maybe be something like public final native <U> U get(String), but I'll admit my grasp of generics is not 100% and I'm not so confident on this one.
  • setValues(HashMap<String,String>) should probably become setValues(Map<String,?>).

The reason I say 'probably' is that this implementation does expose some potential runtime errors if anyone tries to pass non-JSO objects to the setters, but at the same time these methods should permit String objects and the primitives so it's hard to avoid. Would like to hear your thoughts on this.

We've added a common interface Overlay which is implemented by Marker, Polygon, Rectangle et al, which we use for handling visibility and removal of complex overlays composed of many different constituent overlays. It's a fairly inoffensive little interface but it's not in the official API anywhere (I suppose it's unnecessary in javascript), so if you're adamant about remaining faithful to what Google have written down then I guess we'll find a workaround.

The new Icon binding we've added is simple enough. We can add tests for this once we have them working.

Cheers,
Doug Hammond

dslh added 3 commits August 8, 2013 22:56
…ypes

  - Rectangle: setBounds, getVisible, setVisible.
  - Circle: getVisible, setVisible.
 - Created an Overlay interface, implemented by Circle, Marker, Polygon, Polyline, Rectangle.
   Facilitates bulk manipulation of overlay visibility and parent map.
 - Created wrapper for Icon "class" and added getters/setters to Marker and MarkerOptions.
@branflake2267
Copy link
Owner

Nice job, looks pretty good.

There is a test suite that can be run from the ide and it will run all the tests outside of maven. I wouldn't worry about them running in maven, the build server can do that if its running, which I just rebooted.

I didn't lock down some of the the objects in the generics b/c I can't remember. But there are some that can't be changed which seem a bit messing in the handlers, but what your mentioning looks fine from what I can remember :). I haven't had time recently to add additional changes.

This will run all the tests > https://github.com/branflake2267/GWT-Maps-V3-Api/blob/master/gwt-maps-api/src/test/java/com/google/gwt/maps/client/RunAllTestsGwtTestSuite.java

Some of the tests will throw some red javascript exceptions into the console but won't affect the goal of the test at the time.

@branflake2267
Copy link
Owner

Two things I like to know do the tests run the same as before and after. And does the showcase load after the changes :) I usually do that before I merge the source. It happens I'm heading out to vacation and will be offline till monday, so I'll be out of pocket for the next few days. If you have any questions, send them via g+ and I can try to get back by end of day. But looks like your quite proficient :)

public final static Icon newInstance(String url) {
return createJso(url).cast();
}
/**
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you add a nl between :)

A GWT interface and javascript wrapper to be used for implementing map
types that can be overlaid on regular map layers via
google.maps.Map.overlayMapTypes.
Projection.fromLatLngToPoint will create a new Point object if none is
provided.
Added a common Overlay superclass for Marker, Circle, Polygon, Polyline
and Rectangle. Added a few missing methods for draggability, visibility,
editability, etc.
The MouseEvent wrapper was calling stop() on its own jso rather than the
original Google Maps event object. It now retains a reference to the
event object so that it can call the method as required.

If we're retaining a reference to the event object maybe we don't need
to hold a separate reference to the event coordinates...
@F43RY
Copy link

F43RY commented Sep 16, 2014

This pull request fix problems with MVCObject using polygons, circles and so on. Please merge this request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants