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

Notes: Introduce alternate pacing timer, based on total presentation time #2400

Merged
merged 2 commits into from
May 27, 2019

Conversation

fghaas
Copy link
Contributor

@fghaas fghaas commented May 9, 2019

The current pacing timer, introduced in 715cf0b (#1564), operates on the assumption that there is a default amount of time to be allocated to each slide, and that individual slides can deviate from that default by specifying their own data-timing attribute.

This patch introduces an alternate pacing method: by specifying the totalTime configuration option, the presenter can set the total time available to present. The pacing timer will then continue to allocate the exact pacing time for slides that do have data-timing set, as before. However, rather than applying the defaultTiming constant to all others, it will

  • Add up the time already allocated to slides via data-timing;
  • subtract that from totalTime;
  • divide the difference by the number of slides without data-timing set;
  • apply the thus-calculated average to those slides.

totalTime has no default, and if both defaultTiming and totalTime are set, totalTime wins. This preserves backward compatibility: if a presenter has set defaultTiming and updates reveal.js, totalTime will be null and defaultTiming is still applied to all slides without a data-timing attribute. The presenter can then switch to the automatic calculation, if desired, by setting a value for totalTime.

@fghaas
Copy link
Contributor Author

fghaas commented May 9, 2019

@aspiers, since you're the original author of this feature, would you be so kind to take a look and see if you think this is useful?

@aspiers
Copy link
Contributor

aspiers commented May 9, 2019

Brilliant idea, I love it!

Couple of thoughts:

  • It would still be backwards-compatible even if totalTime overrode defaultTiming when both are set, because older decks would not have totalTime set and therefore not change in behaviour when upgraded to newer reveal.js. But the precedence you propose is also fine at first thought.

  • We could go even further (in the future, not in this PR), and dynamically and proportionally adapt timings as the presentation progresses, in order to "home in" on the desired completion time? For example if there are 25 minutes worth of slides left, but you're running behind and only have 20 minutes left, it could dynamically shave 20% off each slide in order to try to help you get back on track. OTOH ...

    • This could be confusing.
    • You might or might not want it to add time if you get ahead of your plans.
    • If you temporarily jumped backwards to quickly show an older slide (e.g. if someone in the audience asks a question about one), then the timing would temporarily look very scary until you returned to your original location. This could be mitigated by keeping track of the furthest on slide shown so far, and dynamically calculating timings based on that rather than on the currently shown slide.

@fghaas
Copy link
Contributor Author

fghaas commented May 10, 2019

@aspiers Thanks for looking into this!

It would still be backwards-compatible even if totalTime overrode defaultTiming when both are set, because older decks would not have totalTime set and therefore not change in behaviour when upgraded to newer reveal.js.

That's a good point.

We could go even further (in the future, not in this PR), and dynamically and proportionally adapt timings as the presentation progresses, in order to "home in" on the desired completion time?

Right, in the future; I'll leave the elliptic-curve implementation involving lambda calculus to you. ;)

I do have a couple of lower-hanging fruit though that I might still want to fix, after sleeping on this for a night:

  • The totalTime approach can presently result in an arbitrarily short calculated timing, including one that is negative. That is clearly not good. I'm thinking about adding another config option that sets the minimum time-per-slide, in seconds,¹ and using alert() to notify the speaker if any slide is below that.
  • When setting totalTime, having an elapsed time counter that counts forwards is a bit silly; it should probably be a countdown timer counting backwards instead. But I don't know what's a good and intuitive behaviour for clicking the timer, then. When you introduced defaultTiming, you made the fine choice of altering the behavior from "click means start over" to "click means right on track," and I wonder what's the good thing to do in the totalTime case. Does click mean "OK so it's total time from now" (meaning the end time is later), or does it mean "on track" (meaning the end time is also later, but not by as much)?

¹ What's a good default? Probably 0, because people might use Lessig style slide decks and go really really fast.

README.md Outdated Show resolved Hide resolved
@fghaas fghaas force-pushed the alternate-timing branch from 9b5e04d to 5bf78d2 Compare May 12, 2019 11:32
@fghaas
Copy link
Contributor Author

fghaas commented May 12, 2019

@aspiers: patch updated.

  • Followed your suggestion about configuration defaults.
  • Implemented a very weak "enforcement" of minimum time-per-slide. Currently uses console.warn(), which the presenter will probably not look at. Considered using alert() but that's probably too invasive, and whether or not it makes sense to include this information in the pacing timer window I'm not sure. Thoughts?

@aspiers
Copy link
Contributor

aspiers commented May 12, 2019

@fghaas commented on May 10, 2019 8:52 AM:

I do have a couple of lower-hanging fruit though that I might still want to fix, after sleeping on this for a night:

  • The totalTime approach can presently result in an arbitrarily short calculated timing, including one that is negative. That is clearly not good. I'm thinking about adding another config option that sets the minimum time-per-slide, in seconds,¹ and using alert() to notify the speaker if any slide is below that.

Makes sense.

  • When setting totalTime, having an elapsed time counter that counts forwards is a bit silly; it should probably be a countdown timer counting backwards instead. But I don't know what's a good and intuitive behaviour for clicking the timer, then. When you introduced defaultTiming, you made the fine choice of altering the behavior from "click means start over" to "click means right on track," and I wonder what's the good thing to do in the totalTime case. Does click mean "OK so it's total time from now" (meaning the end time is later), or does it mean "on track" (meaning the end time is also later, but not by as much)?

IIUC the former feels like a regression in behaviour, and would make decreasing amounts of sense the further through the deck you are when you click. If totalTime is set to 30 and you click with only two slides to go neither of which have data-timing set, that's going to allocate 15 minutes per slide, right? Which doesn't provide any obvious value. OTOH if you were originally given 30 minutes for the slot but you use that all up with two slides remaining, and the event organiser at the back of the room says "don't worry, you can have 5 more minutes", then the latter approach would mean that clicking gives you either 2 or 2.5 minutes per slide depending on how you calculate it, and both are much more useful. Or did I misunderstand something?

¹ What's a good default? Probably 0, because people might use Lessig style slide decks and go really really fast.

Well, 0 seconds per slide is infinitely fast, so that doesn't make much sense to me ;-) I'd suggest 2 seconds per slide as a minimum, since that gives a speaker just about enough time to load a slide (including heavy images etc.) and make a split-second decision to skip it.
@fghaas commented on May 12, 2019 12:37 PM:

@aspiers: patch updated.

  • Followed your suggestion about configuration defaults.

I thought I only made an observation rather than any helpful suggestion, but OK :-)

  • Implemented a very weak "enforcement" of minimum time-per-slide. Currently uses console.warn(), which the presenter will probably not look at.

Yeah IMHO that's too hidden.

Considered using alert() but that's probably too invasive

If it always appears as soon as the deck loads then shouldn't this be fine, assuming that any sane presenter will try loading their deck at least once before the actual live presentation?

But even if that still worries you for some reason:

and whether or not it makes sense to include this information in the pacing timer window I'm not sure. Thoughts?

Yes, I think that could be an OK place for it, maybe even in addition to the alert(). I think there could be presenters who forget to open the speaker notes window (or at least pay close attention to it) until the actual live presentation.

Either way, the important thing is to phrase the warning in a way which makes it clear how to fix the issue.

@fghaas
Copy link
Contributor Author

fghaas commented May 12, 2019

@aspiers Having run through some talk rehearsals using this, I have come to the conclusion that the click behaviour needs no changes at all. It's perfectly useful as-is, particularly during practice runs.

If it always appears as soon as the deck loads then shouldn't this be fine, assuming that any sane presenter will try loading their deck at least once before the actual live presentation?

No it wouldn't appear right when the deck loads but when you open the speaker view — but your comment still applies; any sane speaker would rehearse with notes at least once before presenting.

…time

The current pacing timer operates on the assumption that there is
a default amount of time to be allocated to each slide, and that
individual slides can deviate from that default by specifying their
own data-timing attribute.

This patch introduces an alternate pacing method: by specifying
the totalTime configuration option, the presenter can set the total
time available to present. The pacing timer will then continue to
allocate the exact pacing time for slides that do have data-timing
set, as before. However, rather than applying the defaultTiming
constant to all others, it will

- Add up the time already allocated to slides via data-timing;
- subtract that from totalTime;
- divide the difference by the number of slides without data-timing set;
- apply the thus-calculated average to those slides.

totalTime has no default, and if both defaultTiming and totalTime are
set, totalTime wins. This preserves backward compatibility: if a
presenter has set defaultTiming and updates reveal.js, totalTime will
be null and defaultTiming is still applied to all slides without a
data-timing attribute. The presenter can then switch to the automatic
calculation, if desired, by setting a value for totalTime.
@fghaas fghaas force-pushed the alternate-timing branch from 5bf78d2 to f416c6b Compare May 12, 2019 18:10
plugin/notes/notes.html Outdated Show resolved Hide resolved
@aspiers
Copy link
Contributor

aspiers commented May 13, 2019

@fghaas commented on May 12, 2019 3:54 PM:

@aspiers Having run through some talk rehearsals using this, I have come to the conclusion that the click behaviour needs no changes at all. It's perfectly useful as-is, particularly during practice runs.

Cool :-)

If it always appears as soon as the deck loads then shouldn't this be fine, assuming that any sane presenter will try loading their deck at least once before the actual live presentation?

No it wouldn't appear right when the deck loads but when you open the speaker view — but your comment still applies; any sane speaker would rehearse with notes at least once before presenting.

Ah OK, that sounds fine: if they don't rehearse with notes then I guess they aren't planning to use them at all, in which case timing is irrelevant anyway.

@fghaas
Copy link
Contributor Author

fghaas commented May 13, 2019

Ah OK, that sounds fine: if they don't rehearse with notes then I guess they aren't planning to use them at all, in which case timing is irrelevant anyway.

Completely agree.

@fghaas
Copy link
Contributor Author

fghaas commented May 14, 2019

@hakimel Courtesy ping here, in case you want to consider merging this. Thank you!

@hakimel
Copy link
Owner

hakimel commented May 16, 2019

This is great! Will test it out and merge soon.

When using the totalTime-based pacing calculation, a presenter may
inadvertently set totalTime and per-slide data-timing attributes in
such a way that the pacing time for some slides is impossibly low or
even negative.

Add a check to ensure that the pacing on a slide never falls below a
configurable minimum, defaulting to 0. Display an alert if the
pacing for any slide(s) falls below the threshold.
@fghaas fghaas force-pushed the alternate-timing branch from f416c6b to 23c12d7 Compare May 22, 2019 19:44
@fghaas
Copy link
Contributor Author

fghaas commented May 22, 2019

@hakimel, I added one more force-push that removes the word "calculated" from the pop-up alert, because the alert does (intentionally) also complain if any data-timing attribute on a slide is under minimumTimePerSlide:

diff --git a/plugin/notes/notes.html b/plugin/notes/notes.html
index ac6b11d..fdb81ad 100644
--- a/plugin/notes/notes.html
+++ b/plugin/notes/notes.html
@@ -577,7 +577,7 @@
                                                        }
                                                        var slidesUnderMinimum = timings.filter( function(x) { return (x < minTimePerSlide) } ).length
                                                        if ( slidesUnderMinimum ) {
-                                                               message = "The calculated pacing time for " + slidesUnderMinimum + " slide(s) is under the configured minimum of " + minTimePerSlide + " seconds. Check the data-timing attribute on individual slides, or consider increasing the totalTime or minimumTimePerSlide configuration options (or removing some slides).";
+                                                               message = "The pacing time for " + slidesUnderMinimum + " slide(s) is under the configured minimum of " + minTimePerSlide + " seconds. Check the data-timing attribute on individual slides, or consider increasing the totalTime or minimumTimePerSlide configuration options (or removing some slides).";
                                                                alert(message);
                                                        }
                                                        callback( timings );

@hakimel hakimel merged commit 23c12d7 into hakimel:dev May 27, 2019
@hakimel
Copy link
Owner

hakimel commented May 27, 2019

Merged! Thanks for a great PR @fghaas.

I recently added support for total time to the slides.com speaker view as well. My goal is to bring over some of that design to the reveal.js speaker view. More specifically;

  • Count-up timer turns into a countdown if a total time is provided.
  • Showing presentation progress vs remaining time as visual bars makes it easy to see where you're at, at a glance.
  • Timer can be paused/reset.

fghaas added a commit to fghaas/cephalocon2019-rbdmirror that referenced this pull request May 27, 2019
Now that hakimel/reveal.js#2400 has landed, this no longer needs to
point to my personal fork.
fghaas added a commit to fghaas/cephalocon2019-lightningtalk that referenced this pull request May 27, 2019
Now that hakimel/reveal.js#2400 has landed, this no longer needs to
point to my personal fork.
@fghaas fghaas deleted the alternate-timing branch May 27, 2019 11:14
@fghaas
Copy link
Contributor Author

fghaas commented May 27, 2019

@hakimel & @aspiers, thanks to both of you!

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.

3 participants