Add Doxygen comments to RationalTime

[darbyjohnston]

Link the Issue(s) this Pull Request is related to.

Fixes #1753

Summarize your change.

Adds Doxygen comments to the RationalTime class. These comments are based on the Python doc strings with some editing for consistency. A couple of notes:

• I kept the Doxygen markup as simple as possible to keep it easy to write, and to allow it to be parsed by IDEs like Visual Studio
• I made a couple of modifications to the Doxygen configuration file to support the simpler markup
• This is just a first pass at the comments, it would be nice to get some feedback to help improve them

Here is a screenshot of the Doxygen comments providing help in Visual Studio:

Here is a screenshot of the HTML generated for RationalTime:

[meshula]

Overall this is a great improvement.

I'm not familiar with ABBREVIATE_BRIEF. Does it preclude documenting the parameters formally?

[darbyjohnston]

The ABBREVIATE_BRIEF option is a bit odd, it filters the text to try and make it more concise. So if your comment looks like this:

/// The Addition class provides functionality for adding numbers.

It will be changed to: "Functionality for adding numbers". I always find it a bit surprising so I like to turn it off.

If these changes look OK how should we go about documenting the rest of the code? It would be nice to do the work in chunks to keep it manageable, maybe one PR per class?

[meshula]

My first question about ABBREVIATE_BRIEF was really poking at whether that was supposed to do more, like infer parameters and returns.

I'd prefer we go heavy-handed with documentation, ie with full adornment. I assume ABBREVIATE_BRIEF means that we can skip the @brief tag. Sometimes this feels overboard, but I think it's better to be thorough and consistent in order that all the tooling around doxygen syntax can come into play. e.g. I just did a similar pass on Nanocolor, where everything is this heavy. Doing it this way allowed me to automate the decoration of the entire library using various tools in VsCode, so it wasn't as terrible as it seems. (Right click function, "Add doxygen comment", more or less)

/**
 * @brief Converts a XYZ color to RGB color space.
 * 
 * Converts a XYZ color to RGB color space using the provided color space.
 * 
 * @param cs Pointer to the color space object.
 * @param xyz The XYZ color to convert.
 * @return The RGB color.
 */
NCAPI NcRGB NcXYZToRGB(const NcColorSpace* cs, NcXYZ xyz);

[darbyjohnston]

The ABBREVIATE_BRIEF option doesn't do any inference, it's just for filtering text. To automate brief descriptions I was trying the options MULTILINE_CPP_IS_BRIEF=YES and REPEAT_BRIEF=YES, so a comment like this:

/// Converts a XYZ color to RGB color space. The conversion is performed
/// using the provided color space.

Will have the first sentence used for the brief description, and the entire text used for the full description.

I like the @param and @return keywords, though I wonder if we can omit them for simple functions, especially class member get methods?

[meshula]

Sure, why don't we have a look at that, it sounds fine to me.

[darbyjohnston]

After more testing the MULTILINE_CPP_IS_BRIEF option didn't work very well so I turned it off and added explicit @brief markup as you suggested.

I think the detailed description, @param markup, and @return markup should be optional since it adds a lot of boiler plate for simple functions like member get functions. They should just be used where necessary to provide extra information that isn't obvious from the function name.

Some other notes:

• I disabled the Latex and XML output since they are probably not necessary?
• I used /// for comments since it's a bit easier to type than the C style comments, but can change that if there is already a precedent for using the C style comments.

[meshula]

For more complex functions we could use @return, but I don't spot anything here that really warrant's it. I think this is a good pattern to start with.

[darbyjohnston]

I created a new issue since the original Doxygen issue was closed awhile back. Given the amount of work maybe we can use the issue as an umbrella for multiple PRs: #1753

Any ideas on how to fix the CI build, the error doesn't seem related to this change?

[meshula]

The problem in the CI is that pip on windows under msys is failing because distlib can't be found. @reinecke does that ring any bells? I feel like we struggled through something like that last year but the details escape me.

[darbyjohnston]

Strange, it looks like distlib is being installed as part of the msys setup:

installing mingw-w64-x86_64-python-distlib...