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

code and documentation for membergroups and members-only options #637

Merged
merged 1 commit into from
Mar 28, 2021

Conversation

ropg
Copy link
Contributor

@ropg ropg commented Feb 11, 2021

membergroups
If specified, only the member groups in a space-delimired list following this directive will be displayed.

members-only
This will allow to show only the regular members. It will remove class and group names, descriptions, inheritance information and child classes or structs.

This fixes issue #636 I filed yesterday, but only partly. This would be infinitely more useful if it could also suppress the header with the class name itself so the author is finally free to just discuss a set of members anywhere in documentation.

I cannot for the life of me figure out how to suppress these headings though. Please feel free to supersede this with a version that alse takes out these headings, which is really meant to be included in what my members-only option does.

@jakobandersen
Copy link
Collaborator

I cannot for the life of me figure out how to suppress these headings though. Please feel free to supersede this with a version that alse takes out these headings, which is really meant to be included in what my members-only option does.

If I understand correctly, by this you mean the actual class declaration? In that case it is rather complicated to implement. It is not just a header.
When you do

.. doxygenclass:: someClass

Breathe will execute directives in Sphinx corresponding to something like

.. cpp:class:: someClass

   .. cpp:function:: void someMemberFunction()

so if you want to declare the members without the class then Breathe will still need to direct Sphinx to get into the scope of someClass, otherwise the members will be declared in the wrong scope. The cpp:namespace-push and cpp:namespace-pop directives can be used, though it means changes to some of the core rendering machinery in Breathe.

It may be that there is a better way of achieving what you would like. Can you provide a bit more context to your use case? Eg., is someClass being declared somewhere else with all its members? In that case the cpp:alias directive may be more suitable in the secondary places.

@ropg
Copy link
Contributor Author

ropg commented Feb 11, 2021

Below is from the now-closed issue that preceded this PR. It shows the rationale for this: being able to talk about any given set of members without being distracted by what class actually provides them, or having class definitions show up every time you discuss the next set of members.

I have a project for which functionality for the classes people actually use is inherited from a number of different classes. The structure of that is not necessarily relevant to people just wanting to use the derived class. In my documentation, I follow a narrative that is focussed on what users are trying to do, which doesn't always match how these classes are structured. What would help me is a feature where I can create a member group and then pull only the members of that member group into the documentation, without anything else.

Something like:

.. doxygenclass:: someClass
   :membergroups: someMemberGroup
   :members-only:

Analogous to :members:, which takes a comma-separated list of members, this would take a comma-separated list of member groups. The new option :members-only: would suppress both the name of the class and the name of the member group(s).

This way documentation authors would be free to talk about a groups of members from anywhere, without having to go into what is inherited from where.

The INLINE_INHERITED_MEMB Doxygen config option might seem like it might help here. It doesn't because it's way too global, still follows the underlying structure too much and would force repetition where that's not desired.

@ropg
Copy link
Contributor Author

ropg commented Feb 11, 2021

Thank for helping! Having spent all of yesterday on it, I'm glad you can confirm that it's not easy to do. ;-)

As for cpp:alias, if I understand correctly I would have to list all the members explicitly every time. It's far too easy to forget to add members in all the places in the documentation where they might appear. It's much safer and far less work to add them one of the membergroups in the source and then everything else is automatic. That's the whole point of using breathe/sphinx/doxygen for me.

@jakobandersen
Copy link
Collaborator

As for cpp:alias, if I understand correctly I would have to list all the members explicitly every time. It's far too easy to forget to add members in all the places in the documentation where they might appear. It's much safer and far less work to add them one of the membergroups in the source and then everything else is automatic. That's the whole point of using breathe/sphinx/doxygen for me.

It is indeed tedious, and in the optimal case Breathe should be able to produce alias declarations in this use case. Though, it may be possible to add support in Sphinx as well. In the near future I will merge some improvements to the alias directive (https://github.com/jakobandersen/sphinx/tree/c_cpp_alias) to insert nested declarations as aliases as well, and optionally skip the root symbol, which is interesting in your case as well.
What is missing is then the filtering by membergroup, which could be implemented by Sphinx allowing a user-specified tag on each declaration which Breathe could then set to the membergroup from Doxygen. The alias directive could then learn how to filter nested declarations, both by predefined tags (type, function, var, ...) and by such user-defined tags.
Your case could then become something like

.. cpp:alias:: someClass
   :noroot:
   :maxdepth: 2
   :filter: tag(someMemberGroup)

The Sphinx issue for this is at sphinx-doc/sphinx#8213. Feel free to elaborate on suggestions for alias directive there.

@ropg
Copy link
Contributor Author

ropg commented Feb 11, 2021

I'm super new to both sphinx and breathe, so I'll gladly take your word for it if you say this is better solved in sphinx than in breathe. At the end of the day, what I need is to get my list of members with nothing else, all just by putting them in the right member group in the header file.

It'd be nice to be able to specify multiple groups, but that's not a big deal.

Thanks for giving it thought !!

@vermeeren vermeeren self-requested a review February 13, 2021 15:35
@vermeeren vermeeren self-assigned this Feb 13, 2021
@vermeeren vermeeren added code Source code enhancement Improvements, additions (also cosmetics) labels Feb 13, 2021
Copy link
Collaborator

@vermeeren vermeeren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ropg Thanks a lot for the PR, the logic changes all seem good to me. I note from the discussion with @jakobandersen that this may not be the ideal solution, but I'm in favour of getting this merged and making a release after. For Breathe itself it's probably wise to one day more formally drop some legacy options and bump to version 5.

@ropg Have some minor comments, mostly about documentation. Could you check? When you are done, could you also rebase (git rebase -i upstream/master) and fixup the later commits into the initial one, so the commit history is clean?

breathe/renderer/sphinxrenderer.py Outdated Show resolved Hide resolved
breathe/directives.py Outdated Show resolved Hide resolved
documentation/source/directives.rst Outdated Show resolved Hide resolved
documentation/source/class.rst Outdated Show resolved Hide resolved
documentation/source/struct.rst Outdated Show resolved Hide resolved
documentation/source/class.rst Show resolved Hide resolved
@ropg
Copy link
Contributor Author

ropg commented Feb 15, 2021

Must admit I'm no git genius, and I've never used rebase/fixup. Seemsto me like something went wrong and I just created more commits instead of less. Commit 493c4a3 seems to incldue the previous ones, but i also still see them, they should be gone, no?

@vermeeren
Copy link
Collaborator

vermeeren commented Feb 15, 2021

Must admit I'm no git genius, and I've never used rebase/fixup. Seemsto me like something went wrong and I just created more commits instead of less. Commit 493c4a3 seems to incldue the previous ones, but i also still see them, they should be gone, no?

@RobertoRoos You should have some form of upstream for the Breathe repo in your fork, you can check with git remote -v. If you don't have it, you can run.

git remote add upstream https://github.com/michaeljones/breathe.git

Then git fetch upstream will get the latest stuff from this Breathe repository.

Finally rebase, from the PR branch, on top of latest upstream with git rebase -i upstream/master. This should result in a fairly straightforward interactive display similar to git commit CLI. You should only see the commits unique to this PR in that screen, which you can then shuffle in order or use fixup to merge a commit into the one listed above it. You can verify results with git log and undo the rebase if needed by resetting to a commit via git reflog.

Feel free to post screenshot or commands if you need more help, I can also handle the rebase for you if you prefer that.

See also https://git-scm.com/book/en/v2/Git-Branching-Rebasing and https://git-rebase.io/.

Edit: note that after rebasing you have to force push with -f, this is expected. Do not git pull in such a situation or you will end up with the old history again and additional merge commits and/or conflicts.

@ropg
Copy link
Contributor Author

ropg commented Feb 15, 2021

I think it worked as in it knows the upstream and did the rebase, it's just that the history of my branch still has the old commits too, as well as the new one that has it all-in-one, so I'm a bit confused. I'm not sure if maybe it's supposed to be this way and consolidation happens on merge...

@ropg
Copy link
Contributor Author

ropg commented Feb 15, 2021

Ah, success!!!

I had indeed pulled in the old state in between, git push -f did the trick. I think we're all set now, but am happy to do further changes if needed.

Copy link
Collaborator

@vermeeren vermeeren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ropg Really nice with the updated docs and examples, thanks! Will make a release after checking the remaining PR.

Copy link
Collaborator

@vermeeren vermeeren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ropg I did some final testing just prior to merging and found out this breaks the LaTeX build somehow, as make pdf now results in the following:

 LaTeX Error: Something's wrong--perhaps a missing \item.

See the LaTeX manual or LaTeX Companion for explanation.
Type  H <return>  for immediate help.
 ...                                              
                                                  
l.1719 \begin{fulllineitems}
                            
?

The relevant section of the generated BreatheExample.tex.

\subsubsection{Members\sphinxhyphen{}only}
\label{\detokenize{class:members-only}}\label{\detokenize{class:class-example-membersonly}}
This will only show the members of a class, and not the class name, child
classes or structs, or any information about the class.

\begin{sphinxVerbatim}[commandchars=\\\{\}]
\PYG{p}{..} \PYG{o+ow}{doxygenclass}\PYG{p}{::} ClassTest
   \PYG{n+nc}{:project:} class
   \PYG{n+nc}{:members:}
   \PYG{n+nc}{:members\PYGZhy{}only:}
\end{sphinxVerbatim}

It produces this output:
\index{{[}anonymous{]}::ClassTest (C++ class)@\spxentry{{[}anonymous{]}::ClassTest}\spxextra{C++ class}}

\begin{fulllineitems}
~\index{{[}anonymous{]}::ClassTest::function (C++ function)@\spxentry{{[}anonymous{]}::ClassTest::function}\spxextra{C++ function}}

\begin{fulllineitems}
\phantomsection\label{\detokenize{class:_CPPv4NUt21_ex_class_members_only9ClassTest8functionEi}}%
\pysigstartmultiline
\pysiglinewithargsret{\phantomsection\label{\detokenize{class:classtestclass_class_test_1a6cd2a6337e41876f44d9825326d3bef1}}void \sphinxbfcode{\sphinxupquote{function}}}{int \sphinxstyleemphasis{myParameter}}{}%
\pysigstopmultiline
non\sphinxhyphen{}namespaced class function 

More details in the header file.

More documentation in the impl file 

\end{fulllineitems}

Somewhat difficult to see what the exact problem is, I notice old problems similar to this in #90 and #127, so perhaps a fix can you found there.

Can you investigate a bit to see which exact case in documentation causes the issue and do partial logic changes from there?

@ropg
Copy link
Contributor Author

ropg commented Feb 16, 2021

Problem found, not yet fixed. My trick to not have the class header with :members-only: is to cut away the desc_signature node. Renders to HTML just fine, but apparently Latex has trouble with that. This is the offending fragment from class.pseudoxml:

            <paragraph>
                It produces this output:
            <index entries="['single',\ '[anonymous]::ClassTest\ (C++\ class)',\ '_CPPv4NUt21_ex_class_members_only9ClassTestE',\ '',\ None]">
            <desc classes="cpp" desctype="class" domain="cpp" noindex="False" objtype="class">
                <desc_content>
                    <container classes="breathe-sectiondef" objtype="public-func">
                        <index entries="['single',\ '[anonymous]::ClassTest::function\ (C++\ function)',\ '_CPPv4NUt21_ex_class_members_only9ClassTest8functionEi',\ '',\ None]">
                        <desc classes="cpp" desctype="function" domain="cpp" noindex="False" objtype="function">
                            <desc_signature ids="_CPPv4NUt21_ex_class_members_only9ClassTest8functionEi _CPPv3NUt21_ex_class_members_only9ClassTest8functionEi" is_multiline="True">
                                <desc_signature_line add_permalink="True" xml:space="preserve">
                                    <target dupnames="classtestclass_class_test_1a6cd2a6337e41876f44d9825326d3bef1 classtestclass_class_test_1a6cd2a6337e41876f44d9825326d3bef1" ids="classtestclass_class_test_1a6cd2a6337e41876f44d9825326d3bef1">
                                    void
                                     
                                    <desc_name xml:space="preserve">
                                        function
                                    <desc_parameterlist xml:space="preserve">
                                        <desc_parameter noemph="True" xml:space="preserve">
                                            int
                                             
                                            <emphasis>
                                                myParameter
                            <desc_content>
                                <paragraph>
                                    non-namespaced class function 
                                <paragraph>
                                    More details in the header file.
                                <paragraph>
                                    More documentation in the impl file 

As you can see, unlike the embedded function, the class entry itself has no desc_signature node and goes straight to desc_content. I'll play around a bit more, but I was quite happy finally finding a solution that the rendering engine didn't barf on. Stay tuned.

@ropg
Copy link
Contributor Author

ropg commented Feb 16, 2021

Even if I leave in the desc_signature node and just empty it out (i.e. remove the desc_signature_line node), it still doesn't work. (It does still render the way we want in html.) I don't really understand what the Latex rendering wants here, but if it insists on having something to render there then we can't help it without defeating the purpose.

I can move the children up a level (that was one of my earlier tries, and worked) but that has its own problems, such as being very confusing in the readthedocs theme because the colors for the indent levels are different and then the function definitions look like class definitions all of a sudden. (Maybe you know a trick to change that? Just indenting the directive doesn't work: it does cause an indent, but still the wrong css class.)

This feels too trial and error, given that I know way too little of Latex, docutils, sphinx and/or breathe. Please think along, I don't know how to figure out what is needed here.

Darn... So close...

--

BTW: I always get a lot of warnings when I compile documentation, like:

[...]
writing output... [ 94%] typedef
writing output... [ 97%] union
writing output... [100%] variable

/Users/rop/Documents/github/breathe/documentation/source/class.rst:132: WARNING: cpp:identifier reference target not found: Tool
/Users/rop/Documents/github/breathe/documentation/source/class.rst:: WARNING: cpp:identifier reference target not found: QObject
/Users/rop/Documents/github/breathe/documentation/source/domains.rst:: WARNING: cpp:identifier reference target not found: testnamespace
/Users/rop/Documents/github/breathe/documentation/source/domains.rst:: WARNING: cpp:identifier reference target not found: testnamespace::NamespacedClassTest
/Users/rop/Documents/github/breathe/documentation/source/domains.rst:: WARNING: cpp:identifier reference target not found: testnamespace
[etc...]

I also get this in the master branch, so is this something about my setup, or is this expected behaviour ?

@jakobandersen
Copy link
Collaborator

As for #637 (review), I think it's because you have paragraph breaks (i.e., double newline) inside the list environment (fulllineitems).
I'm not sure exactly how to hax this on the doctree level, but as for what the Latex writer does, a point of entry is at https://github.com/sphinx-doc/sphinx/blob/3.x/sphinx/writers/latex.py#L707 and the following desc_* methods.
The warnings in the documentation is for the most part expected as it runs in nitpicky mode. Though, some refactoring may get rid of some of them.

@ropg
Copy link
Contributor Author

ropg commented Feb 16, 2021

Hmmm. I don't think those are mine, they are everywhere without issue. If I make pseudoxml in the master branch, there's also lots of returns in there. For instance:

                    <container classes="breathe-sectiondef" objtype="private-func">
                        <rubric classes="breathe-sectiondef-title">
                            Private Functions
                        <index entries="['single',\ '[anonymous]::ClassTest::privateFunction\ (C++\ function)',\ '_CPPv4NKUt29_ex_class_members_undocumented9ClassTest15privateFunctionEv',\ '',\ None]">
                        <desc classes="cpp" desctype="function" domain="cpp" noindex="False" objtype="function">
                            <desc_signature ids="_CPPv4NKUt29_ex_class_members_undocumented9ClassTest15privateFunctionEv _CPPv3NKUt29_ex_class_members_undocumented9ClassTest15privateFunctionEv" is_multiline="True">
                                <desc_signature_line add_permalink="True" xml:space="preserve">
                                    <target ids="classtestclass_class_test_1a13efc1addc97a75d6517df7f130615c3" names="classtestclass_class_test_1a13efc1addc97a75d6517df7f130615c3">
                                    <desc_annotation xml:space="preserve">
                                        virtual
                                     
                                    void
                                     
                                    <desc_name xml:space="preserve">
                                        privateFunction
                                    <desc_parameterlist xml:space="preserve">
                                     
                                    <desc_annotation xml:space="preserve">
                                        const
                                     = 0
                            <desc_content>
                                <paragraph>
                                    This is a private function. 
                        <index entries="['single',\ '[anonymous]::ClassTest::undocumentedPrivateFunction\ (C++\ function)',\ '_CPPv4NKUt29_ex_class_members_undocumented9ClassTest27undocumentedPrivateFunctionEv',\ '',\ None]">
                        <desc classes="cpp" desctype="function" domain="cpp" noindex="False" objtype="function">
                            <desc_signature ids="_CPPv4NKUt29_ex_class_members_undocumented9ClassTest27undocumentedPrivateFunctionEv _CPPv3NKUt29_ex_class_members_undocumented9ClassTest27undocumentedPrivateFunctionEv" is_multiline="True">
                                <desc_signature_line add_permalink="True" xml:space="preserve">
                                    <target ids="classtestclass_class_test_1a5d1966a5925affbedf4a9758ed23ba91" names="classtestclass_class_test_1a5d1966a5925affbedf4a9758ed23ba91">
                                    <desc_annotation xml:space="preserve">
                                        virtual
                                     
                                    void
                                     
                                    <desc_name xml:space="preserve">
                                        undocumentedPrivateFunction
                                    <desc_parameterlist xml:space="preserve">
                                     
                                    <desc_annotation xml:space="preserve">
                                        const
                                     = 0
                            <desc_content>

Will look at this further tomorrow...

``membergroups``
  If specified, only the groups in a comma-delimired list following this directive will be displayed.

``members-only``
  This will allow to show only the members. It will remove group names, descriptions as well as the inheritance information.
@ropg
Copy link
Contributor Author

ropg commented Feb 18, 2021

OK, fixed and much, much cleaner now...

As you can see, instead of removing the desc_signature node, I now only return the child nodes, each with their own signature and description. My only problem with this was the rendering in the readthedocs theme. On closer inspection, the reason they rendered differently was the CSS of readthedocs, which had different properties when a <dl> was a child of another <dl>. So that's out of scope for breathe and I fixed it with a custom CSS snippet. I added instructions for that in a .. note:: under the membergroups section of the manual in case others need it.

@ropg ropg requested a review from vermeeren February 18, 2021 13:04
Copy link
Collaborator

@vermeeren vermeeren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ropg Apologies for the massive delay, stuff happened. New changes look good and I can confirm it solves the issue with PDF building. Thanks for documenting the CSS as well!

michaeljones pushed a commit that referenced this pull request Mar 28, 2021
@michaeljones michaeljones merged commit 66d1c35 into breathe-doc:master Mar 28, 2021
@ropg
Copy link
Contributor Author

ropg commented Mar 29, 2021

Yay! Thanks for merging. I learned a lot...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code Source code enhancement Improvements, additions (also cosmetics)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Option to generate output for individual member groups
4 participants