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

[TIMOB-9403] Add centerOffset-property handling for Android map annotations. #79

Merged

Conversation

ndob
Copy link
Contributor

@ndob ndob commented Jan 7, 2015

This pull request adds handling for centerOffset-property on Map.Annotation-objects.
Relevant JIRA-ticket: https://jira.appcelerator.org/browse/TIMOB-9403

This should work as the corresponding iOS-feature: offset is in pixels, positive values move down and right. [1]

Only open issue is, that when centerOffset-property is not set, Android uses the default anchor, which points to horizontal center and vertical bottom [2]. I didn't dare to break the existing functionality by defaulting to horizontal and vertical center, but comments regarding to this are of course welcome.

[1] https://developer.apple.com/library/ios/documentation/MapKit/Reference/MKAnnotationView_Class/index.html#//apple_ref/occ/instp/MKAnnotationView/centerOffset
[2] https://developer.android.com/reference/com/google/android/gms/maps/model/Marker.html

@ndob ndob changed the title Add centerOffset-property handling for Android map annotations. [TIMOB-9403] Add centerOffset-property handling for Android map annotations. Jan 7, 2015
@ndob
Copy link
Contributor Author

ndob commented Mar 7, 2015

Is it possible to get this merged?

@jonatansberg
Copy link

@ndob Thank you for implementing this, you just saved the better part of my day! :)

A small heads up though: with the current implementation the centerOffset point needs to be set in system units, even if you use another unit (e.g. dips) as the default unit.

@ingo
Copy link
Contributor

ingo commented Mar 9, 2015

@ndob
Copy link
Contributor Author

ndob commented Mar 10, 2015

@mrlundis Great to hear. :) The offset is currently in pixels, because in iOS documentation (link number 1) it says: "This x and y offset values are measured in pixels.". So this should be in parity with that.

@ingo I added Android-platform to the documentation now.

@ndob
Copy link
Contributor Author

ndob commented Aug 30, 2015

Is there something preventing the merge of this PR?

@yozef
Copy link
Contributor

yozef commented Aug 30, 2015

I've actually reimplemented your code in my version of Ti.Map for Android, and it does the job.

williamrijksen and others added 2 commits October 27, 2015 18:14
…android-center-offset-property-to-annotations
…rty-to-annotations

Android center offset property to annotations
@williamrijksen
Copy link
Contributor

@ingo Can you take a look at this PR? I resolved the merge conflicts about this PR. The errors occurring in Travis, but i don't know the exact reason.

@williamrijksen
Copy link
Contributor

Ready for review!

@ashcoding
Copy link
Contributor

@williamrijksen I'll be happy to review this. Travis seems to be passing.

@ashcoding
Copy link
Contributor

@williamrijksen could you write a sample app/test for the PR? That would help greatly.

Also could you rebase this branch? We just merged a commit and this seems to have made ur branch to be in conflict.

…android-center-offset-property-to-annotations
…rty-to-annotations

Resolve merge conflict.
@williamrijksen
Copy link
Contributor

@ashcoding merge conflict solved

@williamrijksen
Copy link
Contributor

@ashcoding Because you asked my to write a sample app, i wrote that one. You can find it here: https://github.com/williamrijksen/TiMapsSample. It's still necessary to set you own Google maps-key to do a functional test.

To clarify what is happening I'll insert two screenshots. One with a default annotation and one with a set centerOffset property.

  1. Default
    screenshot_2015-10-30-14-01-51
  2. With centerOffset: {x:0,y:0}
    screenshot_2015-10-30-14-02-09

If you have any questions, don't hesitate to ask them.

@ashcoding
Copy link
Contributor

@williamrijksen would handling offset changes in the onPropertyChanged https://github.com/appcelerator-modules/ti.map/pull/79/files#diff-f3c99af2a7979e334ec513288183b11cR318 be required as well?

@williamrijksen
Copy link
Contributor

@ashcoding I tried to handle the offset changes in the propertyChanged, but the view didn't update. I think it's the same issue as https://jira.appcelerator.org/browse/TIMOB-19543.

@ashcoding
Copy link
Contributor

@williamrijksen Okay. Understand where you're coming from. :)

Jeroen van Dijk added 2 commits June 25, 2016 22:15
* master: (25 commits)
  [TIMOB-20606] Bump iOS to 2.7.1
  [TIMOB-20606] Fix type-casting -> dragging-issues
  regionchanged and regionwillchange returned animated was always true
  Android: Updating module to 2.3.10
  Bumped the version to 2.3.9
  Fixed: application crashing due to map already being null when calling map.clear().
  Fixed: Unable to compile due to an non needed Override annotation.
  [TIMOB-20620] Android: Annotation.pincolor can't be changed if annotation added to mapView - bumped version to 2.3.9
  [TIMOB-20620] Android: Annotation.pincolor can't be changed if annotation added to mapView.
  [MOD-2205] Update indentation + Changelog
  [MOD-2205] Support Blob image, bump Android 2.3.8
  added line return on 316
  re-corrected the examples section
  Fixed line 14 spacing issue
  Fixed typo
  Fixed broken page (TIDOC-2450)
  [MOD-2194] Support 'regionwillchange' event
  [MOD-2189] Updating Docs
  [MOD-2189] Android: Provide the method setMapToolbarEnabled
  [MOD-2172] Support new MKMapView properties
  ...
…-annotations

Android center offset property to annotations
@jvandijk
Copy link
Contributor

jvandijk commented Jul 4, 2016

@ashcoding @hansemannn please have a look at this PR again. I've just updated the merged master into it to make it completely in sync again.
We are already using this in production quite a while.

@hansemannn
Copy link
Contributor

LGTM. @ashcoding has the final decision here.

@ashcoding
Copy link
Contributor

It looks good but I'm having trouble making the offset work. I'm getting both images look like the default.

@hansemannn are you able to functionally test it out?

@williamrijksen
Copy link
Contributor

@ashcoding @hansemannn What's the state of this PR?

@jvandijk
Copy link
Contributor

jvandijk commented Jul 9, 2016

@ashcoding
Copy link
Contributor

Tried it again. Works. I think I did something wrong earlier. In any case, works well. 👍

@ashcoding
Copy link
Contributor

Code reviewed and functionally tested.
Approved

@ashcoding ashcoding merged commit d5a7fff into tidev:master Jul 11, 2016
jvandijk pushed a commit to jvandijk/ti.map that referenced this pull request Nov 1, 2016
…to-annotations

[TIMOB-9403] Add centerOffset-property handling for Android map annotations.
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

8 participants