quarto-dev / quarto-cli

Open-source scientific and technical publishing system built on Pandoc.
https://quarto.org
Other
3.75k stars 306 forks source link

RevealJS: Specifiyng engine introduces spurious title slide. #8012

Open Walser52 opened 8 months ago

Walser52 commented 8 months ago

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](https://github.com/quarto-dev/quarto-cli/assets/101308173/91da6d0e-f133-49a5-95f1-8d5830c0e8c8)

### Actual behavior

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

![image](https://github.com/quarto-dev/quarto-cli/assets/101308173/129512e7-310d-40ad-a5ec-3cae4041e302)

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

````{qmd}
---
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](https://github.com/quarto-dev/quarto-cli/assets/101308173/865f410a-9ec9-4914-a2a6-2de07733a7b4)

### 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
cderv commented 8 months ago

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 https://github.com/quarto-dev/quarto-cli/commit/24672a4affb8d3c881be9a3b9ede641d2e26d9d9 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 commented 8 months ago

@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

---
format:
  revealjs:
    engine: jupyter            
---

## My custom title slide {.center}

Content

## Python slide

```{python}
1 + 1


gives this slide overview 

![image](https://github.com/quarto-dev/quarto-cli/assets/6791940/266c4992-b1d6-417d-b81b-620bdf026f96)

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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.