D25660: Decouple KBookmarksMenu from KActionCollection

[Laurent Montel]

mlaurent added a comment.


  In D25660#635693 , @dfaure wrote:
  
  > @mlaurent AFAICS you removed the deprecation macro completely in master, 
and left it (with 0x06) in release/20.04?
  
  
  nope or I missed to forward port in 20.04. So it's an error.
  but by default There is not some 0x06 in 20.04

REPOSITORY
  R294 KBookmarks

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

To: nicolasfella, #frameworks, dfaure
Cc: mlaurent, bcooksley, kossebau, dfaure, apol, kde-frameworks-devel, 
LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D25660: Decouple KBookmarksMenu from KActionCollection

[Friedrich W. H. Kossebau]

kossebau added a comment.


  Just as reminder:
  When keeping the use of the macro flags, make sure to
  
  - also set the `*_DEPRECATED_WARNINGS_SINCE` flags to 0x06, otherwise 
deprecation warnings for newer API will no longer be shown, due to default set 
by `*_DISABLE_DEPRECATED_BEFORE*`
  - remove the `if (EXISTS "${CMAKE_SOURCE_DIR}/.git")` wrapper, to ensure 
released builds see the same API as developer builds (chance of running 
otherwise into method overloads/implicit conversions)
  
  This would be the recommended use to control limit of deprecated API not 
visible to compiler and limit of until which version use of deprecated API will 
be warned about:
  
add_definitions(
# hide deprecated API of Qt <= 5.9
-DQT_DISABLE_DEPRECATED_BEFORE=0x050900 
# enable warnings for API up to Qt 6.0
-DQT_DEPRECATED_WARNINGS_SINCE=0x06
# hide deprecated API of KF <= 5.48
-DKF_DISABLE_DEPRECATED_BEFORE_AND_AT=0x053000
# enable warnings for API up to KF 6.0
-DKF_DEPRECATED_WARNINGS_SINCE=0x06
)

REPOSITORY
  R294 KBookmarks

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

To: nicolasfella, #frameworks, dfaure
Cc: mlaurent, bcooksley, kossebau, dfaure, apol, kde-frameworks-devel, 
LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D25660: Decouple KBookmarksMenu from KActionCollection

[David Faure]

dfaure added a subscriber: mlaurent.
dfaure added a comment.


  @mlaurent AFAICS you removed the deprecation macro completely in master, and 
left it (with 0x06) in release/20.04?
  
  I thought we wanted
  
  - to remove it in release/20.04 (or set it to a tested value like 0x053A00)
  - to set it to a tested value in master so we can increase it regularly 
(after verifying compilation)
  
  If you don't have a release/20.04 checkout I can look into making the changes 
there.

REPOSITORY
  R294 KBookmarks

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

To: nicolasfella, #frameworks, dfaure
Cc: mlaurent, bcooksley, kossebau, dfaure, apol, kde-frameworks-devel, 
LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D25660: Decouple KBookmarksMenu from KActionCollection

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D25660#635658 , @bcooksley 
wrote:
  
  > Thanks. That macro really does cause quite a bit of trouble...
  
  
  It is not the macro, but using it with a future version of 0x06.
  
  See https://marc.info/?l=kde-devel&m=157190321318565&w=2 for details.

REPOSITORY
  R294 KBookmarks

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

To: nicolasfella, #frameworks, dfaure
Cc: bcooksley, kossebau, dfaure, apol, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D25660: Decouple KBookmarksMenu from KActionCollection

[Ben Cooksley]

bcooksley added a comment.


  Thanks. That macro really does cause quite a bit of trouble...

REPOSITORY
  R294 KBookmarks

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

To: nicolasfella, #frameworks, dfaure
Cc: bcooksley, kossebau, dfaure, apol, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D25660: Decouple KBookmarksMenu from KActionCollection

[David Faure]

dfaure added a comment.


  As usual, it's the too strong "no deprecated API usage" define in the apps.
  
  Fixed.

REPOSITORY
  R294 KBookmarks

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

To: nicolasfella, #frameworks, dfaure
Cc: bcooksley, kossebau, dfaure, apol, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D25660: Decouple KBookmarksMenu from KActionCollection

[Ben Cooksley]

bcooksley added a comment.


  Could this commit be the cause of 
https://build.kde.org/job/Applications/job/krdc/job/stable-kf5-qt5%20FreeBSDQt5.14/3/consoleText
  (ie. this commit is SIC?)

REPOSITORY
  R294 KBookmarks

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

To: nicolasfella, #frameworks, dfaure
Cc: bcooksley, kossebau, dfaure, apol, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, ngraham, bruns

D25660: Decouple KBookmarksMenu from KActionCollection

[Nicolas Fella]

This revision was automatically updated to reflect the committed changes.
Closed by commit R294:0c888e2f8098: Decouple KBookmarksMenu from 
KActionCollection (authored by nicolasfella).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D25660?vs=73666&id=78402#toc

REPOSITORY
  R294 KBookmarks

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25660?vs=73666&id=78402

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

AFFECTED FILES
  src/CMakeLists.txt
  src/kbookmarkmenu.cpp
  src/kbookmarkmenu.h

To: nicolasfella, #frameworks, dfaure
Cc: kossebau, dfaure, apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, ngraham, bruns

D25660: Decouple KBookmarksMenu from KActionCollection

[David Faure]

dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.


  Oops I forgot to review this again after your last changes.
  
  Looks fine to me now, but meanwhile it needs s/5.67/5.69/ everywhere (docu 
and deprecation macros).
  Feel free to land if you check that there's no 67 anywhere in the diff.

REPOSITORY
  R294 KBookmarks

BRANCH
  addd

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

To: nicolasfella, #frameworks, dfaure
Cc: kossebau, dfaure, apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, 
michaelh, ngraham, bruns

D25660: Decouple KBookmarksMenu from KActionCollection

[Nicolas Fella]

nicolasfella updated this revision to Diff 73666.
nicolasfella marked 11 inline comments as done.
nicolasfella added a comment.


  - Decouple from KActionCollection
  - Set action name
  - Restore root check
  - Use constructor delegation
  - Update since
  - fix docs
  - Docs fixes
  - Use QString()
  - Make methods const
  - Rename parameter

REPOSITORY
  R294 KBookmarks

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25660?vs=70694&id=73666

BRANCH
  addd

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

AFFECTED FILES
  src/CMakeLists.txt
  src/kbookmarkmenu.cpp
  src/kbookmarkmenu.h

To: nicolasfella, #frameworks, dfaure
Cc: kossebau, dfaure, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
ngraham, bruns

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

D25660: Decouple KBookmarksMenu from KActionCollection

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  Thanks for the patch.
  
  Please call setObjectName() on the actions so that their object name is fixed 
(no matter which ctor is used).  (this is because KActionCollection::addAction 
calls setObjectName on the actions)
  
  This can even help apps to just get all 3 actions and put them into an action 
collection without having to copy the standard names from the documentation.
  
QAction *addAction = menu->addBookmarkAction();
actionCollection()->addAction(addAction->objectName(), addAction);
  
  Maybe you can use the C++11 feature of one ctor calling another to avoid the 
setup() method which will look even more strange once the deprecated ctor is 
removed.
  
  The @since value needs to be updated obviously.

INLINE COMMENTS

> kbookmarkmenu.cpp:347
> +
> +if (m_actionCollection) {
> +m_actionCollection->addAction(QStringLiteral("add_bookmark"), 
> d->addAddBookmark);

The m_bIsRoot check disappeared in the refactoring!

Actions in submenus should not be named.

Same thing for the action named add_bookmarks_list above.

REPOSITORY
  R294 KBookmarks

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

To: nicolasfella, #frameworks, dfaure
Cc: dfaure, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D25660: Decouple KBookmarksMenu from KActionCollection

[Nicolas Fella]

nicolasfella updated this revision to Diff 70694.
nicolasfella added a comment.


Rebase

REPOSITORY
  R294 KBookmarks

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25660?vs=70693&id=70694

BRANCH
  addd

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

AFFECTED FILES
  src/kbookmarkmenu.cpp
  src/kbookmarkmenu.h

To: nicolasfella, #frameworks
Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25660: Decouple KBookmarksMenu from KActionCollection

[Nicolas Fella]

nicolasfella retitled this revision from "Add member for editbookmarks action" 
to "Decouple KBookmarksMenu from KActionCollection".
nicolasfella edited the summary of this revision.
nicolasfella edited the test plan for this revision.

REPOSITORY
  R294 KBookmarks

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

To: nicolasfella, #frameworks
Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns