D25660: Decouple KBookmarksMenu from KActionCollection

[Friedrich W. H. Kossebau]

kossebau added a comment.


  Some API (docs) nitpicks :)

INLINE COMMENTS

> kbookmarkmenu.cpp:88
> +    m_parentMenu(_parentMenu),
> +    m_parentAddress(QLatin1String(""))   //TODO KBookmarkAdress::root
> +{

Why the `QLatin1String("")` instead of QString()? Is there a need for a non-null empty string? It#s copied from the other constructor, but should get a comment while at it if that is the case, or just get to be QString()

> kbookmarkmenu.h:89
>       */
> +#if KBOOKMARKS_ENABLE_DEPRECATED_SINCE(5, 65)
> +    KBOOKMARKS_DEPRECATED_VERSION(5, 65, "Use overload without KActionCollection and add actions manually to your actioncollection if desired")

The visibility guard for deprecated methods should start before the API dox , if only for consistency, so please move to front.

See https://community.kde.org/Policies/Library_Code_Policy#Deprecation_of_API

> kbookmarkmenu.h:99
> +     *
> +     * @param mgr The bookmark manager to use (i.e. for reading and writing)
> +     * @param owner implementation of the KBookmarkOwner callback interface.

-> lowercase "the", cmp. https://community.kde.org/Frameworks/Frameworks_Documentation_Policy#Document_Public_and_Protected_Members

> kbookmarkmenu.h:101
> +     * @param owner implementation of the KBookmarkOwner callback interface.
> +     * Note: If you pass a null KBookmarkOwner to the constructor, the
> +     * openBookmark signal is not emitted, instead QDesktopServices::openUrl is used to open the bookmark.

`Note:` -> `@note`

> kbookmarkmenu.h:104
> +     * @param parentMenu menu to be filled
> +     * @param collec parent collection for the KActions.
> +     * @since 5.65

No such parameter "collec"?

> kbookmarkmenu.h:107
> +     */
> +    KBookmarkMenu(KBookmarkManager *mgr, KBookmarkOwner *owner, QMenu *parentMenu);
>  

"mgr" -> "manager" while at it, please

> kbookmarkmenu.h:144
> +    /**
> +     * Action for adding a bookmark. If you are using KXMLGui add it to your action collection.
> +     * @code

Official casing is "KXmlGui" :) Also below.

> kbookmarkmenu.h:144
> +    /**
> +     * Action for adding a bookmark. If you are using KXMLGui add it to your action collection.
> +     * @code

This being a method, not a property "Action which is" is not the perfect text, please describe what the method does (getting access to the action owned by the menu).

> kbookmarkmenu.h:149
> +     * @endcode
> +     * @since 5.65
> +     */

Please add a `@return ...` here and to the other methods, so e.g. IDEs can have better metadata avaialble.

> kbookmarkmenu.h:151
> +     */
> +    QAction *addBookmarkAction();
> +

Can be const methods, no?

REPOSITORY
  R294 KBookmarks

REVISION DETAIL
  https://phabricator.kde.org/D25660

To: nicolasfella, #frameworks, dfaure
Cc: kossebau, dfaure, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20200112/20be7edb/attachment.html>