Skip to content

#16367 release VectorSource subscriptions during cleanup #16370

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
Open
megawac wants to merge 1 commit into
base: main
Choose a base branch
from
Open

#16367 release VectorSource subscriptions during cleanup #16370

megawac wants to merge 1 commit into from

Conversation

@megawac
Copy link
Contributor

@megawac megawac commented Nov 15, 2024

I wasn't sure if this requires unit tests, there don't appear to be similar types of unit tests in the VectorSource test suite

Fixes #16367

@github-actions
Copy link

github-actions bot commented Nov 15, 2024

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

jahow
jahow reviewed Jan 9, 2025
View reviewed changes
Copy link
Contributor

@jahow jahow left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution! These were indeed missing.

In order to merge this we need additional unit tests, you can find similar ones here:

openlayers/test/browser/spec/ol/renderer/webgl/VectorLayer.test.js

Lines 574 to 605 in 06bb294

describe('#dispose', () => {
let deleteBufferSpy;
beforeEach(async () => {
// first call prepareFrame to load initial data and register listeners
renderer.prepareFrame(frameState);
await new Promise((resolve) => setTimeout(resolve, 150));
sinonSpy(vectorSource, 'removeEventListener');
deleteBufferSpy = sinonSpy(renderer.helper, 'deleteBuffer');
renderer.dispose();
});
it('unlistens to source events', () => {
expect(
vectorSource.removeEventListener.calledWith(VectorEventType.ADDFEATURE),
).to.be(true);
expect(
vectorSource.removeEventListener.calledWith(
VectorEventType.CHANGEFEATURE,
),
).to.be(true);
expect(
vectorSource.removeEventListener.calledWith(
VectorEventType.REMOVEFEATURE,
),
).to.be(true);
expect(
vectorSource.removeEventListener.calledWith(VectorEventType.CLEAR),
).to.be(true);
});
it('deletes webgl buffers', () => {
expect(deleteBufferSpy.callCount).to.be(12); // 2 buffers * 3 types of geometry * 2 different styles
});
});

Let me know if you can handle this; otherwise I might take a look. Thanks again!

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

Reviewers

@jahow jahow jahow left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

VectorSource does not cleanup subscriptions to bound Collection

2 participants

@megawac @jahow