feat: add support for user comments in presentation rendering#773
feat: add support for user comments in presentation rendering#773mfontanini merged 3 commits intomfontanini:masterfrom
Conversation
mfontanini
left a comment
mfontanini
left a comment
There was a problem hiding this comment.
Sorry, I've been out for a while and this slipped through. A couple of comments but looks good overall!
src/presentation/builder/comment.rs
Outdated
|
|
||
| // Ignore user comments with Comment: prefix (case-insensitive) | ||
| // e.g., <!-- Comment: This is a comment --> | ||
| if trimmed_comment.len() >= 8 { |
There was a problem hiding this comment.
The comment: case is better handled if you add it to the CommentCommand enum, e.g.
enum CommentCommand {
// ....
Comment(String)
}Which is then ignored in the match that handles this. I also don't think making this case insensitive makes sense here given no other comment command is case insensitive.
examples/demo.md
Outdated
|
|
||
| Customizability | ||
| --- | ||
| <!-- Comment: You can add comments like this that won't show up in the presentation --> |
There was a problem hiding this comment.
I wouldn't touch the demo presentation for this feature. The docs are the ultimate guide, this is just here to show the most commonly used features.
|
Hi, this would be a very useful feature. Is there any progress? |
Thanks for the reminder, applied latest changes. Waiting for @mfontanini to confirm and merge. |
There was a problem hiding this comment.
Pull request overview
Adds first-class support for “user comments” in HTML comments so they don’t affect rendered slide spacing, and documents the feature.
Changes:
- Prevents ignored/unparseable HTML comments from introducing an extra rendered line break.
- Adds a
comment:command variant and ignores it during presentation rendering. - Documents supported user comment formats and cleans up a whitespace-only line in the demo markdown.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/presentation/builder/comment.rs |
Implements no-linebreak behavior for ignored comments; adds Comment command parsing and tests. |
examples/demo.md |
Removes a stray whitespace-only line from the demo. |
docs/src/features/commands.md |
Documents user comment formats (// and comment:). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Err(error) => { | ||
| // If we failed to parse this, make sure we shouldn't have ignored it | ||
| if self.should_ignore_comment(comment) { | ||
| // Ignored comments should not add line breaks | ||
| self.slide_state.ignore_element_line_break = true; |
There was a problem hiding this comment.
With command_prefix configured, valid commands that do not start with the prefix can still be parsed/executed because parsing happens before this error branch and only falls back to should_ignore_comment when parsing fails. This contradicts the documented behavior of command_prefix (docs/src/configuration/options.md). Consider enforcing the prefix before attempting to parse (or update docs/tests to reflect the intended semantics).
| if self.should_ignore_comment(comment) { | ||
| // Ignored comments should not add line breaks | ||
| self.slide_state.ignore_element_line_break = true; | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
This fixes the extra newline issue for ignored comments by setting ignore_element_line_break, but there isn’t a regression test asserting the rendered output doesn’t gain a blank line when an ignored comment appears between two rendered elements. Adding a render-based test (e.g., text + ignored comment + text) would help prevent reintroducing this behavior.
| SkipSlide, | ||
| SpeakerNote(String), | ||
| SnippetOutput(String), | ||
| Comment(String), | ||
| } |
There was a problem hiding this comment.
CommentCommand::Comment is now a parseable command, but CommentCommand::generate_samples() (used by --list-comment-commands) still claims to generate samples for “all available commands” and doesn’t include comment. Either add a sample for comment: to keep the listing complete, or adjust the wording/approach so user comments aren’t treated as a command.
src/presentation/builder/comment.rs
Outdated
|
|
||
| // Ignore user comments with // prefix | ||
| // e.g., <!-- // This is a comment --> | ||
| let trimmed_comment = comment.trim_start_matches(&self.options.command_prefix).trim(); |
There was a problem hiding this comment.
Probably should have noticed this before but why the trim_start_matches(command_prefix)? This means you're trying to trim a prefix (say cmd: although an empty string by default) and then trying to see if what you end up with starts with //. This means this code would ignore a command like <!-- cmd: // foo --> and that seems wrong to me.
e.g. test with this presentation:
---
options:
command_prefix: "cmd:"
---
<!-- cmd://hi -->To me this line shouldn't exist and the line below should just check if comment.starts_with("//") since this is already trimmed a few lines above.
src/presentation/builder/comment.rs
Outdated
| // Ignore vim-like code folding tags | ||
| let comment = comment.trim(); | ||
| comment == "{{{" || comment == "}}}" | ||
| if comment == "{{{" || comment == "}}}" { |
There was a problem hiding this comment.
Or just to be concise: this line should just be
comment == "{{{" || comment == "}}}" || comment.starts_with("//")
mfontanini
force-pushed
the
feature/user-comments
branch
from
55ed44c to
bca8dbb
Compare
3 participants
I've found the function
should_ignore_commentand have been relying on it to add user comments inside presentations by abusing the<!-- vim: comment here -->since I don't use vim for this anyways.This isn't clean and a dedicated way to comment should be added IMO. Also, there is an issue with the hacky vim solution above, it adds a newline where the comment should be ignored; fixed by using
self.slide_state.ignore_element_line_break = trueon ignored comments.Current comments:
PR with ignore line breaks:
