Map#on, once and off - support for multiple layers #4570

pstaszek · 2024-08-16T20:48:22Z

Implements: #4279

This PR adds support for adding listener for multiple layers using Map#on and Map#once. It also adds support for removing listeners with Map#off function.

To remove listener in off function the user has to provide array of layerIds with the same content as when creating the listener. I chose this approach because it seems the simplest, there is no need to consider complex cases (i.e. remove only part of layers from listener).

I intentionally did not check the Mapbox GL JS implementation before I wrote my code. I did it after and to some degree it is similar, because it is the most obvious implementation. Is it ok, or should we try to find some other way to implement this feature?

Launch Checklist

Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
Briefly describe the changes in this PR.
Link to related issues.
Include before/after visuals or gifs if this PR includes visual changes.
Write tests for all new functionality.
Document any changes to public APIs.
Post benchmark scores.
Add an entry to CHANGELOG.md under the ## main section.

…o issue-4279-map-on-multiple-layers

…d listeners for each layer

…o issue-4279-map-on-multiple-layers

codecov-commenter · 2024-08-17T03:57:48Z

Codecov Report

Attention: Patch coverage is 93.65079% with 4 lines in your changes missing coverage. Please review.

Project coverage is 87.72%. Comparing base (8a005bd) to head (a0b009b).
Report is 260 commits behind head on main.

Files with missing lines	Patch %	Lines
src/ui/map.ts	93.65%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4570      +/-   ##
==========================================
- Coverage   87.84%   87.72%   -0.12%     
==========================================
  Files         247      247              
  Lines       33574    33601      +27     
  Branches     2366     2391      +25     
==========================================
- Hits        29492    29476      -16     
- Misses       3116     3131      +15     
- Partials      966      994      +28

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

HarelM · 2024-08-17T04:57:15Z

Can you check if the coverage is correct?
Otherwise, looks good.

pstaszek · 2024-08-21T05:28:43Z

@HarelM I have bumped up the coverage with few additional tests.

src/ui/map.ts

…m/pstaszek/maplibre-gl-js into issue-4279-map-on-multiple-layers

pstaszek · 2024-08-22T20:41:08Z

I have some issue with registering once event, but I'm not sure if it is related to this PR. I will investigate it further tomorrow, or during the weekend.

pstaszek · 2024-08-23T21:15:56Z

Ok - this is something unrelated to this PR.
I notice a bug, that it is not possible to deregister a listener added with once method with a layer - this is because removeDelegatedListener function looks for callback reference in this._delegatedListener, but once method is not saving it there.

…o issue-4279-map-on-multiple-layers

HarelM · 2024-08-24T13:08:45Z

src/ui/map.ts

+
+    _removeDelegatedListener(type: string, layerIds: string[], listener: Listener) {
+        if (!this._delegatedListeners || !this._delegatedListeners[type]) {
+            return this;


Not sure returning "this" is needed.
I would simply return void.

Right, done.

HarelM · 2024-08-24T18:18:41Z

Can you fix the build test please? Just update the size.

pstaszek · 2024-08-24T18:27:07Z

Ofc, should I update this value: https://github.com/pstaszek/maplibre-gl-js/blob/1c30ee6ce8a9e37558ba0ea2c09b06fe428e7c89/test/build/min.test.ts#L39 ?

HarelM · 2024-08-24T18:30:53Z

Yes please.

HarelM · 2024-08-24T18:41:40Z

Thanks!!!

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [maplibre-gl](https://maplibre.org/) ([source](https://github.com/maplibre/maplibre-gl-js)) | dependencies | minor | [`4.5.2` -> `4.7.0`](https://renovatebot.com/diffs/npm/maplibre-gl/4.5.2/4.7.0) | --- ### Release Notes <details> <summary>maplibre/maplibre-gl-js (maplibre-gl)</summary> ### [`v4.7.0`](https://github.com/maplibre/maplibre-gl-js/blob/HEAD/CHANGELOG.md#470) [Compare Source](maplibre/maplibre-gl-js@v4.6.0...v4.7.0) ##### ✨ Features and improvements - Support multiple layers in `map.on`, `map.once` and `map.off` methods ([#4570](maplibre/maplibre-gl-js#4570)) - Ensure GeoJSON cluster sources emit a console warning if `maxzoom` is less than or equal to `clusterMaxZoom` since in this case you may see unexpected results. ([#4604](maplibre/maplibre-gl-js#4604)) ##### 🐞 Bug fixes - Heatmap Fix for 3D terrain ([#4571](maplibre/maplibre-gl-js#4571)) - Fix Map#off to not remove listener with layer(s) registered with Map#once ([#4592](maplibre/maplibre-gl-js#4592)) - Improve types a bit for `addSource` and `getSource` ([#4616](maplibre/maplibre-gl-js#4616)) - Fix the color near the horizon when terrain is enabled without any sky ([#4607](maplibre/maplibre-gl-js#4607)) - Fix bug where `fitBounds` and `cameraForBounds` would not display across the 180th meridian (antimeridian) - Fix white flickering on map resize ([#4158](maplibre/maplibre-gl-js#4158)) - Fixed a performance regression related to symbol placement ([#4599](maplibre/maplibre-gl-js#4599)) - Fix a bug where cloning a Transform instance didn't include the `lngRange`. This caused a bug where using `transformCameraUpdate` caused the `maxBounds` to stop working just for east/west bounds. ([#4625](maplibre/maplibre-gl-js#4625)) ### [`v4.6.0`](https://github.com/maplibre/maplibre-gl-js/blob/HEAD/CHANGELOG.md#460) [Compare Source](maplibre/maplibre-gl-js@v4.5.2...v4.6.0) ##### ✨ Features and improvements - Prefer local glyph rendering for all CJKV characters, not just those in the CJK Unified Ideographs, Hiragana, Katakana, and Hangul Syllables blocks. ([#4560](maplibre/maplibre-gl-js#4560))) ##### 🐞 Bug fixes - Fix right-to-left layout of labels that contain characters in the Arabic Extended-B code block. ([#4536](maplibre/maplibre-gl-js#4536)) - Fix 3D map freezing when camera is adjusted against map bounds. ([#4537](maplibre/maplibre-gl-js#4537)) - Fix `getStyle()` to return a clone so the object cannot be internally changed ([#4488](maplibre/maplibre-gl-js#4488)) - Fix issues with setting sky to `undefined` ([#4587](maplibre/maplibre-gl-js#4587))) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Reviewed-on: https://git.tristess.app/alexandresoro/ouca/pulls/52 Reviewed-by: Alexandre Soro <[email protected]> Co-authored-by: renovate <[email protected]> Co-committed-by: renovate <[email protected]>

pstaszek added 13 commits July 12, 2024 21:37

add support for multiple layers in map.on

8a6bae8

add unit tests

64c2db0

update CHANGELOG.md

b377040

update documentation

6b62620

Merge branch 'main' of https://github.com/pstaszek/maplibre-gl-js int…

a38efb9

…o issue-4279-map-on-multiple-layers

delegated listeners with multiple layers instead of separate delegate…

461db4c

…d listeners for each layer

Merge branch 'main' of https://github.com/pstaszek/maplibre-gl-js int…

2b0e716

…o issue-4279-map-on-multiple-layers

fix: use only existing layers in queryRenderedFeatures

f1cf6c8

add unit test of map#off layers comparison

c3920f5

add missing semicolon

f606257

Merge branch 'main' of https://github.com/pstaszek/maplibre-gl-js int…

e6be1fe

…o issue-4279-map-on-multiple-layers

update descriptions

013c057

cleaning

5e411b1

pstaszek added 3 commits August 20, 2024 21:36

add mouseenter and mouseover tests

e47f022

add Map#off - mouseenter and mouseover test

3852c61

add unit test for 'mouseleave', 'mouseout' events

da8b254

Merge branch 'main' into issue-4279-map-on-multiple-layers

741563b

HarelM reviewed Aug 21, 2024

View reviewed changes

src/ui/map.ts Outdated Show resolved Hide resolved

HarelM reviewed Aug 21, 2024

View reviewed changes

src/ui/map.ts Outdated Show resolved Hide resolved

HarelM reviewed Aug 21, 2024

View reviewed changes

src/ui/map.ts Outdated Show resolved Hide resolved

pstaszek added 5 commits August 21, 2024 20:44

add DelegatedListener type

178684d

Merge branch 'issue-4279-map-on-multiple-layers' of https://github.co…

c96986d

…m/pstaszek/maplibre-gl-js into issue-4279-map-on-multiple-layers

move _removeDelegatedListener outside off method

4723723

remove redundant as any

ff08a64

fix off description

7acb91f

Merge branch 'main' of https://github.com/pstaszek/maplibre-gl-js int…

fda5c6e

…o issue-4279-map-on-multiple-layers

HarelM reviewed Aug 24, 2024

View reviewed changes

HarelM approved these changes Aug 24, 2024

View reviewed changes

return void instead of this in _removeDelegatedListener

1c30ee6

increase build size

a0b009b

HarelM merged commit 2b847a3 into maplibre:main Aug 24, 2024
15 checks passed

pstaszek deleted the issue-4279-map-on-multiple-layers branch August 24, 2024 18:44

birkskyum mentioned this pull request Sep 6, 2024

Allow adding multiple layers to map.on() event handler #4279

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Map#on, once and off - support for multiple layers #4570

Map#on, once and off - support for multiple layers #4570

pstaszek commented Aug 16, 2024 •

edited

Loading

codecov-commenter commented Aug 17, 2024 •

edited

Loading

HarelM commented Aug 17, 2024

pstaszek commented Aug 21, 2024

pstaszek commented Aug 22, 2024

pstaszek commented Aug 23, 2024 •

edited

Loading

HarelM Aug 24, 2024

pstaszek Aug 24, 2024

HarelM commented Aug 24, 2024

pstaszek commented Aug 24, 2024

HarelM commented Aug 24, 2024

HarelM commented Aug 24, 2024

Map#on, once and off - support for multiple layers #4570

Map#on, once and off - support for multiple layers #4570

Conversation

pstaszek commented Aug 16, 2024 • edited Loading

Launch Checklist

codecov-commenter commented Aug 17, 2024 • edited Loading

Codecov Report

HarelM commented Aug 17, 2024

pstaszek commented Aug 21, 2024

pstaszek commented Aug 22, 2024

pstaszek commented Aug 23, 2024 • edited Loading

HarelM Aug 24, 2024

Choose a reason for hiding this comment

pstaszek Aug 24, 2024

Choose a reason for hiding this comment

HarelM commented Aug 24, 2024

pstaszek commented Aug 24, 2024

HarelM commented Aug 24, 2024

HarelM commented Aug 24, 2024

pstaszek commented Aug 16, 2024 •

edited

Loading

codecov-commenter commented Aug 17, 2024 •

edited

Loading

pstaszek commented Aug 23, 2024 •

edited

Loading