Annotation binding doesn't remove annotations about rxmkmapview HOT 16 CLOSED

Here's the example of one annotation binding in action:

from rxmkmapview.

Is this repository maintained anymore? I'd be happy to contribute and I have a working version here. Besides removing the annotations it also animates the change.

Here's how it looks:

from rxmkmapview.

I'm not sure if it's maintained but I'm using the latest version on a live app and it works absolutely fine ... So not sure it needs any maintenance ATM ?

BTW animations aren't really an Rx thing IMO ... More like a UI thing , unless it's an option and not "always" happens

from rxmkmapview.

From what I gathered the podspec isn't up to date (last release: May 2016?). Also the PRs seem to have been open for a while now.

I agree that animations aren't really a thing in Rx. However, binding to UI without animations really takes you only halfway. The way it is implemented is basically the same as in RxSwift with table views (RxTableViewSectionedAnimatedDataSource for reference) and would be configurable.

Either way, the current implementation doesn't really clean up the previous annotations and thus I couldn't use it with dynamic content.

from rxmkmapview.

I think it's supposed to be a thin wrapper around the MKMapView delegate and nothing more, like it is for RxCocoa elements. Interesting thought about needing to "clear" the old ones but I'm not sure that is necessarily something you would always want? Even though for Reactiveness it does make sense.

Interesting thoughts here :)

I think there is no active maintainer for this Repo but if you want to make such drastic changed you'll need to have a new Major version since users who expect it to work like a regular delegate would have broken implementation

from rxmkmapview.

The solution as well aims to be as thin as it can. The annotation binding is more equivalent to UITableView's items, where it doesn't really serve the purpose to keep the old items. Same with the annotations. . The change is a breaking one, so definitely a version bump is needed.

In comparison, for instance, text binding of UILabel will overwrite the previous value. Rx bindings similar to this don't have the same challenges.

I could either create a PR for this project or start a new, but if it suits this project, I'd really prefer the former.

from rxmkmapview.

One more thing:

Changing the logic from what it is to the one I'm thinking, would make the following behave really weirdly:

    _ = Observable.from(annotations)
        .bind(to: mapView.rx.annotations)

It would only display the last item in the array, as the previous values would be cleared. However, I personally don't see any use for this kind of binding, as there is no difference to this:

    _ = Observable.from(annotations)
        .subscribe(onNext: { mapView.addAnnotation($0) })

And just repeating myself, my understanding of bindings is that they change over time and the change is reflected one to one. I actually couldn't come up with any example of a binding persisting previous values.

In my case I needed to bind a variable visibleAnnotations in a view model and the behaviour was a bit unexpected when any change to the variable caused all items to be added to the map, some items many times due to subsequent changes in the variable.

If I make the proposal for the change, is it okay for everyone?

from rxmkmapview.

@mkko So I just want to make sure, how do you want to implement this without causing a performance hang?
Clearing all annotations might not be the best thought. Perhaps doing something like RxDataSources or the new UIPickerView thing @sergdort did here. And then we can apply some diff to it.

Clearing the entire view will be too costly IMO.

from rxmkmapview.

That's a good question. Clearing all annotations doesn't really make sense since it would cause flickering.

I have a prototype that I've been playing with here. The gist of it is to keep track of previously added annotations, create the diff based on that and update the map with that information. I believe the diff could be done in a linear time, my implementation was just the quickest way to get something accomplished. Besides having the performance benefit, with this one can have multiple bindings to the same map view.

What do you think?

from rxmkmapview.

@mkko That sounds like a good idea to me to pursue on a separate branch and then we can experiment with it. Think it's more valuable than the current implementation of it (I'm using it and tracking the changes manually as well)

I'm not sure if we should use MKAnnotations here or have a higher abstraction like what's done in RxDataSources etc. Also the Differentiator was extracted from RxDatasources a few days ago which might be a good case for trying some good diff also for this task.

See here: RxSwiftCommunity/RxDataSources#154

from rxmkmapview.

Thanks for your feedback.

For the diff, I don't see any reason to depend on the MKAnnotation type. Only thing that needs to be considered is equality, as currently MKAnnotation is forced to be an object and we can use the reference comparison to determine that.

Also, I would start with simple, improved version of the original diff to as performant as viable. The differentiator you linked looks like a nice reference, I'll look into that a bit further.

I'll apply first batch of changes in my fork within the next few days to get something out.

from rxmkmapview.

For the diff, I don't see any reason to depend on the MKAnnotation type.

In general, totally agree with this. As I suggested we can use some better abstraction there.

And sure I'd love to take a look at your implementation, But make sure it's somewhat comparable to the current interface as much as possible and not a completely separate fork, so it's easy to go through. I'd love to test it on my main codebase since we have a very big VC using RxMKMapView ATM.

from rxmkmapview.

@freak4pc, I created a PR #24 for this. I didn't have the time to look in to a better diff, but for the starters I think the one is performant enough. For larger numbers of annotations I think the biggest bottleneck will be the map itself.

from rxmkmapview.

@mkko Thanks ! I'll try taking a look in a day or two, kinda swamped with work ATM :(

from rxmkmapview.

Thanks, and no worries! In the meanwhile I'll try to find some time to add some tests.

from rxmkmapview.

Closing this, as it was resolved in PR #24 and released with 4.1.0.

from rxmkmapview.