jbms / sphinx-immaterial

Adaptation of the popular mkdocs-material material design theme to the sphinx documentation system
https://jbms.github.io/sphinx-immaterial/
Other
191 stars 29 forks source link

Python API for custom admonitions #183

Closed jbms closed 1 year ago

jbms commented 1 year ago

Ideally, we would offer a Python API for defining custom admonitions, likely in the form of a config option to be specified in conf.py.

Currently it is possible to override the icons for existing admonitions using via the icon/admonition theme option, but to define a new admonition type, the user needs to:

  1. Create a custom CSS file and add some boilerplate to it: https://squidfunk.github.io/mkdocs-material/reference/admonitions/#custom-admonitions
  2. Add an entry to the icon/admonition theme option.
  3. Define a custom sphinx admonition directive for it

It would be nicer if we provided a single option where the user could specify:

Ideally, this would emit the necessary additional CSS to a new combined bundle that contains the static pre-generated CSS bundle with any dynamically-generated CSS appended. That way there is still a single bundle, and we avoid duplicating the additional CSS on every HTML page, as occurs with the current icon/admonition theme option.

2bndy5 commented 1 year ago

This reads like the details/summary tags (collapsible admonitions) are excluded.

If the CSS is written with a template that emits no whitespace, I suppose it wouldn't need to use software like the Sass compiler.

As far as HTML templates, we'd have to add our generated CSS into base.html inside a Jinja if statement. I'd prefer it if the current (icon[admonition]) upstream solution remained in the templates though. We can just discourage its use after this issue is resolved.

jbms commented 1 year ago

Supporting the details directives as well would make sense --- I just didn't think about that.

Yes, seems reasonable to leave the existing icons[admonitions] code in place.

We don't necessarily need to add the generated CSS inline into the HTML at all --- instead we can just replace the normal CSS bundle with a modified one with the extra generated content appended.

Sass provides useful functionality but I think it would significantly complicate the sphinx build to have to invoke sass as part of it; instead we should just emit plain CSS directly (without extra whitespace, as you noted). The mkdocs-material theme provides suggested CSS to add for defining custom admonitions:

https://squidfunk.github.io/mkdocs-material/reference/admonitions/#custom-admonitions

Perhaps that is basically exactly what is needed?

2bndy5 commented 1 year ago

FYI, the upstream CSS has deprecated many of the alias CSS classes - notice that only the remaining CSS classes are demonstrated in upstream docs. So, this would be especially useful for those that want to re-implement them.

2bndy5 commented 1 year ago

Perhaps that is basically exactly what is needed?

Yes. In fact, that CSS example is what I copied into our docs.


I just noticed that there are 2 CSS classes that we haven't documented. These classes allow for inline admonitions on sufficiently wide viewports.

2bndy5 commented 1 year ago

Define a custom sphinx admonition directive for it

I'm trying to imagine how this would get implemented. I think the easiest way would be to provide a CustomAdmonition class that can be used in conf.py

sphinx_immaterial_custom_admonitions = [
    CustomAdmonition(
        name="seealso",
        color=(127, 64, 0),  # must use RGB color space
        icon="octicons/eye-24",  # could also be user defined in static files
        # optional kwargs
        title="See Also",  # could default to self.name.title()
        override=True,  # should default to False, but this example redefines the seealso admonition
    ),
]

resulting in CSS like

:root {
  --si-icon--octicons_eye-24: url('data:image/svg+xml;charset=utf-8,<svg ...>...</svg>')
}

.md-typeset .admonition.seealso {
  border-color: rgb(127, 64, 0);
}

.md-typeset .seealso > .admonition-title {
  background-color: rgba(127, 64, 0, 0.1);
  border-color: rgb(127, 64, 0);
}

.md-typeset .seealso > .admonition-title::before {
  background-color: rgb(127, 64, 0);
  -webkit-mask-image: var(--si-icon--octicons_eye-24);
  mask-image: var(--si-icon--octicons_eye-24);
}

And the directive could be used like so:

.. seealso::
    :collapsible: open

    A collapsible admonition that is expanded by default.
    Only the value "open" will be supported in the ``:collapsible:`` option.

.. seealso::
    :collapsible:

    A collapsible admonition that is closed by default.

.. seealso::
    :no-title:

    Some admonished content without a admonition-title. This seems reasonable to me because
    the user can set the title for the custom admonition in conf.py.

    ``:no-title:`` cannot be combined with ``:collapsible:``.
2bndy5 commented 1 year ago

So, I think I have a working draft of this. I'm going to keep playing around with it and push it when its near finished. I would like to get #182 merged first, so I can rebase and incorporate the inline icon's SVG data as CSS variables.

I had to patch the visitor_admonition() to implement details/summary tags when :collapsible: option is specified. I found out that Sphinx' visitors for admonitions (any but the generic) call the generic visit_admonition() with an added name arg (used as the admonition title) that gets translated in the generic visitor. This means that our current monkeypatching decorator (html_translator_mixin.override) cannot be used to patch the visit_admonition() (unless we augment it to pass through arbitrary parameters - I haven't tried this yet).

2bndy5 commented 1 year ago

Documenting this is going to be a rather big change. While we could still support the current implementations (if we keep the monkeypatch to the details directive), we could also use this to

  1. override the builtin admonitions, so :no-title: and :collapsible: options could be used
  2. add admonitions based on supported CSS classes upstream.

Although, I'm not sure if adding/overriding admonitions by default is a good idea (for translation reasons and conflicts with other extensions).

Once I incorporate the inline icons, I'll push what I have.

2bndy5 commented 1 year ago

Implemented! Now including inline icons' SVG data in CSS variables. I'll push my local branch now.

I'm sure there's much room for improvement (as you'll see), but I feel that this has a solid foundation.

2bndy5 commented 1 year ago

It would be nice to be able to use other CSS variables for the color (instead of just RGB color components). This would help with overwriting existing admonitions and keep the value up-to-date with upstream changes.

I originally chose the RGB color components because the admonition-title bg color has added transparency, so supporting various forms of color input makes a CSS template a bit complicated.

2bndy5 commented 1 year ago

I also found that in trying to support a dynamic title (as opposed to a fixed string), overriding the built-in admonitions breaks with Napoleon style docstrings because a blank line is required after the directive invocation. Specifically in tensorstore_demo.Dim which uses the seealso admonition. https://github.com/jbms/sphinx-immaterial/blob/09b41f950db5b3ba62143aca14276df6c6f3fcc8/docs/tensorstore_demo/__init__.py#L24-L25

So, it might also be a good idea to offer a flag that allows dynamic titles in the config.

jbms commented 1 year ago

It would be nice to be able to use other CSS variables for the color (instead of just RGB color components). This would help with overwriting existing admonitions and keep the value up-to-date with upstream changes.

I originally chose the RGB color components because the admonition-title bg color has added transparency, so supporting various forms of color input makes a CSS template a bit complicated.

The problem with CSS variables that specify colors is that CSS currently doesn't really support manipulating colors at all once they are actual CSS colors --- if you have the individual components as variables you can use calc, etc. but once they are turned into colors you can't really do anything with them, and it is difficult to add alpha or do anything else. There are some hacks using animation to add alpha but I don't think we want to go that route.

We could support CSS color syntax by parsing it in Python easily enough though.

In general it is unfortunate not to be able to interact with the CSS stuff very well from Python. To solve that would probably require running sass as part of the sphinx build, though, and I'm not sure if that is worth the trouble.

jbms commented 1 year ago

I also found that in trying to support a dynamic title (as opposed to a fixed string), overriding the built-in admonitions breaks with Napoleon style docstrings because a blank line is required after the directive invocation. Specifically in tensorstore_demo.Dim which uses the seealso admonition.

https://github.com/jbms/sphinx-immaterial/blob/09b41f950db5b3ba62143aca14276df6c6f3fcc8/docs/tensorstore_demo/__init__.py#L24-L25

So, it might also be a good idea to offer a flag that allows dynamic titles in the config.

Can you explain a bit more what the issue is here?

2bndy5 commented 1 year ago

If optional_arguments is set to 1 for a directive, and the optional argument will be a title (not a single word), then final_argument_whitespace must be set to True. The problem with that is if there is no blank line between the directive and the content, then the content is considered part of the arguments. This is problematic because the directive will also have has_content set to True, and run() will assert that the content exists.

.. ok
.. directive:: title

    Content.
.. directive::

    Content without a custom title.

.. not ok
.. directive:: title
    Content is considered part of the title.
.. directive::
    Content is captured as the directive's
    arguments (and considered as the title).

I thought about splitting the argument from the first \n character and moving the rest into the directive's content, but at the time it didn't seem viable.

jbms commented 1 year ago

I see --- thanks for the explanation.

I didn't know about this quirk in rST parsing.

I guess the options are:

By the way, it could be nice to contribute the features of our custom admonition (title / no-title / collapse) upstream to sphinx.

2bndy5 commented 1 year ago

If I can get a dynamic title working, then I'll be looking into removing our custom md-admonition and overriding the generic admonition from docutils. Sphinx may not be keen to make the title optional because it would structurally look like a div with content in HTML. Our use case is rather unique because the upstream CSS has the proper rules. I doubt all other theme's CSS would be as title-less admonition friendly.

2bndy5 commented 1 year ago

Not to mention that the specific admonitions have special visitors in Sphinx that implement title translation

2bndy5 commented 1 year ago

And don't forget, we treat details/summary tags like admonitions in this theme because the implementation for those tags in pymdownx was originally inherited from admonitions (why upstream CSS makes admonitions mutually exclusive to details/summary tags).

jbms commented 1 year ago

At least as far as the missing title, I think it might be accepted upstream in sphinx even if some themes wouldn't support it --- users wouldn't have to use the feature if it doesn't work well with their theme, and the themes could be updated to support it.

That code you linked already seems to be prepared to handle a missing title.

2bndy5 commented 1 year ago

I'm more inclined to help upstream port this feature when https://github.com/facelessuser/pymdown-extensions/pull/1777 is merged to main.

I opened a feature request in sphinx for title-less admonitions. https://github.com/sphinx-doc/sphinx/issues/10958

2bndy5 commented 1 year ago

I guess the options are:

  • Patch napoleon to add an extra blank line
  • Fix our custom admonition to handle it.

I'm going with the later using the assumption that a title can only be a single line. Multi-lined titles may need a \n\n to signify the end of the title, but the parser already interprets \n\n as a separation between arguments and content... So, we're kinda stuck with single line dynamic titles.

2bndy5 commented 1 year ago

In looking at the Napoleon docs, I think they could use a configurable map for using custom directives. Maybe there's already way to do this already; I'm not an expert in using Napoleon docstrings.

jbms commented 1 year ago

I think you could just assume that the title, if specified, has to start on the same line as the directive, and must be separated by a blank line from the content:

.. directive:: title

   content

.. directive::
   content with no title

.. directive:: title
   and more title

   content

That would retain support for multi-line titles, which may be nice in the case of a long title.

2bndy5 commented 1 year ago

Directive args are just [a list of] strings. I think they need to be a ViewList (or StringList) to have src info that corresponds to the args' values. Otherwise, I'd be looking at the content (a StringList) offset and try comparing to the directive's location which isn't very helpful in this case.

2bndy5 commented 1 year ago

has to start on the same line as the directive, and must be separated by a blank line from the content

I mis-read this. Yeah, the pre-assumption of blank line between title and content is a good idea. I don't know why I was thinking about comparing src offset...

2bndy5 commented 1 year ago

Now I remember why I went with my single line tactic. It is rather typical for builtin admonitions to be used like so:

.. example-admonition::
   This is simple a example.

   Additional content.

but this makes it hard to to tell if the first line is actual content or a title because it gets captured in the directive's arg.

.. directive::
   content with no title

This is easy to tell because there will not be any content until I move it out of the arg.

jbms commented 1 year ago

I see -- I was hoping that if the arguments started on a new line, there would be a newline character. But I see that it gets stripped.

I can see that it is tricky...

2bndy5 commented 1 year ago

Yeah, it gets stripped from the beginning of the content too.

I did try using replace("\\\n", " ") in the arg and then split("\n", 1) which allowed for multi-lined titles that end in \.

2bndy5 commented 1 year ago
.. example-admonition:: A custom title that
                        has multiple lines.

   This is actual content.

Boom! unexpected indent.

But this I can work with:

.. example-admonition:: A custom title that\
   has multiple lines.
   This is actual content.

It looks ugly, but I can differentiate between multi-line title vs content stolen by the arg.

2bndy5 commented 1 year ago

I see that the block_text seems to preserve the \n as provided in the source.

'.. example-admonition::\n   This is simple a example.\n'

Only caveat I can think of is the syntax is difference with various parsers.

And the content_offset is only about the content's line number. I think I also have to adjust the content_offset if the arg is the only used as content.

2bndy5 commented 1 year ago

Last resort would be a special option called :title: so we can safely turn off argument parsing and just use the option's unchanged value.

2bndy5 commented 1 year ago

oh so much cleaner looking (& it works)

.. example-admonition::
   :title: A custom title
           that has multiple lines.

   This example uses a custom title.

I got this idea from looking at how MyST uses YAML syntax to parse the directive options:

```{code-block} python
---
lineno-start: 10
emphasize-lines: 1, 3
caption: |
    This is my
    multi-line caption. It is *pretty nifty* ;-)
---
a = 2
print('my 1st line')
print(f'my {a}nd line')
```
jbms commented 1 year ago

Napoleon is not an issue because it just transforms docstrings and then they get parsed normally as rst. But markdown would presumably require separate handling. Does this argument ambiguity exist with myst?

2bndy5 commented 1 year ago

Does this argument ambiguity exist with myst?

unlikely, notice the YAML syntax is delimited by the ---. This is a requirement for frontmatter and directive parameters. I still need to check under the hood where admonitions take an arg and only content is provided (as exemplified)

2bndy5 commented 1 year ago

I don't think MyST supports arguments that span multiple lines. According to the MyST parse_directive_text() docs. So, This ambiguity seems to be specific to the rST parsing.

2bndy5 commented 1 year ago

I'm beginning to conclude that we can retain original functionality when overriding the built-in admonitions by not supporting any optional arguments. But the overrides can still use the new :title: option (not pushed yet) to use custom titles.

My main goal is to only add functionality in a way that does not break the existing functionality. And seen as how the following was never possible:

.. note:: title
    Content

It only makes sense to use the RST parser under its limitations. But the following will be possible after we override the built-in admonitions:

.. note::
   :title: Custom title
      spanning multiple lines

   Content

As for user-defined and generic (overridden) admonitions, we can still support an optional title as an argument and still use the new :title: option if desired (both values are concatenated as I currently have it written). But as long as there's a blank line between the title and the content, then there won't be a need for the :title: option.

I could only enable the :title: option for the specific admonitions (that won't support arguments), but users might develop a habit of using the option over the arguments. Either way, users will have the flexibility that the RST parser affords.

Can Napoleon docstrings even specify directive options? I didn't see this exemplified in their docs.

jbms commented 1 year ago

I think this makes sense --- better to be consistent with how rST normally works.