Add support for KBookmarkOwner to communicate if it has tabs open. This
is used by KBookmarkMenu to only add the Bookmark Tabs as Folder...
entry if the KBookmarkOwner supports tabs and actually has tabs open.
KBookmarkMenu will also keep track of if the state of tabs open changes
and add/remove the menu entry accordingly.
Details
Details
- Reviewers
ngraham cfeck dfaure - Group Reviewers
Frameworks - Commits
- R294:abed768add92: Add support for KBookmarkOwner to communicate if it has tabs open
Unit test added
Diff Detail
Diff Detail
- Repository
- R294 KBookmarks
- Branch
- add_tabs_open (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 11106 Build 11124: arc lint + arc unit
Comment Actions
Thanks for this contribution.
What you did with member variables in the unittest, is generally done with data-driven testing instead -- which I find preferable, better locality and readability. See "Data Driven Testing" in Qt Assistant.
One issue with member vars is that one test method might affect the next one. This is why it's much better to not have any, in test classes.
| src/kbookmarkowner.h | ||
|---|---|---|
| 120 ↗ | (On Diff #55300) | One day we'll probably wonder how many tabs are actually open, to avoid saying "Bookmark Tabs As Folder" if there's only one tab. So I'd prefer for that and make this an int rather than a bool. Bigger problem: this is a public class. You CANNOT add a new virtual method, that is binary incompatible. |
| src/kbookmarkowner.h | ||
|---|---|---|
| 120 ↗ | (On Diff #55300) | Good point (both of them) What are our binary compatibility promises? Is every version of frameworks always binary backwards compatible? Or do we have a window every now and then where we can make source compatible changes but not binary compatible? I don't mind changing this to be using settings and getters, but that just makes this API stand out from the rest. All the other APIs in the class are virtual functions which should be overwritten by the actual owner. I know this does not argue against our binary compatible promise though :D |
| src/kbookmarkowner.h | ||
|---|---|---|
| 120 ↗ | (On Diff #55300) | Every version of KF5 is source and binary compatible (just like Qt5). Two alternative solutions:
|
| src/kbookmarkowner.h | ||
|---|---|---|
| 120 ↗ | (On Diff #55300) | Ok, in that case I think we should definitely go with your first suggestion with getters/settings and maybe writing a note about refactoring it for KF6. I will cook it up and update the patch set, also including the changes to the unit test as you suggested :D |
Comment Actions
Hi @dfaure - I looked into this a bit more and have run into a problem, I hope you can help solve. The problem is that when I add a getter and a setter for the number of open tabs, I need to store the value somewhere in KBookmarkOwner but I guess I cannot add a member variable, since that breaks binary compatibility. But luckily this class has a d-Pointer KBookmarkOwnerPrivate in the member variable d, but there is no definition of KBookmarkOwnerPrivate and therefore it is not constructed anywhere, this is not a problem since I could just make one, the problem is that the virtual destructor for KBookmarkOwner is implemented directly in the header file, and I don't think I can move (and change) the implementation to the .cpp file without breaking binary compatibility? Also I should probably also implement a copy constructor and assignment operator in KBookmarkOwner if we actually use the d-Pointer, right? And this would also break binary compatibility.
I hope you can provide some guidance :D
Comment Actions
Urgh. Indeed. And looking around I find many inline virtuals in apparently public headers... http://www.davidfaure.fr/2019/inline_virtual_dtors.diff (though maybe some of these don't have d pointers at all...)
About KBookmarkOwner, the lack of an explicit constructor is a problem too (apps wouldn't instanciate the d pointer). I just added TODO KF6 comments, for the next person in your position: https://commits.kde.org/kbookmarks/746ecc8db9a04e4d47adc62b0aa03733ca8ecdf8
Meanwhile we have to find another way. Would my idea of adding methods to KBookmarkMenu work out?
Comment Actions
Reworked the patch to avoid any ABI breakage. Now the new functionality is in KBookmarkMenu instead.
Comment Actions
@dfaure - I have now implemented the functionality in KBookmarkMenu instead, so now there should be no ABI breakage :D
Let me know what you think.
Comment Actions
Thanks!
| src/kbookmarkmenu.cpp | ||
|---|---|---|
| 150 | Technically this is only needed if the number of open tabs went from "< 2" to ">= 2" or vice versa. m_bDirty = (d->numberOfOpenTabs < 2) != (numberOfOpenTabs < 2); d->numberOfOpenTabs = numberOfOpenTabs; | |
| src/kbookmarkmenu.h | ||
| 104 | All the "unsigned int" in this patch is a bit unusual in Qt/KDE code, we use int everywhere. | |
Comment Actions
@hallas you seem to have broken compilation https://build.kde.org/job/Frameworks/job/kbookmarks/job/kf5-qt5%20SUSEQt5.10/20/
Comment Actions
@hallas but tests are still failing since your previous commit. Can you have a look? https://build.kde.org/job/Frameworks/job/kbookmarks/job/kf5-qt5%20SUSEQt5.10/21/testReport/
Comment Actions
@aacid - Sorry about that :/ I will take a look at it today - and thanks for fixing compilation!
Comment Actions
Hi @aacid - I can't reproduce the test failure locally! I am running Gentoo Linux with Qt 5.11.3 - do you have any idea what could be wrong? Can you reproduce the issue?
Comment Actions
It may have to do with how much "extra" stuff is there on your system. You can try running the docker infrastructure (needs some space+internet downloads), this should make it easier to reproduce
https://invent.kde.org/sysadmin/ci-tooling/wikis/CI-On-Local-Machine
It works here too but i don't have all the frameworks as of master, maybe that's the issue and a framework this depends on creates the issues?
Comment Actions
Ok I found the problem and have pushed a fix here: D20860
Thanks for notifying about the build failure!