feat(markers,paths,geojson,watchOptions): More flexible watch options…

[cgat]

… and more

watch-options has been expanded to include paths and redesigned to handle
more types of watching. markersWatchOptions and geojsonWatchOptions are no
longer supported. Instead, this direcitve now supports the watch-options
attribute, which takes an object that contains the watch options for each
map element type:

watchOptions = {
    markers: <markerWatchOptions>,
    geojson: <geojsonWatchOptions>,
    paths: <pathsWatchOptions>
}

The watch options for these map element types looks like:

{
    type: <watchTypes>,
    individual: {
        type: <watchTypes>
    }
}

<watchTypes> can be one of:
-watch: $scope.$watch with no deep watch on the object
-watchDeep: $scope.$watch with a deep watch on the object
-watchCollection: $scope.$watchCollection on the object
-null: Do not watch the collection

I've removed support for the directive attributes watch-markers and
watch-paths. While these make sense when set to false, it's ambiguous
how they should function when set to true, especially when combined
with watch-options.

There are some other minor fixes in this commit (fixing breaking examples,
and another small bug).

BREAKING CHANGE: markersWatchOptions, geojsonWatchOptions, watch-markers,
and watch-paths are no longer supported. The format of the watch options has
also changed.

Before:

<leaflet defaults='defaults' markers-watch-options='markerWatchOptions'>

After:

<leaflet defaults='defaults' watch-options='watchOptions'>

<script>
    $scope.watchOptions = {
        markers: <watchOptions>
    }
</script>

The same can be said for geojson.

WatchOptions. shouldWatch and isDeep are no longer supported, but the
same behaviour can be achieved:

shouldWatch: true, isDeep: true == type: 'watchDeep'
shouldWatch: true, isDeep: false == type: 'watch'
shouldWatch: false, isDeep: false == type: null
shouldWatch: false, isDeep: true == type: null

Furthermore,

Before:

<leaflet watch-markers='false'>

After:

<leaflet watch-options='watchOptions'>

<script>
$scope.watchOptions = {
    markers: {
        type: null,
        individual: {
            type: null
        }
    }
};
</script>

watch-paths='false' can be achieved in a similar manner.

[cgat]

This a continuation of this PR on angular-leaflet-directive tombatossals/angular-leaflet-directive#993

[nmccready]

So is this ready? I am not sure where we left off before.

[cgat]

I think so. As long as everyone is okay with the breaking changes. I wouldn't mind a code review, but if that's not necessary we can pull it in.

[nmccready]

@zacronos or @jessertaylor what do you think about having explicit 'none' to disable watches? IE Why not just disable watches if type is undefined?

[zacronos]

IMHO, both should be allowed. Then there is a way to be explicit if desired, but it isn't required.

[cgat]

If watches were off by default, then I would support undefined or null as a way to turn off watching, however, watches are watchDeep by default as this was how angular-leaflet-directive was originally designed (paths have a bit different defaults). I would argue the absence of a property should probably yield the default behaviour. I would support replacing 'none' with null (explicitly defined) if so desired. Supporting both is more hassle than it is worth as some conditionals check whether a watch is set simply by checking if type != 'none' (supporting both would require longer conditionals).

[nmccready]

Ok I'm good w that.

[nmccready]

So this does look ready. It looks pretty good. I am waiting on feedback about the 'none' stuff.

[nmccready]

Please resolve conflicts and I'll merge.

[nmccready]

@cgat

[cgat]

@nmccready okay, this is ready.

Before I pull this in, I've got a question about the commit message protocol here. My initial commit message on this branch is styled as per the angular guidelines. Subsequent commits (merging with upstream and changing 'none' to null) are not done in this style, as I expect you are using something like clog to modify the changelog, which will ignore commit messages that don't match the style. If this is the case, please make sure to modify the the changelog such that references to 'none' are null.

Alternatively, I could have amended my commit until it was correct, but this would have required a force push to the PR (bad practice among some), but would result in a much cleaner commit.

Let me know what I should be doing in the future.

[nmccready]

Well as long as this feature is commented somewhere with a changelog intiention.. i guess were ok. But the best approach would be to rebase this entire PR into one single atomic commit.

WHY? Well if it turns out to be disastrous (doubtful) it is easy to revert (only one commit). Likewise the traceability is easier as well.

[nmccready]

So ie rebase -i upstream/master , but since you already merged this may blow up. An alternative is to rebase from 000f59d or daece73 .

One thing I am confused on is I thought you were not going to go the undefined / null approach. Your comment above states otherwise. nvm misread LGTM, but let me know if your going to try rebase

[cgat]

Ahh, sorry, miscommunication. When you said "Ok I'm good w that.", I took that as a yes to "I would support replacing 'none' with null (explicitly defined) if so desired.". The way I implemented it does not support undefined, but does support null (where undefined is the absence of something, while null was explicitly set). Whether it's null or 'none' doesn't matter to me, but not supporting undefined is. Let me know your preference.

I'll fix this up into a single commit with a proper commit message. Not sure how github handles force push in these situations, so if it comes down to it I may open another PR (though I will try to avoid this).

[nmccready]

I'm fine with either. So if you want it to be null leave it.

[cgat]

I'm leaving it as null. I've also rebased to a single commit. As a contributor I thought I would be able to pull this in, but alas, that does not seem to be the case.

[nmccready]

@cgat you don't have write perms yet. Basically if you keep pushing; you will get it. There are 3 levels, leaflet-contrib-> leaflet-write-> leaflet-admin , contrib people are assignable for tickets and up for write perms.

[nmccready]

LGTM

[cgat]

@nmccready Got it. Thanks for merging.