Add a new method Leaflet.set_view

[amks1]

Added the set_view method to set the center and zoom in one operation, because calling set_zoom and set_center one after the other was running only the latter command.

Resolves #4491

[evnchn]

Wait. I think the issue is deeper than what you think.

Checking both set_zoom and set_center, what happens is:

• set_zoom lets the library user set zoom, and use self.center for center, but the zoom is not commited to self.zoom right away, but rather only at _handle_zoomend.
• set_center lets the library user set center, and use self.zoom for zoom, but the center is not commited to self.center right away, but rather only at _handle_moveend

This means that set_zoom and set_center are fundamentally illsuited to be called together, which we may consider changing instead of adding another function which is basically set_zoom and set_center bundled together in a trenchcoat to adhere to DRY principle. Or, for that, we can implement set_zoom and set_center via calling set_view.

Also, your set_view commits to self.zoom and self.center right away. There may be some special reasons set_zoom and set_center doesn't do that, and ther may be some consequences of breaking the pattern. Maybe ask around for that first.

[falkoschindler]

Thank you for contributing this pull request, @amks1!

The center and zoom methods of ui.leaflet have indeed quite some history:

• Leaflet: map flickers between centers after flyTo #3035
• Prevent ui.leaflet from flickering after calling flyTo() #3125
• Leaflet does not initialise properly and flickers #4061
• Prevent ui.leaflet from oscillating between two locations #4121
• ui.leaflet location is reset when ui.plotly is updated #4182
• Prevent leaflet from resetting center and zoom when browser resizes #4265

At least three times we tried to fix some weird behavior related to changing the map's center or zoom. So before simply adding another method set_view, I'd like to understand what is actually going on. Even if we would add a new method, the two existing ones would remain broken if used in combination.

I suggest to keep this PR here for reference and continue discussing the problem over at #4491.

[amks1]

Checking both set_zoom and set_center, what happens is:

• set_zoom lets the library user set zoom, and use self.center for center, but the zoom is not commited to self.zoom right away, but rather only at _handle_zoomend.
• set_center lets the library user set center, and use self.zoom for zoom, but the center is not commited to self.center right away, but rather only at _handle_moveend

This means that set_zoom and set_center are fundamentally illsuited to be called together, which we may consider changing instead of adding another function which is basically set_zoom and set_center bundled together in a trenchcoat to adhere to DRY principle. Or, for that, we can implement set_zoom and set_center via calling set_view.

I did spend some time trying to figure out whether I could achieve this without adding another function, but didn't arrive at anything. I suspect it's something "deeper" in the code in the way javascript code is sequenced to execute on the client. The second command always overrides the first.
Personally I feel letting users choose to set one or both of these parameters using the same function is more intuitive, but of course I didn't want to break anybody's code by removing existing methods.

Also, your set_view commits to self.zoom and self.center right away. There may be some special reasons set_zoom and set_center doesn't do that, and ther may be some consequences of breaking the pattern. Maybe ask around for that first.

My code doesn't commit self.zoom or self.center, only self._props['zoom'] and self._props['center']. Like you described above, self.zoom and self.center still get updated in the event handlers _handle_zoomend and _handle_moveend. I've confirmed that the self.zoom and self.center get set correctly after set_view (but I admit I don't know why this is being done in this roundabout way).

But what this means is these event handlers are handling simultaneous map-moveend and map-zoomend events correctly, and this is not a reason why set_zoom and set_center cannot be called together.

Code:

@ui.page('/test')
def test_page():

    LONDON = (51.555981, -0.046957)
    ATHENS = (38.139204, 23.612089)

    def set_marker_zoom_in():
        marker = leaflet.marker(latlng=ATHENS)
        leaflet.set_center(ATHENS)
        leaflet.set_zoom(13)

    def set_marker():
        marker = leaflet.marker(latlng=ATHENS)
        leaflet.set_center(ATHENS)

    def set_center():
        leaflet.set_center(ATHENS)

    def set_center_zoom_in():
        # leaflet.set_center(ATHENS)
        # leaflet.set_zoom(13)
        leaflet.set_view(ATHENS, 13)

    leaflet = ui.leaflet(LONDON, zoom=6).style('height: 400px;')
    ui.button('Set Marker and Zoom In', on_click=set_marker_zoom_in)
    ui.button('Set Marker', on_click=set_marker)
    ui.button('Only Set Center', on_click=set_center)
    ui.button('Set center and zoom in', on_click=set_center_zoom_in)
    ui.button('Show center and zoom', on_click=lambda: ui.notify(f'Center: {leaflet.center}, Zoom: {leaflet.zoom}'))


ui.run(reload=False, show=False)

[evnchn]

@amks1 Sorry to keep you waiting. It's quite mind-bending, but after reviewing the code some more, I think I got it.

self.center and self.zoom: Reported from Leaflet, after the zoom and center operation is completed. Check the below:

nicegui/nicegui/elements/leaflet.py

Lines 96 to 104 in 847c4ab

	def _handle_moveend(self, e: GenericEventArguments) -> None:
	self._send_update_on_value_change = False
	self.center = e.args['center']
	self._send_update_on_value_change = True
	def _handle_zoomend(self, e: GenericEventArguments) -> None:
	self._send_update_on_value_change = False
	self.zoom = e.args['zoom']
	self._send_update_on_value_change = True

self._props['center'] and self._props['zoom']: Updated immediately on set_center and set_zoom methods. Check the below:

nicegui/nicegui/elements/leaflet.py

Lines 111 to 125 in 847c4ab

	def set_center(self, center: Tuple[float, float]) -> None:
	"""Set the center location of the map."""
	if self._props['center'] == center:
	return
	self._props['center'] = center
	if self._send_update_on_value_change:
	self.run_map_method('setView', center, self.zoom)
	def set_zoom(self, zoom: int) -> None:
	"""Set the zoom level of the map."""
	if self._props['zoom'] == zoom:
	return
	self._props['zoom'] = zoom
	if self._send_update_on_value_change:
	self.run_map_method('setView', self.center, zoom)

Now, I think you will agree, that I can change the code slightly for easier readability:

def set_center(self, center: Tuple[float, float]) -> None:
    """Set the center location of the map."""
    if self._props['center'] == center:
        return
    self._props['center'] = center
    if self._send_update_on_value_change:
        self.run_map_method('setView', self._props['center'], self.zoom) # center = self._props['center']

def set_zoom(self, zoom: int) -> None:
    """Set the zoom level of the map."""
    if self._props['zoom'] == zoom:
        return
    self._props['zoom'] = zoom
    if self._send_update_on_value_change:
        self.run_map_method('setView', self.center, self._props['zoom']) # zoom = self._props['zoom']

This makes it obvious: When you do two consecutive set_zoom and set_center, since the reported value from Leaflet has not returned, the second call won't actually use the first call's zoom/center, instead using whatever's in self.center / self.zoom, which is out-of-date.

So, indeed, there is no way with existing functions that a zoom-and-move maneuver can be done in one fell swoop.

Do @ me and correct me, if you think I am wrong, though.

---

Given the above, and how I had to modify the code to make it readable, I take issue with how you do center = self.center. It makes it rather confusing, since center is both the input and the output. I need to think deeply about it, and see if I can make sense of it. Meanwhile, you can try and refactor your new function such that the code flow is more readable and explainable.

[evnchn]

Let's see if this logic is more nice:

def set_view(self, *, center: Optional[Tuple[float, float]] = None, zoom: Optional[int] = None) -> None:
    """Set the view of the map.

    :param center: new center location (latitude/longitude)
    :param zoom: new zoom level
    """
    value_changed = False
    if center is not None and self._props['center'] != center:
        self._props['center'] = center
        value_changed = True
    if zoom is not None and self._props['zoom'] != zoom:
        self._props['zoom'] = zoom
        value_changed = True
    if value_changed and self._send_update_on_value_change:
        self.run_map_method('setView', center or self.center, zoom or self.zoom)

Key points:

1. Your old implementation would still trigger self.run_map_method if I pass in center = self._props['center'], zoom = self._props['zoom']. Mine, however, value_changed would remain False, skipping self.run_map_method, inline with the spirit of the two previous functions.
2. Your old implementation sets the function input variables, which isn't quite readable. Mine, however, does self.run_map_method('setView', center or self.center, zoom or self.zoom), which captures the spirit of "use center if passed in, else self.center if it was not.
3. Namely due to point 1, if my logic thinking is right, set_zoom and set_center can call my newly defined set_view method, and the behaviour would be idempotent to the old behaviour, keeping backwards compatibility.

@falkoschindler code review time. See if this is the better solution.

[evnchn]

Checked for upstream changes in response to potential Leaflet 2.0 release.

TL-DR: They were trying to use more modern syntax. This PR should remain safe to merge and its code should be working, whether Leaflet 2.0 came out or not.

---

Use nullish coalescing and optional chaining (#9661)

options = options || {}; => options ??= {};: Should be fine

this._resetView(center, zoom, options.pan && options.pan.noMoveStart); => this._resetView(center, zoom, options.pan?.noMoveStart);: Should be fine

Use nullish coalescing and optional chaining (#9661)

Also should be fine.

[evnchn]

I think I have a trick in mind which would allow calling set_zoom and set_center one after the other to simply work

This negates the need for this set_view, which also eliminates potential user error where they don't know that this new function exists and don't use them.

Let me cook.

[falkoschindler]

@evnchn That would be great! Apart from being busy with lots of other things, the suboptimal solution with potential user errors is one reason why I haven't reviewed and merged this PR yet. I thought there must be a better way, but couldn't find one.

[evnchn]

#5453

[falkoschindler]

I'm closing this in favor of PR #5453, which solves the original problem without adding a new API.
Thank you anyway for your effort, @amks1!