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

change default for slides to direct to the reveal cdn rather than locally #732

Merged
merged 7 commits into from Jan 26, 2018

Conversation

Projects
None yet
6 participants
@mpacer
Copy link
Member

mpacer commented Jan 10, 2018

This is the quick fix suggested by @fperez (to change the default) for #702.

My intuition is that we should provide a way to to create slides in a "stand-alone" mode. That mode will actually solve the actual problem with local copy of reveal that we're running into in #702.
That would probably also download the relevant copy of the reveal library into the output directory and maybe a bunch more.

@mpacer mpacer changed the title change default for slides to direct to the cdn rather than locally change default for slides to direct to the reveal cdn rather than locally Jan 10, 2018

@fperez

This comment has been minimized.

Copy link
Member

fperez commented Jan 12, 2018

This looks good to me, but a 2nd pair of eyes would be useful, as I've been away from the code for too long to trust that I won't miss something...

@fperez

This comment has been minimized.

Copy link
Member

fperez commented Jan 12, 2018

Thx a lot, btw! Having this work out of the box (even if it doesn't support 100% offline use) will help a lot of regular users.

@mpacer

This comment has been minimized.

Copy link
Member Author

mpacer commented Jan 13, 2018

@takluyver @damianavila could either of you give this a look before we merge it? I'd like to get out a release soon and I'd like this to be part of it.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Jan 15, 2018

I'm wary of slapping a band-aid fix on for a problem that, based on the discussion on #702, we don't seem to properly understand. Damian suggested that this change risks breaking some other people's workflow, and I'd rather leave the problem where it is for another release - where people have likely found some kind of workaround - rather than creating a new problem for a different set of users and causing more confusion.

(The users experiencing the problem at the moment will likely protest that theirs is the typical use case, and we should just fix their bug, because the things it breaks are bound to be less important. We're all prone to assuming that we're typical. I'm not trying to make a judgment on whose use case is more important, just that a consistent problem is better than switching between problems.)

Clearly that answer isn't very satisfactory, so I'm going to try to investigate what's going on with slides.

@damianavila

This comment has been minimized.

Copy link
Member

damianavila commented Jan 15, 2018

Sorry perople for being late here, I was sick the last few days.

In general agree with @takluyver here. I am more than open to discuss the default change, but if we take that path, we need to make it in a more general PR changing not only the default value but also other parts of the slides architecture. Changing just this value I think will bring us more problems than solutions. This is why +1 @takluyver PR on docs and I would be -1 into merging this one as is.

@mpacer

This comment has been minimized.

Copy link
Member Author

mpacer commented Jan 18, 2018

I’ll merge the docs… but I do think this is a default that benefits users “in-the-know” better than users who don’t get these details. It is not just that this is a typical use case but is the major reason people have quoted as their problems with using the slides export. Having a local copy of reveal is much further from “just working” than relying on the cdn.

We should definitely provide a way to handle the local filesystem use case and I’m ok including that in this PR if that’s what is needed to address this default issue… but I genuinely think it should be changed to support easier new users and to support webpages for slides that work by default when opened without needing to pull from a relative directory (which is often undesirable, e.g., in the case of github.io pages).

@damianavila

This comment has been minimized.

Copy link
Member

damianavila commented Jan 19, 2018

If we change the current default, we need to tell people who use the speaker notes (which I presume is a big percentage) to change their workflow from:

jupyter nbconvert my_slide.ipynb --to slides --post serve

to

jupyter nbconvert my_slide.ipynb -- to slides --reveal-prefix="reveal.js" --post serve

Otherwise their speaker notes will be busted. Do you think that is acceptable for the users? It is pretty difficult to me to realize if this change is annoying enough to make people angry... it is not annoying for me but I am sure I am biased in my response to this change.
@takluyver @mpacer @fperez thoughts?

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Jan 19, 2018

My thoughts (as above) are that I don't want to try to judge which course creates the bigger inconvenience for the larger group. Rather, I'd apply the status quo argument: I'd prefer to let people live with the current inconvenience for another release than change it and create a different inconvenience. But as the main nbconvert maintainer, @mpacer has the final call on whether this goes in or not.

Longer term, I think we all agree that we need to look more closely at how our slides export deals with external resources. We certainly want a way to produce a single file, and a way to have slides that work completely offline, not pulling anything from a CDN. I'd also like to end the dependence on the serve post-processor (and get rid of the notion of post-processors in nbconvert altogether).

What are the limitations on the speaker notes feature? Do I recall that it only works when served over HTTP (i.e. not from a file:// URL)? Do you know why that is the case?

@damianavila

This comment has been minimized.

Copy link
Member

damianavila commented Jan 19, 2018

What are the limitations on the speaker notes feature? Do I recall that it only works when served over HTTP (i.e. not from a file:// URL)? Do you know why that is the case?

The CDN is not serving the notes.html file: cdnjs/cdnjs#3317
I guess we can serve that specific file (alonside with notes.js) from another place?

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Jan 19, 2018

Do you want to open a separate issue about the speaker notes thing so this discussion doesn't get sidetracked (more than I already sidetracked it ;-)?

@damianavila

This comment has been minimized.

Copy link
Member

damianavila commented Jan 19, 2018

Do you want to open a separate issue about the speaker notes thing so this discussion doesn't get sidetracked (more than I already sidetracked it ;-)?

Good idea!

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Jan 19, 2018

Actually, the more I think about this, the more I agree with M - I think it would be nicer to have slides export produce a file you can use without a special server, even if that breaks speaker notes.

But if the speaker notes just needs the CDN to serve HTML, could we use rawgit as the prefix? I.e. this URL works:

https://cdn.rawgit.com/hakimel/reveal.js/3.5.0/plugin/notes/notes.html

@damianavila

This comment has been minimized.

Copy link
Member

damianavila commented Jan 19, 2018

Actually, the more I think about this, the more I agree with M - I think it would be nicer to have slides export produce a file you can use without a special server, even if that breaks speaker notes.

Yep, this is starting to be my feeling as well... despite the change in the status quo.

But if the speaker notes just needs the CDN to serve HTML, could we use rawgit as the prefix? I.e. this URL works:

I quickly tried on the fly but there is still some other pieces needed, I guess, because I get errors about Reveal.js not defined and Mixed content...

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Jan 19, 2018

Would this work as a way to deal with the offline use case:

When nbconvert produces the file, let all the URLs point to CDNs, so it's a small file that works standalone. Then have a separate tool that can read the HTML file, fetch the necessary JS and CSS, and either place them alongside the file, or stuff them into <script> and <style> tags inside it to produce an offline version. I've just had some fun writing a very simple version of this tool, but it feels like something that lots of other people must have written already.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Jan 19, 2018

I quickly tried on the fly but there is still some other pieces needed, I guess, because I get errors about Reveal.js not defined and Mixed content...

I see those too. The problem is that the speaker notes page is loaded over HTTPS, but then it tries to connect to the local server showing the presentation, which is HTTP. I can work around it for now by changing the rawgit CDN address to http://..., but that will stop working next month.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Jan 19, 2018

You can also work around it in Firefox by temporarily disabling the mixed content protection - click on the padlock icon and find the button - but I don't know if I'd rely on that working either.

@damianavila

This comment has been minimized.

Copy link
Member

damianavila commented Jan 19, 2018

Would this work as a way to deal with the offline use case:

Interesting idea. I think it should work.

I see those too. The problem is that the speaker notes page is loaded over HTTPS, but then it tries to connect to the local server showing the presentation, which is HTTP. I can work around it for now by changing the rawgit CDN address to http://..., but that will stop working next month.

As a quick turnaround we can serve these files, notes.js and notes.html, from the jupyter cdn, right?
We are already serving css from there in https and http: http://cdn.jupyter.org/notebook/5.1.0/style/style.min.css

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Jan 19, 2018

I think the speaker notes should work from localhost if we can get revealjs from any domain where we can rely on it being plain HTTP for some time yet. CC @minrk @rgbkrk

@@ -90,7 +90,7 @@ def _reveal_url_prefix_default(self):
warn("Please update RevealHelpPreprocessor.url_prefix to "
"SlidesExporter.reveal_url_prefix in config files.")
return self.config.RevealHelpPreprocessor.url_prefix
return 'reveal.js'
return 'https://cdnjs.cloudflare.com/ajax/libs/reveal.js/3.1.0'

This comment has been minimized.

@rgbkrk

rgbkrk Jan 19, 2018

Member

Switch this to:

return '//cdnjs.cloudflare.com/ajax/libs/reveal.js/3.1.0'

to provide a URL that the browser will adhere to HTTPS or HTTP for the user's page.

This comment has been minimized.

@takluyver

takluyver Jan 19, 2018

Member

I'm not sure if it will work, because the CDN redirects HTTP requests to HTTPS. Worth a try, though - thanks @rgbkrk

This comment has been minimized.

@damianavila

damianavila Jan 19, 2018

Member

I used:

{ src: "//cdn.rawgit.com/hakimel/reveal.js/3.5.0/plugin/notes/notes.js",

And it seems to work for me... @takluyver can you check that as well?

This comment has been minimized.

@takluyver

takluyver Jan 19, 2018

Member

Nope, it still opens the speaker notes view with an https URL, unfortunately.

Additionally, using // breaks viewing the slides from a file:// URL (i.e. opening the file without running a local http server), whereas that works if we specify the protocol.

This comment has been minimized.

@takluyver

takluyver Jan 19, 2018

Member

@damianavila the rawgit URL works for now, but I think it will stop working next month, when rawgit starts redirecting all HTTP requests to HTTPS.

I tested with the URL @rgbkrk suggested - the notes.html file for speaker view returns a 403, but you can still see that it's opening with an https URL, which is the cause of the 'mixed content' problem.

This comment has been minimized.

@takluyver

takluyver Jan 20, 2018

Member

Right, I forgot - that was what I was asking @rgbkrk and @minrk about: would it be OK to host a copy of reveal.js on cdn.jupyter.org so that we can access it with plain HTTP?

This comment has been minimized.

@minrk

minrk Jan 22, 2018

Member

I'd prefer to pare down the use of cdn.jupyter.org, especially for things that aren't our own in favor of things like unpkg. The only reason for cdn.jupyter I can see is the non-npm packages we own (basically just the notebook-classic js/css). But pointing to unpkg or another CDN for reveal.js ought to work.

BTW, isn't it fine to load https resources on an http page, just not the other way around? Plus, https will work for a file:// page, whereas // will not, so I think explicit https is preferable to //, unless I'm missing something.

This comment has been minimized.

@takluyver

takluyver Jan 22, 2018

Member

The https:// URL works except for the speaker notes. Speaker notes open an HTML file at a URL based on the one reveal is loaded from (e.g. https://cdn.rawgit.com/hakimel/reveal.js/3.5.0/plugin/notes/notes.html ). That page connects back to the server your slides are served from.

So if the notes page is an HTTP URL, it works. But if the notes page is an HTTPS URL and you're serving the slides with plain HTTP, the browser doesn't like the insecure connection.

But it's a good point that it would also be a problem the other way around - we can't load reveal with HTTP if you serve the slides using HTTPS.

So I think we just have to use HTTPS, and say that you have to use a local copy of reveal.js if you want to use speaker notes.

This comment has been minimized.

@minrk

minrk Jan 22, 2018

Member

So I think we just have to use HTTPS, and say that you have to use a local copy of reveal.js if you want to use speaker notes.

👍

This comment has been minimized.

@damianavila

damianavila Jan 22, 2018

Member

So I think we just have to use HTTPS, and say that you have to use a local copy of reveal.js if you want to use speaker notes.

I am OK with that as well after the whole discussion.

@@ -90,7 +90,7 @@ def _reveal_url_prefix_default(self):
warn("Please update RevealHelpPreprocessor.url_prefix to "
"SlidesExporter.reveal_url_prefix in config files.")
return self.config.RevealHelpPreprocessor.url_prefix
return 'reveal.js'

This comment has been minimized.

@damianavila

damianavila Jan 20, 2018

Member

If we finally accept this change, @mpacer you need to point to reveal 3.5.0 which is the version we already have in the serve postprocessor.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Jan 22, 2018

OK, thanks everyone. @mpacer I think the remaining thing to do is to update the URL to 3.5.0, which is the version we refer to elsewhere for reveal.js.

@damianavila

This comment has been minimized.

Copy link
Member

damianavila commented Jan 22, 2018

I think the remaining thing to do is to update the URL to 3.5.0, which is the version we refer to elsewhere for reveal.js.

I would say we should update the docs PR #735 to add the following incantation for those who want to use the speaker notes:

jupyter nbconvert my_slide.ipynb -- to slides --reveal-prefix="reveal.js" --post serve
@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Jan 23, 2018

Good point, let's not forget to update the docs!

@mpacer

This comment has been minimized.

Copy link
Member Author

mpacer commented Jan 23, 2018

Ok I’ll make the change to update the version number later today.

@mpacer

This comment has been minimized.

Copy link
Member Author

mpacer commented Jan 23, 2018

Thanks everyone for hashing this out!

@mpacer

This comment has been minimized.

Copy link
Member Author

mpacer commented Jan 23, 2018

Do you want me to merge the docs PR into this one? so that it's all changed at once?

@damianavila

This comment has been minimized.

Copy link
Member

damianavila commented Jan 23, 2018

Do you want me to merge the docs PR into this one? so that it's all changed at once?

I think it is good idea, but it is @takluyver 's PR so let's wait for his feedback, I think.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Jan 23, 2018

Yup, it's probably a good idea to merge #735 in here, because then the docs need another change as part of this PR, since putting revealjs beside the HTML file is no longer sufficient.

@mpacer mpacer force-pushed the mpacer:better_reveal_prefix branch from b2b5493 to 2f508ca Jan 24, 2018

mpacer added some commits Jan 25, 2018

Improve usage docs for the reveal slideshow
emphasize that the key feature for speaker notes is a local copy.

Give an example of how to set up speaker notes.
@rgbkrk

rgbkrk approved these changes Jan 26, 2018

of reveal.js and then that you redirect the script away from your CDN to your
local copy.

To make this clearer, let's look at an example.

This comment has been minimized.

@rgbkrk

rgbkrk Jan 26, 2018

Member

Excellent

appending the command line flag ``--post serve`` to the above command. This
will not allow you to use speaker notes if you do not have a local copy of
reveal.js.

This comment has been minimized.

@rgbkrk

rgbkrk Jan 26, 2018

Member

Solid prose here

@@ -90,13 +95,14 @@ def _reveal_url_prefix_default(self):
warn("Please update RevealHelpPreprocessor.url_prefix to "
"SlidesExporter.reveal_url_prefix in config files.")
return self.config.RevealHelpPreprocessor.url_prefix
return 'reveal.js'
return 'https://cdnjs.cloudflare.com/ajax/libs/reveal.js/3.5.0'

This comment has been minimized.

@rgbkrk

rgbkrk Jan 26, 2018

Member

All set!

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Jan 26, 2018

Alright, time to press the button on this. Thanks everyone for the discussion - it's a small technical change, but I'm pleased we've taken the time to think through it and make sure the docs are clear. :-)

@takluyver takluyver merged commit 5f5ae49 into jupyter:master Jan 26, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
This will create file ``your_talk.slides.html``, which you should be able to
access with ``open your_talk.slides.html``. To access the speaker notes, press
``s`` after the slides load and they should open in a new window.

This comment has been minimized.

@damianavila

damianavila Jan 26, 2018

Member

Should we mention that the timer inside the speaker notes will only work if you are serving the slides?

This comment has been minimized.

@mpacer

mpacer Jan 26, 2018

Author Member

Ok I wasn’t sure why that was I thought I had a weird bug and that the serving was required to coordinate 2 windows… why does the timer only work if you serve the slides but it still allows you to communicate between 2 windows?

This comment has been minimized.

@damianavila

damianavila Jan 26, 2018

Member

why does the timer only work if you serve the slides but it still allows you to communicate between 2 windows?

Good question! I have not researched the cause yet. I just verified the behavior.

access with ``open your_talk.slides.html``. To access the speaker notes, press
``s`` after the slides load and they should open in a new window.

This should also allow you to use your slides without an internet connection.

This comment has been minimized.

@damianavila

damianavila Jan 26, 2018

Member

Well, this is not true... we are loading other things from the internet, such as font-awesome, jquery, require, mathjax... and reveal.js itself load google fonts (although they have a fallback in case of lack of connectivity.
This statement as is can cause confussion, I would prefer to delete it.

This comment has been minimized.

@mpacer

mpacer Jan 26, 2018

Author Member

I was making that claim based on the previous docs:

To make slides that don't require an internet connection, just place the Reveal.js library in the same directory where your_talk.slides.html is.

So at least this isn’t a regression…

Makes me think we should have a way to package something that we can guarantee works in an offline context (e.g., where we download the relevant files to the directory in question since this is a more people would like to use).

This comment has been minimized.

@damianavila

damianavila Jan 26, 2018

Member

Makes me think we should have a way to package something that we can guarantee works in an offline context (e.g., where we download the relevant files to the directory in question since this is a more people would like to use).

Yep, I was thinking the same, maybe some nbconvert script to download all the files needed?

@damianavila

This comment has been minimized.

Copy link
Member

damianavila commented Jan 26, 2018

@takluyver @mpacer sorry for the post-merge comments, but I think they are enough important to change some things and prevent users confusion.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Jan 26, 2018

@damianavila no problem, there's always room for a bit more review. Do you want to make a PR yourself?

@damianavila

This comment has been minimized.

Copy link
Member

damianavila commented Jan 26, 2018

@damianavila no problem, there's always room for a bit more review. Do you want to make a PR yourself?

Sure, will submit in a few minutes.

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