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

return last snapTo feature #14676

Merged
merged 5 commits into from Jul 16, 2023
Merged

return last snapTo feature #14676

merged 5 commits into from Jul 16, 2023

Conversation

fmg-lydonchandra
Copy link
Contributor

@fmg-lydonchandra fmg-lydonchandra commented Apr 16, 2023 •
edited by ahocevar

Fixes #14675

@fmg-lydonchandra
Copy link
Contributor Author

fmg-lydonchandra commented Apr 16, 2023 •
edited

working on returning snapTo feature externally

Any thoughts on best way to return the features externally ?

One way is to save snapToResult internally in a variable, and add function getSnapToResult()
(not elegant at all)

return {
      vertex: closestVertex,
      vertexPixel: [
        Math.round(vertexPixel[0]),
        Math.round(vertexPixel[1]),
      ],
      feature: closestFeature,
    };

and have calling code checking

@fmg-lydonchandra
Copy link
Contributor Author

fmg-lydonchandra commented May 24, 2023 •
edited

Implemented suggestion: Dispatch new SnapEvent when snapped to a feature

Hi @ahocevar & @mike-000 , a new SnapEvent is dispatched now, review please ?
i'll add some tests and types information when you are happy with the approach.

Thanks!

@fmg-lydonchandra fmg-lydonchandra mentioned this pull request May 29, 2023
Closed
@github-actions
Copy link

📦 Preview the website for this branch here: https://deploy-preview-14676--ol-site.netlify.app/.

ahocevar
ahocevar requested changes May 31, 2023
View reviewed changes
Copy link
Member

@ahocevar ahocevar left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this, @fmg-lydonchandra. I have added several comments. In addition, it would be great if you could add a test.

src/ol/interaction/Snap.js Outdated
Comment on lines 435 to 440
getLastSnapToResult() {
if (this.lastSnapToResult_) {
return this.lastSnapToResult_;
}
}

Copy link
Member

Choose a reason for hiding this comment

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

This is now unused.

Suggested change
getLastSnapToResult() {
if (this.lastSnapToResult_) {
return this.lastSnapToResult_;
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed unused, as per feedback.

src/ol/interaction/Snap.js Outdated
}
});
}
}
const result = getResult();
if (result) {
this.lastSnapToResult_ = result;
Copy link
Member

Choose a reason for hiding this comment

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

No longer used.

Suggested change
this.lastSnapToResult_ = result;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed unused, as per feedback.

src/ol/interaction/Snap.js Show resolved Hide resolved
src/ol/interaction/Snap.js Show resolved Hide resolved
src/ol/interaction/Snap.js Show resolved Hide resolved
src/ol/events/SnapEvent.js Outdated
Comment on lines 7 to 49
this.vertex = options.vertex;
this.vertexPixel = options.vertexPixel;
this.feature = options.feature;
Copy link
Member

Choose a reason for hiding this comment

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

Types are missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to follow existing pattern with enum SnapEventType , otherwise api-doc won't include SnapEvent properly.

image

image

src/ol/events/SnapEvent.js Outdated Show resolved Hide resolved
src/ol/events/SnapEvent.js Outdated
@@ -0,0 +1,11 @@
import BaseEvent from './Event.js';

const SNAP = 'snap';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const SNAP = 'snap';

src/ol/interaction/Snap.js Outdated
Comment on lines 118 to 143
this.on;

this.once;

this.un;
Copy link
Member

Choose a reason for hiding this comment

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

Types are missing. I'd suggest adding a SnapOnSignature, similar to the Modify interaction's ModifyOnSignature, and using it here as @type {SnapOnSignature<import("../events").EventsKey>}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added SnapOnSignature

src/ol/interaction/Snap.js Outdated
Comment on lines 207 to 210

// As you suggested in Stack Overflow you can use forEachFeatureAtPixel.
// That could be done in a condition function for any interaction, as well as drawstart and drawend, and done using only documented API methods as in https://codesandbox.io/s/draw-and-modify-features-forked-enrgqv?file=/main.js
// If the Snap interaction was constructed with features: snapFeatures instead of source: snapSource just replace snapSource.getFeatures().includes with snapFeatures.getArray().includes.
Copy link
Member

Choose a reason for hiding this comment

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

These comments can be removed.

Suggested change
// As you suggested in Stack Overflow you can use forEachFeatureAtPixel.
// That could be done in a condition function for any interaction, as well as drawstart and drawend, and done using only documented API methods as in https://codesandbox.io/s/draw-and-modify-features-forked-enrgqv?file=/main.js
// If the Snap interaction was constructed with features: snapFeatures instead of source: snapSource just replace snapSource.getFeatures().includes with snapFeatures.getArray().includes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed unused comments

@fmg-lydonchandra fmg-lydonchandra force-pushed the return-last-snap-feature branch 3 times, most recently from 1ffbb2e to 2f3a1ad Compare July 1, 2023 19:15
@fmg-lydonchandra
Copy link
Contributor Author

fmg-lydonchandra commented Jul 1, 2023 •
edited

Re-review please @ahocevar :-)
Added unit tests and types
thanks

ahocevar
ahocevar requested changes Jul 8, 2023
View reviewed changes
src/ol/interaction/Snap.js Outdated
Comment on lines 303 to 304
vertex: result.vertex.slice(0, 2),
vertexPixel: result.vertexPixel,
Copy link
Member

Choose a reason for hiding this comment

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

If you'd assign evt.coordinate and evt.pixel here instead, users would be able to modify the snap coordinate, which might be useful.

Copy link
Member

Choose a reason for hiding this comment

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

If you decide not to change that, then also create a copy of the pixel so users can neither modify coordinate nor pixel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to assign evt.coordinate and evt.pixel as suggested.
Curious what is the example use case for the users wanting to modify snap coordinate?

test/browser/spec/ol/interaction/snap.test.js Outdated
expect(snapEvent.vertexPixel).to.eql([width / 2, height / 2]);
expect(snapEvent.feature).to.eql(point);
done();
});
snapInteraction.handleEvent(event);
// check that the coordinate is in XY and not XYZ
expect(event.coordinate).to.eql([0, 0]);
Copy link
Member

Choose a reason for hiding this comment

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

Since events are dispatched synchronously in OpenLayers, this test won't be executed any more. I'd suggest creating a separate test for the snap event instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved event.coordinate expect inside eventHandler expect.
Keeping it in the same unit test for ease of maintenance, as we are doing snapEvent.feature expect,
(same for all unit tests below)
and reuse event input data.

Is it still better to have separate unit tests for snapEvent @ahocevar ?
If so I'll refactor.

test/browser/spec/ol/interaction/snap.test.js Outdated
Comment on lines 101 to 107
snapInteraction.on('snap', function (snapEvent) {
expect(snapEvent.type).to.be('snap');
expect(snapEvent.vertex).to.eql([0, 0]);
expect(snapEvent.vertexPixel).to.eql([width / 2, height / 2]);
expect(snapEvent.feature).to.be(point);
done();
});
Copy link
Member

Choose a reason for hiding this comment

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

When you change the snap event to return the event pixel and coordinate, this test won't be necessary any more. If you decide not to make that change, the same problem as above applies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see comment above #14676 (comment)

test/browser/spec/ol/interaction/snap.test.js Outdated
Comment on lines 125 to 130
snapInteraction.on('snap', function (snapEvent) {
expect(snapEvent.type).to.be('snap');
expect(snapEvent.vertex).to.eql([7, 0]);
expect(snapEvent.feature).to.be(undefined);
done();
});
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see comment above #14676 (comment)

test/browser/spec/ol/interaction/snap.test.js Outdated
Comment on lines 165 to 169
snapInteraction.on('snap', function (snapEvent) {
expect(snapEvent.vertex[0]).to.roughlyEqual(coordinate[0], 1e-10);
expect(snapEvent.vertex[1]).to.roughlyEqual(coordinate[1], 1e-10);
done();
});
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see comment above #14676 (comment)

test/browser/spec/ol/interaction/snap.test.js Outdated
Comment on lines 293 to 298
snapInteraction.on('snap', function (snapEvent) {
expect(snapEvent.vertex[0]).to.roughlyEqual(coordinate[0], 1e-10);
expect(snapEvent.vertex[1]).to.roughlyEqual(coordinate[1], 1e-10);
expect(snapEvent.feature).to.be(undefined);
done();
});
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see comment above #14676 (comment)

test/browser/spec/ol/interaction/snap.test.js Outdated
Comment on lines 326 to 330
snapInteraction.on('snap', function (snapEvent) {
expect(snapEvent.vertex).to.eql([10, 0]);
expect(snapEvent.feature).to.be(feature);
done();
});
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see comment above #14676 (comment)

test/browser/spec/ol/interaction/snap.test.js Outdated
Comment on lines 359 to 363
snapInteraction.on('snap', function (snapEvent) {
expect(snapEvent.vertex).to.eql([10, 0]);
expect(snapEvent.feature).to.be(line);
done();
});
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see comment above #14676 (comment)

test/browser/spec/ol/interaction/snap.test.js Outdated
Comment on lines 394 to 398
snapInteraction.on('snap', function (snapEvent) {
expect(snapEvent.vertex).to.eql([10, 0]);
expect(snapEvent.feature).to.be(line);
done();
});
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see comment above #14676 (comment)

test/browser/spec/ol/interaction/snap.test.js Outdated
Comment on lines 472 to 477
snapInteraction.on('snap', function (snapEvent) {
expect(snapEvent.vertex).to.eql([lon, lat]);
expect(snapEvent.vertexPixel).to.eql(expectedPixel);
expect(snapEvent.feature).to.be(point);
done();
});
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see comment above #14676 (comment)

fmg-lydonchandra
fmg-lydonchandra commented Jul 16, 2023
View reviewed changes
test/browser/spec/ol/interaction/snap.test.js
expect(snapEvent.feature).to.eql(point);

// check that the coordinate is in XY and not XYZ
expect(event.coordinate).to.eql([0, 0]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Moved event.coordinate check inside eventHandler check
  • keeping it in the same unit test for ease of maintenance
  • reuse event input data

ahocevar
ahocevar approved these changes Jul 16, 2023
View reviewed changes
Copy link
Member

@ahocevar ahocevar left a comment

Choose a reason for hiding this comment

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

Thanks, @fmg-lydonchandra! I think this is a great addition to the API.

@ahocevar ahocevar merged commit a798d81 into openlayers:main Jul 16, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahocevar ahocevar ahocevar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Return Last SnapTo Feature
2 participants
@fmg-lydonchandra @ahocevar