Skip to content

initial_attempt at resolving redraw for ImageOverlay #990

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

initial_attempt at resolving redraw for ImageOverlay #990

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ipyleaflet/leaflet.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -747,6 +747,8 @@ class ImageOverlay(RasterLayer):
# Options
attribution = Unicode().tag(sync=True, o=True)

def redraw(self):
self.send({'msg':'redraw'})

class VideoOverlay(RasterLayer):
"""VideoOverlay class.
Expand Down
7 changes: 7 additions & 0 deletions js/src/layers/ImageOverlay.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export class LeafletImageOverlayView extends rasterlayer.LeafletRasterLayerView
this.model.get('bounds'),
this.get_options()
);
this.model.on('msg:custom', this.handle_message.bind(this));
}

model_events() {
Expand All @@ -51,4 +52,10 @@ export class LeafletImageOverlayView extends rasterlayer.LeafletRasterLayerView
this
);
}

handle_message(content) {
if (content.msg == 'redraw') {
this.obj.redraw();
Copy link
Member

Choose a reason for hiding this comment

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

There doesn't seem to be a redraw method on ImageOverlay according to the Leaflet docs https://leafletjs.com/reference.html#imageoverlay-setopacity
unless I missed it?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe you want to use setUrl instead, but there apparently no way of forcing the image to update if the url is the same.

According to this stackoverflow comment you can append the date to the url to force an update but I'm not sure we'd want to do this in ipyleaflet's codebase. Though nothing stops you from doing it in your notebook.

Copy link
Author

Choose a reason for hiding this comment

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

Martin, thanks for the review.
For the leafletjs link to imageoverlay - I agree redraw is not a method on ImageOverlay, I will search through their github and see if that has been requested/makes sense as a feature request, if not I will file an issue.

The workarounds you suggested are ones I have employed and just seemed to add noise where noise was not needed - so I was trying to implement this redraw.

Since this would first depend on leaflet implementing something - should I just leave this pull request open as I work on that end? Not sure what the proper protocol is for a pull request.

Copy link
Member

Choose a reason for hiding this comment

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

The workarounds you suggested are ones I have employed and just seemed to add noise where noise was not needed

Does the workaround work for the ImageOverlay then?

Since this would first depend on leaflet implementing something - should I just leave this pull request open as I work on that end?

Yes it's fine to keep this PR open for reference. I will turn it into draft.

Copy link
Author

@afonit afonit Jun 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Does the workaround work for the ImageOverlay then?

Yes, I was originally just tearing down the layer and rebuilding as a workaround. Your suggestion of just resetting the url is much cleaner - thank you.

It would be nice to not have to get into url management with a new image for each zoom set so I still filed the request on leaflet as that would simplify end-user code.
Leaflet/Leaflet#8277

Copy link
Member

Choose a reason for hiding this comment

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

One thing that would be nice would also to have the ImageOverlay work with an Image (from ipywidgets) widget

}
}
}