QskSkinlet::updateSubNode ownership documentation

[rick-vogel]

What

The ownership of QSkinny must be documented since it is a huge source of crashes / leaks if done incorrectly.

• [ ] add documentation for all QskSkinlet::updateBoxNode
• [ ] add documentation for all QskSkinlet::qskUpdateBoxNode
• [ ] add documentation for QskSkinlet::updateNode
• [ ] add documentation for QskSkinlet::replaceChildNode
• [ ] add [[nodiscard]] to the aforementioned functions

How

Excerpt of the missing qskUpdateBoxNode documentation!

/// @fn qskUpdateBoxNode
/// @brief Updates @p node if @p is visible and @p rect is not empty
/// @details If @p node is nullptr then a new QskBoxNode is allocated and updated
/// @param node The box node to be updated or nullptr for allocation
/// @param rect The box node's rectangle 
/// @param ...
/// @returns Returns the updated @p node is @p is visible and @p rect is not empty
/// @retval nullptr @p node is @p is invisible or @p rect is empty
/// @retval new QskBoxNode @p node is nullptr
///
/// @warning Ownership is transfered to caller

Scenario - Updating e.g. QskBoxNode

using Q = CustomControl;

QSGNode* Skinlet::updateSubNode( const QskSkinnable* skinnable, quint8, 
    QSGNode* const node // owned by caller or nullptr
    ) const
{
    auto* const oldBoxNode = static_cast<QskBoxNode*>(node);

    // if newBoxNode != oldBoxNode then newBoxNode == nullptr ( implementation detail of updateBoxNode/qskUpdateBoxNode )
    auto* const newBoxNode = updateBoxNode(skinnable, oldBoxNode, Q::Box);

    // newBoxNode is owned by callee if oldBoxNode == nullptr
    // newBoxNode is owned by caller if oldBoxNode != nullptr

    if( newBoxNode == nullptr)
    {        
        if( node == nullptr)
        {
            delete oldBoxNode; // node/oldBoxNode owned by callee 
        }
        return nullptr; // node/oldBoxNode owned by caller
    }
    
    return newBoxNode; // ownership transfered to caller
    // see: QskSkinlet::updateNode -> QskSkinlet::replaceChildNode
}

[uwerat]

Some general notes about the documentation:

• it is not done yet and we probably could point at any not self explaining APIs in the same way as we do at QskBoxNode
• I do not like the concept of having the user documentation inside of the source - even if this is the basic idea of doxygen with good arguments. ( My opinion stems from my experience as maintainer of the Qwt lib, that does in-source docs )
• I also do not like how it is has been setup by me below the docs directory, but I dislike it less than the in-source solution
• I was not able to solve specific requirements of qskinny with doxygen - like how to document subcontrols/states in a similar way to what is available for Q_PROPERTY.
• I'm not happy with doxygen as a tool to generate the documentation for QML and C++ from the same source. When going back the history in the git repository you will find a lex/yacc based tool that was used to preprocess the doxygen sources and feed doxygen for the C++ and Qml use cases. Also hated it.

At some point I decided to ignore the Qml side and worked on the documentation of C++ in the way how you find it today. However I needed more than 1 day for a single class and after realizing, that APIs are not stable yet I postponed my work - somehow hoping for advanced versions of doxygen, or finding another tool.

But of cource: a first release has to come with a proper documentation and it is clear, that decisions have to made this year. Then writing the docs is something that will take me months exclusively.

[uwerat]

qskUpdateBoxNode is an inlined function, that is not even visible outside of QskSkinlet.cpp. So it probably makes more sense to write a documentation for the various QskSkinlet::updateBoxNode calls.

All these updateXYZNode calls are static and you do not need an instance of the skinlet to use them. So the signature already states that there can't be any ownership of the skinlet. Returning a nullptr when there is nothing to display is also common for all of these calls.

IMHO it would be better to write these rules in the global section of QskSkinlet - otherwise it has to be duplicated at many places.

[uwerat]

Also important to mention, that ownership of nodes always depends on the QSGNode::OwnedByParent flag, that could have been erased inside one of the updateXYZNode calls.

Inserting a new node and removing an old node is done automatically done when it is the result of updateSubNode. For all other situations the calling code has to take care of it.

[vrcomputing]

"I do not like the concept of having the user documentation inside of the source"

Well, subjectively I don't mind it in source, because my modern IDE can collapse them and derive documentation from them. Having them at the declaration position makes it somewhat more robust in conjunction with doxygen's output ( treated as warnings or error ). Besides that the documentation is in my oppinion an essential part of the contract. Especially in Qt context where we are using pointers everywhere, treating them like references (assuming not null), or ownership transfer, or different returns values...

With the current state we are e.g. forced to document function using the \fn command which from doxygen's perspective is discouraged as a last resort ( see: https://www.doxygen.nl/manual/commands.html#cmdfn )

"I'm not happy with doxygen as a tool to generate the documentation..."

I too don't like it as the generator (especially its indexer), but its syntax is well established and can be used as input for many different tools. Additionally one can integrate the doxygen output into the build process and create warnings for undocumented entities in order to reach a near 100% documentation coverage ( in conjunctionn with a 0 warning tollerance ).

"...like how to document subcontrols/states in a similar way to what is available for Q_PROPERTY"

I believe this is solvable with one or multiple of these:

• https://www.doxygen.nl/manual/custcmd.html
• https://www.doxygen.nl/manual/customize.html
• https://www.doxygen.nl/manual/preprocessing.html

[uwerat]

Well, subjectively I don't mind it in source, because my modern IDE can collapse them and derive documentation from them ...

I'm aware of these arguments and it might be, that the docs finally will go back to the sources. But I'm not desperate enough yet.

Having clean and readable source files has a pretty high value in my standards. Of course this always kind of subjective, but you won't find spaghetti code, many files beyond 1000 lines etc. Blowing up the source files with stupid doxygen boilerplate information ( the vast majority is exactly this ) does not fit this goal:

/*! \property Qt::Orientation QskSeparator::orientation

\brief Orientation of the separator - Qt::Horizontal (the default) or Qt::Vertical.

A separator is often represented by some line - for a
orientation of Qt::Horizontal it might be a vertical line.

\accessors orientation(), setOrientation(), orientationChanged()

*/

/*! \fn void QskSeparator::setOrientation( Qt::Orientation );

Set the orientation of the separator
\param orientation Qt::Vertical or Qt::Horizontal
\sa orientation

*/

/*! \fn Qt::Orientation QskSeparator::orientation() const; \return Value of the \ref orientation property */

/*! \fn void QskSeparator::orientationChanged()

The orientation of the layout has changed
\sa orientation

*/

"...like how to document subcontrols/states in a similar way to what is available for Q_PROPERTY"

I believe this is solvable with one or multiple of these: ...

I tried all the available options that were available back in the days - without success. But to be honest I can't remember exactly what I wanted to achieve. The only problem I have in memory is that doxygen gets confused when the property getter has the same name as the property: the created links always point to the property section.

But if you are interested in this topic we can put it back on the agenda and work out how we want the generated documentation to look alike ?

[rick-vogel]

After using doxygen's preprocessing features this was generated:

I think it is already pretty accurate, since QSkinny subcontrols are not meta objects like Qt's properties.

Maybe I find a way of "grouping" them in a "Subcontrols" "paragraph" like "Properties" or "Static Public Attributes" like in this screenshot

PS: The documentation can also be done elsewhere while keeping the QSK_SUBCONTROLS macro unchanged:

/// @var const QskAspect::Subcontrol QskPushButton::Panel
/// @brief TODO Description of Panel
/// @var const QskAspect::Subcontrol QskPushButton::Splash
/// @brief TODO Description of Splash
/// @var const QskAspect::Subcontrol QskPushButton::Text
/// @brief TODO Description of Text
/// @var const QskAspect::Subcontrol QskPushButton::Icon
/// @brief TODO Description of Icon

Although I still don't like it since we have to repeat the type, scope/class, name which would be generated for us automatically when placing it in source...

[uwerat]

https://qskinny.github.io/docs/classes/classQskSeparator/ is what we achieved back in the days.

The intention was to have a section "Subcontrols" instead of "Public Properties".

This home page is all Peters ( @peter-ha ) work. I like the way how tutorials and doxygen docs are combined into one document, but as I do not know the toolchain being used I can't comment on possibilities/limitations it introduces.

What can be seen easily is that https://qskinny.github.io is not actively maintained. Considering that it comes second, when googeling "qskinny", this is not good.

[rick-vogel]

Using doxygen's aliases I was able to make this more comfortable introducing the custom @subcontrol command. Aliases can have 1-N parameters that can be used in text replacements.

"subcontrol=@var const QskAspect::Subcontrol \1"

/** @name Subcontrols
 *  Subcontrols are unique identifiers for a set of skin hint table entries
 */

///@{

/// @subcontrol {QskPushButton::Panel}
/// @brief TODO Description of Panel

/// @subcontrol {QskPushButton::Splash}
/// @brief TODO Description of Splash

/// @subcontrol {QskPushButton::Text}
/// @brief TODO Description of Text

/// @subcontrol {QskPushButton::Icon}
/// @brief TODO Description of Icon

///@}

Furthermore one can use member grouping to group and highlight their meaning

[rick-vogel]

And when using @nosubgrouping on a class entity we get what we want: