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

RevealJS: Specifiyng engine introduces spurious title slide. #8012

Open
Walser52 opened this issue Dec 22, 2023 · 7 comments · May be fixed by #8016
Open

RevealJS: Specifiyng engine introduces spurious title slide. #8012

Walser52 opened this issue Dec 22, 2023 · 7 comments · May be fixed by #8016
Assignees
Labels
bug Something isn't working jupyter revealjs Issues with the revealjs format
Milestone

Comments

@Walser52
Copy link

Walser52 commented Dec 22, 2023

Bug description

I want a revealJS presentation with no title slide. Removing the title and author info from the yaml header works but not if there is python code in there. The problem does not occur when I change the python code with R.

Steps to reproduce

---
format:
    revealjs:
        engine: jupyter            
editor: source
---

## {.center}

::: columns
::: {.column width="65%"}
[**Title: Can't remove title slide after adding python**]{style="color:teal"}

[A headache]{style="color:gray"}
:::

::: {.column .left width="35%"}
**Author's Name**

Affiliation
:::
:::

<!-- <div class="vl"></div> -->



## Python slide

```{python}
print(2+2)
```


Expected behavior

The second slide should be the first one:

image

Actual behavior

There is a spurious title slide with {.center} in the middle.

image

This evidently comes from my second slide. If I remove that, then 'Python Slide' becomes the title i.e. this:

---
format:
    revealjs:
        engine: jupyter            
editor: source
---

## 

:::: columns
::: {.column width="65%"}
[**Title: Can't remove title slide after adding python**]{style="color:teal"}

[A headache]{style="color:gray"}
:::

::: {.column .left width="35%"}
**Author's Name**

Affiliation
:::
::::

<!-- <div class="vl"></div> -->



## Python slide

```{python}
print(2+2)
```

gives the following title slide:

image

Your environment

Version: 1.77.1
Commit: b7886d7461186a5eac768481578c1d7ca80e2d21
Date: 2023-04-04T23:20:37.202Z
Electron: 19.1.11
Chromium: 102.0.5005.196
Node.js: 16.14.2
V8: 10.2.154.26-electron.0
OS: Linux x64 5.15.0-69-generic
Sandboxed: No

Quarto check output

[✓] Checking versions of quarto binary dependencies...
Pandoc version 3.1.1: OK
Dart Sass version 1.55.0: OK
[✓] Checking versions of quarto dependencies......OK
[✓] Checking Quarto installation......OK
Version: 1.3.450
Path: /opt/quarto/bin

[✓] Checking basic markdown render....OK

[✓] Checking Python 3 installation....OK
Version: 3.9.13
Path: /home/fes33/Documents/GIK - R&D/Personal - Papers and Reports/10 - Thesis/_Presentation/env/bin/python
Jupyter: 5.5.0
Kernels: python3, julia-1.9

[✓] Checking Jupyter engine render....OK

[✓] Checking R installation...........OK
Version: 4.1.2
Path: /usr/lib/R
LibPaths:
- /home/fes33/R/x86_64-pc-linux-gnu-library/4.1
- /usr/local/lib/R/site-library
- /usr/lib/R/site-library
- /usr/lib/R/library
knitr: 1.43
rmarkdown: 2.22

[✓] Checking Knitr engine render......OK

@Walser52 Walser52 added the bug Something isn't working label Dec 22, 2023
@cderv
Copy link
Collaborator

cderv commented Dec 22, 2023

This is a very bad side effect of a Jupyter 'feature' because you have set no title in your YAML

When there is no title in a document with Jupyter, the first header will be taken as a title in some condition. See related issue:

This was fixed in 24672a4 and commit message gives insights

We promote headings to notebook titles as this is a common pattern in ipynbs. Reducei the footprint where we do this to stop capturing so many headings.

Now we check on the first markdown cell and only do the promotion if there is no title specified and there is no content before the heading.

Providing a title: solves this

If you plan on having your title slide with .center as the first slide, then you'll need to wait for a fix here.

We need two things I believe

cc @dragonstyle

Thanks for the report !

@cderv cderv added this to the v1.4 milestone Dec 22, 2023
@cderv cderv added revealjs Issues with the revealjs format jupyter labels Dec 22, 2023
@cderv cderv linked a pull request Dec 22, 2023 that will close this issue
@cderv
Copy link
Collaborator

cderv commented Dec 22, 2023

@dragonstyle we get the {.center} that is passed in the title because the parsePandocTitle() does not account for empty header. So the attribute part is not match.

This should be the change in regex

diff --git a/src/core/pandoc/pandoc-partition.ts b/src/core/pandoc/pandoc-partition.ts
index b02216553..57459eb15 100644
--- a/src/core/pandoc/pandoc-partition.ts
+++ b/src/core/pandoc/pandoc-partition.ts
@@ -18,7 +18,7 @@ export function firstHeadingFromMarkdown(markdown: string): string | undefined {
   return partitioned.headingText;
 }

-const kPandocTitleRegex = /^\#{1,}\s(.*)\s\{(.*)\}$/;
+const kPandocTitleRegex = /^\#{1,}\s(?:(.*)\s)?\{(.*)\}$/;
 const kRemoveHeadingRegex = /^#{1,}\s*/;

 export function parsePandocTitle(title: string) {

If we do that we get the correct fix behavior it seems because empty header does not create a new title slide alone.

However, it does not solve the problem that catching a header in revealjs presentation will separate from its content with

  • A title slide with the header
  • The content below in its own slide without header
---
format:
  revealjs:
    engine: jupyter            
---

## My custom title slide {.center}

Content

## Python slide

```{python}
1 + 1
```

gives this slide overview

image

IMO not good. From our discussion in #8016 I understand we get header inside document for a reason, but in revealjs context, this is not ok as header means section, and it relates with its content.

@cderv cderv added the needs-discussion Issues that require a team-wide discussion before proceeding further label Dec 22, 2023
@dragonstyle dragonstyle removed their assignment Jan 2, 2024
@cderv
Copy link
Collaborator

cderv commented Jan 4, 2024

#8018 has solved the issue with doing

## {.center}

But there is still the issue of stripping the first header when no title, as it will split the initial slide in two.

@knuesel
Copy link
Contributor

knuesel commented Jan 4, 2024

I'd like to make the case for simply dropping this title promotion feature. Does it carry its own weight in terms of cost vs value?

It keeps on giving troubles, with differences of behavior between kernels, or even with the same kernel when code is executed vs not executed, and now maybe different behaviors for different output formats.

Then there's the complexity in the code due to this feature. From working on #7159 I got the impression that it's a lot.

Even from the user perspective and assuming zero bugs, it's arguably more valuable to have simple rules and consistent behavior across kernels and formats.

And it's even partially redundant: For the particular use case of quarto render existing_notebook.ipynb we already have this:

quarto render existing_notebook.ipynb -M shift-heading-level-by=-1

which will promote an h1 heading to document title. It's consistent with pandoc, and explicit rather than "magically happens sometimes".

What do you think?

@cscheid
Copy link
Collaborator

cscheid commented Jan 4, 2024

Does it carry its own weight in terms of cost vs value?

My understanding is that it's somewhat necessary for .ipynb inputs, where a title is often denoted as the first H1 element in the notebook.

@knuesel
Copy link
Contributor

knuesel commented Jan 4, 2024

But then it's also necessary to not have this feature, for rendering notebooks that don't follow this convention :-)

Maybe the feature could be made opt-in? I'm not sure it's necessary at all since it's easy to update the notebook to have a proper quarto title, and many cases are already covered by the shift-heading-level-by option. But at least having it opt-in would solve the inconsistent behaviors mentioned in my previous comment...

@cscheid cscheid modified the milestones: v1.4, v1.5 Jan 4, 2024
@cscheid
Copy link
Collaborator

cscheid commented Jan 4, 2024

We have a plan to solve this, but we've determined it carries a sufficient regression risk that we want to do it in 1.5 with plenty of room to address regressions.

@cderv cderv removed the needs-discussion Issues that require a team-wide discussion before proceeding further label Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working jupyter revealjs Issues with the revealjs format
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants