feat: support speaker notes #389

dmackdev · 2024-11-06T18:06:42Z

Closes #112.

Adds support for speaker notes via via inter-process communication (IPC) using iceoryx2:
Bumps CI rust version to 1.75.0 (for compatibility with iceoryx2).
Adds new speaker notes example presentation.

Speaker notes implementation:

Speaker notes can be added to a presentation markdown file in the form of HTML comments: .
--speaker-notes-mode=publisher CLI option is used to present slides. In this mode, speaker note comments are ignored and not rendered, and every time the slide changes, an IPC message is published.
--speaker-notes-mode=receiver is used to view speaker notes. In this mode, only the title of each slide and speaker note comments are rendered. When retrieving the next command, we first check for an IPC message to change slides.
If --speaker-notes-mode is not specified, no IPC structures are created, no messages are published/listened for, and speaker notes are ignored and not rendered.

Demo:

Screen.Recording.2024-11-19.at.19.21.07.mov

TODO:

Warning Default config file found but unable to read data, populate config with default values. is observed when exiting presenterm, but this has since been fixed on the main branch.

add speaker note CommentCommand push text when processing speaker note comment

…hen the "publisher" presentation changes slides

…e receiver mode

dmackdev

This requires rust 1.75 or newer, but CI runs rust 1.74.
A rust-toolchain file and Cargo.toml entry would help make this more visible.

dmackdev · 2024-11-06T21:27:20Z

Cargo.toml

@@ -36,6 +36,7 @@ thiserror = "1"
 unicode-width = "0.2"
 os_pipe = "1.1.5"
 libc = "0.2.155"
+iceoryx2 = "0.4.1"


This requires rust 1.75 or newer, but CI runs rust 1.74.
A rust-toolchain file and Cargo.toml entry would help make this more visible.

Any thoughts on bumping the rust version @mfontanini?
Otherwise a different version of iceoryx2 (if one exists for rust 1.74) or different IPC crate will need to be used.

Also need to investigate some warnings and proper configuration for usage of this crate.

Go for it, 1.75 is almost a year old already

mfontanini

A few comments mostly on whose responsibility this is but overall looks good!

mfontanini · 2024-11-07T14:36:54Z

examples/code.md

@@ -6,14 +6,13 @@ theme:
      background: false
 ---

-Code styling
-===
+# Code styling


This is not equivalent, I presume this was done by some prettifier? Can you roll back?

Yes, I think my editor extension did this - I will revert this and add a separate example presentation as you suggested, with consistent syntax.

mfontanini · 2024-11-07T14:38:31Z

examples/code.md

@@ -35,10 +34,11 @@ fn main() {
 }
 ```

+<!-- speaker_note: These are speaker notes on slide 1. -->


I'd create a separate example presentation on speaker notes specifically. I think the speaker notes require enough manual intervention to show up that I don't think it belongs in the main demo one.

Started a new speaker notes presentation.

mfontanini · 2024-11-07T14:41:33Z

src/presentation.rs

@@ -274,6 +289,7 @@ pub(crate) type AsyncPresentationErrorHolder = Arc<Mutex<Option<AsyncPresentatio
 pub(crate) struct PresentationStateInner {
    current_slide_index: usize,
    async_error_holder: AsyncPresentationErrorHolder,
+    pub(crate) channel: Option<SpeakerNoteChannel>,


This belongs in the presenter type. The channel itself should be created outside and is not part of the presentation, it's part of "the app". e.g. you shouldn't have to re-set the channel every time the presentation is reloaded.

mfontanini · 2024-11-07T14:44:05Z

src/processing/builder.rs

@@ -139,6 +143,26 @@ impl<'a> PresentationBuilder<'a> {
        bindings_config: KeyBindingsConfig,
        options: PresentationBuilderOptions,
    ) -> Self {
+        let presentation_state = PresentationState::default();
+
+        if let Some(mode) = &options.speaker_notes_mode {


Yeah like mentioned above, this should be handled externally to the presentation. Re-creating (even if it's trying to re-open an existing channel) the channels every time the presentation is built/reloaded doesn't really make sense.

mfontanini · 2024-11-07T14:46:57Z

src/processing/builder.rs

+        if let Some(SpeakerNotesMode::Receiver) = self.options.speaker_notes_mode {
+            match comment {
+                CommentCommand::SpeakerNote(note) => {
+                    self.push_text(note.into(), ElementType::Paragraph);


What's the reason for this to be handled up here separately? Can't you handle SpeakerNote in the match below and do the if let Some inside the match arm?

I initially did this because, for the speaker notes presentation, only these two comments are handled: speaker notes, and end slide. The other comments (e.g. layout, pauses, etc) need not be applied to the speaker notes presentation.

In general, I think my additions to this file need to be refactored.

mfontanini · 2024-11-07T15:03:52Z

src/presenter.rs

@@ -104,6 +115,11 @@ impl<'a> Presenter<'a> {
            self.render(&mut drawer)?;

            loop {
+                if let Some(idx) = self.state.presentation().listen_for_speaker_note_evt() {


I think it would be interesting for this thing to be inside input/source.rs and act as a normal command, e.g. it could return Command::GoToSlide when such an event is found. This would make it more transparent to the presenter.

That would make sense - it is a source of a command after all.

So are you thinking:

Remove the SpeakerNoteChannel enum,

Store the Option<iceoryx2::Listener> directly in CommandSource if viewing speaker notes,

Store the Option<iceoryx2::Notifier> directly in Presenter for the main presentation, if presenting with speaker notes.
?

Yes, exactly 👌

mfontanini · 2024-11-07T15:05:02Z

src/presentation.rs

@@ -292,6 +308,22 @@ impl PresentationState {

    fn set_current_slide_index(&self, value: usize) {
        self.inner.deref().borrow_mut().current_slide_index = value;
+        if let Some(SpeakerNoteChannel::Notifier(notifier)) = &self.inner.deref().borrow().channel {


Following the idea of moving this to presenter, you can always emit this event after a command is handled. That would help detach this logic from out of here.

always notify channel of current slide index after command is applied pass bool flag to builder for rendering speaker notes only

support implicit slide ends in speaker notes mode

dmackdev · 2024-11-08T18:27:29Z

src/processing/builder.rs

+                MarkdownElement::Comment { comment, source_position } => {
+                    self.process_comment(comment, source_position)?
+                }
+                MarkdownElement::SetexHeading { text } => self.push_slide_title(text),


I was thinking showing the slide title would be good for the speaker notes mode to help match it against the title of the current slide in the main presentation.

This also means we can support implicit slide ends in the speaker notes mode for free.

I think it would be good to have a separate process_element for each mode so this one's not polluted with the logic from both. But I agree that having the heading makes sense 👌

I have split this. process_comment could benefit from the same split, but there are extra processing steps/logic on the raw comment string.

mfontanini · 2024-11-09T18:54:06Z

Sorry I'm not having much free time until next week but I'll try to spend a bit of time I see activity in this PR.

mfontanini · 2024-11-09T19:20:16Z

src/presenter.rs

@@ -136,6 +158,10 @@ impl<'a> Presenter<'a> {
                    CommandSideEffect::None => (),
                };
            }
+            if let Some(SpeakerNoteChannel::Notifier(notifier)) = self.speaker_notes_channel.as_mut() {
+                let current_slide_idx = self.state.presentation().current_slide_index();
+                notifier.notify_with_custom_event_id(EventId::new(current_slide_idx + 1)).unwrap();


Did you consider using the interprocess crate instead? I need to read iceoryx2's docs more but this API feels a bit strange. You're communicating via event ids, which is enough for what we're doing but it feels a bit off. (I admit I haven't looked at the docs enough to understand what events mean here).

No hard reason to keep using the event messaging pattern and EventId- iceoryx2 also supports publish/subscribe messaging pattern with different payloads (with restrictions). The examples in the readme/examples dir of the repo show publishing a usize. We could easily use this pattern instead. Semantically, I suppose publish/subscribe would be more appropriate.

I had come across interprocess, but initially chose iceoryx2 since it appeared to be more popular and documented.
I have since tried the interprocess crate and had a quick go at using it in this branch but have not gotten that to work fully yet. Even running the local socket example from interprocess I noticed a few things: the "client" must strictly be started after the "server" otherwise it panics, and you have to manually remove the socket it creates between invocations, otherwise it panics.
So iceoryx2 seems to be more polished in this regard, and I think has a nicer interface.

Fair, interprocess would also require dealing with all the clients which is pretty annoying.

store IPC publisher in Presenter store IPC receiver in CommandSource

dmackdev · 2024-11-11T19:30:47Z

src/processing/builder.rs

@@ -253,7 +263,7 @@ impl<'a> PresentationBuilder<'a> {
        self.push_line_break();
    }

-    fn process_element(&mut self, element: MarkdownElement) -> Result<(), BuildError> {
+    fn process_element_for_presentation_mode(&mut self, element: MarkdownElement) -> Result<(), BuildError> {


Probably needs a better name so as to not be confused with PresentMode.

dmackdev · 2024-11-11T19:35:53Z

Is there any reason why nightly toolchain is used for cargo fmt/cargo clippy in CI?

mfontanini · 2024-11-14T23:41:32Z

Looking at iceoryx2 I think you should really be using the pub/sub mode. The event one you're using doesn't make sense to me, like I said it's communicating via event ids which only works here because you're passing in a number but it won't work if this needs to be changed in the future.

What I'd suggest is the following:

Use pubsub mode. This shouldn't require many changes to what you have.
Send a specific enum SpeakerNotesCommand type using serde_json to serialize it to JSON. This type should have a single variant for now (GoToSlide or something) that contains a slide number. This will allow changing this in the future in case we want more types of commands to be sent. Actually typing this out now.... I wonder if this should just send commands directly and not have the listener parse the presentation at all. e.g. just send a {"slide_title": "abc", "note": "this is where I talk about abcs"} and let the other side display it. This would simplify the logic a lot since you wouldn't need to mix the listener into the Presenter type, but it would also require restructuring things quite a bit.
Change the way the creation of each side of the pipe works:
- The pipe/service name should contain some specific identifier so you can open more than one presentation at the same time. e.g. presenterm test.md --publish-speaker-notes my-service.
- Creating the pipe on the publisher mode shouldn't use open_or_create, you want to explicitly create it and if it's open already it should fail (I expect this to be what that crate does). Otherwise having 2 presenterm instances running in publisher mode open will break or misbehave.
- Creating the pipe on the listener mode should use open specifically, not open_or_create. I think what you mentioned for interprocess about the publisher needing to be up before the listener is fine, that's how it should be. I shouldn't be allowed to open the listener if no presentations are running in publish mode.

PS: the formatting needs nightly because most of the useful cargo formatting rules used here are nightly only.

dmackdev · 2024-11-16T12:56:47Z

I have now updated the code to use the pubsub messaging pattern.
There are some restrictions on data types that can be sent in IPC messages with iceoryx2 - see here. Namely, "Data types like String or Vec will cause undefined behavior and may result in segmentation faults". So it would appear that heap allocated data (like serialised JSON strings) would require use of a fixed-sized container, which could be problematic. The current enum and its single variant that I have introduced SpeakerNoteCommand::GoToSlide(u32) works fine however, since it complies with the restrictions.
I have changed the pipe creation as suggested, and it works as expected - creating a duplicate publisher panics, and creating a listener before a publish also panics. For the service name, do you not think it would be a better user experience to infer this from the presentation file name/path, rather than requiring a user to specify a service name?

mfontanini · 2024-11-16T20:31:05Z

I see, makes sense, I forgot this uses shared memory so there really isn't a reason to serialize to JSON anyway. I think if there's a need to use strings in the future they can always be self contained [char; N] types. It won't be super flexible but flexible enough.

re: naming makes sense as well, not sure if there are restrictions on the length of the service name. Using the file name should be enough. e.g. presenterm/demo.md could be the name of the service if I run it like presenterm examples/demo.md.

dmackdev · 2024-11-17T15:42:02Z

Updated the service name to incorporate the presentation file name.

There does not appear to be any length restrictions on the service name, only that it cannot be empty (see here).

From my side, I need to address a few unwraps for better error handling, which I can do during the week.
There is also the outstanding misleading warning from iceoryx2 which is fixed on master, but is as yet unreleased.

mfontanini

Just a few comments mostly on error handling. I noticed when you run it it prints

        2 [W] "Config::global_config()"
              | Default config file found but unable to read data, populate config with default values.
```

Do you see this too? Not sure where it's coming from.

mfontanini · 2024-11-20T13:49:11Z

src/main.rs

+fn create_speaker_notes_service_builder(
+    presentation_path: &Path,
+) -> Result<Builder<SpeakerNotesCommand, (), Service>, Box<dyn std::error::Error>> {
+    let file_name = presentation_path.file_name().expect("failed to resolve presentation file name").to_string_lossy();


Can you not panic here and return an error instead?

I've propagated a Cli::command().error(...) here instead.

mfontanini · 2024-11-20T13:57:58Z

src/main.rs

+    presentation_path: &Path,
+) -> Result<Builder<SpeakerNotesCommand, (), Service>, Box<dyn std::error::Error>> {
+    let file_name = presentation_path.file_name().expect("failed to resolve presentation file name").to_string_lossy();
+    let service_name = format!("presenterm/{}", file_name).as_str().try_into()?;


format!("presenterm/{file_name}")

mfontanini · 2024-11-20T14:05:56Z

src/main.rs

@@ -270,9 +305,21 @@ fn run(mut cli: Cli) -> Result<(), Box<dyn std::error::Error>> {
            println!("{}", serde_json::to_string_pretty(&meta)?);
        }
    } else {
-        let commands = CommandSource::new(config.bindings.clone())?;
+        let speaker_notes_event_receiver = if let Some(SpeakerNotesMode::Receiver) = cli.speaker_notes_mode {
+            let receiver = create_speaker_notes_service_builder(&path)?.open()?.subscriber_builder().create()?;


This should fail with a nice looking error. I currently only see

PublishSubscribeOpenError::DoesNotExist

When I run the receiver before the publisher. Maybe use a Cli::command().error(...) like above in this file with a meaningful error message like "no presenterm in publisher mode running" or something like that.

Same for the publisher failing when the channel is already open.

Agreed.
There are several different error enums - each with multiple variants - involved in this chain of method calls, which makes this tricky to nicely isolate these particular variants.

I'll see what I can do.

mfontanini · 2024-11-20T14:09:03Z

src/input/speaker_notes.rs

@@ -0,0 +1,5 @@
+#[derive(Debug)]
+#[repr(C)]
+pub enum SpeakerNotesCommand {


Maybe there could be an Exit command sent when presenterm exits? That way they both would close simultaneously.

dmackdev · 2024-11-20T20:05:19Z

Yes I have been seeing that warning - I had mentioned it in the PR description.
It's since been fixed on the iceoryx2 main branch, but would come in the next release presumably.

I think it would be a blocker for this PR since it is not an ideal user experience - any suggestions?

…tes presentation

mfontanini · 2024-11-21T23:51:42Z

Sorry, I saw that when you created the PR but forgot when trying it out.

I think it would be a blocker for this PR since it is not an ideal user experience - any suggestions?

Yyyyyeaaah I agree, I hope there's a release out soon. Do you have a link to the PR where this was removed? Depending on what they use we can potentially null out that output.

dmackdev added 3 commits November 4, 2024 21:02

add speaker notes mode cli option

de42655

add speaker note CommentCommand push text when processing speaker note comment

use ipc to navigate to specfic slides in speaker notes presentation w…

fd6d57b

…hen the "publisher" presentation changes slides

process only speaker notes and end slide comments when in speaker not…

3151fbd

…e receiver mode

dmackdev mentioned this pull request Nov 6, 2024

Support for speaker notes #112

Open

dmackdev commented Nov 6, 2024

View reviewed changes

fix layout on speaker note slides

cbadf07

mfontanini reviewed Nov 7, 2024

View reviewed changes

dmackdev added 4 commits November 8, 2024 17:18

move SpeakerNotesChannel to Presenter from PresentationStateInner

93a0dcb

always notify channel of current slide index after command is applied pass bool flag to builder for rendering speaker notes only

restore original examples/code.md

22d70f7

add speaker notes example presentation

4c7726e

show titles in speaker notes mode

df051d3

support implicit slide ends in speaker notes mode

dmackdev commented Nov 8, 2024

View reviewed changes

remove ipc event service duplication

81cfbbc

dmackdev requested a review from mfontanini November 8, 2024 19:32

mfontanini reviewed Nov 9, 2024

View reviewed changes

dmackdev added 4 commits November 11, 2024 18:04

remove SpeakerNoteChannel enum

4416af8

store IPC publisher in Presenter store IPC receiver in CommandSource

bump CI rust version to 1.75.0

5f781ac

Merge remote-tracking branch 'origin/master' into speaker-notes

0fd812e

split process_element for speaker notes and presentation modes

0f83b5a

dmackdev commented Nov 11, 2024

View reviewed changes

dmackdev added 2 commits November 16, 2024 10:52

fix formatting

dc0e039

use pubsub ipc messaging pattern instead of event

27a6151

incorporate presentation file name in ipc service name

43b44c5

dmackdev added 3 commits November 19, 2024 18:55

split process comment command for speaker notes mode

ddbaf41

propagate iceoryx2 publisher errors

3114df9

propagate iceoryx2 receiver error

52d3f04

dmackdev marked this pull request as ready for review November 19, 2024 19:24

mfontanini reviewed Nov 20, 2024

View reviewed changes

dmackdev added 3 commits November 21, 2024 17:04

send new SpeakerNotesCommand::Exit ipc message to exit the speaker no…

13e2d46

…tes presentation

use string interpolation for ipc service name

2964ac9

propagate error instead of expect

209cd10

feat: support speaker notes #389

Are you sure you want to change the base?

feat: support speaker notes #389

Conversation

dmackdev commented Nov 6, 2024 • edited Loading

Speaker notes implementation:

Demo:

TODO:

dmackdev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfontanini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmackdev Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

mfontanini commented Nov 9, 2024

Choose a reason for hiding this comment

dmackdev Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

dmackdev Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmackdev commented Nov 11, 2024

mfontanini commented Nov 14, 2024

dmackdev commented Nov 16, 2024

mfontanini commented Nov 16, 2024

dmackdev commented Nov 17, 2024 • edited Loading

mfontanini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmackdev Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmackdev commented Nov 20, 2024 • edited Loading

mfontanini commented Nov 21, 2024

dmackdev commented Nov 6, 2024 •

edited

Loading

dmackdev Nov 11, 2024 •

edited

Loading

dmackdev Nov 14, 2024 •

edited

Loading

dmackdev Nov 14, 2024 •

edited

Loading

dmackdev commented Nov 17, 2024 •

edited

Loading

dmackdev Nov 21, 2024 •

edited

Loading

dmackdev commented Nov 20, 2024 •

edited

Loading