[Libreoffice-commits] core.git: sd/source

[Caolán McNamara (via logerrit)]

 sd/source/ui/view/sdview2.cxx |   19 ++++++++-----------
 sd/source/ui/view/sdview3.cxx |   21 +++++++++++++++++++++
 2 files changed, 29 insertions(+), 11 deletions(-)

New commits:
commit 491678690b5b40f446b40c368ec4b4423ee603c1
Author:     Caolán McNamara <caolanm at redhat.com>
AuthorDate: Tue Sep 7 16:46:38 2021 +0100
Commit:     Caolán McNamara <caolanm at redhat.com>
CommitDate: Tue Sep 7 19:54:03 2021 +0200

    unbalanced [Beg/End]Undo possible on self-dnd in impress
    
    repeatedly holding the mouse button on an object long enough to start a
    drag and then trying again will eventually result in a situation where
    View::StartDrag is called, triggering a gtk_drag_begin which neither
    fails with drag-failure or succeeds with drag-end and so no
    View::DragFinished gets called. This is possibly only an issue under
    (Fedora 34) wayland.
    
    When it happens it leaves an unbalanced BegUndo from StartDrag with no
    EndUndo which will eventually be fatal if the user uses undo.
    
    So remove the BegUndo from StartDrag because we can't be sure to get
    a DragFinished and...
    
    a) put the removal of original objects (when ACTION_MOVE) in
    DragFinished in a simple BegUndo/EndUndo block of its own. Whether the
    dnd is to another application or to ourself won't matter.
    b) in View::InsertData detect if the action is a self-dnd and if so
    put that in another BegUndo/EndUndo block and explicitly call
    DragFinished before EndUndo to put a) within that group so a drag
    of an object from one slide to the other gives a single "Drag and Drop"
    move undo action listed for the copy and delete. DragFinished will
    get called again but that's ok in this case and its 2nd call won't
    affect the undo stack.
    c) when this problem arises, later Drag attempts fail to do anything
    because View::StartDrag returns early if a mpDragSrcMarkList from an
    earlier attempt is set.  Remove that guard and allow mpDragSrcMarkList
    to be replaced after a silently failed drag attempt.
    
    Change-Id: I3933663ff74f1ca99d9410b7752227ecf8d0da46
    Reviewed-on: https://gerrit.libreoffice.org/c/core/+/121787
    Tested-by: Jenkins
    Tested-by: Caolán McNamara <caolanm at redhat.com>
    Reviewed-by: Caolán McNamara <caolanm at redhat.com>

diff --git a/sd/source/ui/view/sdview2.cxx b/sd/source/ui/view/sdview2.cxx
index 6ac19f72e1e7..5f305228c1e3 100644
--- a/sd/source/ui/view/sdview2.cxx
+++ b/sd/source/ui/view/sdview2.cxx
@@ -341,7 +341,7 @@ void View::DoPaste (::sd::Window* pWindow)
 
 void View::StartDrag( const Point& rStartPos, vcl::Window* pWindow )
 {
-    if( !AreObjectsMarked() || !IsAction() || !mpViewSh || !pWindow || mpDragSrcMarkList )
+    if (!AreObjectsMarked() || !IsAction() || !mpViewSh || !pWindow)
         return;
 
     BrkAction();
@@ -359,17 +359,18 @@ void View::StartDrag( const Point& rStartPos, vcl::Window* pWindow )
     mpDragSrcMarkList.reset( new SdrMarkList(GetMarkedObjectList()) );
     mnDragSrcPgNum = GetSdrPageView()->GetPage()->GetPageNum();
 
-    if( IsUndoEnabled() )
-    {
-        OUString aStr(SdResId(STR_UNDO_DRAGDROP));
-        BegUndo(aStr + " " + mpDragSrcMarkList->GetMarkDescription());
-    }
     CreateDragDataObject( this, *pWindow, rStartPos );
 }
 
 void View::DragFinished( sal_Int8 nDropAction )
 {
     const bool bUndo = IsUndoEnabled();
+    const bool bGroupUndo = bUndo && mpDragSrcMarkList;
+    if (bGroupUndo)
+    {
+        OUString aStr(SdResId(STR_UNDO_DRAGDROP));
+        BegUndo(aStr + " " + mpDragSrcMarkList->GetMarkDescription());
+    }
 
     SdTransferable* pDragTransferable = SD_MOD()->pTransferDrag;
 
@@ -419,11 +420,7 @@ void View::DragFinished( sal_Int8 nDropAction )
     if( pDragTransferable )
         pDragTransferable->SetInternalMove( false );
 
-    //This Undo appears to matches with the STR_UNDO_DRAGDROP Undo Start of
-    //View::StartDrag But this DragFinished can be called without a matching
-    //StartDrag. So use the existence of mpDragSrcMarkList as a flag that
-    //this EndUndo has a matching BegUndo
-    if (bUndo && mpDragSrcMarkList)
+    if (bGroupUndo)
         EndUndo();
     mnDragSrcPgNum = SDRPAGE_NOTFOUND;
     mpDragSrcMarkList.reset();
diff --git a/sd/source/ui/view/sdview3.cxx b/sd/source/ui/view/sdview3.cxx
index 8bbb9ce7e24f..a7204648ee4d 100644
--- a/sd/source/ui/view/sdview3.cxx
+++ b/sd/source/ui/view/sdview3.cxx
@@ -293,17 +293,29 @@ bool View::InsertData( const TransferableDataHelper& rDataHelper,
         pImplementation = nullptr;
     }
 
+    bool bSelfDND = false;
+
     // try to get own transfer data
     if( pImplementation )
     {
         if( SD_MOD()->pTransferClip == pImplementation )
             pOwnData = SD_MOD()->pTransferClip;
         else if( SD_MOD()->pTransferDrag == pImplementation )
+        {
             pOwnData = SD_MOD()->pTransferDrag;
+            bSelfDND = true;
+        }
         else if( SD_MOD()->pTransferSelection == pImplementation )
             pOwnData = SD_MOD()->pTransferSelection;
     }
 
+    const bool bGroupUndoFromDragWithDrop = bSelfDND && mpDragSrcMarkList && IsUndoEnabled();
+    if (bGroupUndoFromDragWithDrop)
+    {
+        OUString aStr(SdResId(STR_UNDO_DRAGDROP));
+        BegUndo(aStr + " " + mpDragSrcMarkList->GetMarkDescription());
+    }
+
     // ImageMap?
     if( !pOwnData && aDataHelper.HasFormat( SotClipboardFormatId::SVIM ) )
     {
@@ -1523,6 +1535,15 @@ bool View::InsertData( const TransferableDataHelper& rDataHelper,
     mbIsDropAllowed = true;
     rDnDAction = mnAction;
 
+    if (bGroupUndoFromDragWithDrop)
+    {
+        // this is called eventually by the underlying toolkit anyway in the case of a self-dnd
+        // but we call it early in this case to group its undo actions into this open dnd undo group
+        // and rely on that repeated calls to View::DragFinished are safe to do
+        DragFinished(mnAction);
+        EndUndo();
+    }
+
     return bReturn;
 }