D20209: Add support for KBookmarkOwner to communicate if it has tabs open

[David Hallas]

hallas added a comment.


  In D20209#456311 , @aacid wrote:
  
  > In D20209#456213 , @hallas wrote:
  >
  > > In D20209#455579 , @aacid 
wrote:
  > >
  > > > @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/
  > >
  > >
  > > 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?
  >
  >
  > 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?
  
  
  Ok I found the problem and have pushed a fix here: D20860 

  
  Thanks for notifying about the build failure!

REPOSITORY
  R294 KBookmarks

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

To: hallas, #frameworks, ngraham, cfeck, dfaure
Cc: aacid, kde-frameworks-devel, michaelh, ngraham, bruns

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

[Albert Astals Cid]

aacid added a comment.


  In D20209#456213 , @hallas wrote:
  
  > In D20209#455579 , @aacid wrote:
  >
  > > @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/
  >
  >
  > 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?
  
  
  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?

REPOSITORY
  R294 KBookmarks

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

To: hallas, #frameworks, ngraham, cfeck, dfaure
Cc: aacid, kde-frameworks-devel, michaelh, ngraham, bruns

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

[Albert Astals Cid]

aacid added a comment.


  In D20209#456214 , @ngraham wrote:
  
  > The failing one is using Qt 5.10 FWIW
  
  
  5.12 fails too 
https://build.kde.org/job/Frameworks/job/kbookmarks/job/kf5-qt5%20SUSEQt5.12/18/testReport/

REPOSITORY
  R294 KBookmarks

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

To: hallas, #frameworks, ngraham, cfeck, dfaure
Cc: aacid, kde-frameworks-devel, michaelh, ngraham, bruns

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

[Nathaniel Graham]

ngraham added a comment.


  The failing one is using Qt 5.10 FWIW

REPOSITORY
  R294 KBookmarks

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

To: hallas, #frameworks, ngraham, cfeck, dfaure
Cc: aacid, kde-frameworks-devel, michaelh, ngraham, bruns

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

[David Hallas]

hallas added a comment.


  In D20209#455579 , @aacid wrote:
  
  > @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/
  
  
  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?

REPOSITORY
  R294 KBookmarks

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

To: hallas, #frameworks, ngraham, cfeck, dfaure
Cc: aacid, kde-frameworks-devel, michaelh, ngraham, bruns

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

[David Hallas]

hallas added a comment.


  In D20209#455579 , @aacid wrote:
  
  > @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/
  
  
  @aacid - Sorry about that :/ I will take a look at it today - and thanks for 
fixing compilation!

REPOSITORY
  R294 KBookmarks

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

To: hallas, #frameworks, ngraham, cfeck, dfaure
Cc: aacid, kde-frameworks-devel, michaelh, ngraham, bruns

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

[Albert Astals Cid]

aacid added a comment.


  @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/

REPOSITORY
  R294 KBookmarks

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

To: hallas, #frameworks, ngraham, cfeck, dfaure
Cc: aacid, kde-frameworks-devel, michaelh, ngraham, bruns

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

[Albert Astals Cid]

aacid added a comment.


  I just fixed it FWIW

REPOSITORY
  R294 KBookmarks

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

To: hallas, #frameworks, ngraham, cfeck, dfaure
Cc: aacid, kde-frameworks-devel, michaelh, ngraham, bruns

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

[Albert Astals Cid]

aacid added a comment.


  @hallas you seem to have broken compilation 
https://build.kde.org/job/Frameworks/job/kbookmarks/job/kf5-qt5%20SUSEQt5.10/20/

REPOSITORY
  R294 KBookmarks

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

To: hallas, #frameworks, ngraham, cfeck, dfaure
Cc: aacid, kde-frameworks-devel, michaelh, ngraham, bruns

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

[David Hallas]

hallas closed this revision.

REPOSITORY
  R294 KBookmarks

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

To: hallas, #frameworks, ngraham, cfeck, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

[David Faure]

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

REPOSITORY
  R294 KBookmarks

BRANCH
  add_tabs_open (branched from master)

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

To: hallas, #frameworks, ngraham, cfeck, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

[David Hallas]

hallas added inline comments.

INLINE COMMENTS

> dfaure wrote in kbookmarkmenu.cpp:150
> Technically this is only needed if the number of open tabs went from "< 2" to 
> ">= 2" or vice versa.
> When going from, say, 20 to 21, we don't need to refill the menu.
> So the code could be
> 
>   m_bDirty = (d->numberOfOpenTabs < 2) != (numberOfOpenTabs < 2);
>   d->numberOfOpenTabs = numberOfOpenTabs;

Good point :D

> dfaure wrote in kbookmarkmenu.h:104
> All the "unsigned int" in this patch is a bit unusual in Qt/KDE code, we use 
> int everywhere.

Ok - I have changed it to use int all over instead.

REPOSITORY
  R294 KBookmarks

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

To: hallas, #frameworks, ngraham, cfeck, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

[David Hallas]

hallas updated this revision to Diff 56837.
hallas marked an inline comment as done.
hallas added a comment.


  Review comments

REPOSITORY
  R294 KBookmarks

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20209?vs=56744=56837

BRANCH
  add_tabs_open (branched from master)

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

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

To: hallas, #frameworks, ngraham, cfeck, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

[David Faure]

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


  Thanks!

INLINE COMMENTS

> kbookmarkmenu.cpp:150
> +d->numberOfOpenTabs = numberOfOpenTabs;
> +m_bDirty = true;
> +}

Technically this is only needed if the number of open tabs went from "< 2" to 
">= 2" or vice versa.
When going from, say, 20 to 21, we don't need to refill the menu.
So the code could be

  m_bDirty = (d->numberOfOpenTabs < 2) != (numberOfOpenTabs < 2);
  d->numberOfOpenTabs = numberOfOpenTabs;

> kbookmarkmenu.h:104
> + */
> +void setNumberOfOpenTabs(unsigned int numberOfOpenTabs);
> +/**

All the "unsigned int" in this patch is a bit unusual in Qt/KDE code, we use 
int everywhere.

REPOSITORY
  R294 KBookmarks

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

To: hallas, #frameworks, ngraham, cfeck, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

[David Hallas]

hallas added a comment.


  In D20209#450100 , @dfaure wrote:
  
  > 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?
  
  
  @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.

REPOSITORY
  R294 KBookmarks

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

To: hallas, #frameworks, ngraham, cfeck, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

[David Hallas]

hallas updated this revision to Diff 56744.
hallas added a comment.


  Reworked the patch to avoid any ABI breakage. Now the new functionality is in 
KBookmarkMenu instead.

REPOSITORY
  R294 KBookmarks

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20209?vs=55300=56744

BRANCH
  add_tabs_open (branched from master)

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

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

To: hallas, #frameworks, ngraham, cfeck, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

[David Faure]

dfaure added a comment.


  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?

REPOSITORY
  R294 KBookmarks

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

To: hallas, #frameworks, ngraham, cfeck, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

[David Hallas]

hallas added a comment.


  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

REPOSITORY
  R294 KBookmarks

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

To: hallas, #frameworks, ngraham, cfeck, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

[David Hallas]

hallas added inline comments.

INLINE COMMENTS

> dfaure wrote in kbookmarkowner.h:120
> Every version of KF5 is source and binary compatible (just like Qt5).
> The next time for a possible BC breakage will be KF6 (when Qt6 comes out), so 
> not just yet.
> 
> Two alternative solutions:
> 
> - creating a KBookmarkOwnerV2 interface, deriving from KBookmarkOwner. This 
> is a typical solution for extending interfaces, but it's somewhat cumbersome 
> (ugly name, downcasting...).
> - adding a getter/setter to another class where it would make more sense, 
> maybe KBookmarkMenu? Would the app that you have in mind, have access to it?

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

REPOSITORY
  R294 KBookmarks

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

To: hallas, #frameworks, ngraham, cfeck, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> hallas wrote in kbookmarkowner.h:120
> 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

Every version of KF5 is source and binary compatible (just like Qt5).
The next time for a possible BC breakage will be KF6 (when Qt6 comes out), so 
not just yet.

Two alternative solutions:

- creating a KBookmarkOwnerV2 interface, deriving from KBookmarkOwner. This is 
a typical solution for extending interfaces, but it's somewhat cumbersome (ugly 
name, downcasting...).
- adding a getter/setter to another class where it would make more sense, maybe 
KBookmarkMenu? Would the app that you have in mind, have access to it?

REPOSITORY
  R294 KBookmarks

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

To: hallas, #frameworks, ngraham, cfeck, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

[David Hallas]

hallas added inline comments.

INLINE COMMENTS

> dfaure wrote in kbookmarkowner.h:120
> 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.
> You have to do this with a setter and a getter (and putting the member 
> variable behind the d pointer).

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

REPOSITORY
  R294 KBookmarks

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

To: hallas, #frameworks, ngraham, cfeck, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

[David Faure]

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


  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.

INLINE COMMENTS

> kbookmarkowner.h:120
> + */
> +virtual bool tabsOpen() const
> +{

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.
You have to do this with a setter and a getter (and putting the member variable 
behind the d pointer).

REPOSITORY
  R294 KBookmarks

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

To: hallas, #frameworks, ngraham, cfeck, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

[Nathaniel Graham]

ngraham added reviewers: cfeck, dfaure.

REPOSITORY
  R294 KBookmarks

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

To: hallas, #frameworks, ngraham, cfeck, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

[David Hallas]

hallas added a comment.


  Ping - anyone ;)

REPOSITORY
  R294 KBookmarks

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

To: hallas, #frameworks, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D20209: Add support for KBookmarkOwner to communicate if it has tabs open

[David Hallas]

hallas created this revision.
hallas added reviewers: Frameworks, ngraham.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
hallas requested review of this revision.

REVISION SUMMARY
  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.

TEST PLAN
  Unit test added

REPOSITORY
  R294 KBookmarks

BRANCH
  add_tabs_open (branched from master)

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kbookmarkmenutest.cpp
  src/kbookmarkmenu.cpp
  src/kbookmarkmenu.h
  src/kbookmarkowner.h

To: hallas, #frameworks, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns