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-9332] Added centerOffset property to TiMapView.m #2304

Conversation

joscandreu
Copy link

This commit corresponds to this jira ticket: https://jira.appcelerator.org/browse/TIMOB-9332

It enables to use centerOffset property in a Ti.Map.Annotation.

annotation.setCenterOffset({x: 0, y:-25});
//or
Ti.Map.createAnnotation({
centerOffset: {x:0, y:-25}
});

The CLA is signed with e-mail: joscandreu@gmail.com with the name: Jose Carlos Andreu Galan
I'm going to provide a simple test case in the comments of the jira ticket.

@negupta
Copy link
Contributor

negupta commented Jun 3, 2012

Signed CLA is in place.

@WhichKatieDid
Copy link
Contributor

Question: What happens if this centerOffset is changed while the view exists/is already placed? Read: There should be a setter for the centerOffset in question to avoid race conditions where the centerOffset is set after annotation creation. There also needs to be a change to apidoc/Titanium/Map/Annotation.yml to reflect this. Finally, there needs to be parity with Android/Mobile Web, or at least Jiras for such parity. CR: Needs revisions.

@joscandreu
Copy link
Author

I tried to set centerOffset twice once the annotation was already shown in the map and everything went fine, but I'm going to check and create that setter, should that be in TiMapAnnotationProxy.m, right?

Do I have to open those tickets in Jira by myself as I did with this one?
I didn't know the apidocs where also in version control. I'll add this property.

@WhichKatieDid
Copy link
Contributor

Correct. TiMapAnnotationProxy.m

I'll handle the JIRA tickets.

@joscandreu
Copy link
Author

I added the centerOffset setter in MapAnnotationProxy.m and modified the docs with the description of the property. Are the getters and setters created automatically from the properties in the docs?

@joscandreu
Copy link
Author

Any problems with the last commit? It's been 10 days without any response.

@WhichKatieDid
Copy link
Contributor

Apologies for the delay. I should clarify. The way the Apple Mapkit handles annotations is that they are recycled when they go offscreen. So if you have 2 image annotations, and both go offscreen, the view that was associated with annotation A may be instead used for annotation B and vice versa.

  1. In the current implementation, the offset is only assigned after the view is created. Not on reuse, not while onscreen. For reuse, the centerOffset should be in the same if statement as annView.image = image; Furthermore, it should always be assigned (The TiUtils CGPoint method defaults to (0,0) on nil) in case the view had an offset from a previous annotation.

  2. The getter is automatic, so you don't need to worry about it, but the setter, in order to propagate this change to the view, needs the if statement:
    if (current!=leftview)
    {
    [self setNeedsRefreshingWithSelection:YES];
    }

@joscandreu
Copy link
Author

Thanks for the comments. I'll update ASAP with these changes.

@negupta
Copy link
Contributor

negupta commented Jul 25, 2012

@joseitinerarium - Please let us know if you plan to address these comments.

@joscandreu
Copy link
Author

Yes I plan to address the comments but I've been unable to review and code the missing points.

@negupta
Copy link
Contributor

negupta commented Aug 15, 2012

Closing this pull request due to lack of response. Please open a new pull request if you decide to address and code the comments.

@negupta negupta closed this Aug 15, 2012
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