Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Default view is a View2D #340

Closed
elemoine opened this Issue · 15 comments

4 participants

@elemoine
Owner

Currently, to create a map with a center and a resolution the following should be used:

var map = new ol.Map({
  ...
  view: new ol.View2D({
    center: new ol.Coordinate(x, y),
    resolution: r
 })
});

This is a bit verbose. And it feels "heavy" to have to create an ol.View2D object when what you want is just a simple map centered on a given point.

The map constructor already creates an ol.View2D instance when no view is specified in the map options. I'm considering extending this a bit, and making the following use-case work:

var map = new ol.Map({
  ...
  view: {
    center: new ol.Coordinate(x, y),
    resolution: r
 }
});

The code to handle this in the map constructor would look like this:

var view;
if (goog.isDef(mapOptions.view)) {
  if (typeof mapOptions.view == 'object') {
    view = new ol.View2D(mapOptions.view);
  } else {
    goog.asserts.assert(mapOptions.view instanceof ol.View);
    view = mapOptions.view;
  }
} else {
  view = new ol.View2D();
}

Any feedback before I submit a PR?

@elemoine
Owner

Note: for this to work our exports system needs to work with nested options objects, which I don't think is currently the case.

@twpayne

Looks good. +1 from me.

@elemoine
Owner

Thanks @twpayne. How to extend the exports/externs system isn't obvious to me, but I'll think more about it. Supporting nested object literals is something we'll need eventually anyway.

@twpayne

Agree. We can do nested object literals if we accept typecasts in code that will be compiled.

The problem otherwise is that if you have a typedef with optional properties:

/*
 * @typedef {{foo: number, bar: (string|undefined)}}
 */
ol.MyType;

and an object literal

var myObject = {
  foo: 1
};

Then the compiler does not consider myObject to be an instance of ol.MyType because the bar property is missing.

@bbinet explained why: myObject.hasOwnProperty('bar') will return false whereas someInstanceOfMyType.hasOwnProperty('bar') will return true. So, myObject does not behave like an instance of ol.MyType.

@elemoine
Owner

Then the compiler does not consider myObject to be an instance of ol.MyType because the bar property is missing.

You told me last time (in the train) that you were wrong about that.

@elemoine
Owner

Here's the main problem I see with nested literal objects.

First we will need this in the generated externs file:

/**
 * @type {ol.View|olx.View2DOptionsExtern|undefined}
 */
olx.MapOptionsExtern.prototype.view;

Also I think that olx.*Extern objects should not cross the "exports/externs" boundary. This means that ol.MapExport should not pass an olx.View2DOptionsExtern object to ol.Map. This suggests generating an ol.MapExport that looks like this:

/**
 * @constructor
 * @param {olx.MapOptionsExtern} options Options.
 */ 
ol.MapExport = function(options) {
  options = options || {};
  if (!(options.view instanceof ol.View)) {
    options.view = new ol.View2DExport(options.view);
  }
  goog.base({
    ...
  });
};

which is not nice because (1) we're including some logic in the generated wrapper class, and (2) we're duplicating code/logic in ol.MapExport and ol.Map (the latter too needs to create an ol.View2D instance when it receives a literal object).

@twpayne, did you see things differently?

@ahocevar
Owner

I don't see much benefit in being able to have an object literal instead of creating a View2D instance. However, if we could provide center and resolution directly as map config option, it would be an improvement. And there would be no externs problem I guess.

@elemoine
Owner

Just a note to say that I'm sure we'll have the nested objects problem eventually, be it for the ol.Map constructor or for other stuff.

@elemoine
Owner

I think that designing the API requires "brain diversity", so thanks a lot for your comments @ahocevar.

In my opinion the API should be both consistent and convenient. It won't be convenient if it's not consistent.

(Easy to say.)

On to the view-related map API now:

My idea was to not try to lie to the user by hiding the view to him. Anyone doing serious/real work with OL3 will need to manipulate the view. For example, after the map is created, the user will have to do map.getView().zoomByDelta(1) to zoom the map/view. (And I hope we agree we don't want the map to expose proxy methods for every View2D method.)

I personally don't have major concerns with adding view-related map options, as you're suggesting it. I do think we loose some consistency though. For example, with a "center" option the user may expect to be able to call map.getCenter or map.setCenter. (Again I hope we agree we don't want the map to expose proxy methods for every View2D method. :)

So to me the question is: is the extra convenience (having view-related map options) worth the consistency loss? I tend to think that it's not, but I can be convinced if others think it is.

As a reminder this is what we're comparing:

var map = new ol.Map({
  ...
  view: {
    center: new ol.Coordinate(x, y),
    resolution: r
  }
});

versus

var map = new ol.Map({
  ...
  center: new ol.Coordinate(x, y),
  resolution: r
});

versus

var map = new ol.Map({
  ...
  view: new View2D({
    center: new ol.Coordinate(x, y),
    resolution: r
  })
});

The latter corresponds to what we have today. I still do feel that having to create a View2D object is a bit awkward. I would feel differently if it was ol.View, but it's not (for good reasons).

Comments, opinions, ideas are very welcome.

@ahocevar
Owner

Good point about consistency @elemoine. What you are saying suggests that one option would be to rename ol.View2D to ol.View, which I think would be fine. ol.View3D would work regardless.

@scratscratte
@twpayne

Agree with everything that @elemoine says.

The ol.View name is already used. Renaming ol.View2D to ol.View would cause us to have to rename ol.View to something else (ol.AbstractView? ol.ViewBase?) and if we wanted to keep the API consistent with the class names we'd end up with something like:

map.getAbstractView().getView().setResolution(1000);

I strongly prefer the current naming scheme. It is clear, simple, and consistent.

@elemoine
Owner

FYI, l liked Andreas' idea, but you're making good points @twpayne.

@twpayne

This discussion is now continuing in #457 and #463.

@twpayne twpayne closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.