The usage of KActionCollection makes KBookmarks depend on KXMLGui. To allow to get rid of this rather heavy dependency for KF6 a new constructor is introduced that does not take a KActionCollection. Instead apps that use KBookmarksMenu and KXMLGui are encouraged to manually add the relevant actions to their action collection. It's not pretty, but better than depending on KXMLGui, especially given that KIO depends on KBookmarks and thus on KXMLGui.
Details
Details
- Reviewers
dfaure - Group Reviewers
Frameworks - Maniphest Tasks
- T12056: KBookmarks
- Commits
- R294:0c888e2f8098: Decouple KBookmarksMenu from KActionCollection
Tested with patched Dolphin (new constructor is used) and unpatched Konsole.
Diff Detail
Diff Detail
- Repository
- R294 KBookmarks
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
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.
| src/kbookmarkmenu.cpp | ||
|---|---|---|
| 338 | 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. | |
Comment Actions
Some API (docs) nitpicks :)
| src/kbookmarkmenu.cpp | ||
|---|---|---|
| 89 | 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() | |
| src/kbookmarkmenu.h | ||
| 94 | 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 | |
| 103 | ||
| 105 | Note: -> @note | |
| 108 | No such parameter "collec"? | |
| 111 | "mgr" -> "manager" while at it, please | |
| 147 | Official casing is "KXmlGui" :) Also below. | |
| 147 | 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). | |
| 152 | Please add a @return ... here and to the other methods, so e.g. IDEs can have better metadata avaialble. | |
| 154 | Can be const methods, no? | |
Comment Actions
- 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
Comment Actions
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.
Comment Actions
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?)
Comment Actions
It is not the macro, but using it with a future version of 0x060000.
See https://marc.info/?l=kde-devel&m=157190321318565&w=2 for details.
Comment Actions
@mlaurent AFAICS you removed the deprecation macro completely in master, and left it (with 0x060000) 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.
Comment Actions
Just as reminder:
When keeping the use of the macro flags, make sure to
- also set the *_DEPRECATED_WARNINGS_SINCE flags to 0x060000, 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=0x060000
# 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=0x060000
)Comment Actions
nope or I missed to forward port in 20.04. So it's an error.
but by default There is not some 0x060000 in 20.04