Use pointer events #1840

Merged
merged 101 commits into from Mar 14, 2014

Conversation

Projects
None yet
6 participants
@tsauerwein
Member

tsauerwein commented Mar 11, 2014

This pull request bases the internal event system of OpenLayers on pointer events.

So far, there were separate interactions for touch and mouse events. Now, the interactions only receive pointer events. These pointer events are generated from a PointerEventHandler, which listens to mouse events, touch events or real pointer events (on IE 10 and 11). The code is based on the pointer events polyfill of the Polymer library.

All existing interactions have been ported to pointer events. The modify and draw feature interaction have been adapted by @ahocevar.

Also thanks to @fredj and @elemoine for the feedback!

Closes #1099

@fredj

This comment has been minimized.

Show comment
Hide comment
@fredj

fredj Mar 11, 2014

Member

Should also fixes #984 and #987

Member

fredj commented Mar 11, 2014

Should also fixes #984 and #987

src/ol/interaction/pointerinteraction.js
+ if (mapBrowserPointerEvent.type == ol.MapBrowserEvent.EventType.POINTERDOWN) {
+ var handled = this.handlePointerDown(mapBrowserPointerEvent);
+ if (!this.handled_ && handled) {
+ view.setHint(ol.ViewHint.INTERACTING, 1);

This comment has been minimized.

@tschaub

tschaub Mar 11, 2014

Member

The "interacting" view hint is used elsewhere to mean that user generated events are actually modifying the view state. By setting this hint here, it means that all other interactions that return true from handlePointerDown should be modifying the view state.

Instead, I think that interactions that actually modify the view state should be responsible for setting the view hint. See also #1277.

@tschaub

tschaub Mar 11, 2014

Member

The "interacting" view hint is used elsewhere to mean that user generated events are actually modifying the view state. By setting this hint here, it means that all other interactions that return true from handlePointerDown should be modifying the view state.

Instead, I think that interactions that actually modify the view state should be responsible for setting the view hint. See also #1277.

This comment has been minimized.

@tsauerwein

tsauerwein Mar 12, 2014

Member

This is the original logic from the previous TouchInteraction. But if you want to change it, we should handle it maybe in a separate pull request?

@tsauerwein

tsauerwein Mar 12, 2014

Member

This is the original logic from the previous TouchInteraction. But if you want to change it, we should handle it maybe in a separate pull request?

This comment has been minimized.

@elemoine

elemoine Mar 12, 2014

Member

Agree.

And having the specific interactions set/unset the interaction hint would require that the this.handled_ state is shared between the base class and the specific class.

@elemoine

elemoine Mar 12, 2014

Member

Agree.

And having the specific interactions set/unset the interaction hint would require that the this.handled_ state is shared between the base class and the specific class.

This comment has been minimized.

@tschaub

tschaub Mar 12, 2014

Member

Agree.

With what?

What I'm saying is that an interaction that modifies the view state should set the interacting hint. This doesn't require sharing this.handled_.

Basically, only call view.setHint when you plan on modifying the view state. See also b0362b8.

If I wanted to create an interaction that extended ol.interaction.Pointer but didn't necessarily modify the view state (e.g. maybe it just draws a box), the interacting hint would be incorrectly set.

@tschaub

tschaub Mar 12, 2014

Member

Agree.

With what?

What I'm saying is that an interaction that modifies the view state should set the interacting hint. This doesn't require sharing this.handled_.

Basically, only call view.setHint when you plan on modifying the view state. See also b0362b8.

If I wanted to create an interaction that extended ol.interaction.Pointer but didn't necessarily modify the view state (e.g. maybe it just draws a box), the interacting hint would be incorrectly set.

This comment has been minimized.

@elemoine

elemoine Mar 12, 2014

Member

With what?

I agree with @tsauerwein that this can be addressed with a separate pull request.

What I'm saying is that an interaction that modifies the view state should set the interacting hint. This doesn't require sharing this.handled_.

I think I understand what you are saying, and I agree with you, but I also think that this code/logic is sensitive and easy to break.

Some observations:

  • Currently we make sure we don't set the interacting hint again if we already set it. The this.handled_ variable is used for that. So if a pointerdown was handled already, then we will not set the interaction hint again if a second pointer is added to the screen.
  • In the case of the DragPan interaction we set the interacting hint even though handlePointerDown did not change the view state. We need to set the interacting it because handlePointerDown requested a render frame.

If I wanted to create an interaction that extended ol.interaction.Pointer but didn't necessarily modify the view state (e.g. maybe it just draws a box), the interacting hint would be incorrectly set.

True. We do set the interacting hint when drawing zoom boxes today, although it may not be necessary.

So, I agree with you that the specific pointer interactions should be responsible for setting/unsetting the interacting hint, but I think making the required changes is not trivial. Hence my recommendation to address this in the separate pull request. @tsauerwein "just" ported the interactions and kept the current "view hints" logic.

@elemoine

elemoine Mar 12, 2014

Member

With what?

I agree with @tsauerwein that this can be addressed with a separate pull request.

What I'm saying is that an interaction that modifies the view state should set the interacting hint. This doesn't require sharing this.handled_.

I think I understand what you are saying, and I agree with you, but I also think that this code/logic is sensitive and easy to break.

Some observations:

  • Currently we make sure we don't set the interacting hint again if we already set it. The this.handled_ variable is used for that. So if a pointerdown was handled already, then we will not set the interaction hint again if a second pointer is added to the screen.
  • In the case of the DragPan interaction we set the interacting hint even though handlePointerDown did not change the view state. We need to set the interacting it because handlePointerDown requested a render frame.

If I wanted to create an interaction that extended ol.interaction.Pointer but didn't necessarily modify the view state (e.g. maybe it just draws a box), the interacting hint would be incorrectly set.

True. We do set the interacting hint when drawing zoom boxes today, although it may not be necessary.

So, I agree with you that the specific pointer interactions should be responsible for setting/unsetting the interacting hint, but I think making the required changes is not trivial. Hence my recommendation to address this in the separate pull request. @tsauerwein "just" ported the interactions and kept the current "view hints" logic.

This comment has been minimized.

@elemoine

elemoine Mar 12, 2014

Member

Hmm... There may well be problems here. With b0362b8 the DragPan, DragRotateAndZoom, and DragRotate interactions do set/unset the interacting hints themselves. But these interactions now inherit from the Pointer interaction, which also sets/unsets the interacting hint. This means the interacting hint is set/unset twice on pointerdown/pointerup.

@elemoine

elemoine Mar 12, 2014

Member

Hmm... There may well be problems here. With b0362b8 the DragPan, DragRotateAndZoom, and DragRotate interactions do set/unset the interacting hints themselves. But these interactions now inherit from the Pointer interaction, which also sets/unsets the interacting hint. This means the interacting hint is set/unset twice on pointerdown/pointerup.

This comment has been minimized.

@tschaub

tschaub Mar 13, 2014

Member

Right, this is what I was suggesting we avoid. I think the interacting hint should only be set when modifying the view. This is more specific than the "handled" response. You might handle the event without actually intending to modify the view state.

@tschaub

tschaub Mar 13, 2014

Member

Right, this is what I was suggesting we avoid. I think the interacting hint should only be set when modifying the view. This is more specific than the "handled" response. You might handle the event without actually intending to modify the view state.

This comment has been minimized.

This comment has been minimized.

@tsauerwein

tsauerwein Mar 13, 2014

Member

Your changes look good to me, I added them to the PR.

@tsauerwein

tsauerwein Mar 13, 2014

Member

Your changes look good to me, I added them to the PR.

examples/modify-features.js
@@ -165,6 +166,18 @@ var vectorLayer = new ol.layer.Vector({
style: styleFunction
});
+// FIXME Handle this elsewhere - this is only to not render selected features
+// on the original layer, and it does not work for MultiGeometry geometries.
+vectorLayer.setRenderGeometryFunctions(new ol.Collection([function(geometry) {

This comment has been minimized.

@tschaub

tschaub Mar 11, 2014

Member

Should these changes to the modify features example be removed (or handled in a separate PR)?

@tschaub

tschaub Mar 11, 2014

Member

Should these changes to the modify features example be removed (or handled in a separate PR)?

This comment has been minimized.

@ahocevar

ahocevar Mar 11, 2014

Member

Yes, we can remove this. There is a pull request already: #1834.

@ahocevar

ahocevar Mar 11, 2014

Member

Yes, we can remove this. There is a pull request already: #1834.

src/ol/mapbrowserevent.js
- TOUCHMOVE: goog.events.EventType.TOUCHMOVE,
- TOUCHEND: goog.events.EventType.TOUCHEND
+ DBLCLICK: goog.events.EventType.DBLCLICK,
+ POINTERDRAG: 'pointermove',

This comment has been minimized.

@tschaub

tschaub Mar 11, 2014

Member

Should this value be pointerdrag? As it is, you couldn't separately listen for drag and move events (without keeping track of down).

@tschaub

tschaub Mar 11, 2014

Member

Should this value be pointerdrag? As it is, you couldn't separately listen for drag and move events (without keeping track of down).

This comment has been minimized.

@ahocevar

ahocevar Mar 11, 2014

Member

Good catch. Looks like a typo to me. @tsauerwein, can you confirm? I did some testing after changing the value, and everything seemed right.

@ahocevar

ahocevar Mar 11, 2014

Member

Good catch. Looks like a typo to me. @tsauerwein, can you confirm? I did some testing after changing the value, and everything seemed right.

This comment has been minimized.

@tsauerwein

tsauerwein Mar 12, 2014

Member

Yes, this was a typo. Thanks for noticing!

@tsauerwein

tsauerwein Mar 12, 2014

Member

Yes, this was a typo. Thanks for noticing!

src/ol/pointer/touchsource.js
+ lts.splice(i, 1);
+ }
+ }, null, lts, lt);
+ goog.global.setTimeout(fn, this.DEDUP_TIMEOUT);

This comment has been minimized.

@tschaub

tschaub Mar 11, 2014

Member

Minor, but fn could be substituted for the anonymous function above. That is, the goog.bind call is unnecessary.

@tschaub

tschaub Mar 11, 2014

Member

Minor, but fn could be substituted for the anonymous function above. That is, the goog.bind call is unnecessary.

@tschaub

This comment has been minimized.

Show comment
Hide comment
@tschaub

tschaub Mar 11, 2014

Member

This looks like a lot of great work!

Member

tschaub commented Mar 11, 2014

This looks like a lot of great work!

+ // only the primary finger will synth mouse events
+ if (this.isPrimaryTouch_(t)) {
+ // remember x/y of last touch
+ var lt = new goog.math.Coordinate(t.clientX, t.clientY);

This comment has been minimized.

@fredj

fredj Mar 12, 2014

Member

goog.math.Coordinate should not be necessary here, [t.clientX, t.clientY] is sufficient (not tested)

@fredj

fredj Mar 12, 2014

Member

goog.math.Coordinate should not be necessary here, [t.clientX, t.clientY] is sufficient (not tested)

This comment has been minimized.

@tsauerwein

tsauerwein Mar 12, 2014

Member

I used goog.math.Coordinate on purpose to make it more type safe, especially since the array lastTouches is used across two classes (TouchSource and MouseSource). But if you think it is not necessary, I will remove it.

@tsauerwein

tsauerwein Mar 12, 2014

Member

I used goog.math.Coordinate on purpose to make it more type safe, especially since the array lastTouches is used across two classes (TouchSource and MouseSource). But if you think it is not necessary, I will remove it.

This comment has been minimized.

@fredj

fredj Mar 12, 2014

Member

ok, I had not noticed.

@fredj

fredj Mar 12, 2014

Member

ok, I had not noticed.

@elemoine

This comment has been minimized.

Show comment
Hide comment
@elemoine

elemoine Mar 12, 2014

Member

Impressive work!

I just have a comment on the naming of interactions.

Up to now we've used names like DoubleClickZoom, where DoubleClick is the gesture and Zoom is the action performed on that gesture. And we still have interactions following that naming convention: DoubleClickZoom, DragRotate, etc.

So to continue using that convention I would suggest to rename Pan to DragPan, Zoom to PinchZoom, and Rotate to PinchRotate.

(I know Select, Draw, and Modify do not follow that convention, but they're also more complex/rich interaction.)

Member

elemoine commented Mar 12, 2014

Impressive work!

I just have a comment on the naming of interactions.

Up to now we've used names like DoubleClickZoom, where DoubleClick is the gesture and Zoom is the action performed on that gesture. And we still have interactions following that naming convention: DoubleClickZoom, DragRotate, etc.

So to continue using that convention I would suggest to rename Pan to DragPan, Zoom to PinchZoom, and Rotate to PinchRotate.

(I know Select, Draw, and Modify do not follow that convention, but they're also more complex/rich interaction.)

src/ol/pointer/touchsource.js
+ if (this.pointerMap.getCount() === 0 ||
+ (this.pointerMap.getCount() === 1 && this.pointerMap.containsKey(1))) {
+ this.firstTouchId_ = inTouch.identifier;
+ this.firstXY = {X: inTouch.clientX, Y: inTouch.clientY};

This comment has been minimized.

@fredj

fredj Mar 12, 2014

Member

this.firstXY should be declared in the constructor (BTW the variable doesn't seems to be used)

@fredj

fredj Mar 12, 2014

Member

this.firstXY should be declared in the constructor (BTW the variable doesn't seems to be used)

This comment has been minimized.

@tsauerwein

tsauerwein Mar 12, 2014

Member

You are right, firstXY is used nowhere.

@tsauerwein

tsauerwein Mar 12, 2014

Member

You are right, firstXY is used nowhere.

src/ol/pointer/touchsource.js
+ * @private
+ * @type {?number}
+ */
+ this.firstTouchId_ = null;

This comment has been minimized.

@fredj

fredj Mar 12, 2014

Member

Undefined values for basic types (string, number, ...) should be undefined instead of null (you can cherry-pick fredj/openlayers@e1793f7)

@fredj

fredj Mar 12, 2014

Member

Undefined values for basic types (string, number, ...) should be undefined instead of null (you can cherry-pick fredj/openlayers@e1793f7)

/**
* @inheritDoc
*/
-ol.interaction.DragBox.prototype.handleDrag = function(mapBrowserEvent) {
+ol.interaction.DragBox.prototype.handlePointerDrag = function(mapBrowserEvent) {
+ if (!ol.events.condition.mouseOnly(mapBrowserEvent)) {

This comment has been minimized.

@fredj

fredj Mar 12, 2014

Member

I'm wondering if it's really necessary to deactivate the interaction for non mouse input

@fredj

fredj Mar 12, 2014

Member

I'm wondering if it's really necessary to deactivate the interaction for non mouse input

This comment has been minimized.

@tsauerwein

tsauerwein Mar 12, 2014

Member

For example for the DragZoom interaction: You wouldn't press Shift and move one finger over the screen to zoom it. You would rather use a pinch zoom. That is why these interactions ignore events that do not come from a mouse device.

@tsauerwein

tsauerwein Mar 12, 2014

Member

For example for the DragZoom interaction: You wouldn't press Shift and move one finger over the screen to zoom it. You would rather use a pinch zoom. That is why these interactions ignore events that do not come from a mouse device.

This comment has been minimized.

@fredj

fredj Mar 13, 2014

Member

I think this should be configurable and not hard coded in the lib (same for the other interactions).
this.condition_ can be:

  /**
   * @type {ol.events.ConditionType}
   */
  this.condition_ = goog.isDef(options.condition) ?
      options.condition : ol.events.condition.mouseOnly;
@fredj

fredj Mar 13, 2014

Member

I think this should be configurable and not hard coded in the lib (same for the other interactions).
this.condition_ can be:

  /**
   * @type {ol.events.ConditionType}
   */
  this.condition_ = goog.isDef(options.condition) ?
      options.condition : ol.events.condition.mouseOnly;

This comment has been minimized.

This comment has been minimized.

@elemoine

elemoine Mar 13, 2014

Member

I don't think that the Drag* interactions work properly with touches (except DragPan obviously). So for now I'd leave the mouseOnly condition as non-configurable.

@elemoine

elemoine Mar 13, 2014

Member

I don't think that the Drag* interactions work properly with touches (except DragPan obviously). So for now I'd leave the mouseOnly condition as non-configurable.

This comment has been minimized.

@fredj

fredj Mar 13, 2014

Member

I don't think you need to test in every handle* function; only in handlePointerDown (just like browserEvent.isMouseActionButton())

@fredj

fredj Mar 13, 2014

Member

I don't think you need to test in every handle* function; only in handlePointerDown (just like browserEvent.isMouseActionButton())

This comment has been minimized.

@tsauerwein

tsauerwein Mar 13, 2014

Member

For example for DragZoom: Let's say Shiftis pressed and a mousedown occurs, this activates the interaction. Now, a touchstart occurs (which is not handled by handlePointerDown). But the following pointermove and pointerup events issued from this touch, would be handled by the interaction (without the check). So, the interaction might get deactivated before the mouseup happens.
Anyway, it doesn't work that you can zoom with the DragZoom while touching the screen. But with these checks I try to avoid that the interactions are left in a broken state.

@tsauerwein

tsauerwein Mar 13, 2014

Member

For example for DragZoom: Let's say Shiftis pressed and a mousedown occurs, this activates the interaction. Now, a touchstart occurs (which is not handled by handlePointerDown). But the following pointermove and pointerup events issued from this touch, would be handled by the interaction (without the check). So, the interaction might get deactivated before the mouseup happens.
Anyway, it doesn't work that you can zoom with the DragZoom while touching the screen. But with these checks I try to avoid that the interactions are left in a broken state.

src/ol/interaction/pointerinteraction.js
+ }
+
+ var mapBrowserPointerEvent =
+ /** @type {ol.MapBrowserPointerEvent} */ (mapBrowserEvent);

This comment has been minimized.

@twpayne

twpayne Mar 12, 2014

Contributor

For info, you can use goog.asserts.assertInstanceOf here to avoid the typecast.

@twpayne

twpayne Mar 12, 2014

Contributor

For info, you can use goog.asserts.assertInstanceOf here to avoid the typecast.

This comment has been minimized.

@tsauerwein

tsauerwein Mar 13, 2014

Member

I thought I would need the typecast for the Closure compiler, but apparently I don't. I did not add an assertion, because I am doing an instanceof check in the line above (which is required to ensure that events other than pointer events are not handler by the PointerInteraction).

@tsauerwein

tsauerwein Mar 13, 2014

Member

I thought I would need the typecast for the Closure compiler, but apparently I don't. I did not add an assertion, because I am doing an instanceof check in the line above (which is required to ensure that events other than pointer events are not handler by the PointerInteraction).

src/ol/mapbrowserevent.js
@@ -9,9 +10,12 @@ goog.require('goog.events.BrowserEvent');
goog.require('goog.events.EventTarget');
goog.require('goog.events.EventType');
goog.require('goog.object');
+

This comment has been minimized.

@twpayne

twpayne Mar 12, 2014

Contributor

Stray blank line.

@twpayne

twpayne Mar 12, 2014

Contributor

Stray blank line.

@@ -0,0 +1,177 @@
+

This comment has been minimized.

@twpayne

twpayne Mar 12, 2014

Contributor

Stray blank line.

@twpayne

twpayne Mar 12, 2014

Contributor

Stray blank line.

- TOUCHEND: goog.events.EventType.TOUCHEND
+ DBLCLICK: goog.events.EventType.DBLCLICK,
+ POINTERDRAG: 'pointerdrag',
+

This comment has been minimized.

@twpayne

twpayne Mar 12, 2014

Contributor

Stray blank line.

@twpayne

twpayne Mar 12, 2014

Contributor

Stray blank line.

@@ -0,0 +1,56 @@
+

This comment has been minimized.

@twpayne

twpayne Mar 12, 2014

Contributor

Stray blank line.

@twpayne

twpayne Mar 12, 2014

Contributor

Stray blank line.

+/**
+ * @param {ol.pointer.PointerEventHandler} dispatcher
+ * @param {Object.<string, function(goog.events.BrowserEvent)>} mapping
+ * @constructor

This comment has been minimized.

@twpayne

twpayne Mar 12, 2014

Contributor

It would be good to add a @struct annotation to this class to catch any properties initialised outside the constructor.

@twpayne

twpayne Mar 12, 2014

Contributor

It would be good to add a @struct annotation to this class to catch any properties initialised outside the constructor.

This comment has been minimized.

@tsauerwein

tsauerwein Mar 13, 2014

Member

I can not add the @struct annotation, because PointerEvent is inheriting from goog.events.Event, which is not a struct.

@tsauerwein

tsauerwein Mar 13, 2014

Member

I can not add the @struct annotation, because PointerEvent is inheriting from goog.events.Event, which is not a struct.

This comment has been minimized.

@twpayne

twpayne Mar 13, 2014

Contributor

OK, thanks for trying!

@twpayne

twpayne Mar 13, 2014

Contributor

OK, thanks for trying!

src/ol/pointer/mousesource.js
+
+ /**
+ * @const
+ * @type {goog.structs.Map}

This comment has been minimized.

@twpayne

twpayne Mar 12, 2014

Contributor

Does this have to be a goog.structs.Map? Is it possible to use an Object here instead? Note that it is not possible to use an Object if the keys to the map could include strings like hasOwnProperty.

@twpayne

twpayne Mar 12, 2014

Contributor

Does this have to be a goog.structs.Map? Is it possible to use an Object here instead? Note that it is not possible to use an Object if the keys to the map could include strings like hasOwnProperty.

This comment has been minimized.

@tsauerwein

tsauerwein Mar 13, 2014

Member

The keys for this map are pointer ids, so I could use a plain object. But for me it feels more natural to call map.getCount() or map.containsKey(key) instead of goog.object.getCount(obj) or goog.object.containsKey(obj, key).

@tsauerwein

tsauerwein Mar 13, 2014

Member

The keys for this map are pointer ids, so I could use a plain object. But for me it feels more natural to call map.getCount() or map.containsKey(key) instead of goog.object.getCount(obj) or goog.object.containsKey(obj, key).

This comment has been minimized.

@elemoine

elemoine Mar 13, 2014

Member

FWIW using static functions rather than methods may avoid object property lookup operations at runtime. The compiler may rename goog.object.getCount to a non-namespaced function.

@elemoine

elemoine Mar 13, 2014

Member

FWIW using static functions rather than methods may avoid object property lookup operations at runtime. The compiler may rename goog.object.getCount to a non-namespaced function.

This comment has been minimized.

@twpayne

twpayne Mar 13, 2014

Contributor

Note that goog.structs.Map is both much slower and brings in more code that using a plain old JavaScript object. It may be that goog.structs.Map is already included in the build (I haven't checked) but unless you need to store arbitrary keys then it's better to use an Object.

@twpayne

twpayne Mar 13, 2014

Contributor

Note that goog.structs.Map is both much slower and brings in more code that using a plain old JavaScript object. It may be that goog.structs.Map is already included in the build (I haven't checked) but unless you need to store arbitrary keys then it's better to use an Object.

+
+/**
+ * Warning is suppressed because Closure thinks the MouseEvent
+ * constructor takes no arguments.

This comment has been minimized.

@twpayne

twpayne Mar 12, 2014

Contributor

Does the MouseEvent constructor really take two arguments?

My reading of the DOM level 2 spec that MouseEvent is an interface, not a constructor, and that instances should be created with createEvent, but I might have misunderstood.

@twpayne

twpayne Mar 12, 2014

Contributor

Does the MouseEvent constructor really take two arguments?

My reading of the DOM level 2 spec that MouseEvent is an interface, not a constructor, and that instances should be created with createEvent, but I might have misunderstood.

This comment has been minimized.

@tsauerwein

tsauerwein Mar 13, 2014

Member

The MouseEvent constructor was introduced in DOM level 3 and this check tests if the buttons property is supported (since DOM level 4).

@tsauerwein

tsauerwein Mar 13, 2014

Member

The MouseEvent constructor was introduced in DOM level 3 and this check tests if the buttons property is supported (since DOM level 4).

This comment has been minimized.

@fredj

fredj Mar 13, 2014

Member

This is a bug in closure compiler externs, fixed with https://code.google.com/p/closure-compiler/source/detail?r=654c073d

@fredj

fredj Mar 13, 2014

Member

This is a bug in closure compiler externs, fixed with https://code.google.com/p/closure-compiler/source/detail?r=654c073d

src/ol/pointer/pointereventhandler.js
+
+goog.provide('ol.pointer.PointerEventHandler');
+
+

This comment has been minimized.

@twpayne

twpayne Mar 12, 2014

Contributor

Stray blank line.

@twpayne

twpayne Mar 12, 2014

Contributor

Stray blank line.

src/ol/pointer/pointereventhandler.js
+
+ /**
+ * @const
+ * @type {goog.structs.Map}

This comment has been minimized.

@twpayne

twpayne Mar 12, 2014

Contributor

Same comment about use of goog.structs.Map vs. Object as before.

@twpayne

twpayne Mar 12, 2014

Contributor

Same comment about use of goog.structs.Map vs. Object as before.

src/ol/pointer/pointereventhandler.js
+ */
+ol.pointer.PointerEventHandler.prototype.register_ = function() {
+ var l = this.eventSourceList_.length;
+ for (var i = 0, es; (i < l) && (es = this.eventSourceList_[i]); i++) {

This comment has been minimized.

@twpayne

twpayne Mar 12, 2014

Contributor

Is there a good reason to use an assignment inside an expression here?

@twpayne

twpayne Mar 12, 2014

Contributor

Is there a good reason to use an assignment inside an expression here?

src/ol/pointer/pointereventhandler.js
+ */
+ol.pointer.PointerEventHandler.prototype.unregister_ = function() {
+ var l = this.eventSourceList_.length;
+ for (var i = 0, es; (i < l) && (es = this.eventSourceList_[i]); i++) {

This comment has been minimized.

@twpayne

twpayne Mar 12, 2014

Contributor

Same comment re assignments inside expressions.

@twpayne

twpayne Mar 12, 2014

Contributor

Same comment re assignments inside expressions.

src/ol/pointer/pointereventhandler.js
+ */
+ol.pointer.PointerEventHandler.prototype.down =
+ function(pointerEventData, browserEvent) {
+ this.fireEvent('pointerdown', pointerEventData, browserEvent);

This comment has been minimized.

@twpayne

twpayne Mar 12, 2014

Contributor

Is it possible to use an @enum for the event types (pointerdown, pointermove, etc.). Use of @enums can help catch typos.

@twpayne

twpayne Mar 12, 2014

Contributor

Is it possible to use an @enum for the event types (pointerdown, pointermove, etc.). Use of @enums can help catch typos.

This comment has been minimized.

@twpayne

twpayne Mar 12, 2014

Contributor

ol.pointer.EventType could be used.

@twpayne

twpayne Mar 12, 2014

Contributor

ol.pointer.EventType could be used.

src/ol/pointer/pointereventhandler.js
+
+
+/**
+ * List of properties to copy when cloning an event.

This comment has been minimized.

@twpayne

twpayne Mar 12, 2014

Contributor

I think it's clearer and less error prone to merge ol.pointer.CLONE_PROPS and ol.pointer.CLONE_DEFAULTS into a single array. See twpayne/ol3@858d268.

@twpayne

twpayne Mar 12, 2014

Contributor

I think it's clearer and less error prone to merge ol.pointer.CLONE_PROPS and ol.pointer.CLONE_DEFAULTS into a single array. See twpayne/ol3@858d268.

src/ol/pointer/touchsource.js
+ this.resetId_ = undefined;
+ };
+ this.resetId_ = goog.global.setTimeout(goog.bind(fn, this),
+ ol.pointer.TouchSource.CLICK_COUNT_TIMEOUT);

This comment has been minimized.

@twpayne

twpayne Mar 12, 2014

Contributor

fn and goog.bind(fn, this) can be created once in the constructor, stored in a private member variable, and re-used.

@twpayne

twpayne Mar 12, 2014

Contributor

fn and goog.bind(fn, this) can be created once in the constructor, stored in a private member variable, and re-used.

+/**
+ * @private
+ */
+ol.pointer.TouchSource.prototype.resetClickCount_ = function() {

This comment has been minimized.

@twpayne

twpayne Mar 12, 2014

Contributor

You should probably add an

goog.asserts.assert(!goog.isDef(this.resetId_));

here

@twpayne

twpayne Mar 12, 2014

Contributor

You should probably add an

goog.asserts.assert(!goog.isDef(this.resetId_));

here

This comment has been minimized.

@tsauerwein

tsauerwein Mar 13, 2014

Member

To make sure that an previous reset id is not overridden, agree.

@tsauerwein

tsauerwein Mar 13, 2014

Member

To make sure that an previous reset id is not overridden, agree.

src/ol/pointer/touchsource.js
+ * @return {boolean} True, if the `Touch` with the given id is in the list.
+ */
+ol.pointer.TouchSource.prototype.findTouch_ = function(touchList, searchId) {
+ for (var i = 0, l = touchList.length, t; i < l && (t = touchList[i]); i++) {

This comment has been minimized.

@twpayne

twpayne Mar 12, 2014

Contributor

Assignment inside an expression.

@twpayne

twpayne Mar 12, 2014

Contributor

Assignment inside an expression.

src/ol/pointer/touchsource.js
+ var i = lts.indexOf(lt);
+ if (i > -1) {
+ lts.splice(i, 1);
+ }

This comment has been minimized.

@twpayne

twpayne Mar 12, 2014

Contributor

You can use goog.array.remove here.

@twpayne

twpayne Mar 12, 2014

Contributor

You can use goog.array.remove here.

@twpayne

This comment has been minimized.

Show comment
Hide comment
@twpayne

twpayne Mar 12, 2014

Contributor

This is fantastic work @tsauerwein! I've added a few comments, but am very much looking forward to this being merged.

Contributor

twpayne commented Mar 12, 2014

This is fantastic work @tsauerwein! I've added a few comments, but am very much looking forward to this being merged.

@twpayne twpayne referenced this pull request Mar 12, 2014

Merged

Remove leading whitespace #1853

examples/google-map.js
@@ -3,7 +3,7 @@
goog.require('ol.Map');
goog.require('ol.View2D');
goog.require('ol.interaction');
-goog.require('ol.interaction.DragPan');
+goog.require('ol.interaction.Pan');

This comment has been minimized.

@elemoine

elemoine Mar 13, 2014

Member

Isn't the interaction DragPan rather than Pan now?

@elemoine

elemoine Mar 13, 2014

Member

Isn't the interaction DragPan rather than Pan now?

This comment has been minimized.

@tsauerwein

tsauerwein Mar 13, 2014

Member

It is. Too bad these kind of mistakes are not detected by build.py check.

@tsauerwein

tsauerwein Mar 13, 2014

Member

It is. Too bad these kind of mistakes are not detected by build.py check.

This comment has been minimized.

@elemoine

elemoine Mar 13, 2014

Member

The google-map example includes // NOCOMPILE. This is why this was not
detected.

On Thu, Mar 13, 2014 at 3:09 PM, Tobias Sauerwein
notifications@github.comwrote:

In examples/google-map.js:

@@ -3,7 +3,7 @@
goog.require('ol.Map');
goog.require('ol.View2D');
goog.require('ol.interaction');
-goog.require('ol.interaction.DragPan');
+goog.require('ol.interaction.Pan');

It is. Too bad these kind of mistakes are not detected by build.py check.

Reply to this email directly or view it on GitHubhttps://github.com/openlayers/ol3/pull/1840/files#r10565255
.

Eric Lemoine

Camptocamp France SAS
Savoie Technolac, BP 352
73377 Le Bourget du Lac, Cedex

Tel : 00 33 4 79 44 44 94
Mail : eric.lemoine@camptocamp.com
http://www.camptocamp.com

@elemoine

elemoine Mar 13, 2014

Member

The google-map example includes // NOCOMPILE. This is why this was not
detected.

On Thu, Mar 13, 2014 at 3:09 PM, Tobias Sauerwein
notifications@github.comwrote:

In examples/google-map.js:

@@ -3,7 +3,7 @@
goog.require('ol.Map');
goog.require('ol.View2D');
goog.require('ol.interaction');
-goog.require('ol.interaction.DragPan');
+goog.require('ol.interaction.Pan');

It is. Too bad these kind of mistakes are not detected by build.py check.

Reply to this email directly or view it on GitHubhttps://github.com/openlayers/ol3/pull/1840/files#r10565255
.

Eric Lemoine

Camptocamp France SAS
Savoie Technolac, BP 352
73377 Le Bourget du Lac, Cedex

Tel : 00 33 4 79 44 44 94
Mail : eric.lemoine@camptocamp.com
http://www.camptocamp.com

src/objectliterals.jsdoc
@@ -468,7 +460,7 @@
*/
/**
- * @typedef {Object} olx.interaction.TouchPanOptions
+ * @typedef {Object} olx.interaction.DragPanOptions

This comment has been minimized.

@elemoine

elemoine Mar 13, 2014

Member

Minor, but the type definitions in this file are ordered alphabetically. This means that DragPanOptions should come before DragRotateOptions.

@elemoine

elemoine Mar 13, 2014

Member

Minor, but the type definitions in this file are ordered alphabetically. This means that DragPanOptions should come before DragRotateOptions.

+ * @todo stability experimental
+ */
+ol.events.condition.mouseOnly = function(mapBrowserEvent) {
+ goog.asserts.assertInstanceof(mapBrowserEvent, ol.MapBrowserPointerEvent);

This comment has been minimized.

@elemoine

elemoine Mar 13, 2014

Member

It may be better that mouseOnly takes an ol.MapBrowserPointerEvent rather than a ol.MapBrowserEvent.

@elemoine

elemoine Mar 13, 2014

Member

It may be better that mouseOnly takes an ol.MapBrowserPointerEvent rather than a ol.MapBrowserEvent.

This comment has been minimized.

@fredj

fredj Mar 13, 2014

Member

If ol.MapBrowserPointerEvent is used, the function will not be a ol.MapBrowserPointerEvent anymore

@fredj

fredj Mar 13, 2014

Member

If ol.MapBrowserPointerEvent is used, the function will not be a ol.MapBrowserPointerEvent anymore

This comment has been minimized.

@tsauerwein

tsauerwein Mar 13, 2014

Member

... not be a ol.events.ConditionType anymore, yes.

@tsauerwein

tsauerwein Mar 13, 2014

Member

... not be a ol.events.ConditionType anymore, yes.

This comment has been minimized.

@elemoine

elemoine Mar 13, 2014

Member

I see.

@elemoine

elemoine Mar 13, 2014

Member

I see.

@elemoine

This comment has been minimized.

Show comment
Hide comment
@elemoine

elemoine Mar 13, 2014

Member

"pointerdown" (non-prefixed) should be added to this list https://github.com/tsauerwein/ol3/blob/pointerevents/src/ol/map.js#L259-264.

Member

elemoine commented Mar 13, 2014

"pointerdown" (non-prefixed) should be added to this list https://github.com/tsauerwein/ol3/blob/pointerevents/src/ol/map.js#L259-264.

src/ol/mapbrowserevent.js
+ this.handlePointerUp_, false, this),
+ goog.events.listen(this.pointerEventHandler_,
+ ol.MapBrowserEvent.EventType.POINTERCANCEL,
+ this.handlePointerUp_, false, this)

This comment has been minimized.

@elemoine

elemoine Mar 13, 2014

Member

Any reason for treating pointercancel events as pointerup events. This also means that interactions will get a pointerup event when a pointercancel event actually occurred.

@elemoine

elemoine Mar 13, 2014

Member

Any reason for treating pointercancel events as pointerup events. This also means that interactions will get a pointerup event when a pointercancel event actually occurred.

This comment has been minimized.

@tsauerwein

tsauerwein Mar 13, 2014

Member

On touch devices, if a finger leaves the screen outside the viewport element, you might not receive a pointerup but a pointercancel instead. That is why I also listen to pointercancel to properly stop the dragging.

@tsauerwein

tsauerwein Mar 13, 2014

Member

On touch devices, if a finger leaves the screen outside the viewport element, you might not receive a pointerup but a pointercancel instead. That is why I also listen to pointercancel to properly stop the dragging.

This comment has been minimized.

@elemoine

elemoine Mar 13, 2014

Member

The conversion from pointercancel to pointerup bothers me, but we can address this with a separate PR I guess.

@elemoine

elemoine Mar 13, 2014

Member

The conversion from pointercancel to pointerup bothers me, but we can address this with a separate PR I guess.

This comment has been minimized.

@tsauerwein

tsauerwein Mar 13, 2014

Member

I understand your concern. This pointerup is more like dragend. Especially since pointerup events are only dispatched if a pointerdown happened before.

@tsauerwein

tsauerwein Mar 13, 2014

Member

I understand your concern. This pointerup is more like dragend. Especially since pointerup events are only dispatched if a pointerdown happened before.

This comment has been minimized.

@elemoine

elemoine Mar 13, 2014

Member

I am realizing that the drag listeners are on the document rather than the viewport, which makes sense. pointercancel is the exception though. Is this normal? Shouldn't it be this.documentPointerEventHandler_ as well on line 360?

@elemoine

elemoine Mar 13, 2014

Member

I am realizing that the drag listeners are on the document rather than the viewport, which makes sense. pointercancel is the exception though. Is this normal? Shouldn't it be this.documentPointerEventHandler_ as well on line 360?

This comment has been minimized.

@tsauerwein

tsauerwein Mar 14, 2014

Member

I wasn't aware of it, but I think it still makes sense. TouchSource.vacuumTouches_() issues pointercancel events, when there was no touchend for a touchstart.
Now, let's say a first touchstart is registered on pointerEventHandler_. The documentPointerEventHandler_ is set up. But documentPointerEventHandler_ doesn't know about the first touchstart. If there is no touchend for the touchstart, we can only receive a touchcancel from pointerEventHandler_, because it is only registered there.

I hope this makes sense.

@tsauerwein

tsauerwein Mar 14, 2014

Member

I wasn't aware of it, but I think it still makes sense. TouchSource.vacuumTouches_() issues pointercancel events, when there was no touchend for a touchstart.
Now, let's say a first touchstart is registered on pointerEventHandler_. The documentPointerEventHandler_ is set up. But documentPointerEventHandler_ doesn't know about the first touchstart. If there is no touchend for the touchstart, we can only receive a touchcancel from pointerEventHandler_, because it is only registered there.

I hope this makes sense.

@tsauerwein

This comment has been minimized.

Show comment
Hide comment
@tsauerwein

tsauerwein Mar 14, 2014

Member

Thanks a lot for all the comments! From my side the pull request is now ready to merge.

Member

tsauerwein commented Mar 14, 2014

Thanks a lot for all the comments! From my side the pull request is now ready to merge.

@elemoine

This comment has been minimized.

Show comment
Hide comment
@elemoine

elemoine Mar 14, 2014

Member

So merging... :-) Thanks again everyone.

Member

elemoine commented Mar 14, 2014

So merging... :-) Thanks again everyone.

elemoine pushed a commit that referenced this pull request Mar 14, 2014

@elemoine elemoine merged commit c8f981b into openlayers:master Mar 14, 2014

1 check passed

default The Travis CI build passed
Details

@adube adube referenced this pull request Mar 14, 2014

Closed

Fix modifyinteraction #1841

@tschaub

This comment has been minimized.

Show comment
Hide comment
@tschaub

tschaub May 13, 2014

Member

Looks like a missing clock.restore() here (see #2063).

Looks like a missing clock.restore() here (see #2063).

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