Ensure toolbar buttons toggle widgets, and turn off when widgets are closed. #1763

sufyanAbbasi · 2023-10-06T07:19:21Z

Objective

To design a scalable system to make it easy to implement new toolbar items that clean themselves up when closed or un-toggled.

Implementation

Added a new decorator that wraps toggle button callbacks that will automatically take care of constructing and deconstructing the widget when the respective toggle is pressed. It also ensures that when the "close" button is clicked on the respective widgets, the toolbar toggle is turned off automatically.
It works by ensuring that the decorated callback returns the control widget that has a cleanup property that is a method that safely removes it from the map and clean up any state. Then, the decorator modifies the cleanup property to also call the toggle_off method which turns off the respective toggle in the toolbar. Then, when the "close" button is clicked, the callback uses the modified version of the cleanup so that cleanup and toggle off both occur.

Backwards Incompatibility Notes

Two widgets can't be added at the same time. I think this is done for safety as the map.tool_control property is shared amongst widgets.
The whitebox toolbox doesn't turn off its own toggle when closed since that tool is implemented in a different repository.

Tested

Added unit tests to test cleanup behaviors and tested in Jupyter-Lab and Colab: https://colab.research.google.com/drive/1HHQomXhy-vS68qiMu6AOhDmrsLq6OZwe#scrollTo=6-4Mn2sAFNrO

- Note that closing the tool directly does not update the toolbar button yet.

giswqs · 2023-10-06T13:21:13Z

@sufyanAbbasi Thanks a lot for the great improvement. I am on travel today. I might not be able to review this until tonight or tomorrow. If @naschmitz or @bengalin has time, please feel free to review and merge it.

naschmitz · 2023-10-06T19:49:40Z

@sufyanAbbasi – Currently reviewing the PR. How difficult would it be to update this PR to support multiple widgets being open at the same time? It's a common use case.

naschmitz

Looks great! Some nits, but approach looks good. It would be nice to do away with the toggle_off attr if possible.

naschmitz · 2023-10-06T19:38:59Z

geemap/toolbar.py

@@ -15,9 +15,11 @@
 import ipyevents
 import ipyleaflet
 import ipywidgets as widgets
+
+from ipywidgets.widgets import Widget


Nit: ipywidgets.widgets.Widget

Removed and using widgets.Widget below.

naschmitz · 2023-10-06T19:40:23Z

geemap/toolbar.py

        reset: bool = True
+        control: Optional[Widget] = None
+        toggle_button: Any = None


Optional[ipywidgets.ToggleButton]? Will this ever be something else?

Nope, done.

naschmitz · 2023-10-06T19:51:34Z

geemap/core.py

        if selected:
            common.open_url("https://geemap.org")

    def _toolbar_main_tools(self) -> List[toolbar.Toolbar.Item]:
+        @toolbar._cleanup_toolbar_item
+        def inspector_tool_callback(map, _, item):


Nit: Could you add type annotations here + call del on the unused properties?

naschmitz · 2023-10-06T19:53:57Z

geemap/core.py

        if selected:
            common.open_url("https://geemap.org")

    def _toolbar_main_tools(self) -> List[toolbar.Toolbar.Item]:
+        @toolbar._cleanup_toolbar_item
+        def inspector_tool_callback(map, _, item):
+            map.add("inspector")


I wonder if add should return the ipywidget widget that was just added... It would make it quite a bit more useful. We'd be changing the public API, but I'm okay with that.

I think this would be a great addition to the API, but I'm less inclined to make the change in this PR.

naschmitz · 2023-10-06T19:54:54Z

geemap/core.py

        del host_map  # Unused.
+        del item  # Unused.


Nit: del host_map, item # Unused.

naschmitz · 2023-10-06T20:13:53Z

geemap/toolbar.py

@@ -163,7 +179,7 @@ def _reset_others(self, current):
            if other is not current:
                other.value = False

-    def _toggle_callback(self, m, selected):
+    def _toggle_callback(self, m, selected, _):


Nit: _toggle_callback(self, m, selected, item) and then call del item # Unused..

naschmitz · 2023-10-06T20:14:40Z

geemap/toolbar.py


-                if hasattr(m, "pixel_values"):
-                    delattr(m, "pixel_values")
+            if hasattr(m, "marker_cluster"):


Why isn't it necessary to delattr pixel_values anymore?

See line 414, I think this is a diff artifact.

naschmitz · 2023-10-06T20:16:33Z

geemap/toolbar.py

@@ -587,6 +585,7 @@ def handle_interaction(**kwargs):
        toolbar_control = ipyleaflet.WidgetControl(
            widget=toolbar_widget, position="topright"
        )
+        setattr(toolbar_control, "cleanup", cleanup)


toolbar_control.cleanup = cleanup

Done and below.

naschmitz · 2023-10-06T20:17:35Z

geemap/toolbar.py

    close_button.observe(close_btn_click, "value")

    toolbar_button.value = True
    if m is not None:
        toolbar_control = ipyleaflet.WidgetControl(
            widget=toolbar_widget, position="topright"
        )
-
+        setattr(toolbar_control, "cleanup", cleanup)


toolbar_control.cleanup = cleanup

Done and below.

naschmitz · 2023-10-06T20:17:53Z

geemap/toolbar.py

    toolbar_button.value = True
    if m is not None:
        toolbar_control = ipyleaflet.WidgetControl(
            widget=toolbar_widget, position="topright"
        )
+        setattr(toolbar_control, "cleanup", cleanup)


Same here and some other place in the code.

Done and below.

sufyanAbbasi · 2023-10-06T23:12:52Z

Currently reviewing the PR. How difficult would it be to update this PR to support multiple widgets being open at the same time? It's a common use case.

Very easy, just removed two lines in curry_callback to make it work! The only thing is that it may be unclear how certain widgets interact with each other while both are open, since some state is shared between components, however, I expect those interactions to be worked out later!

Sufyan Abbasi added 7 commits October 4, 2023 22:15

Enable toggle on all relevant toolbar tools.

1ac3fad

- Note that closing the tool directly does not update the toolbar button yet.

Merge branch 'master' into toolbar-toggle

baf65f3

Ensure that only one tool is active at a time

a085486

Make sure that buttons untoggle when widget is closed

8f927be

Document the cleanup_toolbar_item decorator

6d0d9e2

Merge branch 'master' into toolbar-toggle

ef22459

Add unit tests for toolbar cleanup functionality

75ffea1

sufyanAbbasi requested review from bengalin, giswqs and naschmitz October 6, 2023 07:19

naschmitz requested changes Oct 6, 2023

View reviewed changes

Allow multiple widgets, code cleanup

4ba90ca

giswqs added 2 commits October 7, 2023 16:19

Merge branch 'master' into toolbar-toggle

c5d8197

Removed unused webbrowser

6701c97

giswqs approved these changes Oct 7, 2023

View reviewed changes

giswqs merged commit 6aad7d2 into gee-community:master Oct 7, 2023
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure toolbar buttons toggle widgets, and turn off when widgets are closed. #1763

Ensure toolbar buttons toggle widgets, and turn off when widgets are closed. #1763

sufyanAbbasi commented Oct 6, 2023

giswqs commented Oct 6, 2023

naschmitz commented Oct 6, 2023

naschmitz left a comment

naschmitz Oct 6, 2023

sufyanAbbasi Oct 6, 2023

naschmitz Oct 6, 2023

sufyanAbbasi Oct 6, 2023

naschmitz Oct 6, 2023

sufyanAbbasi Oct 6, 2023

naschmitz Oct 6, 2023

sufyanAbbasi Oct 6, 2023

naschmitz Oct 6, 2023

sufyanAbbasi Oct 6, 2023

naschmitz Oct 6, 2023

sufyanAbbasi Oct 6, 2023

naschmitz Oct 6, 2023

sufyanAbbasi Oct 6, 2023 •

edited

naschmitz Oct 6, 2023

sufyanAbbasi Oct 6, 2023

naschmitz Oct 6, 2023

sufyanAbbasi Oct 6, 2023

naschmitz Oct 6, 2023

sufyanAbbasi Oct 6, 2023

sufyanAbbasi commented Oct 6, 2023

Ensure toolbar buttons toggle widgets, and turn off when widgets are closed. #1763

Ensure toolbar buttons toggle widgets, and turn off when widgets are closed. #1763

Conversation

sufyanAbbasi commented Oct 6, 2023

Objective

Implementation

Backwards Incompatibility Notes

Tested

giswqs commented Oct 6, 2023

naschmitz commented Oct 6, 2023

naschmitz left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sufyanAbbasi Oct 6, 2023 • edited

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

sufyanAbbasi commented Oct 6, 2023

sufyanAbbasi Oct 6, 2023 •

edited