-
Notifications
You must be signed in to change notification settings - Fork 567
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
Conversation
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... |
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. |
@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. |
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. |
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. |
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). |
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:
to
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. |
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 |
The CDN is not serving the |
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! |
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 |
Yep, this is starting to be my feeling as well... despite the change in the
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... |
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 |
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 |
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. |
Interesting idea. I think it should work.
As a quick turnaround we can serve these files, |
nbconvert/exporters/slides.py
Outdated
@@ -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' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if it will work, because the CDN redirects HTTP requests to HTTPS. Worth a try, though - thanks @rgbkrk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
I would say we should update the docs PR #735 to add the following incantation for those who want to use the speaker notes:
|
Good point, let's not forget to update the docs! |
Ok I’ll make the change to update the version number later today. |
Thanks everyone for hashing this out! |
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. |
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. |
b2b5493
to
2f508ca
Compare
emphasize that the key feature for speaker notes is a local copy. Give an example of how to set up speaker notes.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All set!
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. :-) |
|
||
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we mention that the timer inside the speaker notes will only work if you are serving the slides?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
@takluyver @mpacer sorry for the post-merge comments, but I think they are enough important to change some things and prevent users confusion. |
@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. |
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.