Properly incorporated pre_embedimages into EmbedHTMLExporter

[gabyx]

I have incoporated the changes in the pull request #1067 into the pull request #1052
Once #1052 is merged, I will sync and we can discuss this here:

So far I hooked the pre_embedimages.py preprocesser into the EmbedHTMLExporter (embedhtml.py) such that :

• First pre_embedimages.py in Preprocessor to embed markdown images #1067 embeds all markdown images ([](/path/to/image.png)) as attachments with base64 to make a standalone notebook.
• Another preprocessor in embedhtml.py gives all attachements a unique id
  this is necessary since we can have same attachment names in different cells
  and after the HTMLExporter has run, we are doomed since we don't know which <img src:"attachement:name"/> map to which attachements. So we save all unique attachements in the resources which we then use again in the next step:
• EmbedHTMLExporter runs which converts all <img src="..."> tags to base64 embeded tags (attachements and all urls) by using resources["unique-attachements"]

I think this is pretty neat :-).
I am not sure about the more hacky unique id stuff... but was the only way I could solve it now.

Important Test Notebook:
EmbedImages.zip

[gabyx]

some linting and python2.7 issues needs fixing...

[jcb91]

Once #1052 is merged, I will sync and we can discuss this here

#1052 was already merged, 2017-10-05, at 10:52:23 GMT.

[jcb91]

Did you mean once #1067 is merged?

[gabyx]

No, the pull request I opened before the merge of #1052
Here I sync the developments on the branch in #1067:
These changes here are improvements to the embedhtml.py
and it only depends on #1067 by (pre_emebedimages.py).
Would be cool if someone could review my idea :-)

[gabyx]

ON linux (except linting for pre_embedimages.py which should be fixed in #1064
it works properly, on windows it fails completey, wtf...? really?
Some strange issue,-> PITA

[jcb91]

Ok, I'm afraid I'm a little confused about the status of this & #1067 & where we're aiming to go with the two. As I currently understand it, the idea seems to be:

• Preprocessor to embed markdown images #1067 aims to create a preprocessor which takes a notebook, and adds any images in markdown cells in the new attachments metadata
• this PR aims to have the EmbedImagesPreprocessor from Preprocessor to embed markdown images #1067 do the embedding for markdown cells for the EmbedHTMLExporter, in addition to the html embedding done afterwards through lxml.
• nbconvert doesn't seem to support the attachments metadata - I can find no instance of attachments in the repo, at any rate - is that the case?! It seems crazy, but if it is true, then shouldn't the markdown embedder in Preprocessor to embed markdown images #1067 be using data: type URIs @juhasch?

For me the first problem with this PR as it stands is that it seems to be based on an older version (2293ab9) of #1067, and need rebasing onto the current version in order for much to make sense...

[gabyx]

sure we can update this of course. can i just merge the branch from #1067 into this request again? I tested this with nbconvert and had no problems with attachements? everything works as expected

[jcb91]

can i just merge the branch from #1067 into this request again?

you could, but it would be neater (I think) to rebase onto it. This (rebasing a published branch) could cause confusion if others have already based work on it, but it hasn't been published long, so I think we can risk that for a little more clarity with what's happening. Does that make sense?

I tested this with nbconvert and had no problems with attachements?

Sure, but that's the point of this PR, isn't it? What I was asking about was whether, if I attempt to export a notebook that has attachments using base nbconvert with no extensions, it actually works (attachment images appear in output)?

[gabyx]

true that, I will try rebasing my changes onto #1067. ehm not sure if I understand correctly, but the point of this pr is only to have a clearer pipeline of converting images to finally html. since we have already a pre_embedimages I thought a conversion process notebook -> notebook -> html where each converter: first the preprocessor pre_embedimages runs and in the second stage every thing left over is embedded. I think this way each step can focus on its part and do this as best as it can. This also means we could think of again reducing the complexity in embedhtml again (not done here) since pre_embedimages has already done its main work. whats left over is html in markdown cells basically (if i am not mistaken). what do you mean with "with no extension" for your question with a nb with attachments ? If you mean running to=embed_html then this pr the embedhtml automatically runs the preprocessor pre_embedimages. The other case running nbconvert with to=html -> i dont think images will appear i. the html - so ig it does not worl this should be a feature for nbconvert directly hmm so the bade Html exporter should already handle this! thats I thinl what should happen. So the best way IMO is that our embedhtml uses pre_embedimages as preproc and the base html exporter should hanfle embedding attachments since that should work out of the box.. what do you think? Von meinem iPhone gesendet

…

Am 13.10.2017 um 17:44 schrieb Josh Barnes ***@***.***>: can i just merge the branch from #1067 into this request again? you could, but it would be neater (I think) to rebase onto it. This (rebasing a published branch) could cause confusion if others have already based work on it, but it hasn't been published long, so I think we can risk that for a little more clarity with what's happening. Does that make sense? I tested this with nbconvert and had no problems with attachements? Sure, but that's the point of this PR, isn't it? What I was asking about was whether, if I attempt to export a notebook that has attachments using base nbconvert with no extensions, it actually works (attachment images appear in output)? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[juhasch]

Having the preprocessor allows generating notebooks with all images included, which is helpful when exchanging notebooks. That said, people use <img> tags, too. This is required because the current markdown flavor does not provide a way to center or size images. You can't embed those images in the preprocessor. Also, using <img> tags breaks non-html exports...

So exporting is a mess, and the best way I see is to have a preprocessor and an exporter.

@jcb91 nbconvert seems to ignore attachments right now. The extractoutput preprocessor only looks at output data right now.

I would prefer two individual PRs: one for the preprocessor and one for the exporter. As the feature set of both things is independent, this makes things easier, IMO.

[jcb91]

So exporting is a mess, and the best way I see is to have a preprocessor and an exporter.

👍 Ok, I think I follow that, and it seems like a sensible solution.

@jcb91 nbconvert seems to ignore attachments right now. The extractoutput preprocessor only looks at output data right now.

Ok, that did (does!) seem strange, but at least I'm not missing something obvious! I guess the notebook format has jumped without nbconvert having time to catch up 🤦‍♂️ Would it be worth adding somethign like #1067 as a PR to nbconvert itself?

I would prefer two individual PRs: one for the preprocessor and one for the exporter. As the feature set of both things is independent, this makes things easier, IMO.

That would seem reasonable, except that the feature-set doesn't quite seem independent - it makes sense to be running the preprocessor for the exporter, no? Otherwise don't we miss out any attachments in the markdown images? But yeah, it seems that #1067 is the one to be working on for now, at any rate

[gabyx]

That would seem reasonable, except that the feature-set doesn't quite seem independent - it makes sense to be running the preprocessor for the exporter, no?

I strongly think so too, they are not really independent, but should be viewed as combined in a single exporting pipeline, like running the preoproc. first and afterwards the exporter...

Just to be clear, this PR's idea is exactly to run the pre_processor (automatically) before the exporter, thats all it does. And so far (also not with #1067) we have no solution which properly exports everything to html like the above attached notebook file, this PR covers this:
https://github.com/ipython-contrib/jupyter_contrib_nbextensions/files/1335074/EmbedImages.zip

[gabyx]

Configuration options are now available: jupyter/notebook#2413
So we will be able to upload a config json for example to adjust image conversion =)

[mpacer]

well… it's not merged yet. And I have a huge backlog of issues that various people want fixed before I can get to merging it… and it gets set back every time another change is made to the nbconvert handlers 😭 , which we'll need to do to get jupyter/notebook#2974 fixed

[juhasch]

@gabyx Would you like to give this another go ?
Let's get it merged here, and eventually get it into nbconvert later.