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

[WIP] Imagemap #2739

Open
wants to merge 35 commits into
base: main
Choose a base branch
from
Open

[WIP] Imagemap #2739

wants to merge 35 commits into from

Conversation

zerline
Copy link
Contributor

@zerline zerline commented Jan 15, 2020

Passing click coordinates to the backend. For now, just for Buttons.

@martinRenou
Copy link
Member

Do we really want to add this kind of APIs to the Button? This looks really out of scope for a Button...
Why do you need to get click coordinates on the button?

@zerline
Copy link
Contributor Author

zerline commented Jan 15, 2020

Do we really want to add this kind of APIs to the Button? This looks really out of scope for a Button...
Why do you need to get click coordinates on the button?

Ok @martinRenou. I'm working on a MappedImage now, with a completely different approach, using <map> and <area> tags. It will possibly be more useful. Yes we could consider integrating the _get_relative_xy method in parent class WidgetView (code coming from ipyevents) just in case ..

@martinRenou
Copy link
Member

I'm working on a MappedImage now

Why not adding click events to the Image widget directly instead of introducing a new widget? I can see more use-cases for this than for buttons (retrieve pixel color under the image click location, draw polygon with the mouse for ML image recognition...).

@zerline
Copy link
Contributor Author

zerline commented Jan 15, 2020

Why not adding click events to the Image widget directly instead of introducing a new widget?
I can see more use-cases for this than for buttons (retrieve pixel color under the image click location, draw polygon with the mouse for ML image recognition...).

Yes, we could do that too.
MappedImage will address the needs for specified areas in the image. For instance, we could specify "clicking close to the top-right corner" ..

@martinRenou
Copy link
Member

MappedImage will address the needs for specified areas in the image. For instance, we could specify "clicking close to the top-right corner" ..

I feel like this kind of custom logic could be easily implemented, in Python, once the Image has click events with click coordinates. We might not want to add this to ipywidgets core though.

@zerline
Copy link
Contributor Author

zerline commented Jan 15, 2020

@martinRenou Do you think the method _get_relative_xy() is the way to go towards what you have in mind?

@zerline
Copy link
Contributor Author

zerline commented Jan 15, 2020

I feel like this kind of custom logic could be easily implemented, in Python, once the Image has click events with click coordinates.

The idea of using <map> tag, is precisely to save the user having to write any logic by taking advantage of what the browser offers.

@martinRenou
Copy link
Member

The idea of using tag, is precisely to avoid writing any logic by taking advantage of what the browser offers.

Oh sorry! I did not know about this <map> tag, thanks! Indeed it sounds like it could be a good idea to add it to ipywidgets as a core widget, or as an extension to the Image (the Image widget could have an areas attribute which could be a list of duck-typed dictionaries or Area widgets). Is it what you have in mind?

@martinRenou
Copy link
Member

The <map> tag is supported by default by all major browsers

@zerline
Copy link
Contributor Author

zerline commented Jan 15, 2020

the Image widget could have an areas attribute which could be a list of duck-typed dictionaries or Area widgets. Is it what you have in mind?

Yes, I've written a MapArea class, and am currently passing it to MappedImage via a TypedTuple.

@martinRenou
Copy link
Member

I feel like introducing a new widget for mapped images is not needed, because a MappedImage without areas (areas set to an empty list, which will be the default value) is really just an Image.

And expanding the interactivity on the Image widget is good, as it did not have any interactions implemented before.

self.name = name
self.shape = shape
self.coords = coords # a tuple of integers
self.href = href
Copy link
Member

Choose a reason for hiding this comment

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

Those attributes could be traits? Allowing to validate/observe/link values.

self.name = name
self.shape = shape
self.coords = coords # a tuple of integers
self.href = href
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if href is what we need, maybe an on_click event would be easier to play with for users in Python?

Copy link
Member

Choose a reason for hiding this comment

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

Similar to the Button API

_view_name = Unicode('MappedImageView').tag(sync=True)
_model_name = Unicode('MappedImageModel').tag(sync=True)

map_name = Unicode("Map", help="The map name").tag(sync=True)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if the map_name should be exposed to the user, as far as I understand this is an implementation detail and does not bring anything to the Python API.

Maybe we should set the map name to a random uuid? And not expose it to Python?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @martinRenou
Yes, traits.
Yes, why not not expose map_name.
At the moment, I experience some difficulties in displaying the map areas. I could possible create a new HTMLElement widget, that would be like HTML, without its surrounding <div>. Don't you think such an element would bring some flexibility?

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure I understand what you are saying.

But looking at the code I understand the ImageView implementation should be changed in order for this to work, this.el should be an HTMLDivElement instead of an HTMLImageElement. And it would contain the HTMLImageElement and the HTMLMapElement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hesitate.
I guess having an <img> without <div> is there for a reason, in Image widget.
But I would like to use this HTMLElement to create and display the map areas, and then the map itself.
Maybe such a strategy is too complicated. I want to test these things anyway, but if you tell me such an HTMLElement (with arbitraty tag name, not necessarily 'div') would be useful / or not useful, that will help guiding my efforts anyway.

Copy link
Member

Choose a reason for hiding this comment

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

I am not talking about a new HTMLElement widget. I am talking about the html tags.

I was suggesting doing:

<div>
    <img src="...">
    <map>
        <area ...>
        <area ...>
    </map>
</div>

instead of the current:

<img src="...">

In the Image widget

@@ -40,6 +40,19 @@ class HTML(_String):
_view_name = Unicode('HTMLView').tag(sync=True)
_model_name = Unicode('HTMLModel').tag(sync=True)


@register
class HTMLElement(_String):
Copy link
Member

Choose a reason for hiding this comment

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

@zerline
Copy link
Contributor Author

zerline commented Jan 17, 2020

I made a new version, with no new widget in widget_string.
I did not sync() the traits for areas (name, shape, coords and href). It seems difficult to ask the browser to change the shape without the coords, or coords without shape .. But in Python you can change both in the same line. Open to suggestion on that matter!

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.

None yet

2 participants