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

Fix for admonitions being displayed out of their original context. #910

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

intelkevinputnam
Copy link
Contributor

Removes 'pullup' for admonitions in detailed description to keep them in context.

File changed: breathe/renderer/sphinxrenderer.py

This is a fix for #889

… in context.

Signed-off-by: Putnam, Kevin <kevin.putnam@intel.com>

Changes to be committed:
	modified:   breathe/renderer/sphinxrenderer.py
@vermeeren
Copy link
Collaborator

(Sort of replies to #889 (comment))

@intelkevinputnam Via blame I see the commit that added this is 7f579b1, PR #670. Quite long ago but rationale seems to be that it matched Sphinx's own style.

As a result, I'm not sure about potential side effects this might have for other users. @jakobandersen Do you remember or have insight is whether pullup is needed for these cases?

If it was only for stylistic reason with the default big block rendering of note and warning (PR #670 screenshot), it might be best to turn this into a configuration option as there are likely users that prefer pullup style of rendering.

@intelkevinputnam
Copy link
Contributor Author

Sounds good. I'll update the PR as a configurable option. Any guidance on naming the parameter?

@vermeeren
Copy link
Collaborator

@intelkevinputnam Initially I'm thinking something like breathe_detaileddesc_pullup_types which one can set to desired types from https://pydoc.dev/docutils/latest/docutils.nodes.html. I'm hoping this extra flexibility will help with some other requests made over the years about changing the order, as you can manually input everything in a very specific order if desired.

# user in conf.py will have to import
# (unless there is a better way, but this errors early, instead of at runtime)
from docutils import nodes

# this would match current behaviour.
breathe_detaileddesc_pullup_types = [ nodes.note, nodes.warning ]

Then the implementation can loop over the supplied types and pull them up.

As for the default value, I think empty [] is good, as I agree this is kind of an unexpected ordering change when using Breathe. We can document in the documentation/source/directives.rst that this is since v4.36.0 and to get v4.35.0 or older behaviour one can set it to above snippet. That should help with ctrl+f on version numbers when this change is released.

Thanks for the help!

@intelkevinputnam
Copy link
Contributor Author

@vermeeren

Gotcha. Easily doable.

Some additional thoughts:

TL;DR - would it make sense to define different behaviors for doxygenpage vs doxygengroup or doxygenclass?

I ran into this problem because I am dealing with pages and pages of documentation written in Markdown with admonitions scattered throughout. The presentation is meant to be linear for all the content which is all contained in the detaileddescription field of the Doxygen XML. I use the doxygenpage directive to render them.

Based on the original PR, I think this feature was developed for classes (for example) where there are many objects that might be rendered in any order. This would help to collect them in a sensible way.

Which might mean that a given user might want one behavior for pages and another for groups and classes. What do you think?

@vermeeren
Copy link
Collaborator

@intelkevinputnam Very good catch, makes sense to do it based upon directive then. We could update the setting value to use an array per directive in a dictionary instead of a plain array in this case.

# no entry for "doxygenpage", so would default to []
breathe_detaileddesc_pullup_types = {
	"doxygenclass": [ nodes.note, nodes.warning ],
	"doxygengroup": []
}

Not 100% sure if above syntax is correct, but the general idea should be good. I am not sure if we should set one or more directives to not-[] by default however, I still sort of feel that Breathe shouldn't modify (sort) input data too much if it wasn't asked to. Your thoughts on this?

@intelkevinputnam
Copy link
Contributor Author

@vermeeren I agree that sorting should be a requested behavior. I'll take a stab at implementing it.

…n detailed descriptions.

TODO:

Determine method for toggling whether to pull them up for different object types.

Signed-off-by: Putnam, Kevin <kevin.putnam@intel.com>
…e used eg. doxygenpage.

Syntax: breathe_detaileddesc_pullup_types = {"doxygenpage":["note","warning"]}

Signed-off-by: Putnam, Kevin <kevin.putnam@intel.com>
@intelkevinputnam
Copy link
Contributor Author

@vermeeren I used strings to denote the node to pull up rather than the type value.

breathe_detaileddesc_pullup_types = {"doxygenpage":["note","warning","fieldlist"]}

Signed-off-by: Putnam, Kevin <kevin.putnam@intel.com>
Signed-off-by: Putnam, Kevin <kevin.putnam@intel.com>
@intelkevinputnam
Copy link
Contributor Author

@vermeeren can you help with this check error? I tried to match self.context.directive_args usage in other places in sphinxrenderer.py, but the linter doesn't seem to like it.

Signed-off-by: Putnam, Kevin <kevin.putnam@intel.com>
@intelkevinputnam
Copy link
Contributor Author

@michaeljones It looks to me like the linter is now blocking my PR on a file I haven't changed. Should I go ahead and submit the correction it is asking for?

@anjalirajasekhar
Copy link

Hi @intelkevinputnam. I had a similar issue where the notes where not displayed in order. I tried removing the pullups. But still the behavior is same. Is there some other reasons that might lead to this kind of behavior

@intelkevinputnam
Copy link
Contributor Author

@anjalirajasekhar I just returned from vacation, so I'm a little rusty. Which directive are you using?

@anjalirajasekhar
Copy link

anjalirajasekhar commented Nov 8, 2023

Hi @intelkevinputnam , sorry for the late reply. I am using doxygenpage directive

@2bndy5
Copy link
Contributor

2bndy5 commented Mar 25, 2024

there are likely users that prefer pullup style of rendering

I would expect the same order shown in doxygen's HTML output. This problem has surfaced in #835, #889, and #957.

Although I do understand the desire to preserve previous behavior, I suspect it was a stylistic decision. Looking at #670, I think it was trying to consolidate various/similar "simple sections" (<dl><dt><dd> in HTML) that can be partitioned in doxygen's XML. I think it was an attempt to consolidated groups of simple sections in the way that doxygen's HTML renderer does.

Note

Doxygen's HTML renderer only consolidates block-level sections that are written consecutively.

@param p1 some text
@param p2 some text

If a block-level section is interrupted by another block-level section, then doxygen will output the sections in the given order.

@param p1 some text
@note some info.
@param p2 some text

Admonitions (like doxygen's @note, @warning, etc) are technically block-level sections (per popular markdown conventions that support admonitions), but they can be nested within block-level sections using doxygen's @parblock ... @endparblock commands (something most markdown parsers don't support).

@param p1 
@parblock
some text about p1
@note some info about p1.
@endparblock
@param p2 some text

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants