sc/inc/postit.hxx | 121 +++++++- sc/qa/unit/ucalc.cxx | 89 ++++- sc/qa/unit/ucalc.hxx | 5 sc/qa/unit/ucalc_sort.cxx | 3 sc/source/core/data/postit.cxx | 568 +++++++++++++++++++++++++++++++------- sc/source/filter/xml/xmlcelli.cxx | 2 sc/source/ui/docshell/docfunc.cxx | 2 sc/source/ui/inc/notemark.hxx | 4 sc/source/ui/undo/undocell.cxx | 25 + sc/source/ui/view/drawview.cxx | 2 sc/source/ui/view/notemark.cxx | 22 - sc/source/ui/view/viewdata.cxx | 6 12 files changed, 697 insertions(+), 152 deletions(-)
New commits: commit 601898857272de205e8dfcc0be0bae590babae75 Author: Eike Rathke <[email protected]> Date: Thu Apr 13 00:46:33 2017 +0200 finally switch the workaround off Change-Id: I284292a2749c2b38ef874315d5b526e403d578e8 diff --git a/sc/source/core/data/postit.cxx b/sc/source/core/data/postit.cxx index 2ac98a57cc0b..044393426ce1 100644 --- a/sc/source/core/data/postit.cxx +++ b/sc/source/core/data/postit.cxx @@ -697,8 +697,9 @@ void ScCaptionPtr::decRefAndDestroy() assert(!mpNext); // this must be one and only one assert(mpCaption); -#if 1 - // FIXME: there are still cases where the caption pointer is dangling +#if 0 + // Quick workaround for when there are still cases where the caption + // pointer is dangling mpCaption = nullptr; mbNotOwner = false; #else commit 23d06332abb81f4eef2a5dcc63fab1ce9d42aaaa Author: Eike Rathke <[email protected]> Date: Thu Apr 13 00:34:34 2017 +0200 turn assert into SAL_INFO The old assert conditions don't hold anymore since removeFromDrawPageAndFree() only deletes the SdrCaptionObj on the last refcount, but info can be useful. Change-Id: I456149b8799a0509dcd7a2da09d627fb0de1a912 diff --git a/sc/source/core/data/postit.cxx b/sc/source/core/data/postit.cxx index 310bddfa606f..2ac98a57cc0b 100644 --- a/sc/source/core/data/postit.cxx +++ b/sc/source/core/data/postit.cxx @@ -1138,20 +1138,16 @@ void ScPostIt::RemoveCaption() if (pDrawLayer == maNoteData.mxCaption->GetModel()) maNoteData.mxCaption.removeFromDrawPageAndFree(); -#if 0 - // Either the caption object is gone or, because of Undo or clipboard is - // held in at least two instances, or only one instance in Undo because the - // original sheet in this document is just deleted, or the Undo document is - // just destroyed which leaves us with one reference. - // Let's detect other use cases.. - assert(!maNoteData.mxCaption || maNoteData.mxCaption.getRefs() >= 2 || - (!mrDoc.IsUndo() && !mrDoc.IsClipboard()) || (mrDoc.IsUndo() && mrDoc.IsInDtorClear())); -#endif + SAL_INFO("sc.core","ScPostIt::RemoveCaption - refs: " << maNoteData.mxCaption.getRefs() << + " IsUndo: " << mrDoc.IsUndo() << " IsClip: " << mrDoc.IsClipboard() << + " Dtor: " << mrDoc.IsInDtorClear()); - // Forget the caption object if removeFromDrawPageAndFree() above did not - // free it. + // Forget the caption object if removeFromDrawPageAndFree() did not free it. if (maNoteData.mxCaption) + { + SAL_INFO("sc.core","ScPostIt::RemoveCaption - forgetting one ref"); maNoteData.mxCaption.forget(); + } } ScCaptionPtr ScNoteUtil::CreateTempCaption( commit 5ef67aa7c9a4f370b091fdf4b280596d5668553a Author: Eike Rathke <[email protected]> Date: Wed Apr 12 23:24:34 2017 +0200 control deletion of SdrCaptionObj within ScCaptionPtr by refcount I guess this is about the first time ever that repeated Undo and Redo of Cut&Paste of a cell comment does not crash.. Change-Id: I493a0a5439efde133a07d73ddcbcdf5bda4bc276 diff --git a/sc/source/core/data/postit.cxx b/sc/source/core/data/postit.cxx index b48bdcf8d206..310bddfa606f 100644 --- a/sc/source/core/data/postit.cxx +++ b/sc/source/core/data/postit.cxx @@ -761,7 +761,9 @@ void ScCaptionPtr::removeFromDrawPageAndFree( bool bIgnoreUndo ) } // remove the object from the drawing page, delete if undo is disabled removeFromDrawPage( *pDrawPage ); - if (!bRecording) + // If called from outside mnRefs must be 1 to delete. If called from + // decRefAndDestroy() mnRefs is already 0. + if (!bRecording && getRefs() <= 1) { SdrObject* pObj = release(); SdrObject::Free( pObj ); @@ -1136,6 +1138,7 @@ void ScPostIt::RemoveCaption() if (pDrawLayer == maNoteData.mxCaption->GetModel()) maNoteData.mxCaption.removeFromDrawPageAndFree(); +#if 0 // Either the caption object is gone or, because of Undo or clipboard is // held in at least two instances, or only one instance in Undo because the // original sheet in this document is just deleted, or the Undo document is @@ -1143,6 +1146,7 @@ void ScPostIt::RemoveCaption() // Let's detect other use cases.. assert(!maNoteData.mxCaption || maNoteData.mxCaption.getRefs() >= 2 || (!mrDoc.IsUndo() && !mrDoc.IsClipboard()) || (mrDoc.IsUndo() && mrDoc.IsInDtorClear())); +#endif // Forget the caption object if removeFromDrawPageAndFree() above did not // free it. commit b7280da0b67cc53bfb8d384389832cfdef4a9b48 Author: Eike Rathke <[email protected]> Date: Tue Apr 11 21:23:27 2017 +0200 bail out early if there is no caption to remove Change-Id: Id08d82751560092fd6225131970f607dbb2e4801 diff --git a/sc/source/core/data/postit.cxx b/sc/source/core/data/postit.cxx index a197f53a46dc..b48bdcf8d206 100644 --- a/sc/source/core/data/postit.cxx +++ b/sc/source/core/data/postit.cxx @@ -1126,12 +1126,14 @@ void ScPostIt::CreateCaption( const ScAddress& rPos, const SdrCaptionObj* pCapti void ScPostIt::RemoveCaption() { + if (!maNoteData.mxCaption) + return; /* Remove caption object only, if this note is its owner (e.g. notes in undo documents refer to captions in original document, do not remove them from drawing layer here). */ ScDrawLayer* pDrawLayer = mrDoc.GetDrawLayer(); - if (maNoteData.mxCaption && (pDrawLayer == maNoteData.mxCaption->GetModel())) + if (pDrawLayer == maNoteData.mxCaption->GetModel()) maNoteData.mxCaption.removeFromDrawPageAndFree(); // Either the caption object is gone or, because of Undo or clipboard is commit 9c28bab7a9404ab36fd7cee3ef47b1ccafa3638b Author: Eike Rathke <[email protected]> Date: Tue Apr 11 20:58:34 2017 +0200 in RemoveCaption() forget() instead of reset(nullptr) Change-Id: Id97d4d97c1d46ac6de6198515756a0786a54626e diff --git a/sc/source/core/data/postit.cxx b/sc/source/core/data/postit.cxx index f4e5b3bbc723..a197f53a46dc 100644 --- a/sc/source/core/data/postit.cxx +++ b/sc/source/core/data/postit.cxx @@ -1133,6 +1133,7 @@ void ScPostIt::RemoveCaption() ScDrawLayer* pDrawLayer = mrDoc.GetDrawLayer(); if (maNoteData.mxCaption && (pDrawLayer == maNoteData.mxCaption->GetModel())) maNoteData.mxCaption.removeFromDrawPageAndFree(); + // Either the caption object is gone or, because of Undo or clipboard is // held in at least two instances, or only one instance in Undo because the // original sheet in this document is just deleted, or the Undo document is @@ -1140,7 +1141,11 @@ void ScPostIt::RemoveCaption() // Let's detect other use cases.. assert(!maNoteData.mxCaption || maNoteData.mxCaption.getRefs() >= 2 || (!mrDoc.IsUndo() && !mrDoc.IsClipboard()) || (mrDoc.IsUndo() && mrDoc.IsInDtorClear())); - maNoteData.mxCaption.reset(nullptr); + + // Forget the caption object if removeFromDrawPageAndFree() above did not + // free it. + if (maNoteData.mxCaption) + maNoteData.mxCaption.forget(); } ScCaptionPtr ScNoteUtil::CreateTempCaption( commit f57c78d9164b3e3677d650322fa9cb4b24ec8d88 Author: Eike Rathke <[email protected]> Date: Tue Apr 11 19:05:46 2017 +0200 deleting pModel also deletes the SdrCaptionObj Change-Id: Icf3aed35ede1c211d6238dc66d86cb2866b247cd diff --git a/sc/source/ui/view/notemark.cxx b/sc/source/ui/view/notemark.cxx index 564d98d83329..55f8812048ff 100644 --- a/sc/source/ui/view/notemark.cxx +++ b/sc/source/ui/view/notemark.cxx @@ -65,6 +65,9 @@ ScNoteMarker::ScNoteMarker( vcl::Window* pWin, vcl::Window* pRight, vcl::Window* ScNoteMarker::~ScNoteMarker() { + if (pModel) + mxObject.release(); // deleting pModel also deletes the SdrCaptionObj + InvalidateWin(); delete pModel; commit 033bd8233e328b4881864c3ff1868951ec8f8ab6 Author: Eike Rathke <[email protected]> Date: Tue Apr 11 18:53:36 2017 +0200 set mbNotOwner at various places Change-Id: I1ff14c573d556cad15513dfe3f0fecbf9107fa41 diff --git a/sc/source/core/data/postit.cxx b/sc/source/core/data/postit.cxx index c6a91d25bec0..f4e5b3bbc723 100644 --- a/sc/source/core/data/postit.cxx +++ b/sc/source/core/data/postit.cxx @@ -481,6 +481,7 @@ ScCaptionPtr::ScCaptionPtr( ScCaptionPtr&& r ) : { r.replaceInList( this ); r.mpCaption = nullptr; + r.mbNotOwner = false; } ScCaptionPtr& ScCaptionPtr::operator=( ScCaptionPtr&& r ) @@ -490,9 +491,11 @@ ScCaptionPtr& ScCaptionPtr::operator=( ScCaptionPtr&& r ) mpHead = r.mpHead; mpNext = r.mpNext; mpCaption = r.mpCaption; + mbNotOwner = r.mbNotOwner; r.replaceInList( this ); r.mpCaption = nullptr; + r.mbNotOwner = false; return *this; } @@ -525,6 +528,7 @@ ScCaptionPtr& ScCaptionPtr::operator=( const ScCaptionPtr& r ) removeFromList(); mpCaption = r.mpCaption; + mbNotOwner = r.mbNotOwner; // That head is this' master. mpHead = r.mpHead; // Insert into list. @@ -656,6 +660,7 @@ void ScCaptionPtr::reset( SdrCaptionObj* p ) decRefAndDestroy(); removeFromList(); mpCaption = p; + mbNotOwner = false; if (p) { newHead(); @@ -776,6 +781,7 @@ bool ScCaptionPtr::forget() bool bRet = decRef(); removeFromList(); mpCaption = nullptr; + mbNotOwner = false; return bRet; } @@ -800,6 +806,7 @@ void ScCaptionPtr::clear() mpHead = nullptr; mpNext = nullptr; mpCaption = nullptr; + mbNotOwner = false; } commit 5c1566503cbf3310bca6cf4c121cc421ec8d5808 Author: Eike Rathke <[email protected]> Date: Tue Apr 11 18:05:09 2017 +0200 reset variables when not owner Change-Id: Ieab4bf36b89abac2d2ff377fc2b6f31ce0e1d3aa diff --git a/sc/source/core/data/postit.cxx b/sc/source/core/data/postit.cxx index 87b03142fb18..c6a91d25bec0 100644 --- a/sc/source/core/data/postit.cxx +++ b/sc/source/core/data/postit.cxx @@ -695,9 +695,16 @@ void ScCaptionPtr::decRefAndDestroy() #if 1 // FIXME: there are still cases where the caption pointer is dangling mpCaption = nullptr; + mbNotOwner = false; #else - // Destroying Draw Undo deletes its SdrObject, don't attempt that twice. - if (!mbNotOwner) + // Destroying Draw Undo and some other delete the SdrObject, don't + // attempt that twice. + if (mbNotOwner) + { + mpCaption = nullptr; + mbNotOwner = false; + } + else { removeFromDrawPageAndFree( true ); // ignoring Undo if (mpCaption) commit 9f18a9dd14561eb54fdbab098bea9d79b6307baa Author: Eike Rathke <[email protected]> Date: Tue Apr 11 17:16:32 2017 +0200 rename ScCaptionPtr (mb|set)InUndo to (mb|set)NotOwner ... which better suits the general purpose we'll need Change-Id: I32805c91d17180d5f18225a02c8a436826242e19 diff --git a/sc/inc/postit.hxx b/sc/inc/postit.hxx index 1d81db68b32e..8d2b5f7823c5 100644 --- a/sc/inc/postit.hxx +++ b/sc/inc/postit.hxx @@ -86,7 +86,7 @@ public: bool forget(); /** Flag that this instance is in Undo, so drawing layer owns it. */ - void setInUndo(); + void setNotOwner(); oslInterlockedCount getRefs() const; @@ -104,7 +104,7 @@ private: Head* mpHead; ///< points to the "master" entry mutable ScCaptionPtr* mpNext; ///< next in list SdrCaptionObj* mpCaption; ///< the caption object, managed by head master - bool mbInUndo; ///< whether this caption object is held in Undo + bool mbNotOwner; ///< whether this caption object is owned by something else, e.g. held in Undo /* TODO: can that be moved to Head? * It's unclear when to reset, so * each instance has its own flag. diff --git a/sc/source/core/data/postit.cxx b/sc/source/core/data/postit.cxx index cd76cebcc65f..87b03142fb18 100644 --- a/sc/source/core/data/postit.cxx +++ b/sc/source/core/data/postit.cxx @@ -445,12 +445,12 @@ ScNoteCaptionCreator::ScNoteCaptionCreator( ScDocument& rDoc, const ScAddress& r ScCaptionPtr::ScCaptionPtr() : - mpHead(nullptr), mpNext(nullptr), mpCaption(nullptr), mbInUndo(false) + mpHead(nullptr), mpNext(nullptr), mpCaption(nullptr), mbNotOwner(false) { } ScCaptionPtr::ScCaptionPtr( SdrCaptionObj* p ) : - mpHead(nullptr), mpNext(nullptr), mpCaption(p), mbInUndo(false) + mpHead(nullptr), mpNext(nullptr), mpCaption(p), mbNotOwner(false) { if (p) { @@ -459,7 +459,7 @@ ScCaptionPtr::ScCaptionPtr( SdrCaptionObj* p ) : } ScCaptionPtr::ScCaptionPtr( const ScCaptionPtr& r ) : - mpHead(r.mpHead), mpCaption(r.mpCaption), mbInUndo(false) + mpHead(r.mpHead), mpCaption(r.mpCaption), mbNotOwner(false) { if (r.mpCaption) { @@ -477,7 +477,7 @@ ScCaptionPtr::ScCaptionPtr( const ScCaptionPtr& r ) : } ScCaptionPtr::ScCaptionPtr( ScCaptionPtr&& r ) : - mpHead(r.mpHead), mpNext(r.mpNext), mpCaption(r.mpCaption), mbInUndo(false) + mpHead(r.mpHead), mpNext(r.mpNext), mpCaption(r.mpCaption), mbNotOwner(false) { r.replaceInList( this ); r.mpCaption = nullptr; @@ -534,9 +534,9 @@ ScCaptionPtr& ScCaptionPtr::operator=( const ScCaptionPtr& r ) return *this; } -void ScCaptionPtr::setInUndo() +void ScCaptionPtr::setNotOwner() { - mbInUndo = true; + mbNotOwner = true; } ScCaptionPtr::Head::Head( ScCaptionPtr* p ) : @@ -697,7 +697,7 @@ void ScCaptionPtr::decRefAndDestroy() mpCaption = nullptr; #else // Destroying Draw Undo deletes its SdrObject, don't attempt that twice. - if (!mbInUndo) + if (!mbNotOwner) { removeFromDrawPageAndFree( true ); // ignoring Undo if (mpCaption) diff --git a/sc/source/ui/undo/undocell.cxx b/sc/source/ui/undo/undocell.cxx index 1e39da276459..44a56627b214 100644 --- a/sc/source/ui/undo/undocell.cxx +++ b/sc/source/ui/undo/undocell.cxx @@ -712,12 +712,12 @@ ScUndoReplaceNote::ScUndoReplaceNote( ScDocShell& rDocShell, const ScAddress& rP if (bInsert) { maNewData = rNoteData; - maNewData.mxCaption.setInUndo(); + maNewData.mxCaption.setNotOwner(); } else { maOldData = rNoteData; - maOldData.mxCaption.setInUndo(); + maOldData.mxCaption.setNotOwner(); } } @@ -731,8 +731,8 @@ ScUndoReplaceNote::ScUndoReplaceNote( ScDocShell& rDocShell, const ScAddress& rP { OSL_ENSURE( maOldData.mxCaption || maNewData.mxCaption, "ScUndoReplaceNote::ScUndoReplaceNote - missing note captions" ); OSL_ENSURE( !maOldData.mxInitData.get() && !maNewData.mxInitData.get(), "ScUndoReplaceNote::ScUndoReplaceNote - unexpected unitialized note" ); - maOldData.mxCaption.setInUndo(); - maNewData.mxCaption.setInUndo(); + maOldData.mxCaption.setNotOwner(); + maNewData.mxCaption.setNotOwner(); } ScUndoReplaceNote::~ScUndoReplaceNote() commit a4418108d13a9b19a6934f542c3b64499ecf18d2 Author: Eike Rathke <[email protected]> Date: Tue Apr 11 13:41:44 2017 +0200 there are still cases where the caption pointer is dangling Change-Id: I8c186fa32d7fc3f26d7952268cb1e614025ecf37 diff --git a/sc/source/core/data/postit.cxx b/sc/source/core/data/postit.cxx index c8f244fb3e5b..cd76cebcc65f 100644 --- a/sc/source/core/data/postit.cxx +++ b/sc/source/core/data/postit.cxx @@ -692,6 +692,10 @@ void ScCaptionPtr::decRefAndDestroy() assert(!mpNext); // this must be one and only one assert(mpCaption); +#if 1 + // FIXME: there are still cases where the caption pointer is dangling + mpCaption = nullptr; +#else // Destroying Draw Undo deletes its SdrObject, don't attempt that twice. if (!mbInUndo) { @@ -705,6 +709,7 @@ void ScCaptionPtr::decRefAndDestroy() SdrObject::Free( pObj ); } } +#endif delete mpHead; mpHead = nullptr; } commit d091cc41bf44ce3f69151e2765d1148969d25170 Author: Eike Rathke <[email protected]> Date: Mon Apr 10 23:31:14 2017 +0200 finally free the SdrObject in ScCaptionPtr::decRefAndDestroy() There may be cases left still to be discovered where a setInUndo() is necessary in some Undo situations, but this is a start. Change-Id: Ic62267e3c3d24e4587343ff42da0292fbb166929 diff --git a/sc/source/core/data/postit.cxx b/sc/source/core/data/postit.cxx index 8f0f00200d76..c8f244fb3e5b 100644 --- a/sc/source/core/data/postit.cxx +++ b/sc/source/core/data/postit.cxx @@ -691,18 +691,20 @@ void ScCaptionPtr::decRefAndDestroy() assert(mpHead->mpFirst == this); // this must be one and only one assert(!mpNext); // this must be one and only one assert(mpCaption); -#if 0 - if (what?) + + // Destroying Draw Undo deletes its SdrObject, don't attempt that twice. + if (!mbInUndo) { - /* FIXME: this should eventually remove the caption from drawing layer - * foo and call SdrObject::Free(), likely with mpCaption, see - * ScPostIt::RemoveCaption(). Further work needed to be able to do so. - * */ + removeFromDrawPageAndFree( true ); // ignoring Undo + if (mpCaption) + { + // There's no draw page associated so removeFromDrawPageAndFree() + // didn't do anything, but still we want to delete the caption + // object. release()/dissolve() also resets mpCaption. + SdrObject* pObj = release(); + SdrObject::Free( pObj ); + } } - /* FIXME: once we got ownership right */ - //SdrObject::Free( mpCaption ); -#endif - mpCaption = nullptr; delete mpHead; mpHead = nullptr; } commit dc1833e012c581c8b534652c28ebf635108ba279 Author: Eike Rathke <[email protected]> Date: Mon Apr 10 23:25:34 2017 +0200 flag ScCaptionPtr::setInUndo() in ScUndoReplaceNote Change-Id: I174be1262074e1fed784806d2f052b36749dff0d diff --git a/sc/source/ui/undo/undocell.cxx b/sc/source/ui/undo/undocell.cxx index ee8ec67237c2..1e39da276459 100644 --- a/sc/source/ui/undo/undocell.cxx +++ b/sc/source/ui/undo/undocell.cxx @@ -709,7 +709,16 @@ ScUndoReplaceNote::ScUndoReplaceNote( ScDocShell& rDocShell, const ScAddress& rP mpDrawUndo( pDrawUndo ) { OSL_ENSURE( rNoteData.mxCaption, "ScUndoReplaceNote::ScUndoReplaceNote - missing note caption" ); - (bInsert ? maNewData : maOldData) = rNoteData; + if (bInsert) + { + maNewData = rNoteData; + maNewData.mxCaption.setInUndo(); + } + else + { + maOldData = rNoteData; + maOldData.mxCaption.setInUndo(); + } } ScUndoReplaceNote::ScUndoReplaceNote( ScDocShell& rDocShell, const ScAddress& rPos, @@ -722,6 +731,8 @@ ScUndoReplaceNote::ScUndoReplaceNote( ScDocShell& rDocShell, const ScAddress& rP { OSL_ENSURE( maOldData.mxCaption || maNewData.mxCaption, "ScUndoReplaceNote::ScUndoReplaceNote - missing note captions" ); OSL_ENSURE( !maOldData.mxInitData.get() && !maNewData.mxInitData.get(), "ScUndoReplaceNote::ScUndoReplaceNote - unexpected unitialized note" ); + maOldData.mxCaption.setInUndo(); + maNewData.mxCaption.setInUndo(); } ScUndoReplaceNote::~ScUndoReplaceNote() commit 26337c1307a1cdee72b2d457cd0d210227662e29 Author: Eike Rathke <[email protected]> Date: Mon Apr 10 23:24:00 2017 +0200 introduce ScCaptionPtr InUndo Change-Id: Iccc2671b61f524244107233b77b56aaa45f5c72a diff --git a/sc/inc/postit.hxx b/sc/inc/postit.hxx index b5a99206aab8..1d81db68b32e 100644 --- a/sc/inc/postit.hxx +++ b/sc/inc/postit.hxx @@ -85,6 +85,9 @@ public: */ bool forget(); + /** Flag that this instance is in Undo, so drawing layer owns it. */ + void setInUndo(); + oslInterlockedCount getRefs() const; private: @@ -101,6 +104,14 @@ private: Head* mpHead; ///< points to the "master" entry mutable ScCaptionPtr* mpNext; ///< next in list SdrCaptionObj* mpCaption; ///< the caption object, managed by head master + bool mbInUndo; ///< whether this caption object is held in Undo + /* TODO: can that be moved to Head? + * It's unclear when to reset, so + * each instance has its own flag. + * The last reference count + * decrement automatically has the + * then current state available. + * */ void newHead(); //< Allocate a new Head and init. void incRef() const; diff --git a/sc/source/core/data/postit.cxx b/sc/source/core/data/postit.cxx index 60225e639f74..8f0f00200d76 100644 --- a/sc/source/core/data/postit.cxx +++ b/sc/source/core/data/postit.cxx @@ -445,12 +445,12 @@ ScNoteCaptionCreator::ScNoteCaptionCreator( ScDocument& rDoc, const ScAddress& r ScCaptionPtr::ScCaptionPtr() : - mpHead(nullptr), mpNext(nullptr), mpCaption(nullptr) + mpHead(nullptr), mpNext(nullptr), mpCaption(nullptr), mbInUndo(false) { } ScCaptionPtr::ScCaptionPtr( SdrCaptionObj* p ) : - mpHead(nullptr), mpNext(nullptr), mpCaption(p) + mpHead(nullptr), mpNext(nullptr), mpCaption(p), mbInUndo(false) { if (p) { @@ -459,7 +459,7 @@ ScCaptionPtr::ScCaptionPtr( SdrCaptionObj* p ) : } ScCaptionPtr::ScCaptionPtr( const ScCaptionPtr& r ) : - mpHead(r.mpHead), mpCaption(r.mpCaption) + mpHead(r.mpHead), mpCaption(r.mpCaption), mbInUndo(false) { if (r.mpCaption) { @@ -477,7 +477,7 @@ ScCaptionPtr::ScCaptionPtr( const ScCaptionPtr& r ) : } ScCaptionPtr::ScCaptionPtr( ScCaptionPtr&& r ) : - mpHead(r.mpHead), mpNext(r.mpNext), mpCaption(r.mpCaption) + mpHead(r.mpHead), mpNext(r.mpNext), mpCaption(r.mpCaption), mbInUndo(false) { r.replaceInList( this ); r.mpCaption = nullptr; @@ -534,6 +534,11 @@ ScCaptionPtr& ScCaptionPtr::operator=( const ScCaptionPtr& r ) return *this; } +void ScCaptionPtr::setInUndo() +{ + mbInUndo = true; +} + ScCaptionPtr::Head::Head( ScCaptionPtr* p ) : mpFirst(p), mnRefs(1) { commit 21049854751bcc2b1f42947181eed3ee2a39404b Author: Eike Rathke <[email protected]> Date: Mon Apr 10 21:26:35 2017 +0200 Add bool bIgnoreUndo parameter to removeFromDrawPageAndFree() In preparation of using that in the dtor. Change-Id: I9a8713390c548e774c1e23cef201effe00a29be9 diff --git a/sc/inc/postit.hxx b/sc/inc/postit.hxx index bff01bf95e86..b5a99206aab8 100644 --- a/sc/inc/postit.hxx +++ b/sc/inc/postit.hxx @@ -70,7 +70,7 @@ public: /** Remove from draw page and free caption object if no Undo recording. */ - void removeFromDrawPageAndFree(); + void removeFromDrawPageAndFree( bool bIgnoreUndo = false ); /** Release all management of the SdrCaptionObj* in all instances of this list and dissolve. The SdrCaptionObj pointer returned is ready to be diff --git a/sc/source/core/data/postit.cxx b/sc/source/core/data/postit.cxx index be2896447f52..60225e639f74 100644 --- a/sc/source/core/data/postit.cxx +++ b/sc/source/core/data/postit.cxx @@ -717,7 +717,7 @@ void ScCaptionPtr::removeFromDrawPage( SdrPage& rDrawPage ) assert(pObj == mpCaption); (void)pObj; } -void ScCaptionPtr::removeFromDrawPageAndFree() +void ScCaptionPtr::removeFromDrawPageAndFree( bool bIgnoreUndo ) { assert(mpHead && mpCaption); SdrPage* pDrawPage = mpCaption->GetPage(); @@ -725,12 +725,16 @@ void ScCaptionPtr::removeFromDrawPageAndFree() if (pDrawPage) { pDrawPage->RecalcObjOrdNums(); - ScDrawLayer* pDrawLayer = dynamic_cast<ScDrawLayer*>(mpCaption->GetModel()); - SAL_WARN_IF( !pDrawLayer, "sc.core", "ScCaptionPtr::removeFromDrawPageAndFree - object without drawing layer"); - // create drawing undo action (before removing the object to have valid draw page in undo action) - const bool bRecording = (pDrawLayer && pDrawLayer->IsRecording()); - if (bRecording) - pDrawLayer->AddCalcUndo( new SdrUndoDelObj( *mpCaption )); + bool bRecording = false; + if (!bIgnoreUndo) + { + ScDrawLayer* pDrawLayer = dynamic_cast<ScDrawLayer*>(mpCaption->GetModel()); + SAL_WARN_IF( !pDrawLayer, "sc.core", "ScCaptionPtr::removeFromDrawPageAndFree - object without drawing layer"); + // create drawing undo action (before removing the object to have valid draw page in undo action) + bRecording = (pDrawLayer && pDrawLayer->IsRecording()); + if (bRecording) + pDrawLayer->AddCalcUndo( new SdrUndoDelObj( *mpCaption )); + } // remove the object from the drawing page, delete if undo is disabled removeFromDrawPage( *pDrawPage ); if (!bRecording) commit acfd6f78c75812031bc310ab26bf60e9a703cd85 Author: Eike Rathke <[email protected]> Date: Mon Apr 10 20:00:52 2017 +0200 move implementation from RemoveCaption() to removeFromDrawPageAndFree() Change-Id: I4f98112c13dfcd5c6c2fdb5b682cca494d63a954 diff --git a/sc/inc/postit.hxx b/sc/inc/postit.hxx index d741cdf86bbc..bff01bf95e86 100644 --- a/sc/inc/postit.hxx +++ b/sc/inc/postit.hxx @@ -68,6 +68,10 @@ public: */ void removeFromDrawPage( SdrPage& rDrawPage ); + /** Remove from draw page and free caption object if no Undo recording. + */ + void removeFromDrawPageAndFree(); + /** Release all management of the SdrCaptionObj* in all instances of this list and dissolve. The SdrCaptionObj pointer returned is ready to be managed elsewhere. diff --git a/sc/source/core/data/postit.cxx b/sc/source/core/data/postit.cxx index 1156ba2e9258..be2896447f52 100644 --- a/sc/source/core/data/postit.cxx +++ b/sc/source/core/data/postit.cxx @@ -717,6 +717,30 @@ void ScCaptionPtr::removeFromDrawPage( SdrPage& rDrawPage ) assert(pObj == mpCaption); (void)pObj; } +void ScCaptionPtr::removeFromDrawPageAndFree() +{ + assert(mpHead && mpCaption); + SdrPage* pDrawPage = mpCaption->GetPage(); + SAL_WARN_IF( !pDrawPage, "sc.core", "ScCaptionPtr::removeFromDrawPageAndFree - object without drawing page"); + if (pDrawPage) + { + pDrawPage->RecalcObjOrdNums(); + ScDrawLayer* pDrawLayer = dynamic_cast<ScDrawLayer*>(mpCaption->GetModel()); + SAL_WARN_IF( !pDrawLayer, "sc.core", "ScCaptionPtr::removeFromDrawPageAndFree - object without drawing layer"); + // create drawing undo action (before removing the object to have valid draw page in undo action) + const bool bRecording = (pDrawLayer && pDrawLayer->IsRecording()); + if (bRecording) + pDrawLayer->AddCalcUndo( new SdrUndoDelObj( *mpCaption )); + // remove the object from the drawing page, delete if undo is disabled + removeFromDrawPage( *pDrawPage ); + if (!bRecording) + { + SdrObject* pObj = release(); + SdrObject::Free( pObj ); + } + } +} + SdrCaptionObj* ScCaptionPtr::release() { SdrCaptionObj* pTmp = mpCaption; @@ -1077,27 +1101,8 @@ void ScPostIt::RemoveCaption() undo documents refer to captions in original document, do not remove them from drawing layer here). */ ScDrawLayer* pDrawLayer = mrDoc.GetDrawLayer(); - if( maNoteData.mxCaption && (pDrawLayer == maNoteData.mxCaption->GetModel()) ) - { - OSL_ENSURE( pDrawLayer, "ScPostIt::RemoveCaption - object without drawing layer" ); - SdrPage* pDrawPage = maNoteData.mxCaption->GetPage(); - OSL_ENSURE( pDrawPage, "ScPostIt::RemoveCaption - object without drawing page" ); - if( pDrawPage ) - { - pDrawPage->RecalcObjOrdNums(); - // create drawing undo action (before removing the object to have valid draw page in undo action) - const bool bRecording = (pDrawLayer && pDrawLayer->IsRecording()); - if( bRecording ) - pDrawLayer->AddCalcUndo( new SdrUndoDelObj( *maNoteData.mxCaption.get() ) ); - // remove the object from the drawing page, delete if undo is disabled - maNoteData.mxCaption.removeFromDrawPage( *pDrawPage ); - if( !bRecording ) - { - SdrObject* pObj = maNoteData.mxCaption.release(); - SdrObject::Free( pObj ); - } - } - } + if (maNoteData.mxCaption && (pDrawLayer == maNoteData.mxCaption->GetModel())) + maNoteData.mxCaption.removeFromDrawPageAndFree(); // Either the caption object is gone or, because of Undo or clipboard is // held in at least two instances, or only one instance in Undo because the // original sheet in this document is just deleted, or the Undo document is commit 774d59059777ef0d08b5fd6ff9f04e0cc323adff Author: Eike Rathke <[email protected]> Date: Mon Apr 10 18:54:17 2017 +0200 narrow the assert condition further down Change-Id: Ia9b1db652b2f15b66b89b51038d16fb0da6ffb6d diff --git a/sc/source/core/data/postit.cxx b/sc/source/core/data/postit.cxx index 520f0553df0a..1156ba2e9258 100644 --- a/sc/source/core/data/postit.cxx +++ b/sc/source/core/data/postit.cxx @@ -1100,10 +1100,11 @@ void ScPostIt::RemoveCaption() } // Either the caption object is gone or, because of Undo or clipboard is // held in at least two instances, or only one instance in Undo because the - // original sheet was deleted, or the Undo document is just destroyed - // which leaves us with one reference. + // original sheet in this document is just deleted, or the Undo document is + // just destroyed which leaves us with one reference. // Let's detect other use cases.. - assert(!maNoteData.mxCaption || maNoteData.mxCaption.getRefs() >= 2 || !mrDoc.IsUndo() || mrDoc.IsInDtorClear()); + assert(!maNoteData.mxCaption || maNoteData.mxCaption.getRefs() >= 2 || + (!mrDoc.IsUndo() && !mrDoc.IsClipboard()) || (mrDoc.IsUndo() && mrDoc.IsInDtorClear())); maNoteData.mxCaption.reset(nullptr); } commit a01559a579537956def399bbbbe99b76b7039b23 Author: Eike Rathke <[email protected]> Date: Mon Apr 10 16:25:45 2017 +0200 can't keep track of drawlayer insertion Draw Undo independently can reinsert a caption to the drawlayer, which is beyond our knowledge. To track that cumbersome manual tracking (or callbacks or whatever) would be needed, which actually this tries to get rid of instead of increasing.. Change-Id: I373843ad61d0b6e19b9d3f98fd8f9e01a448296d diff --git a/sc/inc/postit.hxx b/sc/inc/postit.hxx index e3a91ab2d73a..d741cdf86bbc 100644 --- a/sc/inc/postit.hxx +++ b/sc/inc/postit.hxx @@ -89,7 +89,6 @@ private: { ScCaptionPtr* mpFirst; ///< first in list oslInterlockedCount mnRefs; ///< use count - bool mbInDrawPage; ///< caption object is owned by draw page Head() = delete; explicit Head( ScCaptionPtr* ); diff --git a/sc/source/core/data/postit.cxx b/sc/source/core/data/postit.cxx index 2077de6c84ee..520f0553df0a 100644 --- a/sc/source/core/data/postit.cxx +++ b/sc/source/core/data/postit.cxx @@ -535,7 +535,7 @@ ScCaptionPtr& ScCaptionPtr::operator=( const ScCaptionPtr& r ) } ScCaptionPtr::Head::Head( ScCaptionPtr* p ) : - mpFirst(p), mnRefs(1), mbInDrawPage(false) + mpFirst(p), mnRefs(1) { } @@ -686,7 +686,8 @@ void ScCaptionPtr::decRefAndDestroy() assert(mpHead->mpFirst == this); // this must be one and only one assert(!mpNext); // this must be one and only one assert(mpCaption); - if (mpHead->mbInDrawPage) +#if 0 + if (what?) { /* FIXME: this should eventually remove the caption from drawing layer * foo and call SdrObject::Free(), likely with mpCaption, see @@ -695,6 +696,7 @@ void ScCaptionPtr::decRefAndDestroy() } /* FIXME: once we got ownership right */ //SdrObject::Free( mpCaption ); +#endif mpCaption = nullptr; delete mpHead; mpHead = nullptr; @@ -704,24 +706,15 @@ void ScCaptionPtr::decRefAndDestroy() void ScCaptionPtr::insertToDrawPage( SdrPage& rDrawPage ) { assert(mpHead && mpCaption); - assert(!mpHead->mbInDrawPage); // multiple assignments not possible rDrawPage.InsertObject( mpCaption ); - mpHead->mbInDrawPage = true; } void ScCaptionPtr::removeFromDrawPage( SdrPage& rDrawPage ) { assert(mpHead && mpCaption); - SAL_WARN_IF(!mpHead->mbInDrawPage,"sc.core","ScCaptionPtr::removeFromDrawPage - not in draw page"); - /* FIXME: that should assert, but currently fails in - * Test::testCopyToDocument() probably due to CopyStaticToDocument() - * lacking something. */ - //assert(mpHead->mbInDrawPage); // did we lose track anywhere? - SdrObject* pObj = rDrawPage.RemoveObject( mpCaption->GetOrdNum() ); assert(pObj == mpCaption); (void)pObj; - mpHead->mbInDrawPage = false; } SdrCaptionObj* ScCaptionPtr::release() commit 656142a690ee341f2409cbd5be21a04959e25b6f Author: Eike Rathke <[email protected]> Date: Fri Apr 7 16:58:13 2017 +0200 yet another mxCaption refs==1 case to exclude from assert Change-Id: Iffa8f2bc7d0bb77d5145a569da2c03aefbb9de4a diff --git a/sc/source/core/data/postit.cxx b/sc/source/core/data/postit.cxx index dd451734be28..2077de6c84ee 100644 --- a/sc/source/core/data/postit.cxx +++ b/sc/source/core/data/postit.cxx @@ -1106,9 +1106,11 @@ void ScPostIt::RemoveCaption() } } // Either the caption object is gone or, because of Undo or clipboard is - // held in at least two instances, or the Undo document is just destroyed + // held in at least two instances, or only one instance in Undo because the + // original sheet was deleted, or the Undo document is just destroyed // which leaves us with one reference. - assert(!maNoteData.mxCaption || maNoteData.mxCaption.getRefs() >= 2 || (mrDoc.IsUndo() && mrDoc.IsInDtorClear())); + // Let's detect other use cases.. + assert(!maNoteData.mxCaption || maNoteData.mxCaption.getRefs() >= 2 || !mrDoc.IsUndo() || mrDoc.IsInDtorClear()); maNoteData.mxCaption.reset(nullptr); } commit e9d1c62d8f7cfbc22174d34ee603139b9872c0f7 Author: Eike Rathke <[email protected]> Date: Tue Mar 14 19:57:04 2017 +0100 no assert for one reference while destroying the Undo document Change-Id: Idf9e0b2600d503ff50cd6269e8d528c0fad12a3e diff --git a/sc/source/core/data/postit.cxx b/sc/source/core/data/postit.cxx index 078a30fa37b3..dd451734be28 100644 --- a/sc/source/core/data/postit.cxx +++ b/sc/source/core/data/postit.cxx @@ -1106,8 +1106,9 @@ void ScPostIt::RemoveCaption() } } // Either the caption object is gone or, because of Undo or clipboard is - // held in at least two instances. - assert(!maNoteData.mxCaption || maNoteData.mxCaption.getRefs() >= 2); + // held in at least two instances, or the Undo document is just destroyed + // which leaves us with one reference. + assert(!maNoteData.mxCaption || maNoteData.mxCaption.getRefs() >= 2 || (mrDoc.IsUndo() && mrDoc.IsInDtorClear())); maNoteData.mxCaption.reset(nullptr); } commit 83cca3c8d7f93f792a2b2415bd02378c348c1e65 Author: Eike Rathke <[email protected]> Date: Thu Mar 9 23:15:42 2017 +0100 finally turn this into a hard assert Change-Id: Iba6abafeaa2542fc94b76a642ddb0eb5b70b572d diff --git a/sc/source/core/data/postit.cxx b/sc/source/core/data/postit.cxx index 66e91517338e..078a30fa37b3 100644 --- a/sc/source/core/data/postit.cxx +++ b/sc/source/core/data/postit.cxx @@ -959,7 +959,9 @@ void ScPostIt::UpdateCaptionPos( const ScAddress& rPos ) void ScPostIt::CreateCaptionFromInitData( const ScAddress& rPos ) const { - OSL_ENSURE( maNoteData.mxCaption || maNoteData.mxInitData.get(), "ScPostIt::CreateCaptionFromInitData - need caption object or initial caption data" ); + // Captions are not created in Undo documents and only rarely in Clipboard, + // but otherwise we need caption or initial data. + assert((maNoteData.mxCaption || maNoteData.mxInitData.get()) || mrDoc.IsUndo() || mrDoc.IsClipboard()); if( maNoteData.mxInitData.get() ) { /* This function is called from ScPostIt::Clone() when copying cells commit 3deb09f16d9c68914be4f89c5fc4bed005cb7c0a Author: Eike Rathke <[email protected]> Date: Thu Mar 9 22:38:57 2017 +0100 it's raining drawing layers Change-Id: Ieee5cb5792535185ef09c3775072ed739fb0e4b0 diff --git a/sc/qa/unit/ucalc.cxx b/sc/qa/unit/ucalc.cxx index d64dbf7efde0..9e0f9496b9d4 100644 --- a/sc/qa/unit/ucalc.cxx +++ b/sc/qa/unit/ucalc.cxx @@ -724,6 +724,9 @@ void Test::testCopyToDocument() { CPPUNIT_ASSERT_MESSAGE ("failed to insert sheet", m_pDoc->InsertTab (0, "src")); + // We need a drawing layer in order to create caption objects. + m_pDoc->InitDrawLayer(&getDocShell()); + m_pDoc->SetString(0, 0, 0, "Header"); m_pDoc->SetString(0, 1, 0, "1"); m_pDoc->SetString(0, 2, 0, "2"); @@ -1934,6 +1937,9 @@ void Test::testSheetCopy() CPPUNIT_ASSERT_EQUAL_MESSAGE("document should have one sheet to begin with.", static_cast<SCTAB>(1), m_pDoc->GetTableCount()); + // We need a drawing layer in order to create caption objects. + m_pDoc->InitDrawLayer(&getDocShell()); + // Insert text in A1. m_pDoc->SetString(ScAddress(0,0,0), "copy me"); @@ -5062,6 +5068,9 @@ void Test::testNoteDeleteCol() ScDocument& rDoc = getDocShell().GetDocument(); rDoc.InsertTab(0, "Sheet1"); + // We need a drawing layer in order to create caption objects. + m_pDoc->InitDrawLayer(&getDocShell()); + ScAddress rAddr(1, 1, 0); ScPostIt* pNote = m_pDoc->GetOrCreateNote(rAddr); pNote->SetText(rAddr, "Hello"); @@ -5227,6 +5236,9 @@ void Test::testAreasWithNotes() ScDocument& rDoc = getDocShell().GetDocument(); rDoc.InsertTab(0, "Sheet1"); + // We need a drawing layer in order to create caption objects. + m_pDoc->InitDrawLayer(&getDocShell()); + ScAddress rAddr(1, 5, 0); ScPostIt* pNote = m_pDoc->GetOrCreateNote(rAddr); pNote->SetText(rAddr, "Hello"); @@ -6026,6 +6038,9 @@ void Test::testSetStringAndNote() { m_pDoc->InsertTab(0, "Test"); + // We need a drawing layer in order to create caption objects. + m_pDoc->InitDrawLayer(&getDocShell()); + //note on A1 ScAddress aAdrA1 (0, 0, 0); ScPostIt* pNote = m_pDoc->GetOrCreateNote(aAdrA1); commit d1903be02477369990e7f1eff53224f820dc2fe8 Author: Eike Rathke <[email protected]> Date: Thu Mar 9 22:09:55 2017 +0100 add/use ScCaptionPtr::removeFromDrawPage() Change-Id: Ibe073f071b120b61738b7e813a14824248f1fcfc diff --git a/sc/inc/postit.hxx b/sc/inc/postit.hxx index 7266109200c0..e3a91ab2d73a 100644 --- a/sc/inc/postit.hxx +++ b/sc/inc/postit.hxx @@ -63,6 +63,11 @@ public: */ void insertToDrawPage( SdrPage& rDrawPage ); + /** Remove from draw page. The caption object is not owned anymore by the + draw page then. + */ + void removeFromDrawPage( SdrPage& rDrawPage ); + /** Release all management of the SdrCaptionObj* in all instances of this list and dissolve. The SdrCaptionObj pointer returned is ready to be managed elsewhere. diff --git a/sc/source/core/data/postit.cxx b/sc/source/core/data/postit.cxx index ee9b89e78ef7..66e91517338e 100644 --- a/sc/source/core/data/postit.cxx +++ b/sc/source/core/data/postit.cxx @@ -710,6 +710,20 @@ void ScCaptionPtr::insertToDrawPage( SdrPage& rDrawPage ) mpHead->mbInDrawPage = true; } +void ScCaptionPtr::removeFromDrawPage( SdrPage& rDrawPage ) +{ + assert(mpHead && mpCaption); + SAL_WARN_IF(!mpHead->mbInDrawPage,"sc.core","ScCaptionPtr::removeFromDrawPage - not in draw page"); + /* FIXME: that should assert, but currently fails in + * Test::testCopyToDocument() probably due to CopyStaticToDocument() + * lacking something. */ + //assert(mpHead->mbInDrawPage); // did we lose track anywhere? + + SdrObject* pObj = rDrawPage.RemoveObject( mpCaption->GetOrdNum() ); + assert(pObj == mpCaption); (void)pObj; + mpHead->mbInDrawPage = false; +} + SdrCaptionObj* ScCaptionPtr::release() { SdrCaptionObj* pTmp = mpCaption; @@ -1081,10 +1095,10 @@ void ScPostIt::RemoveCaption() if( bRecording ) pDrawLayer->AddCalcUndo( new SdrUndoDelObj( *maNoteData.mxCaption.get() ) ); // remove the object from the drawing page, delete if undo is disabled - SdrObject* pObj = pDrawPage->RemoveObject( maNoteData.mxCaption->GetOrdNum() ); + maNoteData.mxCaption.removeFromDrawPage( *pDrawPage ); if( !bRecording ) { - maNoteData.mxCaption.release(); + SdrObject* pObj = maNoteData.mxCaption.release(); SdrObject::Free( pObj ); } } commit d3fa5013570cfe3c0785ebb3304e8d98f1ee6004 Author: Eike Rathke <[email protected]> Date: Thu Mar 9 22:08:38 2017 +0100 sprinkle some drawing layers over test cases ... so things actually work like intended and creation of caption objects doesn't silently fail. Well, it does SAL_WARN or OSL_ENSURE but that's never displayed unless a test fails. Change-Id: Ibf4cc075cc3d6dadbe8f6208b2949310124b5749 diff --git a/sc/qa/unit/ucalc.cxx b/sc/qa/unit/ucalc.cxx index 68c84c24627d..d64dbf7efde0 100644 --- a/sc/qa/unit/ucalc.cxx +++ b/sc/qa/unit/ucalc.cxx @@ -738,22 +738,29 @@ void Test::testCopyToDocument() // Copy statically to another document. - ScDocument aDestDoc(SCDOCMODE_DOCUMENT); - aDestDoc.InsertTab(0, "src"); - m_pDoc->CopyStaticToDocument(ScRange(0,1,0,0,3,0), 0, &aDestDoc); // Copy A2:A4 - m_pDoc->CopyStaticToDocument(ScAddress(0,0,0), 0, &aDestDoc); // Copy A1 - m_pDoc->CopyStaticToDocument(ScRange(0,4,0,0,7,0), 0, &aDestDoc); // Copy A5:A8 - - CPPUNIT_ASSERT_EQUAL(m_pDoc->GetString(0,0,0), aDestDoc.GetString(0,0,0)); - CPPUNIT_ASSERT_EQUAL(m_pDoc->GetString(0,1,0), aDestDoc.GetString(0,1,0)); - CPPUNIT_ASSERT_EQUAL(m_pDoc->GetString(0,2,0), aDestDoc.GetString(0,2,0)); - CPPUNIT_ASSERT_EQUAL(m_pDoc->GetString(0,3,0), aDestDoc.GetString(0,3,0)); - CPPUNIT_ASSERT_EQUAL(m_pDoc->GetString(0,4,0), aDestDoc.GetString(0,4,0)); + ScDocShellRef xDocSh2; + getNewDocShell(xDocSh2); + ScDocument* pDestDoc = &xDocSh2->GetDocument(); + pDestDoc->InsertTab(0, "src"); + pDestDoc->InitDrawLayer(xDocSh2.get()); // for note caption objects + + m_pDoc->CopyStaticToDocument(ScRange(0,1,0,0,3,0), 0, pDestDoc); // Copy A2:A4 + m_pDoc->CopyStaticToDocument(ScAddress(0,0,0), 0, pDestDoc); // Copy A1 + m_pDoc->CopyStaticToDocument(ScRange(0,4,0,0,7,0), 0, pDestDoc); // Copy A5:A8 + + CPPUNIT_ASSERT_EQUAL(m_pDoc->GetString(0,0,0), pDestDoc->GetString(0,0,0)); + CPPUNIT_ASSERT_EQUAL(m_pDoc->GetString(0,1,0), pDestDoc->GetString(0,1,0)); + CPPUNIT_ASSERT_EQUAL(m_pDoc->GetString(0,2,0), pDestDoc->GetString(0,2,0)); + CPPUNIT_ASSERT_EQUAL(m_pDoc->GetString(0,3,0), pDestDoc->GetString(0,3,0)); + CPPUNIT_ASSERT_EQUAL(m_pDoc->GetString(0,4,0), pDestDoc->GetString(0,4,0)); // verify note - CPPUNIT_ASSERT_MESSAGE("There should be a note in A1 destDocument", aDestDoc.HasNote(ScAddress(0, 0, 0))); + CPPUNIT_ASSERT_MESSAGE("There should be a note in A1 destDocument", pDestDoc->HasNote(ScAddress(0, 0, 0))); CPPUNIT_ASSERT_EQUAL_MESSAGE("The notes content should be the same on both documents", - m_pDoc->GetNote(ScAddress(0, 0, 0))->GetText(), aDestDoc.GetNote(ScAddress(0, 0, 0))->GetText()); + m_pDoc->GetNote(ScAddress(0, 0, 0))->GetText(), pDestDoc->GetNote(ScAddress(0, 0, 0))->GetText()); + + pDestDoc->DeleteTab(0); + closeDocShell(xDocSh2); m_pDoc->DeleteTab(0); } @@ -3041,6 +3048,10 @@ void Test::testCopyPaste() { m_pDoc->InsertTab(0, "Sheet1"); m_pDoc->InsertTab(1, "Sheet2"); + + // We need a drawing layer in order to create caption objects. + m_pDoc->InitDrawLayer(&getDocShell()); + //test copy&paste + ScUndoPaste //copy local and global range names in formulas //string cells and value cells @@ -3256,6 +3267,9 @@ void Test::testCopyPasteTranspose() m_pDoc->InsertTab(0, "Sheet1"); m_pDoc->InsertTab(1, "Sheet2"); + // We need a drawing layer in order to create caption objects. + m_pDoc->InitDrawLayer(&getDocShell()); + m_pDoc->SetValue(0, 0, 0, 1); m_pDoc->SetString(1, 0, 0, "=A1+1"); m_pDoc->SetString(2, 0, 0, "test"); @@ -3844,6 +3858,9 @@ void Test::testMoveBlock() { m_pDoc->InsertTab(0, "SheetNotes"); + // We need a drawing layer in order to create caption objects. + m_pDoc->InitDrawLayer(&getDocShell()); + m_pDoc->SetValue(0, 0, 0, 1); m_pDoc->SetString(1, 0, 0, "=A1+1"); m_pDoc->SetString(2, 0, 0, "test"); @@ -4835,6 +4852,9 @@ void Test::testShiftCells() { m_pDoc->InsertTab(0, "foo"); + // We need a drawing layer in order to create caption objects. + m_pDoc->InitDrawLayer(&getDocShell()); + OUString aTestVal("Some Text"); // Text into cell E5. @@ -4872,6 +4892,9 @@ void Test::testNoteBasic() { m_pDoc->InsertTab(0, "PostIts"); + // We need a drawing layer in order to create caption objects. + m_pDoc->InitDrawLayer(&getDocShell()); + CPPUNIT_ASSERT(!m_pDoc->HasNotes()); // Check for note's presence in all tables before inserting any notes. diff --git a/sc/qa/unit/ucalc_sort.cxx b/sc/qa/unit/ucalc_sort.cxx index 7ec786d7a5ad..d99132f7dda5 100644 --- a/sc/qa/unit/ucalc_sort.cxx +++ b/sc/qa/unit/ucalc_sort.cxx @@ -31,6 +31,9 @@ void Test::testSort() { m_pDoc->InsertTab(0, "test1"); + // We need a drawing layer in order to create caption objects. + m_pDoc->InitDrawLayer(&getDocShell()); + ScRange aDataRange; ScAddress aPos(0,0,0); { commit 108edf7c758c6001554a24ef99f9bba55a1d8cbc Author: Eike Rathke <[email protected]> Date: Thu Mar 9 19:50:31 2017 +0100 probably should work like sketched Change-Id: I5ad52c718cf53c2f3fb14a7970917e0012d01875 diff --git a/sc/source/core/data/postit.cxx b/sc/source/core/data/postit.cxx index 618bbe462b9f..ee9b89e78ef7 100644 --- a/sc/source/core/data/postit.cxx +++ b/sc/source/core/data/postit.cxx @@ -683,13 +683,19 @@ void ScCaptionPtr::decRefAndDestroy() { if (decRef()) { - /* FIXME: this should eventually remove the caption from drawing layer - * foo and call SdrObject::Free(), likely with mpHead->mpCaption, see - * ScPostIt::RemoveCaption(). Further work needed to be able to do so. - * */ assert(mpHead->mpFirst == this); // this must be one and only one - mpCaption = nullptr; assert(!mpNext); // this must be one and only one + assert(mpCaption); + if (mpHead->mbInDrawPage) + { + /* FIXME: this should eventually remove the caption from drawing layer + * foo and call SdrObject::Free(), likely with mpCaption, see + * ScPostIt::RemoveCaption(). Further work needed to be able to do so. + * */ + } + /* FIXME: once we got ownership right */ + //SdrObject::Free( mpCaption ); + mpCaption = nullptr; delete mpHead; mpHead = nullptr; } commit 326cfd39d5ddafc938cc6281452f9062d5b118e1 Author: Eike Rathke <[email protected]> Date: Thu Mar 9 16:05:12 2017 +0100 use ScCaptionPtr::insertToDrawPage() Change-Id: I98dafdf8e571e4745e05df6cbcbf00fd9ecd8ec6 diff --git a/sc/source/core/data/postit.cxx b/sc/source/core/data/postit.cxx index 638dfa643204..618bbe462b9f 100644 --- a/sc/source/core/data/postit.cxx +++ b/sc/source/core/data/postit.cxx @@ -419,7 +419,7 @@ ScNoteCaptionCreator::ScNoteCaptionCreator( ScDocument& rDoc, const ScAddress& r // store note position in user data of caption object ScCaptionUtil::SetCaptionUserData( *rNoteData.mxCaption, rPos ); // insert object into draw page - pDrawPage->InsertObject( rNoteData.mxCaption.get() ); + rNoteData.mxCaption.insertToDrawPage( *pDrawPage ); } } } @@ -1117,10 +1117,11 @@ ScCaptionPtr ScNoteUtil::CreateTempCaption( // create the caption object ScCaptionCreator aCreator( rDoc, rPos, bTailFront ); - SdrCaptionObj* pCaption = aCreator.GetCaption().get(); // just for ease of use // insert caption into page (needed to set caption text) - rDrawPage.InsertObject( pCaption ); + aCreator.GetCaption().insertToDrawPage( rDrawPage ); + + SdrCaptionObj* pCaption = aCreator.GetCaption().get(); // just for ease of use // clone the edit text object, unless user text is present, then set this text if( pNoteCaption && rUserText.isEmpty() ) commit 58f894c86505c95f3924fa5e6d0c9523e062430e Author: Eike Rathke <[email protected]> Date: Thu Mar 9 15:22:57 2017 +0100 add ScCaptionPtr::insertToDrawPage() Change-Id: I1266b55c2558d306b20b0f2d9fba07b0bc46544e diff --git a/sc/inc/postit.hxx b/sc/inc/postit.hxx index e198870479f6..7266109200c0 100644 --- a/sc/inc/postit.hxx +++ b/sc/inc/postit.hxx @@ -31,6 +31,7 @@ class EditTextObject; class OutlinerParaObject; class SdrCaptionObj; class SdrPage; + class SfxItemSet; class ScDocument; class Rectangle; @@ -58,6 +59,10 @@ public: // Does not default to nullptr to make it visually obvious where such is used. void reset( SdrCaptionObj* p ); + /** Insert to draw page. The caption object is owned by the draw page then. + */ + void insertToDrawPage( SdrPage& rDrawPage ); + /** Release all management of the SdrCaptionObj* in all instances of this list and dissolve. The SdrCaptionObj pointer returned is ready to be managed elsewhere. @@ -77,8 +82,12 @@ private: struct Head { - ScCaptionPtr* mpFirst; ///< first in list - oslInterlockedCount mnRefs; ///< use count + ScCaptionPtr* mpFirst; ///< first in list + oslInterlockedCount mnRefs; ///< use count + bool mbInDrawPage; ///< caption object is owned by draw page + + Head() = delete; + explicit Head( ScCaptionPtr* ); }; Head* mpHead; ///< points to the "master" entry diff --git a/sc/source/core/data/postit.cxx b/sc/source/core/data/postit.cxx index 2ae4a75d7750..638dfa643204 100644 --- a/sc/source/core/data/postit.cxx +++ b/sc/source/core/data/postit.cxx @@ -534,12 +534,15 @@ ScCaptionPtr& ScCaptionPtr::operator=( const ScCaptionPtr& r ) return *this; } +ScCaptionPtr::Head::Head( ScCaptionPtr* p ) : + mpFirst(p), mnRefs(1), mbInDrawPage(false) +{ +} + void ScCaptionPtr::newHead() { assert(!mpHead); - mpHead = new Head; - mpHead->mpFirst = this; - mpHead->mnRefs = 1; + mpHead = new Head(this); } void ScCaptionPtr::replaceInList( ScCaptionPtr* pNew ) @@ -692,6 +695,15 @@ void ScCaptionPtr::decRefAndDestroy() } } +void ScCaptionPtr::insertToDrawPage( SdrPage& rDrawPage ) +{ + assert(mpHead && mpCaption); + assert(!mpHead->mbInDrawPage); // multiple assignments not possible + + rDrawPage.InsertObject( mpCaption ); + mpHead->mbInDrawPage = true; +} + SdrCaptionObj* ScCaptionPtr::release() { SdrCaptionObj* pTmp = mpCaption; commit 4c004a4bb09afbb69a1345d218ae79b6fa011034 Author: Eike Rathke <[email protected]> Date: Thu Mar 9 14:29:40 2017 +0100 move assignment onto self should not happen Change-Id: Ic44f4362762cb1c1fe027b69a78baf768c0a53da diff --git a/sc/source/core/data/postit.cxx b/sc/source/core/data/postit.cxx index 2111b8ebb4e4..2ae4a75d7750 100644 --- a/sc/source/core/data/postit.cxx +++ b/sc/source/core/data/postit.cxx @@ -485,8 +485,7 @@ ScCaptionPtr::ScCaptionPtr( ScCaptionPtr&& r ) : ScCaptionPtr& ScCaptionPtr::operator=( ScCaptionPtr&& r ) { - if (this == &r) - return *this; + assert(this != &r); mpHead = r.mpHead; mpNext = r.mpNext; commit 961d286ea355feda57b3138cb8339d6e2200d98a Author: Eike Rathke <[email protected]> Date: Wed Mar 8 18:14:29 2017 +0100 let ScNoteUtil::CreateTempCaption() return ScCaptionPtr Change-Id: Idcb7fc24a13d650d88bec9ba359d7c78006096ec diff --git a/sc/inc/postit.hxx b/sc/inc/postit.hxx index 0801b2493abc..e198870479f6 100644 --- a/sc/inc/postit.hxx +++ b/sc/inc/postit.hxx @@ -247,7 +247,7 @@ class SC_DLLPUBLIC ScNoteUtil public: /** Creates and returns a caption object for a temporary caption. */ - static SdrCaptionObj* CreateTempCaption( ScDocument& rDoc, const ScAddress& rPos, + static ScCaptionPtr CreateTempCaption( ScDocument& rDoc, const ScAddress& rPos, SdrPage& rDrawPage, const OUString& rUserText, const Rectangle& rVisRect, bool bTailFront ); diff --git a/sc/source/core/data/postit.cxx b/sc/source/core/data/postit.cxx index 4071bb45993b..2111b8ebb4e4 100644 --- a/sc/source/core/data/postit.cxx +++ b/sc/source/core/data/postit.cxx @@ -1078,7 +1078,7 @@ void ScPostIt::RemoveCaption() maNoteData.mxCaption.reset(nullptr); } -SdrCaptionObj* ScNoteUtil::CreateTempCaption( +ScCaptionPtr ScNoteUtil::CreateTempCaption( ScDocument& rDoc, const ScAddress& rPos, SdrPage& rDrawPage, const OUString& rUserText, const Rectangle& rVisRect, bool bTailFront ) { @@ -1095,7 +1095,7 @@ SdrCaptionObj* ScNoteUtil::CreateTempCaption( // create a caption if any text exists if( !pNoteCaption && aBuffer.isEmpty() ) - return nullptr; + return ScCaptionPtr(); // prepare visible rectangle (add default distance to all borders) Rectangle aVisRect( @@ -1138,9 +1138,8 @@ SdrCaptionObj* ScNoteUtil::CreateTempCaption( // move caption into visible area aCreator.AutoPlaceCaption( &aVisRect ); - // The caption object returned is completely unmanaged and stored elsewhere. // XXX Note it is already inserted to the draw page. - return aCreator.GetCaption().release(); + return aCreator.GetCaption(); } ScPostIt* ScNoteUtil::CreateNoteFromCaption( diff --git a/sc/source/ui/inc/notemark.hxx b/sc/source/ui/inc/notemark.hxx index 2d83b8fe500e..b02bf812f623 100644 --- a/sc/source/ui/inc/notemark.hxx +++ b/sc/source/ui/inc/notemark.hxx @@ -24,9 +24,9 @@ #include <vcl/timer.hxx> #include "global.hxx" #include "address.hxx" +#include "postit.hxx" class SdrModel; -class SdrObject; class ScNoteMarker { @@ -46,7 +46,7 @@ private: Rectangle aRect; SdrModel* pModel; - SdrObject* pObject; + ScCaptionPtr mxObject; bool bVisible; Point aGridOff; DECL_LINK( TimeHdl, Timer*, void ); diff --git a/sc/source/ui/view/notemark.cxx b/sc/source/ui/view/notemark.cxx index 9d5282e58a77..564d98d83329 100644 --- a/sc/source/ui/view/notemark.cxx +++ b/sc/source/ui/view/notemark.cxx @@ -48,7 +48,6 @@ ScNoteMarker::ScNoteMarker( vcl::Window* pWin, vcl::Window* pRight, vcl::Window* bLeft( bLeftEdge ), bByKeyboard( bKeyboard ), pModel( nullptr ), - pObject( nullptr ), bVisible( false ) { Size aSizePixel = pWindow->GetOutputSizePixel(); @@ -94,11 +93,11 @@ IMPL_LINK_NOARG(ScNoteMarker, TimeHdl, Timer *, void) if( SdrPage* pPage = pModel->AllocPage( false ) ) { - pObject = ScNoteUtil::CreateTempCaption( *pDoc, aDocPos, *pPage, aUserText, aVisRect, bLeft ); - if( pObject ) + mxObject = ScNoteUtil::CreateTempCaption( *pDoc, aDocPos, *pPage, aUserText, aVisRect, bLeft ); + if( mxObject ) { - pObject->SetGridOffset( aGridOff ); - aRect = pObject->GetCurrentBoundRect(); + mxObject->SetGridOffset( aGridOff ); + aRect = mxObject->GetCurrentBoundRect(); } // Insert page so that the model recognise it and also deleted @@ -141,21 +140,21 @@ static MapMode lcl_MoveMapMode( const MapMode& rMap, const Size& rMove ) void ScNoteMarker::Draw() { - if ( pObject && bVisible ) + if ( mxObject && bVisible ) { - lcl_DrawWin( pObject, pWindow, aMapMode ); + lcl_DrawWin( mxObject.get(), pWindow, aMapMode ); if ( pRightWin || pBottomWin ) { Size aWinSize = pWindow->PixelToLogic( pWindow->GetOutputSizePixel(), aMapMode ); if ( pRightWin ) - lcl_DrawWin( pObject, pRightWin, + lcl_DrawWin( mxObject.get(), pRightWin, lcl_MoveMapMode( aMapMode, Size( aWinSize.Width(), 0 ) ) ); if ( pBottomWin ) - lcl_DrawWin( pObject, pBottomWin, + lcl_DrawWin( mxObject.get(), pBottomWin, lcl_MoveMapMode( aMapMode, Size( 0, aWinSize.Height() ) ) ); if ( pDiagWin ) - lcl_DrawWin( pObject, pDiagWin, lcl_MoveMapMode( aMapMode, aWinSize ) ); + lcl_DrawWin( mxObject.get(), pDiagWin, lcl_MoveMapMode( aMapMode, aWinSize ) ); } } } commit 2401cac9ff2d219520079c93736a947b9e99bacf Author: Eike Rathke <[email protected]> Date: Tue Mar 7 21:26:56 2017 +0100 coverity#1401471 implement move assignment and move ctor at ScCaptionPtr Change-Id: Ic429f5e177bb1a35857f00c6e13e5cbb34d46578 diff --git a/sc/inc/postit.hxx b/sc/inc/postit.hxx index 129989b94fa5..0801b2493abc 100644 --- a/sc/inc/postit.hxx +++ b/sc/inc/postit.hxx @@ -45,9 +45,11 @@ public: ScCaptionPtr(); explicit ScCaptionPtr( SdrCaptionObj* p ); ScCaptionPtr( const ScCaptionPtr& r ); + ScCaptionPtr( ScCaptionPtr&& r ); ~ScCaptionPtr(); ScCaptionPtr& operator=( const ScCaptionPtr& r ); + ScCaptionPtr& operator=( ScCaptionPtr&& r ); inline explicit operator bool() const { return mpCaption != nullptr; } inline SdrCaptionObj* get() const { return mpCaption; } inline SdrCaptionObj* operator->() const { return mpCaption; } @@ -96,6 +98,12 @@ private: */ void removeFromList(); + /** Replace this instance with pNew in a list, if any. + + Used by move-ctor and move assignment operator. + */ + void replaceInList( ScCaptionPtr* pNew ); + /** Dissolve list when the caption object is released or gone. */ void dissolve(); diff --git a/sc/source/core/data/postit.cxx b/sc/source/core/data/postit.cxx index 24b3aad8b88f..4071bb45993b 100644 --- a/sc/source/core/data/postit.cxx +++ b/sc/source/core/data/postit.cxx @@ -476,6 +476,28 @@ ScCaptionPtr::ScCaptionPtr( const ScCaptionPtr& r ) : } } +ScCaptionPtr::ScCaptionPtr( ScCaptionPtr&& r ) : + mpHead(r.mpHead), mpNext(r.mpNext), mpCaption(r.mpCaption) +{ + r.replaceInList( this ); + r.mpCaption = nullptr; +} + +ScCaptionPtr& ScCaptionPtr::operator=( ScCaptionPtr&& r ) +{ + if (this == &r) + return *this; + + mpHead = r.mpHead; + mpNext = r.mpNext; + mpCaption = r.mpCaption; + + r.replaceInList( this ); + r.mpCaption = nullptr; + + return *this; +} + ScCaptionPtr& ScCaptionPtr::operator=( const ScCaptionPtr& r ) { if (this == &r) @@ -521,6 +543,32 @@ void ScCaptionPtr::newHead() mpHead->mnRefs = 1; } +void ScCaptionPtr::replaceInList( ScCaptionPtr* pNew ) +{ + if (!mpHead && !mpNext) + return; + + assert(mpHead); + assert(mpCaption == pNew->mpCaption); + + ScCaptionPtr* pThat = mpHead->mpFirst; + while (pThat && pThat != this && pThat->mpNext != this) + { + pThat = pThat->mpNext; + } + if (pThat && pThat != this) + { + assert(pThat->mpNext == this); + pThat->mpNext = pNew; + } + pNew->mpNext = mpNext; + if (mpHead->mpFirst == this) + mpHead->mpFirst = pNew; + + mpHead = nullptr; + mpNext = nullptr; +} + void ScCaptionPtr::removeFromList() { if (!mpHead && !mpNext && !mpCaption) commit bd18465a70ec734ba266df421bf660dbb1ab4642 Author: Eike Rathke <[email protected]> Date: Tue Mar 7 20:35:45 2017 +0100 Resolves: tdf#106385 don't release ScCaptionPtr too early Change-Id: Ibf0afa1d6c582251a5a2e00f2d6d9c1f267bf746 diff --git a/sc/source/core/data/postit.cxx b/sc/source/core/data/postit.cxx index 1e30882e70b7..24b3aad8b88f 100644 --- a/sc/source/core/data/postit.cxx +++ b/sc/source/core/data/postit.cxx @@ -1058,9 +1058,7 @@ SdrCaptionObj* ScNoteUtil::CreateTempCaption( // create the caption object ScCaptionCreator aCreator( rDoc, rPos, bTailFront ); - // The caption object returned is completely unmanaged and stored - // elsewhere. - SdrCaptionObj* pCaption = aCreator.GetCaption().release(); + SdrCaptionObj* pCaption = aCreator.GetCaption().get(); // just for ease of use // insert caption into page (needed to set caption text) rDrawPage.InsertObject( pCaption ); @@ -1091,7 +1089,10 @@ SdrCaptionObj* ScNoteUtil::CreateTempCaption( // move caption into visible area aCreator.AutoPlaceCaption( &aVisRect ); - return pCaption; + + // The caption object returned is completely unmanaged and stored elsewhere. + // XXX Note it is already inserted to the draw page. + return aCreator.GetCaption().release(); } ScPostIt* ScNoteUtil::CreateNoteFromCaption( commit 6c151cf87819568ab26cfb5bb4f500e44becae46 Author: Eike Rathke <[email protected]> Date: Tue Mar 7 10:46:12 2017 +0100 a size is a size Change-Id: I98b54ac353bc0c078f31b447aec2eff7e80b4308 diff --git a/sc/source/ui/view/viewdata.cxx b/sc/source/ui/view/viewdata.cxx index 87a77ad4692d..309528e3619d 100644 --- a/sc/source/ui/view/viewdata.cxx +++ b/sc/source/ui/view/viewdata.cxx @@ -1803,9 +1803,9 @@ void ScViewData::CreateSelectedTabData() void ScViewData::EnsureTabDataSize(size_t nSize) { - if (nSize >= maTabData.size()) + if (nSize > maTabData.size()) { - size_t n = nSize - maTabData.size() + 1; + size_t n = nSize - maTabData.size(); maTabData.insert(maTabData.end(), n, nullptr); } } @@ -3135,7 +3135,7 @@ void ScViewData::ReadUserDataSequence(const uno::Sequence <beans::PropertyValue> sal_Int16 nTemp16(0); bool bPageMode(false); - EnsureTabDataSize(GetDocument()->GetTableCount()-1); + EnsureTabDataSize(GetDocument()->GetTableCount()); for (sal_Int32 i = 0; i < nCount; i++) { commit 6a3f36e5e6edaddf80132a071901b4e6ed1057b6 Author: Eike Rathke <[email protected]> Date: Tue Feb 28 18:28:53 2017 +0100 in decRefAndDestroy() the remaining element must be one and only one So head can be destroyed already there and removeFromList() take a short cut. Change-Id: I8f53d252c4e0ad867674ee410ecfaa300ac0c731 diff --git a/sc/source/core/data/postit.cxx b/sc/source/core/data/postit.cxx index 9f25cfe6ab9b..1e30882e70b7 100644 --- a/sc/source/core/data/postit.cxx +++ b/sc/source/core/data/postit.cxx @@ -637,14 +637,11 @@ void ScCaptionPtr::decRefAndDestroy() * foo and call SdrObject::Free(), likely with mpHead->mpCaption, see * ScPostIt::RemoveCaption(). Further work needed to be able to do so. * */ - ScCaptionPtr* pThat = (mpHead ? mpHead->mpFirst : this); - do - { - pThat->mpCaption = nullptr; - pThat = pThat->mpNext; - } - while (pThat); - assert(!mpCaption); // this ought to be in list and nulled + assert(mpHead->mpFirst == this); // this must be one and only one + mpCaption = nullptr; + assert(!mpNext); // this must be one and only one + delete mpHead; + mpHead = nullptr; } } commit 85ba0caf47f43c1542762c0b13793d6958f80ad0 Author: Eike Rathke <[email protected]> Date: Tue Feb 28 17:16:33 2017 +0100 dissolve() needs to delete head now that it's not a list element anymore Change-Id: I9949a1006e6d1b4b50dd5350106ad69b643e833c diff --git a/sc/source/core/data/postit.cxx b/sc/source/core/data/postit.cxx index 0f456611eebb..9f25cfe6ab9b 100644 --- a/sc/source/core/data/postit.cxx +++ b/sc/source/core/data/postit.cxx @@ -665,15 +665,18 @@ bool ScCaptionPtr::forget() void ScCaptionPtr::dissolve() { + ScCaptionPtr::Head* pHead = mpHead; ScCaptionPtr* pThat = (mpHead ? mpHead->mpFirst : this); while (pThat) { - assert(!pThat->mpNext || mpHead); // next without head is bad + assert(!pThat->mpNext || pThat->mpHead); // next without head is bad + assert(pThat->mpHead == pHead); // same head required within one list ScCaptionPtr* p = pThat->mpNext; pThat->clear(); pThat = p; } - clear(); + assert(!mpHead && !mpNext && !mpCaption); // should had been cleared during list walk + delete pHead; } void ScCaptionPtr::clear() commit 318ded2d7b9a03c8f76e2adda50997d5352e5607 Author: Eike Rathke <[email protected]> Date: Tue Feb 28 13:47:54 2017 +0100 assert that nullptr captions are not in a list Change-Id: I0c286891454d290ec4373dbc37e31d65c22c746d diff --git a/sc/source/core/data/postit.cxx b/sc/source/core/data/postit.cxx index 8c636693b3b3..0f456611eebb 100644 --- a/sc/source/core/data/postit.cxx +++ b/sc/source/core/data/postit.cxx @@ -494,6 +494,8 @@ ScCaptionPtr& ScCaptionPtr::operator=( const ScCaptionPtr& r ) // Let's find some weird usage. // Assigning without head doesn't make sense unless it is a nullptr caption. assert(r.mpHead || !r.mpCaption); + // A nullptr caption must not be in a list and thus not have a head. + assert(!r.mpHead || r.mpCaption); // Same captions were caught above, so here different heads must be present. assert(r.mpHead != mpHead); @@ -533,7 +535,7 @@ void ScCaptionPtr::removeFromList() // Use the walk to check consistency on the fly. assert(pThat->mpHead == mpHead); // all belong to the same assert(pThat->mpHead || !pThat->mpNext); // next without head is bad - assert(pThat->mpCaption == nullptr || pThat->mpCaption == mpCaption); + assert(pThat->mpCaption == mpCaption); pThat = pThat->mpNext; #if OSL_DEBUG_LEVEL > 0 ++nCount; @@ -556,7 +558,7 @@ void ScCaptionPtr::removeFromList() { assert(pThat->mpHead == mpHead); // all belong to the same assert(pThat->mpHead || !pThat->mpNext); // next without head is bad - assert(pThat->mpCaption == nullptr || pThat->mpCaption == mpCaption); + assert(pThat->mpCaption == mpCaption); ++nCount; } while ((pThat = pThat->mpNext) != nullptr); commit 344dd43b3f790d44abfb4c0addc4378b827a7fe1 Author: Eike Rathke <[email protected]> Date: Mon Feb 27 22:24:14 2017 +0100 move assign() into operator=() and add the relevant bits to copy-ctor Change-Id: Iac606a8e5e4fc9beb019d95d2a05f0ab9d9dad34 diff --git a/sc/inc/postit.hxx b/sc/inc/postit.hxx index a6bfe232dd4b..129989b94fa5 100644 --- a/sc/inc/postit.hxx +++ b/sc/inc/postit.hxx @@ -88,9 +88,6 @@ private: bool decRef() const; //< @returns <TRUE/> if the last reference was decremented. void decRefAndDestroy(); //< Destroys caption object if the last reference was decremented. - /** Operations common to ctor and assignment operator, maintaining the lists. */ - void assign( const ScCaptionPtr& r, bool bAssignment ); - /** Remove from current list and close gap. Usually there are only very few instances, so maintaining a doubly diff --git a/sc/source/core/data/postit.cxx b/sc/source/core/data/postit.cxx index f79a70e8256c..8c636693b3b3 100644 --- a/sc/source/core/data/postit.cxx +++ b/sc/source/core/data/postit.cxx @@ -459,21 +459,28 @@ ScCaptionPtr::ScCaptionPtr( SdrCaptionObj* p ) : } ScCaptionPtr::ScCaptionPtr( const ScCaptionPtr& r ) : - mpHead(nullptr), mpNext(nullptr), mpCaption(nullptr) + mpHead(r.mpHead), mpCaption(r.mpCaption) { - assign( r, false); + if (r.mpCaption) + { + assert(r.mpHead); + r.incRef(); + // Insert into list. + mpNext = r.mpNext; + r.mpNext = this; + } + else + { + assert(!r.mpHead); + mpNext = nullptr; + } } -void ScCaptionPtr::newHead() +ScCaptionPtr& ScCaptionPtr::operator=( const ScCaptionPtr& r ) { - assert(!mpHead); - mpHead = new Head; - mpHead->mpFirst = this; - mpHead->mnRefs = 1; -} + if (this == &r) + return *this; -void ScCaptionPtr::assign( const ScCaptionPtr& r, bool bAssignment ) -{ if (mpCaption == r.mpCaption) { // Two lists for the same caption is bad. @@ -481,7 +488,7 @@ void ScCaptionPtr::assign( const ScCaptionPtr& r, bool bAssignment ) assert(!mpCaption); // assigning same caption pointer within same list is weird // Nullptr captions are not inserted to the list, so nothing to do here // if both are. - return; + return *this; } // Let's find some weird usage. @@ -491,17 +498,25 @@ void ScCaptionPtr::assign( const ScCaptionPtr& r, bool bAssignment ) assert(r.mpHead != mpHead); r.incRef(); - if (bAssignment) - { - decRefAndDestroy(); - removeFromList(); - } + decRefAndDestroy(); + removeFromList(); + mpCaption = r.mpCaption; // That head is this' master. mpHead = r.mpHead; // Insert into list. mpNext = r.mpNext; r.mpNext = this; + + return *this; +} + +void ScCaptionPtr::newHead() +{ + assert(!mpHead); + mpHead = new Head; + mpHead->mpFirst = this; + mpHead->mnRefs = 1; } void ScCaptionPtr::removeFromList() @@ -666,15 +681,6 @@ void ScCaptionPtr::clear() mpCaption = nullptr; } -ScCaptionPtr& ScCaptionPtr::operator=( const ScCaptionPtr& r ) -{ - if (this == &r) - return *this; - - assign( r, true); - return *this; -} - struct ScCaptionInitData { commit 36ce8cb4ed9d811a59159cfd4018d56cfa660ec9 Author: Eike Rathke <[email protected]> Date: Mon Feb 27 19:50:07 2017 +0100 rework ScCaptionPtr to have a distinct head element Not only saves a pointer per list element, but future versions could hold a document pointer and/or drawing layer's draw page as well. Change-Id: I85e05981239223bec88c47f2ebe4c22e50cd9a0d diff --git a/sc/inc/postit.hxx b/sc/inc/postit.hxx index e8f212208289..a6bfe232dd4b 100644 --- a/sc/inc/postit.hxx +++ b/sc/inc/postit.hxx @@ -72,11 +72,18 @@ public: oslInterlockedCount getRefs() const; private: - ScCaptionPtr* mpHead; ///< points to the "master" entry - mutable ScCaptionPtr* mpNext; ///< next in list - SdrCaptionObj* mpCaption; ///< the caption object, managed by head master - mutable oslInterlockedCount mnRefs; ///< use count, managed by head master + struct Head + { + ScCaptionPtr* mpFirst; ///< first in list + oslInterlockedCount mnRefs; ///< use count + }; + + Head* mpHead; ///< points to the "master" entry + mutable ScCaptionPtr* mpNext; ///< next in list + SdrCaptionObj* mpCaption; ///< the caption object, managed by head master + + void newHead(); //< Allocate a new Head and init. void incRef() const; bool decRef() const; //< @returns <TRUE/> if the last reference was decremented. void decRefAndDestroy(); //< Destroys caption object if the last reference was decremented. diff --git a/sc/source/core/data/postit.cxx b/sc/source/core/data/postit.cxx index baeca99d25b2..f79a70e8256c 100644 --- a/sc/source/core/data/postit.cxx +++ b/sc/source/core/data/postit.cxx @@ -445,34 +445,50 @@ ScNoteCaptionCreator::ScNoteCaptionCreator( ScDocument& rDoc, const ScAddress& r ScCaptionPtr::ScCaptionPtr() : - mpHead(nullptr), mpNext(nullptr), mpCaption(nullptr), mnRefs(0) + mpHead(nullptr), mpNext(nullptr), mpCaption(nullptr) { } ScCaptionPtr::ScCaptionPtr( SdrCaptionObj* p ) : - mpHead( p ? this : nullptr ), mpNext(nullptr), mpCaption(p), mnRefs( p ? 1 : 0 ) + mpHead(nullptr), mpNext(nullptr), mpCaption(p) { + if (p) + { + newHead(); + } } ScCaptionPtr::ScCaptionPtr( const ScCaptionPtr& r ) : - mpHead(nullptr), mpNext(nullptr), mpCaption(nullptr), mnRefs(0) + mpHead(nullptr), mpNext(nullptr), mpCaption(nullptr) { assign( r, false); } +void ScCaptionPtr::newHead() +{ + assert(!mpHead); + mpHead = new Head; + mpHead->mpFirst = this; + mpHead->mnRefs = 1; +} + void ScCaptionPtr::assign( const ScCaptionPtr& r, bool bAssignment ) { if (mpCaption == r.mpCaption) + { + // Two lists for the same caption is bad. + assert(!mpCaption || mpHead == r.mpHead); + assert(!mpCaption); // assigning same caption pointer within same list is weird + // Nullptr captions are not inserted to the list, so nothing to do here + // if both are. return; + } // Let's find some weird usage. - // Though assigning some other of the same list to head may syntactically - // be valid, why would we do that? - assert(r.mpHead != this); - // Same for assigning head to some other. - assert(mpHead != &r); - // And any other of the same list. - assert(mpHead != r.mpHead); + // Assigning without head doesn't make sense unless it is a nullptr caption. + assert(r.mpHead || !r.mpCaption); + // Same captions were caught above, so here different heads must be present. + assert(r.mpHead != mpHead); r.incRef(); if (bAssignment) @@ -483,7 +499,6 @@ void ScCaptionPtr::assign( const ScCaptionPtr& r, bool bAssignment ) mpCaption = r.mpCaption; // That head is this' master. mpHead = r.mpHead; - mnRefs = 0; // Insert into list. mpNext = r.mpNext; r.mpNext = this; @@ -491,54 +506,62 @@ void ScCaptionPtr::assign( const ScCaptionPtr& r, bool bAssignment ) void ScCaptionPtr::removeFromList() { - if (!mpHead && !mpNext && !mpCaption && !mnRefs) + if (!mpHead && !mpNext && !mpCaption) return; - ScCaptionPtr* pNewHead = (mpHead == this ? mpNext : nullptr); - ScCaptionPtr* pThat = (mpHead == this ? mpNext : mpHead); // do not replace on self if head - while (pThat && pThat->mpNext != this) +#if OSL_DEBUG_LEVEL > 0 + oslInterlockedCount nCount = 0; +#endif + ScCaptionPtr* pThat = (mpHead ? mpHead->mpFirst : nullptr); + while (pThat && pThat != this && pThat->mpNext != this) { // Use the walk to check consistency on the fly. + assert(pThat->mpHead == mpHead); // all belong to the same + assert(pThat->mpHead || !pThat->mpNext); // next without head is bad assert(pThat->mpCaption == nullptr || pThat->mpCaption == mpCaption); - assert(pThat->mnRefs == 0 || pThat == mpHead); - if (pNewHead) - { - assert(pThat->mpHead == mpHead); - pThat->mpHead = pNewHead; - } pThat = pThat->mpNext; +#if OSL_DEBUG_LEVEL > 0 + ++nCount; +#endif } - assert(pThat || mpHead == this); // not found only if this was head + assert(pThat || !mpHead); // not found only if this was standalone if (pThat) { - pThat->mpNext = mpNext; - if (pNewHead) + if (pThat != this) { - do - { - assert(pThat->mpCaption == nullptr || pThat->mpCaption == mpCaption); - assert(pThat->mnRefs == 0 || pThat == mpHead); - assert(pThat->mpHead == mpHead); - pThat->mpHead = pNewHead; - } - while ((pThat = pThat->mpNext) != nullptr); +#if OSL_DEBUG_LEVEL > 0 + // The while loop above was not executed, and for this + // (pThat->mpNext) the loop below won't either. + ++nCount; +#endif + pThat->mpNext = mpNext; } - else +#if OSL_DEBUG_LEVEL > 0 + do { + assert(pThat->mpHead == mpHead); // all belong to the same + assert(pThat->mpHead || !pThat->mpNext); // next without head is bad + assert(pThat->mpCaption == nullptr || pThat->mpCaption == mpCaption); + ++nCount; + } + while ((pThat = pThat->mpNext) != nullptr); +#endif + } #if OSL_DEBUG_LEVEL > 0 - // XXX exactly as for the if() branch but without replacing head. - do - { - assert(pThat->mpCaption == nullptr || pThat->mpCaption == mpCaption); - assert(pThat->mnRefs == 0 || pThat == mpHead); - assert(pThat->mpHead == mpHead); - } - while ((pThat = pThat->mpNext) != nullptr); + // If part of a list then refs were already decremented. + assert(nCount == (mpHead ? mpHead->mnRefs + 1 : 0)); #endif + if (mpHead && mpHead->mpFirst == this) + { + if (mpNext) + mpHead->mpFirst = mpNext; + else + { + // The only one destroys also head. + assert(mpHead->mnRefs == 0); // cough + delete mpHead; // DEAD now } } - if (pNewHead) - pNewHead->mnRefs = mnRefs; mpHead = nullptr; mpNext = nullptr; } @@ -550,7 +573,7 @@ void ScCaptionPtr::reset( SdrCaptionObj* p ) if (p) { // Check if we end up with a duplicated management in this list. - ScCaptionPtr* pThat = mpHead; + ScCaptionPtr* pThat = (mpHead ? mpHead->mpFirst : nullptr); while (pThat) { assert(pThat->mpCaption != p); @@ -563,13 +586,7 @@ void ScCaptionPtr::reset( SdrCaptionObj* p ) mpCaption = p; if (p) { - mpHead = this; - mnRefs = 1; - } - else - { - mpHead = nullptr; - mnRefs = 0; + newHead(); } } @@ -581,8 +598,7 @@ ScCaptionPtr::~ScCaptionPtr() oslInterlockedCount ScCaptionPtr::getRefs() const { - assert(!mnRefs || mpHead == this); - return mpHead ? mpHead->mnRefs : mnRefs; + return mpHead ? mpHead->mnRefs : 0; } void ScCaptionPtr::incRef() const @@ -604,16 +620,14 @@ void ScCaptionPtr::decRefAndDestroy() * foo and call SdrObject::Free(), likely with mpHead->mpCaption, see * ScPostIt::RemoveCaption(). Further work needed to be able to do so. * */ - ScCaptionPtr* pThat = mpHead; + ScCaptionPtr* pThat = (mpHead ? mpHead->mpFirst : this); do { pThat->mpCaption = nullptr; - assert(pThat->mnRefs == 0); - pThat->mnRefs = 0; pThat = pThat->mpNext; } while (pThat); - assert(!mpCaption); // this ought to be in list and nulled + assert(!mpCaption); // this ought to be in list and nulled } } @@ -629,15 +643,15 @@ bool ScCaptionPtr::forget() bool bRet = decRef(); removeFromList(); mpCaption = nullptr; - mnRefs = 0; return bRet; } void ScCaptionPtr::dissolve() { - ScCaptionPtr* pThat = mpHead; + ScCaptionPtr* pThat = (mpHead ? mpHead->mpFirst : this); while (pThat) { + assert(!pThat->mpNext || mpHead); // next without head is bad ScCaptionPtr* p = pThat->mpNext; pThat->clear(); pThat = p; @@ -650,7 +664,6 @@ void ScCaptionPtr::clear() mpHead = nullptr; mpNext = nullptr; mpCaption = nullptr; - mnRefs = 0; } ScCaptionPtr& ScCaptionPtr::operator=( const ScCaptionPtr& r ) commit f4fe5251ee225a383d6a9b3b439793990a694a75 Author: Eike Rathke <[email protected]> Date: Fri Feb 24 15:30:24 2017 +0100 a first stab against the note caption ownership mess This should not change any existing behavior, but may help tracking down what happens where and when. The final goal is to let ScCaptionPtr also handle deletion of caption objects once they're unreferenced and guard against dangling pointers and double delete, and/or to manage transfer of ownership to the drawing layer. Further improvement to the structure could involve a head data element so that the duplicated (and unused except in head) mnRefs field could be eliminated and the walk simplified when removing an element from the list. Change-Id: Ifbb2fb1d9dc4d2594a1eae2a8489270dd1fe0d0c Reviewed-on: https://gerrit.libreoffice.org/34616 Reviewed-by: Eike Rathke <[email protected]> Tested-by: Jenkins <[email protected]> diff --git a/sc/inc/postit.hxx b/sc/inc/postit.hxx index 87553f3f5c3e..e8f212208289 100644 --- a/sc/inc/postit.hxx +++ b/sc/inc/postit.hxx @@ -36,6 +36,69 @@ class ScDocument; class Rectangle; struct ScCaptionInitData; +/** Some desperate attempt to fight against the caption object ownership mess, + to which none of shared/weak/plain pointer is a cure. + */ +class ScCaptionPtr +{ +public: + ScCaptionPtr(); + explicit ScCaptionPtr( SdrCaptionObj* p ); + ScCaptionPtr( const ScCaptionPtr& r ); + ~ScCaptionPtr(); + + ScCaptionPtr& operator=( const ScCaptionPtr& r ); + inline explicit operator bool() const { return mpCaption != nullptr; } + inline SdrCaptionObj* get() const { return mpCaption; } + inline SdrCaptionObj* operator->() const { return mpCaption; } + inline SdrCaptionObj& operator*() const { return *mpCaption; } + + // Does not default to nullptr to make it visually obvious where such is used. + void reset( SdrCaptionObj* p ); + + /** Release all management of the SdrCaptionObj* in all instances of this + list and dissolve. The SdrCaptionObj pointer returned is ready to be + managed elsewhere. + */ + SdrCaptionObj* release(); + + /** Forget the SdrCaptionObj pointer in this one instance. + Decrements a use count but does not destroy the object, it's up to the + caller to manage this mess.. + @returns <TRUE/> if the last reference was decremented. + */ + bool forget(); + + oslInterlockedCount getRefs() const; + +private: + ScCaptionPtr* mpHead; ///< points to the "master" entry + mutable ScCaptionPtr* mpNext; ///< next in list + SdrCaptionObj* mpCaption; ///< the caption object, managed by head master + mutable oslInterlockedCount mnRefs; ///< use count, managed by head master + + void incRef() const; + bool decRef() const; //< @returns <TRUE/> if the last reference was decremented. + void decRefAndDestroy(); //< Destroys caption object if the last reference was decremented. + + /** Operations common to ctor and assignment operator, maintaining the lists. */ + void assign( const ScCaptionPtr& r, bool bAssignment ); + + /** Remove from current list and close gap. + + Usually there are only very few instances, so maintaining a doubly + linked list isn't worth memory/performance wise and a simple walk does + it. + */ + void removeFromList(); + + /** Dissolve list when the caption object is released or gone. */ + void dissolve(); + + /** Just clear everything, while dissolving the list. */ + void clear(); +}; + /** Internal data for a cell annotation. */ struct SC_DLLPUBLIC ScNoteData { @@ -44,7 +107,7 @@ struct SC_DLLPUBLIC ScNoteData OUString maDate; /// Creation date of the note. OUString maAuthor; /// Author of the note. ScCaptionInitDataRef mxInitData; /// Initial data for invisible notes without SdrObject. - SdrCaptionObj* mpCaption; /// Drawing object representing the cell note. + ScCaptionPtr mxCaption; /// Drawing object representing the cell note. bool mbShown; /// True = note is visible. explicit ScNoteData( bool bShown = false ); @@ -124,10 +187,12 @@ public: void SetText( const ScAddress& rPos, const OUString& rText ); /** Returns an existing note caption object. returns null, if the note - contains initial caption data needed to construct a caption object. */ - SdrCaptionObj* GetCaption() const { return maNoteData.mpCaption;} + contains initial caption data needed to construct a caption object. + The SdrCaptionObj* returned is unmanaged and must not be stored elsewhere. */ + SdrCaptionObj* GetCaption() const { return maNoteData.mxCaption.get();} /** Returns the caption object of this note. Creates the caption object, if - the note contains initial caption data instead of the caption. */ + the note contains initial caption data instead of the caption. + The SdrCaptionObj* returned is unmanaged and must not be stored elsewhere. */ SdrCaptionObj* GetOrCreateCaption( const ScAddress& rPos ) const; /** Forgets the pointer to the note caption object. @@ -179,8 +244,10 @@ public: This function is used in import filters to reuse the imported drawing object as note caption object. - @param rCaption The drawing object for the cell note. This object MUST + @param pCaption The drawing object for the cell note. This object MUST be inserted into the document at the correct drawing page already. + The underlying ScPostIt::ScNoteData::ScCaptionPtr takes managing + ownership of the pointer. @return Pointer to the new cell note object if insertion was successful (i.e. the passed cell position was valid), null @@ -190,7 +257,7 @@ public: */ static ScPostIt* CreateNoteFromCaption( ScDocument& rDoc, const ScAddress& rPos, - SdrCaptionObj& rCaption, bool bShown ); + SdrCaptionObj* pCaption, bool bShown ); /** Creates a cell note based on the passed caption object data. diff --git a/sc/source/core/data/postit.cxx b/sc/source/core/data/postit.cxx index 83c6111823fa..baeca99d25b2 100644 --- a/sc/source/core/data/postit.cxx +++ b/sc/source/core/data/postit.cxx @@ -161,12 +161,12 @@ public: /** Create a new caption. The caption will not be inserted into the document. */ explicit ScCaptionCreator( ScDocument& rDoc, const ScAddress& rPos, bool bTailFront ); /** Manipulate an existing caption. */ - explicit ScCaptionCreator( ScDocument& rDoc, const ScAddress& rPos, SdrCaptionObj& rCaption ); + explicit ScCaptionCreator( ScDocument& rDoc, const ScAddress& rPos, ScCaptionPtr& xCaption ); /** Returns the drawing layer page of the sheet contained in maPos. */ SdrPage* GetDrawPage(); /** Returns the caption drawing object. */ - inline SdrCaptionObj* GetCaption() { return mpCaption; } + inline ScCaptionPtr GetCaption() { return mxCaption; } /** Moves the caption inside the passed rectangle. Uses page area if 0 is passed. */ void FitCaptionToRect( const Rectangle* pVisRect = nullptr ); @@ -193,7 +193,7 @@ private: private: ScDocument& mrDoc; ScAddress maPos; - SdrCaptionObj* mpCaption; + ScCaptionPtr mxCaption; Rectangle maPageRect; Rectangle maCellRect; bool mbNegPage; @@ -201,25 +201,23 @@ private: ScCaptionCreator::ScCaptionCreator( ScDocument& rDoc, const ScAddress& rPos, bool bTailFront ) : mrDoc( rDoc ), - maPos( rPos ), - mpCaption( nullptr ) + maPos( rPos ) { Initialize(); CreateCaption( true/*bShown*/, bTailFront ); } -ScCaptionCreator::ScCaptionCreator( ScDocument& rDoc, const ScAddress& rPos, SdrCaptionObj& rCaption ) : +ScCaptionCreator::ScCaptionCreator( ScDocument& rDoc, const ScAddress& rPos, ScCaptionPtr& xCaption ) : mrDoc( rDoc ), maPos( rPos ), - mpCaption( &rCaption ) + mxCaption( xCaption ) { Initialize(); } ScCaptionCreator::ScCaptionCreator( ScDocument& rDoc, const ScAddress& rPos ) : mrDoc( rDoc ), - maPos( rPos ), - mpCaption( nullptr ) + maPos( rPos ) { Initialize(); } @@ -235,13 +233,13 @@ void ScCaptionCreator::FitCaptionToRect( const Rectangle* pVisRect ) const Rectangle& rVisRect = GetVisRect( pVisRect ); // tail position - Point aTailPos = mpCaption->GetTailPos(); + Point aTailPos = mxCaption->GetTailPos(); aTailPos.X() = ::std::max( ::std::min( aTailPos.X(), rVisRect.Right() ), rVisRect.Left() ); aTailPos.Y() = ::std::max( ::std::min( aTailPos.Y(), rVisRect.Bottom() ), rVisRect.Top() ); - mpCaption->SetTailPos( aTailPos ); + mxCaption->SetTailPos( aTailPos ); // caption rectangle - Rectangle aCaptRect = mpCaption->GetLogicRect(); + Rectangle aCaptRect = mxCaption->GetLogicRect(); Point aCaptPos = aCaptRect.TopLeft(); // move textbox inside right border of visible area aCaptPos.X() = ::std::min< long >( aCaptPos.X(), rVisRect.Right() - aCaptRect.GetWidth() ); @@ -253,7 +251,7 @@ void ScCaptionCreator::FitCaptionToRect( const Rectangle* pVisRect ) aCaptPos.Y() = ::std::max< long >( aCaptPos.Y(), rVisRect.Top() ); // update caption aCaptRect.SetPos( aCaptPos ); - mpCaption->SetLogicRect( aCaptRect ); + mxCaption->SetLogicRect( aCaptRect ); } void ScCaptionCreator::AutoPlaceCaption( const Rectangle* pVisRect ) @@ -261,7 +259,7 @@ void ScCaptionCreator::AutoPlaceCaption( const Rectangle* pVisRect ) const Rectangle& rVisRect = GetVisRect( pVisRect ); // caption rectangle - Rectangle aCaptRect = mpCaption->GetLogicRect(); + Rectangle aCaptRect = mxCaption->GetLogicRect(); long nWidth = aCaptRect.GetWidth(); long nHeight = aCaptRect.GetHeight(); @@ -319,7 +317,7 @@ void ScCaptionCreator::AutoPlaceCaption( const Rectangle* pVisRect ) // update textbox position in note caption object aCaptRect.SetPos( aCaptPos ); - mpCaption->SetLogicRect( aCaptRect ); + mxCaption->SetLogicRect( aCaptRect ); FitCaptionToRect( pVisRect ); } @@ -328,33 +326,33 @@ void ScCaptionCreator::UpdateCaptionPos() ScDrawLayer* pDrawLayer = mrDoc.GetDrawLayer(); // update caption position - const Point& rOldTailPos = mpCaption->GetTailPos(); + const Point& rOldTailPos = mxCaption->GetTailPos(); Point aTailPos = CalcTailPos( false ); if( rOldTailPos != aTailPos ) { // create drawing undo action if( pDrawLayer && pDrawLayer->IsRecording() ) - pDrawLayer->AddCalcUndo( new SdrUndoGeoObj( *mpCaption ) ); + pDrawLayer->AddCalcUndo( new SdrUndoGeoObj( *mxCaption ) ); // calculate new caption rectangle (#i98141# handle LTR<->RTL switch correctly) - Rectangle aCaptRect = mpCaption->GetLogicRect(); + Rectangle aCaptRect = mxCaption->GetLogicRect(); long nDiffX = (rOldTailPos.X() >= 0) ? (aCaptRect.Left() - rOldTailPos.X()) : (rOldTailPos.X() - aCaptRect.Right()); if( mbNegPage ) nDiffX = -nDiffX - aCaptRect.GetWidth(); long nDiffY = aCaptRect.Top() - rOldTailPos.Y(); aCaptRect.SetPos( aTailPos + Point( nDiffX, nDiffY ) ); // set new tail position and caption rectangle - mpCaption->SetTailPos( aTailPos ); - mpCaption->SetLogicRect( aCaptRect ); + mxCaption->SetTailPos( aTailPos ); + mxCaption->SetLogicRect( aCaptRect ); // fit caption into draw page FitCaptionToRect(); } // update cell position in caption user data - ScDrawObjData* pCaptData = ScDrawLayer::GetNoteCaptionData( mpCaption, maPos.Tab() ); + ScDrawObjData* pCaptData = ScDrawLayer::GetNoteCaptionData( mxCaption.get(), maPos.Tab() ); if( pCaptData && (maPos != pCaptData->maStart) ) { // create drawing undo action if( pDrawLayer && pDrawLayer->IsRecording() ) - pDrawLayer->AddCalcUndo( new ScUndoObjData( mpCaption, pCaptData->maStart, pCaptData->maEnd, maPos, pCaptData->maEnd ) ); + pDrawLayer->AddCalcUndo( new ScUndoObjData( mxCaption.get(), pCaptData->maStart, pCaptData->maEnd, maPos, pCaptData->maEnd ) ); // set new position pCaptData->maStart = maPos; } @@ -376,9 +374,9 @@ void ScCaptionCreator::CreateCaption( bool bShown, bool bTailFront ) // create the caption drawing object Rectangle aTextRect( Point( 0 , 0 ), Size( SC_NOTECAPTION_WIDTH, SC_NOTECAPTION_HEIGHT ) ); Point aTailPos = CalcTailPos( bTailFront ); - mpCaption = new SdrCaptionObj( aTextRect, aTailPos ); + mxCaption.reset( new SdrCaptionObj( aTextRect, aTailPos )); // basic caption settings - ScCaptionUtil::SetBasicCaptionSettings( *mpCaption, bShown ); + ScCaptionUtil::SetBasicCaptionSettings( *mxCaption, bShown ); } void ScCaptionCreator::Initialize() @@ -402,7 +400,7 @@ public: /** Create a new caption object and inserts it into the document. */ explicit ScNoteCaptionCreator( ScDocument& rDoc, const ScAddress& rPos, ScNoteData& rNoteData ); /** Manipulate an existing caption. */ - explicit ScNoteCaptionCreator( ScDocument& rDoc, const ScAddress& rPos, SdrCaptionObj& rCaption, bool bShown ); + explicit ScNoteCaptionCreator( ScDocument& rDoc, const ScAddress& rPos, ScCaptionPtr& xCaption, bool bShown ); }; ScNoteCaptionCreator::ScNoteCaptionCreator( ScDocument& rDoc, const ScAddress& rPos, ScNoteData& rNoteData ) : @@ -414,37 +412,257 @@ ScNoteCaptionCreator::ScNoteCaptionCreator( ScDocument& rDoc, const ScAddress& r { // create the caption drawing object CreateCaption( rNoteData.mbShown, false ); - rNoteData.mpCaption = GetCaption(); - OSL_ENSURE( rNoteData.mpCaption, "ScNoteCaptionCreator::ScNoteCaptionCreator - missing caption object" ); - if( rNoteData.mpCaption ) + rNoteData.mxCaption = GetCaption(); + OSL_ENSURE( rNoteData.mxCaption, "ScNoteCaptionCreator::ScNoteCaptionCreator - missing caption object" ); + if( rNoteData.mxCaption ) { // store note position in user data of caption object - ScCaptionUtil::SetCaptionUserData( *rNoteData.mpCaption, rPos ); + ScCaptionUtil::SetCaptionUserData( *rNoteData.mxCaption, rPos ); // insert object into draw page - pDrawPage->InsertObject( rNoteData.mpCaption ); + pDrawPage->InsertObject( rNoteData.mxCaption.get() ); } } } -ScNoteCaptionCreator::ScNoteCaptionCreator( ScDocument& rDoc, const ScAddress& rPos, SdrCaptionObj& rCaption, bool bShown ) : - ScCaptionCreator( rDoc, rPos, rCaption ) +ScNoteCaptionCreator::ScNoteCaptionCreator( ScDocument& rDoc, const ScAddress& rPos, ScCaptionPtr& xCaption, bool bShown ) : + ScCaptionCreator( rDoc, rPos, xCaption ) { SdrPage* pDrawPage = GetDrawPage(); OSL_ENSURE( pDrawPage, "ScNoteCaptionCreator::ScNoteCaptionCreator - no drawing page" ); - OSL_ENSURE( rCaption.GetPage() == pDrawPage, "ScNoteCaptionCreator::ScNoteCaptionCreator - wrong drawing page in caption" ); - if( pDrawPage && (rCaption.GetPage() == pDrawPage) ) + OSL_ENSURE( xCaption->GetPage() == pDrawPage, "ScNoteCaptionCreator::ScNoteCaptionCreator - wrong drawing page in caption" ); + if( pDrawPage && (xCaption->GetPage() == pDrawPage) ) { // store note position in user data of caption object - ScCaptionUtil::SetCaptionUserData( rCaption, rPos ); + ScCaptionUtil::SetCaptionUserData( *xCaption, rPos ); // basic caption settings - ScCaptionUtil::SetBasicCaptionSettings( rCaption, bShown ); + ScCaptionUtil::SetBasicCaptionSettings( *xCaption, bShown ); // set correct tail position - rCaption.SetTailPos( CalcTailPos( false ) ); + xCaption->SetTailPos( CalcTailPos( false ) ); } } } // namespace + +ScCaptionPtr::ScCaptionPtr() : + mpHead(nullptr), mpNext(nullptr), mpCaption(nullptr), mnRefs(0) +{ +} + +ScCaptionPtr::ScCaptionPtr( SdrCaptionObj* p ) : + mpHead( p ? this : nullptr ), mpNext(nullptr), mpCaption(p), mnRefs( p ? 1 : 0 ) +{ +} + +ScCaptionPtr::ScCaptionPtr( const ScCaptionPtr& r ) : + mpHead(nullptr), mpNext(nullptr), mpCaption(nullptr), mnRefs(0) +{ + assign( r, false); +} + +void ScCaptionPtr::assign( const ScCaptionPtr& r, bool bAssignment ) +{ + if (mpCaption == r.mpCaption) + return; + + // Let's find some weird usage. + // Though assigning some other of the same list to head may syntactically + // be valid, why would we do that? + assert(r.mpHead != this); + // Same for assigning head to some other. + assert(mpHead != &r); + // And any other of the same list. + assert(mpHead != r.mpHead); + + r.incRef(); + if (bAssignment) + { + decRefAndDestroy(); + removeFromList(); + } + mpCaption = r.mpCaption; + // That head is this' master. + mpHead = r.mpHead; + mnRefs = 0; + // Insert into list. + mpNext = r.mpNext; + r.mpNext = this; +} + +void ScCaptionPtr::removeFromList() +{ + if (!mpHead && !mpNext && !mpCaption && !mnRefs) + return; + + ScCaptionPtr* pNewHead = (mpHead == this ? mpNext : nullptr); + ScCaptionPtr* pThat = (mpHead == this ? mpNext : mpHead); // do not replace on self if head + while (pThat && pThat->mpNext != this) + { + // Use the walk to check consistency on the fly. + assert(pThat->mpCaption == nullptr || pThat->mpCaption == mpCaption); + assert(pThat->mnRefs == 0 || pThat == mpHead); + if (pNewHead) + { + assert(pThat->mpHead == mpHead); + pThat->mpHead = pNewHead; + } + pThat = pThat->mpNext; + } + assert(pThat || mpHead == this); // not found only if this was head + if (pThat) + { + pThat->mpNext = mpNext; + if (pNewHead) + { + do + { + assert(pThat->mpCaption == nullptr || pThat->mpCaption == mpCaption); + assert(pThat->mnRefs == 0 || pThat == mpHead); + assert(pThat->mpHead == mpHead); + pThat->mpHead = pNewHead; + } + while ((pThat = pThat->mpNext) != nullptr); + } + else + { +#if OSL_DEBUG_LEVEL > 0 + // XXX exactly as for the if() branch but without replacing head. + do + { + assert(pThat->mpCaption == nullptr || pThat->mpCaption == mpCaption); + assert(pThat->mnRefs == 0 || pThat == mpHead); + assert(pThat->mpHead == mpHead); + } + while ((pThat = pThat->mpNext) != nullptr); +#endif + } + } + if (pNewHead) ... etc. - the rest is truncated _______________________________________________ Libreoffice-commits mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits
