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

hallas added a comment. In D20209#456311 , @aacid wrote: > In D20209#456213 , @hallas wrote: > > > In D20209#455579 , @aacid wrote: > > > > > @hallas

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

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?

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

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

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

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

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/

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

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/

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

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:

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

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

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,

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

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

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,

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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: > > -

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

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

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

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

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

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

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

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

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

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