[TIMOB-23988] iOS: Add click event to "callout" bubble to return infoWindow (Parity) #159
Conversation
| @@ -30,6 +30,9 @@ | |||
| NSMutableArray *polygonProxies; | |||
| NSMutableArray *circleProxies; | |||
| NSMutableArray *polylineProxies; | |||
|
|
|||
| //selected annotation | |||
| MKAnnotationView<TiMapAnnotation> * selectedAnnotation; | |||
There was a problem hiding this comment.
Make sure to also deallocate it during dealloc by using RELEASE_TO_NIL(selectedAnnotation).
There was a problem hiding this comment.
It is just a pointer - I don't do alloc so I don't need to dealloc.
There was a problem hiding this comment.
selectedAnnotation = nil; in destructor should be enough
There was a problem hiding this comment.
When you assign it with selectedAnnotation = ann;, it can be deallocated, since it's not using ARC. That's why you need to retain and release it.
|
Left some review comments, thanks! |
|
@darknos, ping! 🚀 |
|
Notice, there is a mix of tabs and spaces in this file. I use 4 spaces but github shows one tab as 8 spaces so it looks like indentation issue in diff views. |
|
@hansemannn, ping! |
|
@darknos I requested changes a few months ago, can we still expect those or should I close this? |
| //add tap recogniser to this view | ||
| UITapGestureRecognizer *tap = [[UITapGestureRecognizer alloc] initWithTarget:self action:@selector(handleCalloutTap:)]; | ||
| [node addGestureRecognizer:tap]; | ||
| }else{ |
| @@ -34,7 +34,7 @@ -(void)dealloc | |||
| CFRelease(mapObjects2View); | |||
| mapObjects2View = nil; | |||
| } | |||
|
|
|||
| selectedAnnotation = nil; | |||
| RELEASE_TO_NIL(locationManager); | |||
| RELEASE_TO_NIL(polygonProxies); | |||
| RELEASE_TO_NIL(polylineProxies); | |||
There was a problem hiding this comment.
While you're on it, this proxies have an incorrect indentation as well.
|
@darknos Will you still update the issues in this PR and file a JIRA ticket? Would like to have the changes in, but we can't accept it at this point. |
|
I already reported the issue in JIRA but no info since September. What do you need more to merge this PR? |
- code clean up
hansemannn
changed the title
| //if canShowCallout == true we will try to find calloutView to hadleTap on callout | ||
| if ([ann canShowCallout]) { | ||
| //run after delay because we have to wait for animation finish | ||
| [self performSelector:@selector(findCalloutView:) withObject:ann afterDelay:0.2]; |
There was a problem hiding this comment.
Ok one thing here. I'd like to use GCD instead of the delayed performSelector method. It's recommended to migrate all of those to common async-patterns. Can you do that? 😊
There was a problem hiding this comment.
do you meant something like this?
dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(0.2f * NSEC_PER_SEC)), dispatch_get_main_queue(), ^(void){
[self findCalloutView: ann];
});There was a problem hiding this comment.
Yeah, but if you dispatch it async, wouldn't the event fire before the callout view has been found? Or why exactly do you need 0.2 seconds? Maybe it should be block-based or synced instead.
There was a problem hiding this comment.
I guess the callout contents created after wrapper animation finished (callout pop}, if delay less then 0.2 it is not found.
There was a problem hiding this comment.
But if the click was made on the callout, the callout should already be visible. We should avoid hardcoded delays in any case
There was a problem hiding this comment.
From Apple SDK we have an event "click on pin" but don't have an event "callout showed and all subviews in it are already created".
So we wait 200ms of animation after "click on pin" event and then try to find subview which class is "_MKSmallCalloutPassthroughButton" recursively in callout views hierarchy.
It is not a "SDK way" - it is a "hack".
Do you have any idea how to do it more "nice" without fixed timeouts? I don't know :( and didn't find any ideas in the net too.
There was a problem hiding this comment.
-(void)refreshAfterDelay
{
[self performSelector:@selector(refreshIfNeeded) withObject:nil afterDelay:0.1];
}I hope this hardcoded delays is not a problem in TiMapAnnotationProxy.m ? :) :) :) :) why 0.1? :)
hansemannn
changed the title
|
I'm not sure that I understand word "contribution" as it should be. I pay $90 monthly for SDK. I need this functionality. a lot of guys like me need this SDK parity. I got an idea. I proposed a solution. I made a PR. If you need it, please, fix indentations, avoid hardcoded delays, make it "legacy way", etc. I signed all required agreements to allow you it. I already created custom version of this module for myself, I already finished a project where it was required and got money from a customer. Is $90 monthly not enough for such "contribution"? Am I bad guy? No. I'm quite busy guy who tried to "contribute" something. Please, don't stop me and don't make this "contribution" the last one. |
|
@darknos Yes, you understand the term "contribution" incorrect. It has nothing to do with your subscription or other people using this patch. Is has to be integrated properly, so thousands of developers can use it the correct way. I don't like the way this discussion is going, only because I like to discuss different approaches. I am reviewing this PR in my spare time, suggesting best practices. If that's too much for you, I am sorry, but then we cannot accept this PR. That's how it's done in the main titanium_mobile repo and all other module repositories. People adhere to that conventions and the quality improves. And for the Sorry again for being so direct, but that's my personal opinion from doing a lot of work in open source communities. |
|
ok. I don't know better solution then 0.2s delay. close PR? |
|
and I don't know which kind of indentation is legacy. what to do? kill myself? |
|
Closing in favor of #193 |
|
New release 2.8.2: https://github.com/appcelerator-modules/ti.map/releases/tag/ios-2.8.2 |
JIRA: https://jira.appcelerator.org/browse/TIMOB-23988