asciidoctor / asciidoctor-vscode

AsciiDoc support for Visual Studio Code using Asciidoctor
Other
336 stars 97 forks source link

Preview does not respect :toclevels: in top level doc #762

Closed glynb closed 1 year ago

glynb commented 1 year ago

Preview doesn't apply :TOCLEVELS: as specified in top level document. Included (child) docs appear to override the value of :toclevels: in the parent document. This is new behaviour in version 3.1.2 (this was also incorrect in 3.1.0, correct behaviour in 2.9.8)

Please see the difference in the Table of Contents in the preview window in screenshots below

Expected behaviour ( in 2.9.8)...

image

Incorrect behaviour ( in 3.1.2)...

image

example adoc content:


parent.adoc

:toc: :toclevels: 1 :sectnums:

:xrefstyle: full

= Title - V3.1.2

== Level 1 - first

=== Level 2

:leveloffset: +2

include::child.adoc[]

:leveloffset: -2

==== Level 3

===== Level 4

== Level 1 - second


child.adoc

:toc: :toclevels: 3 :sectnums:

= Child

I'm the child doc

== child level 1

=== child level 2

==== child level 3

System info:

Version: 1.80.1 (user setup) Commit: 74f6148eb9ea00507ec113ec51c489d6ffb4b771 Date: 2023-07-12T17:22:07.651Z Electron: 22.3.14 ElectronBuildId: 21893604 Chromium: 108.0.5359.215 Node.js: 16.17.1 V8: 10.8.168.25-electron.0 OS: Windows_NT x64 10.0.19042

ggrossetie commented 1 year ago

To be honest, I don't have any clue... we delegate the work to the built-in HTML5 converter:

https://github.com/asciidoctor/asciidoctor-vscode/blob/ea37e351f185cd86f1c3b93f0afed5b9ec074b6f/src/asciidoctorWebViewConverter.ts#L311

My only guess is that somehow the node is not the parent document and we retrieve :toclevels: 3 instead of :toclevels: 1 but I don't understand how.

@mojavelinux If you have time I would love your insight/guess

mojavelinux commented 1 year ago

The explanation is pretty simple. The toclevels attribute must be set in the header. In the document provide, it is not. Please see https://docs.asciidoctor.org/asciidoc/latest/document/header/#document-header-structure

ggrossetie commented 1 year ago

I assumed that something was wrong in the extension because I get the "expected" behavior using the browser extension 🤔 I also get the same result when using:

main.adoc

= Title - V3.1.2
:toc:
:toclevels: 1
:sectnums:
:xrefstyle: full

== Level 1 - first

=== Level 2

:leveloffset: +2

include::child.adoc[]

:leveloffset: -2

==== Level 3

===== Level 4

== Level 1 - second

child.adoc

= Child
:toc:
:toclevels: 3
:sectnums:

I'm the child doc

== child level 1

=== child level 2

==== child level 3
glynb commented 1 year ago

Noted comment on header structure. (as per previous comment from @ggrossetie ) , applying the correct structure and simplifying the adocs, does still produce the unexpected output. Also tried asciidoctor and asciidoctor-pdf cli and they both give the expected result (with either header structure)

image

mojavelinux commented 1 year ago

Also tried asciidoctor and asciidoctor-pdf cli and they both give the expected result (with either header structure)

That tells me it's a problem with the extension.

I think I'm detecting what the issue may be. The extension pieces together the HTML output by calling methods on the document object. The method it calls to get the content of the document is getContent(). See https://github.com/asciidoctor/asciidoctor-vscode/blob/master/src/asciidoctorWebViewConverter.ts#L144 This is not correct. It must call convert() to get the full result of conversion. The convert method sets up document attributes which would not otherwise be available. See https://github.com/asciidoctor/asciidoctor/blob/main/lib/asciidoctor/abstract_block.rb#L71-L78.

ggrossetie commented 1 year ago

I think I'm detecting what the issue may be. The extension pieces together the HTML output by calling methods on the document object. The method it calls to get the content of the document is getContent()

Aaaah! I have the tendency to use getContent() and convert() interchangeably 😓 Thanks for taking a look, you saved me from potentially hours of debug 🤗

ggrossetie commented 1 year ago

I get a RangeError: Maximum call stack size exceeded when calling convert here.

The built-in HTML5 converter is also using node.content in the convert_document method: https://github.com/asciidoctor/asciidoctor/blob/f3800cc9c92faf8370041b2b27a61124318ed289/lib/asciidoctor/converter/html5.rb#L228

But I was calling const content = node.getContent() too soon.

mojavelinux commented 1 year ago

Was it a matter of when it was called?

ggrossetie commented 1 year ago

Yes, I moved it after calling getDocumentHeader, see: https://github.com/asciidoctor/asciidoctor-vscode/commit/2ff98610c0974a69419a43ddaf76744c5c0685ee

mojavelinux commented 1 year ago

That means my suggested fix was incorrect, but the idea was the same. That ensures the right document attributes are available at the time getContent() is run.