Skip to content
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.

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

Defines an AbstractDrawControl and implementation for the current draw control features. #1666

Merged
merged 6 commits into from
Aug 19, 2023

Conversation

sufyanAbbasi
Copy link
Collaborator

AbstractDrawControl

  • The goal for this PR is to extract as much of the draw control functionality into its own abstract class so that we can build any draw control for any map implementation but will always provide the same, backwards compatible API for users of geemap. To do this, we define a new abstract class, AbstractDrawControl, which acts an interface between any map's draw control functionality (like ipyleaflet's DrawControl) to the user's geemap code.
  • The class requires the following methods to be implemented for a given draw control, like ipyleaflet's DrawControl, and the rest of the API takes care of keeping the geometries, feature list, and feature collections in sync for the user to use in their script:
    def _bind_to_draw_control(self):
        """Set up draw control event handling like create, edit, and delete."""
        raise NotImplementedError()
    
    def _remove_geometry_at_index_on_draw_control(self):
        """Remove the geometry at the given index on the draw control."""
        raise NotImplementedError()

    def _clear_draw_control(self):
        """Clears the geometries from the draw control."""
        raise NotImplementedError()

    def _get_synced_geojson_from_draw_control(self):
        """Returns an up-to-date of GeoJSON from the draw control."""
        raise NotImplementedError()
  • I've implemented a new class, MapDrawControl which extends the ipyleaflet's DrawControl so that user's can continue using the draw control API exactly as they have been (I hope).
  • Unfortunately, there's a bug in the base ipyleaflet DrawControl that disallows us to keep the edited geometries in sync with the stored geometries in geemap. Notably, this bug is not present in Colab notebooks, but is present in Jupyter Labs. I have proposed a fix for it in ipyleaflet. After this fix is in, we can consider keeping the geometries in sync with the drawn geometries using ipyleaflet's DrawControl data field. Until then, I kept the existing behavior where edited geometries are added instead of modified in place.
  • I also implemented some fixes to the toolbar's "Collect training samples" drawing feature. Now it will properly reset the draw control back to the default one after the user hits "Close."

Unfortunately, I'm not expert at using geemap so I will need your help to figure out if this change won't break existing scripts!

@@ -377,3 +380,183 @@ def _objects_info(self, latlon):
nodes.append(tree_node)

return self._root_node("Objects", nodes)


class DrawActions(enum.StrEnum):
Copy link
Member

Choose a reason for hiding this comment

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

Thanks a lot for your work on this. Some CI tests failed because of enum.StrEnum, which seems only available in Python 3.11. We need an alternative to support Python 3.8-3.10

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, thanks for catching this so quickly, working on a fix now. And I found one other issue related to clear_draw_control keyword didn't get properly initialized on the reset() function.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've amended the last commit to use the normal enum.Enum, so hopefully the tests will not break there.

@sufyanAbbasi sufyanAbbasi force-pushed the abstract-draw-control branch from 1042517 to 7c0d85a Compare August 19, 2023 02:53
@giswqs
Copy link
Member

giswqs commented Aug 19, 2023

It looks great! I added some docstrings so that the classes can show up in the documentation. I also fixed a bug in the draw control reset() function by setting self.data = []. Otherwise, the drawn features will remain on the map even though the ee objects have been removed.

@giswqs giswqs merged commit 761afaa into gee-community:master Aug 19, 2023
@sufyanAbbasi
Copy link
Collaborator Author

Thank you for the fix!

@sufyanAbbasi sufyanAbbasi deleted the abstract-draw-control branch August 22, 2023 04:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants