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

Add a LatLng generic abstract class #2873

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

Conversation

ramsestom
Copy link

Having to frequently manipulate different objects representing a LatLng geographic position (with potentially additional associated information), I was bored having to always convert back and forth beetween the different CN1 objects of this type (Location, Coord) and my own objects with a geo position. So I added an abstract LatLng class to the CN1 core that all these objects can extends, allowing me to have generic methods that will works with all LatLng objects without having to perform some conversion first.
This change could potentially be usefull for other peoples having to play with LatLng in their CN1 app. So you can accept this pull request if you find it usefull.

… and forth from different objects representing a LatLng position (like Location, Coord, geojson Position...)
@shannah
Copy link
Collaborator

shannah commented Jul 30, 2019

This doesn't seem to include the LatLng class itself, unless I'm just missing it

@shannah
Copy link
Collaborator

shannah commented Aug 1, 2019

If we're going to have a base class, wouldn't it make sense to just have one set of lat, lang instance variables in the base class, rather than each subclass having their own?

@ramsestom
Copy link
Author

ramsestom commented Aug 1, 2019

Actually I switched from abstract class to an Interface, it would make more sense (I don't have a lat, lng variable instance at the base class because some of my child classes have lat lng that may change from other class parametters or time or that can be inherited from a nested class).
There may have been a good reason why I used an abstract class rather than an Interface at first but I can't remember (I create this class months ago in my app code before moving it to CN1 core) and my app seems to run fine like this so keeping it like that should be OK (else I would revert to an abstract class in my CN1 core version. My version already diverge from the official CN1 core on some points anyway so it's not an issue if LatLng is an interface on the official version and an abstract Class in mine as I already can't use the official CN1 core in my apps anyway).

@codenameone
Copy link
Collaborator

My one concern with this PR is that some signature changes might trigger odd compilation issues specifically changes such as: public boolean contains(LatLng cur). If someone sends a build linked against the previous version of the library compilation will fail. This is relatively simple to fix by updating the libraries, but from my experience changes like that lead to a great deal of frustration with developers.

I'd rather have different method names so we can migrate developers to a new interface in their own pace.

Another option might be deriving Coord from Location. We can also deprecate Coord as part of that change and move to using Location for everything.

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.

None yet

3 participants