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

Add width tags for jpeg and png when present #588

Merged
merged 3 commits into from May 24, 2017

Conversation

Projects
None yet
4 participants
@mscuthbert
Copy link
Contributor

mscuthbert commented May 16, 2017

The display(Image(..., retina=True)) command creates metadata about the image explaining the size that it should be displayed at in the notebook. This information, if present, can be carried into the notebook and converted to rst for use in documenters such as Sphinx. If not present, retina images appear in .rst (and from there to other formats such as HTML) as twice their intended size.

Add width tags for jpeg and png when present
The display(Image(..., retina=2)) command creates metadata about the image explaining the size that it should be displayed at in the notebook.  This information, if present, can be carried into the notebook and converted to rst for use in documenters such as Sphinx.  If not present, retina images appear in .rst (and from there to other formats such as HTML) as twice their intended size.
@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented May 16, 2017

Thanks, this looks good to me. I assume you've tested this fix and it works?

@takluyver takluyver requested a review from mpacer May 16, 2017

@takluyver takluyver added this to the 5.2 milestone May 16, 2017

@mscuthbert

This comment has been minimized.

Copy link
Contributor Author

mscuthbert commented May 16, 2017

Yes, tested with both PNG and JPEG w/ and w/o retina=True flag.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented May 17, 2017

Great :-). I'd like @mpacer to have a quick look over this, but I'm happy for it to go in.

@rgbkrk

rgbkrk approved these changes May 17, 2017

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented May 17, 2017

Is width special or would height do as well? If so why probably it'd help to check and set that as well.

It looks like we have a get_metadata filter that addresses this problem, as you can see in the html basic template. I think since we have a supported API that we use elsewhere for this purpose and which is tested directly, using that will be more foolproof going forward.

I'd slightly prefer it if we could test that this is adhered to so that this doesn't happen in the future.
Could you modify the current rst exporter tests to include this check? Here's an html exporter tests that is basically checking for the same thing.

An aside: there's no way to update the image metadata in pngmetadata.ipynb directly, but it's already there as the file to use in your test.

@mpacer
Copy link
Member

mpacer left a comment

This should also add an official test in nbconvert/exporters/tests/test_rst.py.

@@ -50,10 +50,16 @@

{% block data_png %}
.. image:: {{ output.metadata.filenames['image/png'] | urlencode }}
{%- if 'image/png' in output.metadata and output.metadata['image/png'].get('width', '') %}

This comment has been minimized.

@mpacer

mpacer May 17, 2017

Member

It's cleaner to use the get_metadata filter. You can find an example of its use for an analogous purpose in the html template.

@@ -50,10 +50,16 @@

{% block data_png %}
.. image:: {{ output.metadata.filenames['image/png'] | urlencode }}
{%- if 'image/png' in output.metadata and output.metadata['image/png'].get('width', '') %}
:width: {{ output.metadata['image/png'].width}}px
{% endif -%}

This comment has been minimized.

@mpacer

mpacer May 17, 2017

Member

You should also add height information if it is present.

{% endblock data_png %}

{% block data_jpg %}
.. image:: {{ output.metadata.filenames['image/jpeg'] | urlencode }}
{%- if 'image/jpeg' in output.metadata and output.metadata['image/jpeg'].get('width', '') %}

This comment has been minimized.

@mpacer

mpacer May 17, 2017

Member

Same as above, use get_metadata and add height.

@mscuthbert

This comment has been minimized.

Copy link
Contributor Author

mscuthbert commented May 18, 2017

Thanks for "teaching me to fish" -- I've made these changes and added a test equivalent to the metadata test in test_html. I now feel like I understand the rst remplate system better so may be able to make some further improvements in the future. THANKS! @mpacer

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented May 18, 2017

Happy to help — thank you for contributing!

It's good as it stands, but if you want I'm confident that by tweaking the test to be closer to testing the behaviour you actually care about it can be even better. Happy to help with that too.

But that's your call — if you want to call it "done" it's ready to be merged.

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented May 23, 2017

@mscuthbert Did you want to keep working on this and learn a bit more or do you want me to merge it as is and make the changes myself?

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented May 23, 2017

Also, I'm ok with you leaving the eclipse stuff there for now, but you should know that you could have just added them to .git/info/exclude.

@mscuthbert

This comment has been minimized.

Copy link
Contributor Author

mscuthbert commented May 24, 2017

Hi @mpacer -- I'll try to get the changes in there soon. Thanks for the poke!

@mpacer

mpacer approved these changes May 24, 2017

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented May 24, 2017

@mscuthbert, is it ok if you make a separate PR to improve your test? I'm planning on releasing nbconvert today and I want to include the work you've done already. Does that seem reasonable?

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented May 24, 2017

I'm just going to go ahead with that — it isn't too much overhead to make a new PR and then this feature will be available for everyone

@mpacer mpacer merged commit 08a2d64 into jupyter:master May 24, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment