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

Hovertool safe tags #5460

Merged
merged 3 commits into from Nov 17, 2016
Merged

Hovertool safe tags #5460

merged 3 commits into from Nov 17, 2016

Conversation

astraw38
Copy link
Contributor

@astraw38 astraw38 commented Nov 13, 2016

All pull requests must have an associated issue in the issue tracker. If there
isn't one, please go open an issue describing the defect, deficiency or desired
feature. You can read more about our issue and PR processes in the
wiki.

Check if the format tag included in HoverTool templating
is == safe, and will disable any HTML escaping if true.

Updated docs, and updated existing example

Check if the format tag included in HoverTool templating
is == safe, and will disable any HTML escaping if true.

Updated docs, and updated existing example
@bryevdv
Copy link
Member

bryevdv commented Nov 14, 2016

Thanks for the PR @astraw38! My only comment is about the syntax. We already have e.g. color[options] I wonder if maybe foo[safe] is worth considering, so that there are fewer different ways to "spell" things.

Thoughts @mattpap @birdsarah ?

@astraw38
Copy link
Contributor Author

I agree -- especially since the {} format would be overloaded with either Numbro syntax or what I added w/ 'safe'. I'll make the changes this evening -- will involve modifying the regex too.

@astraw38
Copy link
Contributor Author

@bryevdv am I missing something, or is there no documentation for setting the Numbro format with HoverTool? Is that covered by docs somewhere else (perhaps another location that replace_placeholders is used)?

When I do another pass on this, I can add something in for that too, since that might have been overlooked .

@astraw38 astraw38 closed this Nov 14, 2016
@astraw38 astraw38 reopened this Nov 14, 2016
@mattpap
Copy link
Contributor

mattpap commented Nov 14, 2016

We already have e.g. color[options] (...)

Which is a special-case (it's not even handled in replace_placeholders()).

@mattpap
Copy link
Contributor

mattpap commented Nov 14, 2016

This looks good. Though on a side note, I would recommend to stick with code formatting of the module/library, e.g. use #{prefix}#{value} instead of '' + prefix + value, etc.

@bryevdv
Copy link
Member

bryevdv commented Nov 14, 2016

When I do another pass on this, I can add something in for that too, since that might have been overlooked

I has been overlooked, it's been on to TODO for a long time, just not gotten to, so if it's easy to tack on something in this PR that would actually be very appreciated!

@bryevdv
Copy link
Member

bryevdv commented Nov 14, 2016

Which is a special-case (it's not even handled in replace_placeholders())

Open to suggestions if you have alternatives to propose

@astraw38
Copy link
Contributor Author

astraw38 commented Nov 15, 2016

I don't think it would be difficult to use [] like $color[hex]:foo, but I'm concerned about consistency and backwards compatibility:

  1. Use [] for all options after a field name -- breaks backwards compatibility with Numbro formatting
  2. Use [] or {} for options after a field name -- would ideally also apply to $color, so would be additional changes (still doesn't look to be difficult though)
  3. Use {} for all options other than $color after a field name -- not consistent, but easiest.

Personally, I would lean towards #2, but I'll let you guys make the final decision because you're by far more invested in it :)

@bryevdv
Copy link
Member

bryevdv commented Nov 15, 2016

@astraw38 just so I understand, are you saying numbro itself has formats that use [...]? If so then let's definitely not use that. If that's the case, then I'd say we should:

  • find a bracket that will not conflict, for general options, that can just be "pre-processed" (i.e., we just always strip a trailing {....}off of @foo{...} before doing anything else, to get any "options"
  • auto-convert @color[...] to @color{...} and warn (this could happen later maybe)

Thoughts @mattpap @birdsarah ?

@astraw38
Copy link
Contributor Author

Sorry, I originally meant that if we change any postfix formatting to use [] instead of the current {}, it could break people's projects on upgrade. Though now that you mention it, there could be some conflicts with Numbro, since there are format strings that use []. Nothing that couldn't be gotten around with fancy regex I imagine, but I would need a lot more coffee before going into that.

@bryevdv
Copy link
Member

bryevdv commented Nov 15, 2016

@astraw38 well, we can "fix up" these values on the python side during serialization. So I'd say the things to decide is what's best overall, for going forward, and let's just do that. Then we can fix up color however we need to to conform to that. Does {...} seem like the best options for "options"?

@mattpap
Copy link
Contributor

mattpap commented Nov 15, 2016

Thoughts @mattpap

Yes. I'm not sure why a self-contained, perfectly valid PR is turned into unnecessary overhaul of something that works and there is just one feature that wasn't ported originally, which is this "color widget", that mostly makes sense only in hover tool context. Note that replace_placeholders() is much more universal tool.

Also, if syntax is discussed, then lets start from the full syntax which is @{column name}{formatter}. The choice of curly brackets isn't arbitrary and is derived from string interpolation in various programming languages (e.g. in coffeescript it's #{foo}). The second part uses curly brackets as well, to minimize the number of top-level characters in use, thus limiting need for escaping and reducing interaction with contents within curly bracket groups. Coincidentally, numbro doesn't use curly brackets, so it's a good choice for delimiting column format. Overall syntax (which may seem odd at first) i.e. adjacent {}{} isn't a coincidence as well. We don't want to have a column name-format separator, because then this could interact with column names (which can be arbitrary) and we would have resort to escaping the separator or confuse users like with : and categorical labels.

@bryevdv
Copy link
Member

bryevdv commented Nov 15, 2016

@mattpap I'm not trying to overhaul anything. But I wanted to take the user experience into consideration, and have a discussion about trying to avoid multiple ways to spell things for users. It seems like we are agreed that {...} is the best suitable option for providing additional options at the end.

@bryevdv
Copy link
Member

bryevdv commented Nov 15, 2016

@astraw38 let's leave changing @color for later.

Also updated templating.coffee to use string formatting
instead of concatenation.
@bryevdv
Copy link
Member

bryevdv commented Nov 16, 2016

OK @astraw38 this LGTM! If you would be inclined, it would be good to try to add some tests for the templating.coffee module here:

https://github.com/astraw38/bokeh/tree/master/bokehjs/test/core/util

since there are none currently. If this is something you have some experience with (so that it would not be a big effort for you hopefully), or if not, but its something you'd like to learn more about, let me know! Otherwise I'm happy to merge this as-is. (I will plan to do that tomorrow if I don't hear anything)

Thanks for the nice PR!

@Zulko
Copy link

Zulko commented Nov 17, 2016

Thank you very much @astraw38 , that's very useful !

@astraw38 astraw38 deleted the hovertool-safe-tags branch November 19, 2016 19:00
@Zulko
Copy link

Zulko commented Dec 1, 2016

Hi guys - this doesn't this to work for me. I am using the latest version from Github, and when I apply the {safe} tag for a field whose values are text, I get a "NaN" value instead of the text. Here is some code to reproduce the problem.

from bokeh.io import show, output_notebook
from bokeh.plotting import ColumnDataSource, figure
from bokeh.models import HoverTool, Range1d

output_notebook()
fig = figure(tools=[HoverTool(tooltips=[("html", "@html{safe}")])])
fig.quad(left="left", top="top", bottom="bottom", right="right",
         source=ColumnDataSource({"left": [1,3], "bottom": [1,3],
                                  "right": [2,4], "top": [2,4],
                                  "html":["<b>I'm bold</b>", "some text"]}))
show(fig)

@astraw38
Copy link
Contributor Author

astraw38 commented Dec 1, 2016

@Zulko I had to follow bokeh's dev guide to make sure that my manual test code was using locally built JS instead of hitting the CDN. Maybe that's the problem? Though I wouldn't discount problems with my submission, this was my first time using coffeescript :)

@bryevdv
Copy link
Member

bryevdv commented Dec 1, 2016

@Zulko as @astraw38 mentions, this is not yet in any released version of Bokeh. You'll have to build from scratch or install a dev build once there is a dev build created with this change. I hope to make a new dev build today or tomorrow.

@Zulko
Copy link

Zulko commented Dec 1, 2016

I am using the latest master with a locally built BokehJS. By the way, when I run it in a Jupyter notebook i get this message (I assume it just means I am using the development version of BokehJS ?)

Warning: Requesting CDN BokehJS version '0.12.4dev6' from Bokeh development version '0.12.4dev6-38-gd66b70a'. This configuration is unsupported and may not work!

@bryevdv
Copy link
Member

bryevdv commented Dec 1, 2016

did you set the env var BOKEH_RESOURCES=inline ? if not it's going to CDN and grabbing the latest dev build (dev6) and I am not sure this change is in dev6 or not.

@Zulko
Copy link

Zulko commented Dec 1, 2016

No I didnt. I assume I would need to do this before installing bokeh ?

@bryevdv
Copy link
Member

bryevdv commented Dec 1, 2016

right now, if you want to test this feature, you need to build Bokeh, not install it. I would suggest waiting for the next dev build to be made available as that can just be installed.

Edit: maybe you mean setup.py install? in which case, no you set the env var before you run any scripts, notebooks, etc.

BOKEH_RESOURCES=inline ipython notebook

@Zulko
Copy link

Zulko commented Dec 1, 2016

Yes, by "install" I meant "I built the js sources then I ran setup.py develop". Note that setup.py showed me a menu asking me if I wanted to use my locally-built BokehJS and I said yes, so its confusing that it's actually not using my local built version.

I'll retry installing and report back when I have some success.

@bryevdv
Copy link
Member

bryevdv commented Dec 1, 2016

@Zulko that is asking if you want to install an already-locally-built BokehJS, or if you want to build BokehJS from scratch again and install that. It's the difference between:

python setup.py develop --build-js   # build BokehJS from scratch

and

python setup.py develop --install-js   # install the last locally-built BokehJS 

In any case it only affects what BokehJS is installed. What BokehJS is used when running things is always only dependent on the Resources used, either explicitly or via things like BOKEH_RESOURCES env var.

@draperjames
Copy link

maybe you mean setup.py install? in which case, no you set the env var before you run any scripts, notebooks, etc.
BOKEH_RESOURCES=inline ipython notebook

@bryevdv how do you set the env var?

@draperjames
Copy link

@bryevdv @astraw38 the custom tooltip example appears to not be rendering <span>@fonts{safe}</span> on the example webpage itself. I have tried to view it on several different computers and browsers but it just does show up. I can't get the example to render in python either. Does it work for either of you? Thanks for your time.

@draperjames
Copy link

@bryevdv @astraw38 I tried running the example @Zulko mentioned and setting the env var BOKEH_RESOURCES=inline ipython notebook through %env magic in the following cell;

%env BOKEH_RESOURCES=inline ipython notebook
from bokeh.io import show, output_notebook
from bokeh.plotting import ColumnDataSource, figure
from bokeh.models import HoverTool, Range1d

output_notebook()
fig = figure(tools=[HoverTool(tooltips=[("html", "@html{safe}")])])

fig.quad(left="left", top="top", bottom="bottom", right="right",
         source=ColumnDataSource({"left": [1,3], "bottom": [1,3],
                                  "right": [2,4], "top": [2,4],
                                  "html":["<b>I'm bold</b>", "some text"]}))
show(fig)

So @html{safe} appears to be replaced in the tooltip by #{prefix}#{value}. Any idea what's going on?

@astraw38
Copy link
Contributor Author

astraw38 commented Jan 11, 2017

It looks like it should be double quoted and not single quoted (in this file) allow for interpolation.

@draperjames
Copy link

Okay I opened PR #5700 to do that. Even if that PR is merged relatively soon will that change be reflected in the pip install? or will I have to build it from scratch and install?

@bryevdv
Copy link
Member

bryevdv commented Jan 11, 2017

Hi @draperjames Thanks very much for the PR! This change will be in the next full release. As it happens, we just released 0.12.4 on Monday, so another full release is probably 2-3 months away. We do have the ability to publish "dev builds" anytime, I and I am happy to make one in the next few days after this PR is merged, but those are only published as conda packages, and not to PyPI. If you have the ability to use Anaconda / conda, then those will be easy to install. Otherwise building from a source checkout is probably the only other immediate term option.

@draperjames draperjames mentioned this pull request Jan 11, 2017
@draperjames
Copy link

Would you mind elaborating a little on how to install the dev build? I just posted answer to my question on stack overflow and I want to make it easy for the people I work with to install. I'd really appreciate that 😃.

@bryevdv
Copy link
Member

bryevdv commented Jan 11, 2017

Sure, dev build install instructions here here:

http://bokeh.pydata.org/en/latest/docs/installation.html#developer-builds

I guess I'd forgotten, it may be possible to use pip to install, just not from PyPI. We only publish full releases to PyPI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using HoverTool to display arbitrary HTML
5 participants