#2498: Don't show cooperativeGestures messages when scrollZoom is dis… #3240

arekgotfryd · 2023-10-20T21:23:04Z

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.
CooperativeGesture message will not get show when scrollZoom is disbaled
Link to related issues.
Don't show cooperativeGestures messages when scrollZoom is disabled #2498

codecov-commenter · 2023-10-20T21:28:54Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (efdb76c) 75.23% compared to head (3da57ef) 75.27%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3240      +/-   ##
==========================================
+ Coverage   75.23%   75.27%   +0.03%     
==========================================
  Files         241      242       +1     
  Lines       19256    19277      +21     
  Branches     4339     4339              
==========================================
+ Hits        14488    14511      +23     
+ Misses       4768     4766       -2

Files	Coverage Δ
src/ui/control/fullscreen_control.ts	`75.51% <ø> (ø)`
src/ui/handler/scroll_zoom.ts	`94.15% <100.00%> (+1.48%)`	⬆️
src/ui/handler/touch_pan.ts	`100.00% <ø> (ø)`
src/ui/map.ts	`83.02% <100.00%> (-0.21%)`	⬇️
src/ui/control/cooperative_gesture_contol.ts	`95.65% <95.65%> (ø)`

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

src/ui/handler/scroll_zoom.ts

arekgotfryd · 2023-10-24T19:35:46Z

Ready for review

src/ui/handler/scroll_zoom.ts

src/ui/map.test.ts

src/ui/control/cooperative_gesture_contol.ts

…m is disabled

…omHandler

…serAgent

arekgotfryd · 2023-10-27T18:52:46Z

Ready for review.

arekgotfryd · 2023-10-29T07:32:07Z

Ready for review.

HarelM · 2023-10-29T15:00:51Z

src/ui/control/control.ts

@@ -4,7 +4,7 @@ import type {Map} from '../map';
 * A position defintion for the control to be placed, can be in one of the corners of the map.
 * When two or more controls are places in the same location they are stacked toward the center of the map.
 */
-export type ControlPosition = 'top-left' | 'top-right' | 'bottom-left' | 'bottom-right';
+export type ControlPosition = 'top-left' | 'top-right' | 'bottom-left' | 'bottom-right' | 'full';


I'm not sure how I feel about this, since there isn't another control that is full and I'm not sure what would be the expected behavior.
Can this control simply ignore this input maybe?
IDK...

The reason for 'full' position to be there is following method in map.ts
https://github.com/arekgotfryd/maplibre-gl-js/blob/3da57effa8c864fd7cad9682f5a84816eed42f33/src/ui/map.ts#L717
if I remove getDefaultPosition implementation from CooperativeGestureControl class addControl method will assume top-right position for it.

While not "accurate" the default seems to be OK I think.
Otherwise we'll need to support configuring the scale control, or zoom control to be full screen, which is not what we want I believe.

HarelM · 2023-10-29T15:06:44Z

src/ui/handler/scroll_zoom.ts

@@ -147,14 +150,15 @@ export class ScrollZoomHandler implements Handler {
     * ```
     */
    disable() {
+        this._map.setCooperativeGestures(false);


Wouldn't this disable cooperative gesture even if it was enabled to begin with when disabling scroll zoom?

I don't follow...
What is the point of cooperative gestures message when zooming is disabled?

You can enable and disable scroll zoom after the map is initialized, this will "forget" the initial state I think.

Ok I get it now but in that case I would need two additional methods on Map class:
enableCooperativeGestures
disableCooperativeGestures
Are you fine with that?

Having these separate methods would avoid cluttering of setCooperativeGestures method.

HarelM · 2023-10-29T15:07:52Z

src/ui/handler/touch_pan.ts

    }

    touchstart(e: TouchEvent, points: Array<Point>, mapTouches: Array<Touch>) {
        return this._calculateTransform(e, points, mapTouches);
    }

    touchmove(e: TouchEvent, points: Array<Point>, mapTouches: Array<Touch>) {
-        if (this._map._cooperativeGestures) {


I haven't seen this code moved to the controller, am I missing something?

I can't see that line in touch_pan.ts file.
https://github.com/arekgotfryd/maplibre-gl-js/blob/3da57effa8c864fd7cad9682f5a84816eed42f33/src/ui/handler/touch_pan.ts#L40

Because you removed it...

maplibre-gl-js/src/ui/handler/touch_pan.ts

Line 45 in 838bb63

touchmove(e: TouchEvent, points: Array<Point>, mapTouches: Array<Touch>) {

HarelM · 2023-10-29T15:10:20Z

src/ui/map.ts

@@ -1176,9 +1157,14 @@ export class Map extends Camera {
    setCooperativeGestures(gestureOptions?: GestureOptions | boolean | null): Map {
        this._cooperativeGestures = gestureOptions;
        if (this._cooperativeGestures) {
-            this._setupCooperativeGestures();
+            if (this._cooperativeGesturesControl) {


Is there a more elegant way instead of remove and then add - simply keep it as is, or call some enable method...?

I couldn't come up with better way to handle this without changing public interface of Map class but I'm open to any suggestions.

HarelM · 2023-11-16T14:25:23Z

@arekgotfryd Are you no longer interested in this feature?

arekgotfryd · 2023-11-16T16:48:05Z

@HarelM I just don't have time at the moment to look into this. I decided to close it in order to give someone else possibility to pick it up.

HarelM reviewed Oct 20, 2023

View reviewed changes

src/ui/handler/scroll_zoom.ts Outdated Show resolved Hide resolved

HarelM added the waiting for code review fixes label Oct 21, 2023

HarelM reviewed Oct 22, 2023

View reviewed changes

src/ui/handler/scroll_zoom.ts Outdated Show resolved Hide resolved

HarelM reviewed Oct 24, 2023

View reviewed changes

src/ui/handler/scroll_zoom.ts Outdated Show resolved Hide resolved

HarelM reviewed Oct 24, 2023

View reviewed changes

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

HarelM reviewed Oct 24, 2023

View reviewed changes

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

github-advanced-security bot found potential problems Oct 25, 2023

View reviewed changes

src/ui/control/cooperative_gesture_contol.ts Fixed Show resolved Hide resolved

src/ui/control/cooperative_gesture_contol.ts Fixed Show fixed Hide fixed

arekgotfryd force-pushed the 2498-fix branch from 5e7aaf6 to f02798b Compare October 25, 2023 14:33

arekgotfryd and others added 11 commits October 26, 2023 21:29

maplibre#2498: Don't show cooperativeGestures messages when scrollZoo…

a9156dc

…m is disabled

maplibre#2498: code review remarks addressed

68ba8ed

maplibre#2498: moved logic related to cooperativeGestures to ScrollZo…

2d2d87b

…omHandler

maplibre#2498: change some function calls + add done calls in unit tests

784cbf6

Update _metaKey in scroll_zoom.ts

e3677eb

maplibre#2498: decrease size

ddeebe1

maplibre#2498: decrease dist size

af5c20a

maplibre#2498: remove unused import type

239e94c

maplibre#2498: replace deprecated navigator.platform with navigator.u…

8feb7f0

…serAgent

maplibre#2498: remove abundant done() calls

eaf50c5

maplibre#2498: new CooperativeGestureControl

51e8305

arekgotfryd force-pushed the 2498-fix branch from f02798b to 51e8305 Compare October 26, 2023 19:29

arekgotfryd added 2 commits October 27, 2023 16:42

maplibre#2498: completely separated CooperativeGestureControl

6e734ce

maplibre#2498: fix failing unit test

841a392

HarelM self-assigned this Oct 28, 2023

maplibre#2498: Unsafe HTML constructed from library input - fix

3da57ef

HarelM reviewed Oct 29, 2023

View reviewed changes

arekgotfryd closed this Nov 16, 2023

HarelM mentioned this pull request Nov 30, 2023

Cooperative gestures redesign #3430

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#2498: Don't show cooperativeGestures messages when scrollZoom is dis… #3240

#2498: Don't show cooperativeGestures messages when scrollZoom is dis… #3240

arekgotfryd commented Oct 20, 2023

codecov-commenter commented Oct 20, 2023 •

edited

Loading

arekgotfryd commented Oct 24, 2023

arekgotfryd commented Oct 27, 2023

arekgotfryd commented Oct 29, 2023

HarelM Oct 29, 2023

arekgotfryd Oct 29, 2023

HarelM Oct 31, 2023

HarelM Oct 29, 2023

arekgotfryd Oct 29, 2023

HarelM Oct 29, 2023

arekgotfryd Oct 29, 2023

arekgotfryd Oct 29, 2023

HarelM Oct 29, 2023

arekgotfryd Oct 29, 2023

HarelM Oct 29, 2023

HarelM Oct 29, 2023

arekgotfryd Oct 29, 2023

HarelM commented Nov 16, 2023

arekgotfryd commented Nov 16, 2023

#2498: Don't show cooperativeGestures messages when scrollZoom is dis… #3240

#2498: Don't show cooperativeGestures messages when scrollZoom is dis… #3240

Conversation

arekgotfryd commented Oct 20, 2023

Launch Checklist

codecov-commenter commented Oct 20, 2023 • edited Loading

Codecov Report

arekgotfryd commented Oct 24, 2023

arekgotfryd commented Oct 27, 2023

arekgotfryd commented Oct 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HarelM commented Nov 16, 2023

arekgotfryd commented Nov 16, 2023

codecov-commenter commented Oct 20, 2023 •

edited

Loading