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

Add width tags for jpeg and png when present #588

Merged
merged 3 commits into from
May 24, 2017

Conversation

mscuthbert
Copy link
Contributor

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.

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
Copy link
Member

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 15:40
@takluyver takluyver added this to the 5.2 milestone May 16, 2017
@mscuthbert
Copy link
Contributor Author

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

@takluyver
Copy link
Member

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

@mpacer
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.

Copy link
Member

@mpacer mpacer left a comment

Choose a reason for hiding this comment

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

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', '') %}
Copy link
Member

Choose a reason for hiding this comment

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

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 -%}
Copy link
Member

Choose a reason for hiding this comment

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

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', '') %}
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, use get_metadata and add height.

@mscuthbert
Copy link
Contributor Author

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
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
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
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
Copy link
Contributor Author

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

@mpacer
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
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
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.

4 participants