sw/inc/PostItMgr.hxx | 2 sw/qa/extras/tiledrendering/data/comments-on-load.docx |binary sw/qa/extras/tiledrendering/tiledrendering.cxx | 39 +++++++++++++++++ sw/source/uibase/docvw/PostItMgr.cxx | 28 ++++++++++-- 4 files changed, 64 insertions(+), 5 deletions(-)
New commits: commit 3c2975d63e634f8879125e122454197e447332a4 Author: Miklos Vajna <[email protected]> AuthorDate: Fri Apr 11 08:12:35 2025 +0200 Commit: Miklos Vajna <[email protected]> CommitDate: Fri Jun 27 16:22:42 2025 +0200 cool#11592 sw lok: fix sometimes lost comments on load Load a document with the LOK API, execute getCommandValues(".uno:ViewAnnotations"), then listen for comment changes. If the document is complex enough, some comments won't be included anyhwere: neither in the initial comment list, nor as a notification later. What happens is that SwXTextDocument::getPostIts() iterates over the postits of the postit manager, but in case the comment is not yet laid out (pWin is nullptr), then we just ignore that comment when creating the initial comment list. But then SwPostItMgr::LayoutPostIts() won't emit notifications about these comments, either -- assuming these comments are not new. So if the LOK client is fast enough to execute getCommandValues() and the core is slow enough with the layout of the comments, then some initial comments in the documents are lost (they are in the document, but they are not visible). Fix the problem by extending SwPostItMgr::GetOrCreateAnnotationWindow() to give info when it created the annotation window, and then in SwPostItMgr::LayoutPostIts() notify about both freshly laid out comments and about explicitly inserted comments. Adding a bit of debug output confirms that now comments show up even if we hit the unlucky "no annotation window yet" case when testing manually: debug:31555:31497: SwXTextDocument::getPostIts: pWin is 0 debug:31555:31497: SwPostItMgr::LayoutPostIts: lok notify, type is add The first is the state right after load, the second is called by SwPostItMgr::CalcHdl() as a user event ("on idle"); Change-Id: I32082d0014454bd492d2ae87fea42e28b639e8e5 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/183999 Tested-by: Jenkins CollaboraOffice <[email protected]> Tested-by: Caolán McNamara <[email protected]> Reviewed-by: Caolán McNamara <[email protected]> Reviewed-on: https://gerrit.libreoffice.org/c/core/+/187057 Reviewed-by: Miklos Vajna <[email protected]> diff --git a/sw/inc/PostItMgr.hxx b/sw/inc/PostItMgr.hxx index 823fdb375383..b0299cd7e887 100644 --- a/sw/inc/PostItMgr.hxx +++ b/sw/inc/PostItMgr.hxx @@ -172,7 +172,7 @@ class SAL_DLLPUBLIC_RTTI SwPostItMgr final : public SfxListener SwSidebarItem* InsertItem( SfxBroadcaster* pItem, bool bCheckExistence, bool bFocus); void RemoveItem( SfxBroadcaster* pBroadcast ); - VclPtr<sw::annotation::SwAnnotationWin> GetOrCreateAnnotationWindow(SwSidebarItem& rItem); + VclPtr<sw::annotation::SwAnnotationWin> GetOrCreateAnnotationWindow(SwSidebarItem& rItem, bool& rCreated); public: SwPostItMgr(SwView* aDoc); diff --git a/sw/qa/extras/tiledrendering/data/comments-on-load.docx b/sw/qa/extras/tiledrendering/data/comments-on-load.docx new file mode 100644 index 000000000000..858817730ff2 Binary files /dev/null and b/sw/qa/extras/tiledrendering/data/comments-on-load.docx differ diff --git a/sw/qa/extras/tiledrendering/tiledrendering.cxx b/sw/qa/extras/tiledrendering/tiledrendering.cxx index 14afbd430818..a64387605570 100644 --- a/sw/qa/extras/tiledrendering/tiledrendering.cxx +++ b/sw/qa/extras/tiledrendering/tiledrendering.cxx @@ -793,6 +793,7 @@ namespace { boost::property_tree::ptree m_aRedlineTableModified; /// Post-it / annotation payload. boost::property_tree::ptree m_aComment; + int m_nCommentCallbackCount = 0; std::vector<OString> m_aStateChanges; TestLokCallbackWrapper m_callbackWrapper; OString m_aExportFile; @@ -966,6 +967,7 @@ namespace { break; case LOK_CALLBACK_COMMENT: { + ++m_nCommentCallbackCount; m_aComment.clear(); std::stringstream aStream(pPayload); boost::property_tree::read_json(aStream, m_aComment); @@ -5150,6 +5152,43 @@ CPPUNIT_TEST_FIXTURE(SwTiledRenderingTest, testTrackChangesInsertUndo) CPPUNIT_ASSERT(dynamic_cast<SfxBoolItem*>(pItem.get())->GetValue()); } +CPPUNIT_TEST_FIXTURE(SwTiledRenderingTest, testCommentsOnLoad) +{ + // Given a document of 3 pages, with a small enough visible area that document load doesn't lay + // out the entire document: + awt::Rectangle aVisibleArea{ 0, 0, 12240, 15840 }; + comphelper::LibreOfficeKit::setInitialClientVisibleArea(aVisibleArea); + comphelper::ScopeGuard g([] { comphelper::LibreOfficeKit::setInitialClientVisibleArea({}); }); + SwXTextDocument* pXTextDocument = createDoc("comments-on-load.docx"); + ViewCallback aView; + tools::JsonWriter aWriter; + + // When getting the list of comments from the document + listening for notifications from idle + // layout: + pXTextDocument->getPostIts(aWriter); + Scheduler::ProcessEventsToIdle(); + + // Then make sure that: + // 1) We test the interesting scenario, so getPostIts() reports 0 comments and + // 2) A callback is emitted to notify about the comment on the last page once user events are + // processed. + OString aPostIts = aWriter.finishAndGetAsOString(); + std::string aPostItsStr(aPostIts); + std::stringstream aStream(aPostItsStr); + boost::property_tree::ptree aTree; + boost::property_tree::read_json(aStream, aTree); + size_t nCommentCount = aTree.get_child("comments").size(); + CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(0), nCommentCount); + CPPUNIT_ASSERT_EQUAL(1, aView.m_nCommentCallbackCount); + // Without the accompanying fix in place, this test would have failed with: + // uncaught exception of type std::exception (or derived). + // - No such node (action) + // i.e. there was no notification about the comment that was positioned by the user event, + // seemingly the comment was load on load (it was there, but not visible). + auto aAction = aView.m_aComment.get_child("action").get_value<std::string>(); + CPPUNIT_ASSERT_EQUAL(std::string("Add"), aAction); +} + CPPUNIT_PLUGIN_IMPLEMENT(); /* vim:set shiftwidth=4 softtabstop=4 expandtab: */ diff --git a/sw/source/uibase/docvw/PostItMgr.cxx b/sw/source/uibase/docvw/PostItMgr.cxx index b46015e077d0..4ce502d40ad0 100644 --- a/sw/source/uibase/docvw/PostItMgr.cxx +++ b/sw/source/uibase/docvw/PostItMgr.cxx @@ -81,6 +81,7 @@ #include <annotsh.hxx> #include <swabstdlg.hxx> #include <memory> +#include <o3tl/temporary.hxx> // distance between Anchor Y and initial note position #define POSTIT_INITIAL_ANCHOR_DISTANCE 20 @@ -875,7 +876,7 @@ void SwPostItMgr::PreparePageContainer() } } -VclPtr<SwAnnotationWin> SwPostItMgr::GetOrCreateAnnotationWindow(SwSidebarItem& rItem) +VclPtr<SwAnnotationWin> SwPostItMgr::GetOrCreateAnnotationWindow(SwSidebarItem& rItem, bool& rCreated) { VclPtr<SwAnnotationWin> pPostIt = rItem.mpPostIt; if (!pPostIt) @@ -893,6 +894,8 @@ VclPtr<SwAnnotationWin> SwPostItMgr::GetOrCreateAnnotationWindow(SwSidebarItem& } mpAnswer.reset(); } + + rCreated = true; } return rItem.mpPostIt; } @@ -907,6 +910,7 @@ void SwPostItMgr::LayoutPostIts() if (bEnableMapMode) mpEditWin->EnableMapMode(); + std::set<VclPtr<SwAnnotationWin>> aCreatedPostIts; if ( !mvPostItFields.empty() && !mbWaitingForCalcRects ) { mbLayouting = true; @@ -928,7 +932,14 @@ void SwPostItMgr::LayoutPostIts() { if (pItem->mbShow) { - VclPtr<SwAnnotationWin> pPostIt = GetOrCreateAnnotationWindow(*pItem); + bool bCreated = false; + VclPtr<SwAnnotationWin> pPostIt = GetOrCreateAnnotationWindow(*pItem, bCreated); + if (bCreated) + { + // The annotation window was created for a previously existing, but not + // laid out comment. + aCreatedPostIts.insert(pPostIt); + } pPostIt->SetChangeTracking( pItem->mLayoutStatus, @@ -1086,7 +1097,10 @@ void SwPostItMgr::LayoutPostIts() if (bLoKitActive && !bTiledAnnotations) { if (visiblePostIt->GetSidebarItem().mbPendingLayout && visiblePostIt->GetSidebarItem().mLayoutStatus != SwPostItHelper::DELETED) - lcl_CommentNotification(mpView, CommentNotificationType::Add, &visiblePostIt->GetSidebarItem(), 0); + { + // Notify about a just inserted comment. + aCreatedPostIts.insert(visiblePostIt); + } else if (visiblePostIt->IsAnchorRectChanged()) { lcl_CommentNotification(mpView, CommentNotificationType::Modify, &visiblePostIt->GetSidebarItem(), 0); @@ -1142,6 +1156,12 @@ void SwPostItMgr::LayoutPostIts() mbLayouting = false; } + // Now that comments are laid out, notify about freshly laid out or just inserted comments. + for (const auto& pPostIt : aCreatedPostIts) + { + lcl_CommentNotification(mpView, CommentNotificationType::Add, &pPostIt->GetSidebarItem(), 0); + } + if (bEnableMapMode) mpEditWin->EnableMapMode(false); } @@ -1920,7 +1940,7 @@ SwPostItField* SwPostItMgr::GetLatestPostItField() sw::annotation::SwAnnotationWin* SwPostItMgr::GetOrCreateAnnotationWindowForLatestPostItField() { - return GetOrCreateAnnotationWindow(*mvPostItFields.back()); + return GetOrCreateAnnotationWindow(*mvPostItFields.back(), o3tl::temporary(bool())); } SwAnnotationWin* SwPostItMgr::GetNextPostIt( sal_uInt16 aDirection,
