damianavila / RISE

RISE: "Live" Reveal.js Jupyter/IPython Slideshow Extension
Other
3.67k stars 416 forks source link

returning to slideshow mode slightly broken under 5.6.0-dev3 #504

Closed parmentelat closed 5 years ago

parmentelat commented 5 years ago

with the devel release 5.6.0-dev3:

returning to slideshow mode after leaving it causes the selected slide to show up misplaced; most of the time it is way below its expected position

to work around that, it's enough to make any kind of a move, like going one slide back and forward; or to click twice on the 'w' key

this does not happen when entering slideshow mode for the first time, only when quitting and returning; slide selection is not affected and works as expected

parmentelat commented 5 years ago

I have been investigating this, and have experimentally determined that a call to Reveal.sync() is enough to put things back in order and have everything displayed at the right location

specifically, I could bind a keyboard event to Reveal.sync() and check that just pressing that key would fix the display

Everything points at a small regression between the 2 versions of reveal (3.5.0 and 3.8.0 IIRC)

I have not however been able to fix the issue properly, and I actually have a rather specific question for @damianavila; in main.js there are a few mentions of a Reveal variable; this of course references the reveal.js module, but at this point I can't for the hell of me figure out how this binding happens.

The other kind of globals in the code, like Jupyter, are bound to corresponding module with that toplevel define(), that much I do get;

But it comes to Reveal I can see no sort of a declaration…

To make things even worse, in some functions inside main.js, Reveal actually is defined, but in some others it's not defined at all and raises ReferenceError: Reveal is not defined

I tried many different approaches to figure out some logic here; I played with the idea that Reveal would be defined inside the require bloc https://github.com/damianavila/RISE/blob/maint-5.6/rise/static/main.js#L503 but even that does not seem to work;

I thought I had understood a few things about JS but that one beats me. So I could really use some hints here; What I'd need primarily is to add a call to Reveal.sync() in revealMode() circa this line https://github.com/damianavila/RISE/blob/maint-5.6/rise/static/main.js#L1052

mind you, it might not the exact right thing to do, it's the general method for accessing the reveal module that I need to clarify here

I'd love to fix this quickly as I am in heavy writing mode and am constantly switching back and forth between classic and slideshow mode, and this is a curse ;( so the sooner I can nail this down the better !

damianavila commented 5 years ago

Sorry for the delay @parmentelat. A lot of stuff going on...

OK, coming to your question... you are loading Reveal with this require: https://github.com/damianavila/RISE/blob/dc44d849aeb0dc42e01edb2e738fd81c356b7c4d/rise/static/main.js#L517-L522, as you mentioned above.

You can see that we are even using Reveal.sync here: https://github.com/damianavila/RISE/blob/dc44d849aeb0dc42e01edb2e738fd81c356b7c4d/rise/static/main.js#L415.

We are using that Reveal.sync inside the setupOutputObserver function which is actually called inside that require block here: https://github.com/damianavila/RISE/blob/dc44d849aeb0dc42e01edb2e738fd81c356b7c4d/rise/static/main.js#L643

So, if you want to use Reveal.sync in https://github.com/damianavila/RISE/blob/dc44d849aeb0dc42e01edb2e738fd81c356b7c4d/rise/static/main.js#L361, you should see Reveal defined there because you are calling that function here: https://github.com/damianavila/RISE/blob/dc44d849aeb0dc42e01edb2e738fd81c356b7c4d/rise/static/main.js#L646`

There should not be a need for that set_timeout... in fact, you are using Reveal.slide in that very same function, a few lines above.

What I'd need primarily is to add a call to Reveal.sync() in revealMode() circa this line https://github.com/damianavila/RISE/blob/maint-5.6/rise/static/main.js#L1052

It is not clear to me from that link where you actually need to use Reveal.sync, I suggest you use permalinks, otherwise, the references get lost after changing the branch :wink:

parmentelat commented 5 years ago

Hey Damian

Thanks for your answer; and sorry for the moving links, I’ll try to remember that

First off I did add this timeout eventually after messing about for some time; I will admit it is not nice, at all, but it does the trick for me, so as of 5.6.dev5 that I issued yesterday, the symptom is no longer there

I ‘d be interested to understand the root cause for the issue some day :)

Now in terms of my question, I still fail to understand why the javascript variable that is bound to the module is called Reveal and not foobar; in the initial call to define we get to bind e.g. Jupyter when we load that module, I don’t see the same mechanism with Reveal What am I missing?

parmentelat commented 5 years ago

Oh and btw I reiterate that we should issue 5.6 as the official release, especially as this was the last usability bug on the way, if you can please give it a thorough test

damianavila commented 5 years ago

Thanks for your answer; and sorry for the moving links, I’ll try to remember that

No problem!!

I ‘d be interested to understand the root cause for the issue some day :)

😉

What am I missing?

The Reveal object is returned at the end of reveal.js https://github.com/hakimel/reveal.js/blob/master/js/reveal.js#L6026. When you require reveal.js here: https://github.com/damianavila/RISE/blob/master/rise/static/main.js#L503, you make that object available inside that function.

damianavila commented 5 years ago

Oh and btw I reiterate that we should issue 5.6 as the official release, especially as this was the last usability bug on the way, if you can please give it a thorough test

I will test it during the weekend and we may release it after that, thoughts?

parmentelat commented 5 years ago

yes please, that would be great :)

damianavila commented 4 years ago

yes please, that would be great :)

I did not forget about this... just trying to find one hour to do this right... but I was not able to find that hour yet ;-)