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.

Sign up for GitHub

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

Jump to bottom

Don't reuse Map#{moving,zooming} state #6183

Merged
merged 3 commits into from
Feb 17, 2018
Merged

Don't reuse Map#{moving,zooming} state #6183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
105eaea
Don't reuse Map#{moving,zooming} state
jfirebaugh Feb 16, 2018
9a4bfec
Add isRotating(), _-prefix private properties
jfirebaugh Feb 16, 2018
a0b86f7
Add a test for isZooming on double click
jfirebaugh Feb 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 31 additions & 51 deletions src/ui/camera.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,10 @@ type AnimationOptions = {

class Camera extends Evented {
transform: Transform;
moving: boolean;
zooming: boolean;
rotating: boolean;
pitching: boolean;
_moving: boolean;
_zooming: boolean;
_rotating: boolean;
_pitching: boolean;

_bearingSnap: number;
_onEaseEnd: number;
Expand All @@ -82,8 +82,8 @@ class Camera extends Evented {

constructor(transform: Transform, options: {bearingSnap: number}) {
super();
this.moving = false;
this.zooming = false;
this._moving = false;
this._zooming = false;
this.transform = transform;
this._bearingSnap = options.bearingSnap;
}
Expand Down Expand Up @@ -562,22 +562,22 @@ class Camera extends Evented {
aroundPoint = tr.locationPoint(around);
}

this.zooming = (zoom !== startZoom);
this.rotating = (startBearing !== bearing);
this.pitching = (pitch !== startPitch);
this._zooming = (zoom !== startZoom);
this._rotating = (startBearing !== bearing);
this._pitching = (pitch !== startPitch);

this._prepareEase(eventData, options.noMoveStart);

clearTimeout(this._onEaseEnd);

this._ease((k) => {
if (this.zooming) {
if (this._zooming) {
tr.zoom = interpolate(startZoom, zoom, k);
}
if (this.rotating) {
if (this._rotating) {
tr.bearing = interpolate(startBearing, bearing, k);
}
if (this.pitching) {
if (this._pitching) {
tr.pitch = interpolate(startPitch, pitch, k);
}

Expand Down Expand Up @@ -607,43 +607,43 @@ class Camera extends Evented {
}

_prepareEase(eventData?: Object, noMoveStart: boolean) {
this.moving = true;
this._moving = true;

if (!noMoveStart) {
this.fire('movestart', eventData);
}
if (this.zooming) {
if (this._zooming) {
this.fire('zoomstart', eventData);
}
if (this.rotating) {
if (this._rotating) {
this.fire('rotatestart', eventData);
}
if (this.pitching) {
if (this._pitching) {
this.fire('pitchstart', eventData);
}
}

_fireMoveEvents(eventData?: Object) {
this.fire('move', eventData);
if (this.zooming) {
if (this._zooming) {
this.fire('zoom', eventData);
}
if (this.rotating) {
if (this._rotating) {
this.fire('rotate', eventData);
}
if (this.pitching) {
if (this._pitching) {
this.fire('pitch', eventData);
}
}

_afterEase(eventData?: Object) {
const wasZooming = this.zooming;
const wasRotating = this.rotating;
const wasPitching = this.pitching;
this.moving = false;
this.zooming = false;
this.rotating = false;
this.pitching = false;
const wasZooming = this._zooming;
const wasRotating = this._rotating;
const wasPitching = this._pitching;
this._moving = false;
this._zooming = false;
this._rotating = false;
this._pitching = false;

if (wasZooming) {
this.fire('zoomend', eventData);
Expand Down Expand Up @@ -825,9 +825,9 @@ class Camera extends Evented {
options.duration = 0;
}

this.zooming = true;
this.rotating = (startBearing !== bearing);
this.pitching = (pitch !== startPitch);
this._zooming = true;
this._rotating = (startBearing !== bearing);
this._pitching = (pitch !== startPitch);

this._prepareEase(eventData, false);

Expand All @@ -837,10 +837,10 @@ class Camera extends Evented {
const scale = 1 / w(s);
tr.zoom = startZoom + tr.scaleZoom(scale);

if (this.rotating) {
if (this._rotating) {
tr.bearing = interpolate(startBearing, bearing, k);
}
if (this.pitching) {
if (this._pitching) {
tr.pitch = interpolate(startPitch, pitch, k);
}

Expand All @@ -858,26 +858,6 @@ class Camera extends Evented {
return !!this._isEasing;
}

/**
* Returns a Boolean indicating whether the camera is moving.
*
* @memberof Map#
* @returns A Boolean indicating whether the camera is moving.
*/
isMoving(): boolean {
return this.moving;
}

/**
* Returns a Boolean indicating whether the camera is zooming.
*
* @memberof Map#
* @returns A Boolean indicating whether the camera is zooming.
*/
isZooming(): boolean {
return this.zooming;
}

/**
* Stops any animated transition underway.
*
Expand Down
2 changes: 0 additions & 2 deletions src/ui/handler/drag_pan.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,6 @@ class DragPanHandler {
// we treat the first move event (rather than the mousedown event)
// as the start of the drag
this._active = true;
this._map.moving = true;
this._fireEvent('dragstart', e);
this._fireEvent('movestart', e);
}
Expand Down Expand Up @@ -170,7 +169,6 @@ class DragPanHandler {
this._drainInertiaBuffer();

const finish = () => {
this._map.moving = false;
this._fireEvent('moveend', e);
};

Expand Down
2 changes: 0 additions & 2 deletions src/ui/handler/drag_rotate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,6 @@ class DragRotateHandler {

if (!this.isActive()) {
this._active = true;
this._map.moving = true;
this._fireEvent('rotatestart', e);
this._fireEvent('movestart', e);
if (this._pitchWithRotate) {
Expand Down Expand Up @@ -206,7 +205,6 @@ class DragRotateHandler {
if (Math.abs(mapBearing) < this._bearingSnap) {
map.resetNorth({noMoveStart: true}, { originalEvent: e });
} else {
this._map.moving = false;
this._fireEvent('moveend', e);
}
if (this._pitchWithRotate) this._fireEvent('pitchend', e);
Expand Down
4 changes: 0 additions & 4 deletions src/ui/handler/scroll_zoom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,8 +189,6 @@ class ScrollZoomHandler {
if (!this._delta) return;

this._active = true;
this._map.moving = true;
this._map.zooming = true;
this._map.fire('movestart', {originalEvent: e});
this._map.fire('zoomstart', {originalEvent: e});
clearTimeout(this._finishTimeout);
Expand Down Expand Up @@ -251,8 +249,6 @@ class ScrollZoomHandler {
if (!this.isActive()) return;
this._active = false;
this._finishTimeout = setTimeout(() => {
this._map.moving = false;
this._map.zooming = false;
this._map.fire('zoomend');
this._map.fire('moveend');
delete this._targetZoom;
Expand Down
30 changes: 28 additions & 2 deletions src/ui/map.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -580,6 +580,32 @@ class Map extends Camera {
return this.transform.pointLocation(Point.convert(point));
}

/**
* Returns true if the map is panning, zooming, rotating, or pitching due to a camera animation or user gesture.
*/
isMoving(): boolean {
return this._moving ||
this.dragPan.isActive() ||
this.dragRotate.isActive() ||
this.scrollZoom.isActive();
}

/**
* Returns true if the map is zooming due to a camera animation or user gesture.
*/
isZooming(): boolean {
return this._zooming ||
this.scrollZoom.isActive();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this.doubleClickZoom.isActive() also be called here?

Copy link
Contributor Author

@jfirebaugh jfirebaugh Feb 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DoubleClickZoomHandler implements the zoom using zoomTo, which will set _zooming, so it isn't strictly necessary. (In fact I think DoubleClickZoomHandler should probably not have an isActive() method at all. Unlike drag panning/rotating, where there's an interval bookended by down/up events, double-clicking is an instantaneous action.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DoubleClickZoomHandler #isActive() turns out to be used internally, so I'll abandon that idea for now.

}

/**
* Returns true if the map is rotating due to a camera animation or user gesture.
*/
isRotating(): boolean {
return this._rotating ||
this.dragRotate.isActive();
}

/**
* Adds a listener for events of a specified type.
*
Expand Down Expand Up @@ -1526,8 +1552,8 @@ class Map extends Camera {
this.painter.render(this.style, {
showTileBoundaries: this.showTileBoundaries,
showOverdrawInspector: this._showOverdrawInspector,
rotating: this.rotating,
zooming: this.zooming,
rotating: this.isRotating(),
zooming: this.isZooming(),
fadeDuration: this._fadeDuration
});

Expand Down
16 changes: 16 additions & 0 deletions test/node_modules/mapbox-gl-js-test/simulate_interaction.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

26 changes: 13 additions & 13 deletions test/unit/ui/camera.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -701,9 +701,9 @@ test('camera', (t) => {
.on('movestart', (d) => { movestarted = d.data; })
.on('move', (d) => { moved = d.data; })
.on('moveend', (d) => {
t.notOk(camera.zooming);
t.notOk(camera.panning);
t.notOk(camera.rotating);
t.notOk(camera._zooming);
t.notOk(camera._panning);
t.notOk(camera._rotating);

t.equal(movestarted, 'ok');
t.equal(moved, 'ok');
Expand Down Expand Up @@ -997,9 +997,9 @@ test('camera', (t) => {
.on('rotate', (d) => { rotated = d.data; })
.on('pitch', (d) => { pitched = d.data; })
.on('moveend', function(d) {
t.notOk(this.zooming);
t.notOk(this.panning);
t.notOk(this.rotating);
t.notOk(this._zooming);
t.notOk(this._panning);
t.notOk(this._rotating);

t.equal(movestarted, 'ok');
t.equal(moved, 'ok');
Expand Down Expand Up @@ -1064,9 +1064,9 @@ test('camera', (t) => {
.on('pitch', (d) => { pitched = d.data; })
.on('pitchend', (d) => { pitchended = d.data; })
.on('moveend', function(d) {
t.notOk(this.zooming);
t.notOk(this.panning);
t.notOk(this.rotating);
t.notOk(this._zooming);
t.notOk(this._panning);
t.notOk(this._rotating);

t.equal(movestarted, 'ok');
t.equal(moved, 'ok');
Expand Down Expand Up @@ -1579,19 +1579,19 @@ test('camera', (t) => {
});

t.test('#stop', (t) => {
t.test('resets camera.zooming', (t) => {
t.test('resets camera._zooming', (t) => {
const camera = createCamera();
camera.zoomTo(3.2);
camera.stop();
t.ok(!camera.zooming);
t.ok(!camera._zooming);
t.end();
});

t.test('resets camera.rotating', (t) => {
t.test('resets camera._rotating', (t) => {
const camera = createCamera();
camera.rotateTo(90);
camera.stop();
t.ok(!camera.rotating);
t.ok(!camera._rotating);
t.end();
});

Expand Down
8 changes: 2 additions & 6 deletions test/unit/ui/handler/scroll_zoom.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,6 @@ function createMap(options) {
}, options));
}

// magic deltaY value that indicates the event is from a mouse wheel
// (rather than a trackpad)
const magicWheelZoomDelta = 4.000244140625;

test('ScrollZoomHandler zooms in response to wheel events', (t) => {
const browserNow = t.stub(browser, 'now');
let now = 1555555555555;
Expand All @@ -35,7 +31,7 @@ test('ScrollZoomHandler zooms in response to wheel events', (t) => {
// simulate a single 'wheel' event
const startZoom = map.getZoom();

simulate.wheel(map.getCanvas(), {type: 'wheel', deltaY: -magicWheelZoomDelta});
simulate.wheel(map.getCanvas(), {type: 'wheel', deltaY: -simulate.magicWheelZoomDelta});
map._updateCamera();

now += 400;
Expand Down Expand Up @@ -68,7 +64,7 @@ test('ScrollZoomHandler zooms in response to wheel events', (t) => {
const startZoom = map.getZoom();

const events = [
[2, {type: 'wheel', deltaY: -magicWheelZoomDelta}],
[2, {type: 'wheel', deltaY: -simulate.magicWheelZoomDelta}],
[7, {type: 'wheel', deltaY: -41}],
[30, {type: 'wheel', deltaY: -169}],
[1, {type: 'wheel', deltaY: -801}],
Expand Down
Loading