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

Refactor to simplify Map #2741

Closed
lucaswoj opened this issue Jun 14, 2016 · 5 comments
Closed

Refactor to simplify Map #2741

lucaswoj opened this issue Jun 14, 2016 · 5 comments

Comments

@lucaswoj
Copy link
Contributor

lucaswoj commented Jun 14, 2016

Map is 1342 lines of code and exposes about a hundred methods, properties, and events. We should simplify Map's interface and implementation by moving as much logic downstream as possible and maybe moving some interface components downstream.

moving as much logic downstream as possible

Having many methods, properties, and events on Map would be more manageable if Map did not have any logic or state of its own.

For example, we should endevour to rewrite setMaxBounds

    setMaxBounds: function (lnglatbounds) {
        if (lnglatbounds) {
            var b = LngLatBounds.convert(lnglatbounds);
            this.transform.lngRange = [b.getWest(), b.getEast()];
            this.transform.latRange = [b.getSouth(), b.getNorth()];
            this.transform._constrain();
            this._update();
        } else if (lnglatbounds === null || lnglatbounds === undefined) {
            this.transform.lngRange = [];
            this.transform.latRange = [];
            this._update();
        }
        return this;

    },

such that it reads a little more like

    setMaxBounds: function (lnglatbounds) {
        transform.setMaxBounds(lnglatbounds);
        return this;
    },

(maybe) moving some interface components downstream

i.e. change the public interface from map.setPaintProperty(...) to map.style.setPaintProperty(...) (or map.style.paint['circle-color'].set(...))

@jfirebaugh
Copy link
Contributor

i.e. change the public interface from map.setPaintProperty(...) to map.style.setPaintProperty(...) (or map.style.paint['circle-color'].set(...))

The core mbgl native equivalent is map.getLayer(id).setCircleColor(...) (modulo some downcasting from the base Layer type to CircleLayer).

The difficulty of pushing the setters down to below the map level is how do you trigger the subsequent style recalculations? When I was first implementing map.setPaintProperty, I considered several alternatives:

  • Require downstream code to manually trigger style recalculation. For native I'm going to see if the "downstream code" in the iOS SDK or Android SDK bindings can do this. I don't think we want this implementation detail to leak to actual user code.
  • Have every mutable object have a reference back to the containing Map, so that the setter can trigger the recalculation.
  • Implement the observer pattern and have the map register for notifications from each mutable object. If individual layers or properties have a set method, this becomes cumbersome.

In the end none of these seemed worthwhile when compared to the simplicity of map.setPaintProperty(...). But I agree it does bloat Map. I think map.style.setPaintProperty(...), and an observable Style, could be a good compromise.

@mourner
Copy link
Member

mourner commented Jun 16, 2016

I think map.style.setPaintProperty(...), and an observable Style, could be a good compromise.

It was like that before, why did we move it upstream in the first place? I don't remember exactly but there must have been a reason.

@jfirebaugh
Copy link
Contributor

@mourner style.setPaintProperty(...) has existed, but map.setPaintProperty(...) has always been the public API, starting with setPaintProperty's introduction in a374e74#diff-274e90d38623e2e16a5e7d34414f33c7R250.

@jfirebaugh
Copy link
Contributor

  • Implement the observer pattern and have the map register for notifications from each mutable object. If individual layers or properties have a set method, this becomes cumbersome.

Looks like this is the route native is going to go, with style sources and layers being the lowest level of observable object, which isn't too cumbersome.

@mourner
Copy link
Member

mourner commented Aug 1, 2018

I addressed the first part of the original issue in #7060, but the issue of changing the API to call methods on layer instances directly is still open, so maybe let's make a new issue or reopen/edit this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants