Patch to fix multiple undo command on pasting page in kpresenter

Hello,

today Thomas and I had the following discussion about if the patch should go
into koffice 2.1 or not. We really like to know what the other think about it.

Thorsten

[20:57:58] <Zagge> TZander: on hoe to reproduce the bug
[20:58:11] <Zagge> create a presentation with a testshape that has some
content
[20:58:21] <TZander> Zagge: yes, did that :)
[20:58:30] <Zagge> then save it and close and repopen it and then copy the
slide
[20:58:33] <Zagge> then paste it
[20:58:36] atomopawn [n=robert@...] has quit
IRC: Read error: 110 (Connection timed out)
[20:58:40] <Zagge> more then one undo command is created
[21:00:16] TZander used import presentation before
[21:00:16] TZander tries
[21:00:17] <TZander> Zagge: only one undo is created
[21:00:36] <boud> I get about ten undoes when I try itOct 26 21:00:55
<TZander> huh?
Oct 26 21:00:58 <Zagge> TZander: is may patch active
Oct 26 21:01:10 <Zagge> TZander: the same I get with out my patch
Oct 26 21:01:40 <boud> TZander: I create a presentation, add a text shape,
copy the page, press ctrl-v: I see undo copy page, I undo, then I have to
press ctrl-z ten times before I get to the undo add shape stage.
Oct 26 21:02:21 <TZander> I did exactly the same.
Oct 26 21:02:30 <TZander> well, used the menu to do 'ctrl-v'.
Oct 26 21:02:42 <boud> that might make a difference, haven't tested it that
way
Oct 26 21:02:57 <TZander> its the same action, so, no that can't be different.
Oct 26 21:03:27 * TZander plays more.
Oct 26 21:03:34 <boud> could it be a difference in the qt version? I'm still
using 4.5
Oct 26 21:04:27 <TZander> ooh, now I see it.  press 1 undo and then everything
is properly undone, but somehow there is more on the stack.
Oct 26 21:04:27 --> blueck (n=bb@...) has joined
#koffice
Oct 26 21:04:50 <boud> about ten anonymous undo commands, as far as I can see
Oct 26 21:04:57 <Zagge> TZander: and exactly that is fixed by my patch
Oct 26 21:05:04 <-- LukasT has left #koffice ()
Oct 26 21:05:11 <TZander> Zagge: did you try my suggestion?
Oct 26 21:05:45 <Zagge> why should I? my fix fixes the cause of the problem
and does not work around it
Oct 26 21:06:03 --> hannaskott (n=hanna@c-
e948e055.471-1-64736c10.cust.bredbandsbolaget.se) has joined #koffice
Oct 26 21:06:04 <TZander> ok, I thought I explained why; let me do so
Oct 26 21:06:29 <TZander> the KoTextEditor is an unfinished class; only
integrated because it would require us to rewrite all the stuff that pierre
did otherwise.
Oct 26 21:06:57 <TZander> its got limitations; one of them is the 'too-many-
undo commands' issue. Which is a design issue.  There are various others
isseus.
Oct 26 21:07:36 <TZander> Fixing a class thats unfinished is no fun;  
regressions are expected and more design problems show up.
Oct 26 21:07:40 <blueck> Hi koffice devels. What are the minimal requirements
to build KOffice 2.1? Qt: 4.5.2 + kde branch 4.3?
Oct 26 21:07:51 <Zagge> blueck: yes
Oct 26 21:08:12 <Zagge> blueck: it also works with kde 4.2
Oct 26 21:08:27 <TZander> so, if you provided this patch 2 months ago I'd be
ok with it,  now I really really don't want to touch this code if at all
possible.
Oct 26 21:08:33 <TZander> for 2.1, that is
Oct 26 21:08:46 <Zagge> TZander: I discussed this fix with Pierre and he has
thinks the same as me
Oct 26 21:08:53 <TZander> so, i'm ok with committing this to trunk
Oct 26 21:08:57 <TZander> Zagge: yes, I know
Oct 26 21:09:03 <Zagge> TZander: I asked you 2 month ago to fix that bug and
you did not do it
Oct 26 21:09:18 <Zagge> now I came up with a fix and you propose not to commit
it?
Oct 26 21:09:43 <blueck> Zagge: thanks, but than I have a serious problem with
koffice 2.1 build from sources yesterday, every application except kword
crashes at start :-(
Oct 26 21:09:55 <TZander> Not on the branch, if you came with that patch 2
moths ago it would have had time to stabilize; we just don't have that luxery
Oct 26 21:09:55 <Zagge> I explained you it is not possiblt to use the
beginMacro as it does not allow you to modify the text of the command
Oct 26 21:10:19 <Zagge> TZander: what is there that is problematic?
Oct 26 21:10:33 <TZander> the class is, its unfinished.
Oct 26 21:10:39 <TZander> KoTextEditor, that is
Oct 26 21:10:42 <Zagge> but not my patch
Oct 26 21:10:54 <Zagge> the patch works perfectly well
Oct 26 21:10:55 <TZander> it touches that class, so, yes, it is.
Oct 26 21:10:56 <boud> it look like a pretty simple patch for a bug that's
very much right in the face for a user.
Oct 26 21:11:31 <Zagge> then this class should have not be commited at all
during feature freeze
Oct 26 21:11:56 <TZander> it was a hard choice, for sure.
Oct 26 21:11:58 <boud> and I don't see why the class being unfinished, and
presumably staying in that state in 2.1 should prevent a fix from going.
Oct 26 21:12:28 <Zagge> and Pierre thinks that is the right solution too
Oct 26 21:12:36 <TZander> I spent a lot of time fixing up any loose ends as
the choice of not committing meant pierre would have to redo weeks of work
Oct 26 21:13:03 <Zagge> and now one more loose end is fixed
Oct 26 21:13:50 <TZander> I'm sorry, I explained why its not going in.
Oct 26 21:13:59 <boud> TZander: sorry, that's unreasonable
Oct 26 21:14:06 <boud> TZander: you cannot decide that
Oct 26 21:14:17 <TZander> I just did
Oct 26 21:14:23 <boud> TZander: consensus, remember (which is something
different from "tzander has to agree")
Oct 26 21:14:25 <Zagge> TZander: that was not a very good explanation
Oct 26 21:14:40 <boud> TZander: you simply do not have the right to decide
that.
Oct 26 21:15:07 <TZander> Zagge: if my explaining why the class I've been
debugging for months is not ready for more pathes in 2.1, then I don't know
what is.
Oct 26 21:15:27 <-- hannaskott has quit ("Leaving" (null))
Oct 26 21:15:41 <boud> TZander: pity, but then you do not have enough
arguments to block a bug fix, in my eyes.
Oct 26 21:16:07 <TZander> boud: thanks for the trust; you are really in a
fighting mood lately :(
Oct 26 21:16:12 <Zagge> it fixed a very very nasty bug you were not even
looking into fixeing even if I said there is a problem
Oct 26 21:16:28 <boud> TZander: I simply don't accept your autocratic style of
"cooperation" anymore.
Oct 26 21:16:49 <Zagge> TZander: maybe you should step back and look at that
from the outside. Maybe it is you who is on the fighting
Oct 26 21:17:47 <TZander> hmm, if I step back I see two simple solutions; one
that has a slight problem with a string and one that the author of the class
feels very uneasy with to not cause regressions.  If I step back I'd agree
with myself.
Oct 26 21:17:58 <boud> wow, surprise!
Oct 26 21:18:21 <TZander> boud: just stop being so sarcastic.  Its unhelpful
Oct 26 21:19:19 <TZander> Really, can we just work together?
Oct 26 21:19:34 <Zagge> TZander: no you make it impossible
Oct 26 21:19:53 <TZander> why?  You seem to not even want to try alternatives.
Oct 26 21:20:30 <boud> TZander: I think you haven't shown an open mind
yourself.
Oct 26 21:20:35 <Zagge> TZander: I stop here it is not possible to discuss
with you
Oct 26 21:20:41 <TZander> if you want consensus; try more than one solution
and choose the one that works best for all.
Oct 26 21:21:02 <boud> I'm going down as well.
Oct 26 21:21:06 <boud> This is useless
Oct 26 21:21:30 <TZander> Zagge: I think if you truthfully can say this, maybe
you can try the solution I suggested.  Otherwise you are just as bad.
Oct 26 21:21:46 <TZander> you say no, I say no. Whats the difference
Oct 26 21:22:28 <TZander> are you guys really being reasonable by saying that
regressions are fine but a string issue is not?
Oct 26 21:22:43 <Zagge> what regressions?
Oct 26 21:23:10 <Zagge> show me one regression introduces by the patch then we
can discuss again
Oct 26 21:23:37 <TZander> you know how silly that sounds, right?
Oct 26 21:24:36 <TZander> As I mentioned; if this patch went in 2 months ago
we would have had plenty of time to find any regressions.
Oct 26 21:24:53 <TZander> now, with just mere weeks till the final release, I
am just not going to risk that.
Oct 26 21:26:03 <TZander> Zagge: so, can you try the solution I suggested?
Please?  Really, you can't blame me for refusing a patch when you do exactly
the same.
Oct 26 21:26:47 <Zagge> it does not I don't get different texts
Oct 26 21:27:09 <TZander> sorry, I don't follow?
Oct 26 21:27:52 <TZander> if you have a patch and there is a problem we can
compare problems and see what works best.
Oct 26 21:30:05 <-- ervin has quit (Remote closed the connection)
Oct 26 21:30:59 <Zagge> I cannot create different names for the commands which
is used by pating
Oct 26 21:31:01 <Zagge> pasting
Oct 26 21:31:38 <TZander> can you create a patch so we can look at the issue?
Oct 26 21:31:48 <Zagge> TZander: I'l write a mail to the list containing this
discusson and we can let the others decide how about that
Oct 26 21:32:22 <TZander> are you honestly going to just push your opinion and
at the same time blame me for doing that?
Oct 26 21:32:41 <TZander> and even refuse to create a patch for an alternative
solutin?
Oct 26 21:33:03 <Zagge> TZander: I'm not going to push it I will ask
Oct 26 21:33:21 <TZander> then at least create the patch with the alternative
solution
Oct 26 21:33:47 <Zagge> TZander: feel free to do so and post it as reply

[no_commands_on_load.diff]

---

Index: libs/kotext/KoTextEditor_p.h
===================================================================
--- libs/kotext/KoTextEditor_p.h (revision 1040051)
+++ libs/kotext/KoTextEditor_p.h (working copy)
@@ -64,6 +64,7 @@ public:
     QString commandTitle;
     KoText::Direction direction;
     bool isBidiDocument;
+    bool addCommand;
 
     State editorState;
 
Index: libs/kotext/KoTextEditor.h
===================================================================
--- libs/kotext/KoTextEditor.h (revision 1040051)
+++ libs/kotext/KoTextEditor.h (working copy)
@@ -72,6 +72,8 @@ QTextCursor ( const QTextCursor & cursor
 
     bool operator>=(const QTextCursor &other) const;
 
+    void setAddCommand(bool add);
+
 public slots:
     ///This should be used only as read-only cursor or within a QUndoCommand sub-class which will be added to the textEditor with addCommand. For examples of proper implementation of such undoCommands, see the TextShape commands.
     QTextCursor* cursor();
Index: libs/kotext/KoTextEditor.cpp
===================================================================
--- libs/kotext/KoTextEditor.cpp (revision 1040051)
+++ libs/kotext/KoTextEditor.cpp (working copy)
@@ -87,7 +87,8 @@ KoTextEditor::Private::Private(KoTextEdi
     : q(qq),
     document (document),
     headCommand(0),
-    isBidiDocument(false)
+    isBidiDocument(false),
+    addCommand(true)
 {
     caret = QTextCursor(document);
     editorState = NoOp;
@@ -120,6 +121,10 @@ void KoTextEditor::Private::documentComm
 
         QPointer<QTextDocument> m_document;
     };
+    if (!addCommand) {
+        return;
+    }
+
     //kDebug() << "editor state: " << editorState << " headcommand: " << headCommand;
     if (!headCommand || editorState == NoOp) {
         headCommand = new QUndoCommand(commandTitle);
@@ -307,6 +312,11 @@ void KoTextEditor::updateDefaultTextDire
     d->direction = direction;
 }
 
+void KoTextEditor::setAddCommand(bool add)
+{
+    d->addCommand = add;
+}
+
 QTextCursor* KoTextEditor::cursor()
 {
     return &(d->caret);
Index: plugins/textshape/TextShape.cpp
===================================================================
--- plugins/textshape/TextShape.cpp (revision 1040051)
+++ plugins/textshape/TextShape.cpp (working copy)
@@ -329,6 +329,7 @@ void TextShape::saveOdf(KoShapeSavingCon
 
 bool TextShape::loadOdf(const KoXmlElement &element, KoShapeLoadingContext &context)
 {
+    KoTextDocument(m_textShapeData->document()).textEditor()->setAddCommand(false);
     loadOdfAttributes(element, context, OdfAllAttributes);
 
     // load the (text) style of the frame
@@ -361,7 +362,9 @@ bool TextShape::loadOdf(const KoXmlEleme
         paragraphStyle.applyStyle(block, false);
     }
 
-    return loadOdfFrame(element, context);
+    bool retval = loadOdfFrame(element, context);
+    KoTextDocument(m_textShapeData->document()).textEditor()->setAddCommand(true);
+    return retval;
 }
 
 bool TextShape::loadOdfFrameElement(const KoXmlElement &element, KoShapeLoadingContext &context)
Index: kpresenter/part/KPrPlaceholderTextStrategy.cpp
===================================================================
--- kpresenter/part/KPrPlaceholderTextStrategy.cpp (revision 1040051)
+++ kpresenter/part/KPrPlaceholderTextStrategy.cpp (working copy)
@@ -37,6 +37,7 @@
 #include <KoShapeSavingContext.h>
 #include <KoTextShapeData.h>
 #include <KoTextDocument.h>
+#include <KoTextEditor.h>
 #include <KoTextDocumentLayout.h>
 #include <KoStyleManager.h>
 #include <KoXmlReader.h>
@@ -142,7 +143,9 @@ bool KPrPlaceholderTextStrategy::loadOdf
         Q_ASSERT( factory );
         m_textShape = factory->createDefaultShapeAndInit( context.dataCenterMap() );
 
-        KoTextShapeData * shapeData = qobject_cast<KoTextShapeData*>(  m_textShape->userData() );
+        KoTextShapeData * shapeData = qobject_cast<KoTextShapeData*>( m_textShape->userData() );
+        KoTextDocument( shapeData->document() ).textEditor()->setAddCommand( false );
+
         QTextDocument * document = shapeData->document();
         QTextCursor cursor( document );
         QTextBlock block = cursor.block();
@@ -150,6 +153,7 @@ bool KPrPlaceholderTextStrategy::loadOdf
         cursor.insertText( text() );
         shapeData->foul();
         m_paragraphStyle = paragraphStyle.clone();
+        KoTextDocument( shapeData->document() ).textEditor()->setAddCommand( true );
     }
 
     styleStack.restore();

---

_______________________________________________
koffice-devel mailing list
koffice-devel@...
https://mail.kde.org/mailman/listinfo/koffice-devel

On Monday 26. October 2009 21.43.48 Thorsten Zachmann wrote:
> today Thomas and I had the following discussion about if the patch should
> go into koffice 2.1 or not. We really like to know what the other think
> about it.

As suggested by Thorsten I made a patch that solves the user problem using
beginMacro/endMacro. I attached the patch and I've tried it. Seems to do the
job.
This also seems to fix a bug in KPresenter that loading a document creates
an undo command called "Paste Slide", which is a bit confusing.
I have not found any bugs in behavior, Thorsten, do you see any?

I made sure that all strings are reused, so this can go to 2.1 if nobody
objects.

Additionally, I used the patch from Thorsten and based a fix on it. I
committed it to trunk.
--
Thomas Zander

[0001-Make-undo-work-better-for-inserting-a-doc-or-paste.patch]

---

From c43cde4f7f62e7fd0d7eaf14a98702a4f58a33dd Mon Sep 17 00:00:00 2001
From: Thomas Zander <zander@...>
Date: Tue, 27 Oct 2009 00:15:08 +0100
Subject: [PATCH] Make undo work better for inserting a doc or paste

Use beginMacro / endMacro to make sure that inserting a document
or pasting will be at most one undo command no matter if any
shape plugins creates undo commands on the stack.
---
 libs/kopageapp/KoPAView.cpp |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/libs/kopageapp/KoPAView.cpp b/libs/kopageapp/KoPAView.cpp
index fe1f9ec..2d89ab3 100644
--- a/libs/kopageapp/KoPAView.cpp
+++ b/libs/kopageapp/KoPAView.cpp
@@ -41,6 +41,7 @@
 #include <KoToolBoxFactory.h>
 #include <KoShapeController.h>
 #include <KoShapeManager.h>
+#include <KoUndoStack.h>
 #include <KoZoomAction.h>
 #include <KoZoomController.h>
 #include <KoInlineTextObjectManager.h>
@@ -423,9 +424,11 @@ void KoPAView::importDocument()
     dialog->setMode( KFile::File );
     if ( d->doc->pageType() == KoPageApp::Slide ) {
         dialog->setCaption(i18n("Import Slideshow"));
+        koDocument()->beginMacro(i18n("Import Slideshow"));
     }
     else {
         dialog->setCaption(i18n("Import Document"));
+        koDocument()->beginMacro(i18n("Import Document"));
     }
 
     // TODO make it possible to select also other supported types (then the default format) here.
@@ -439,6 +442,7 @@ void KoPAView::importDocument()
                                               KoDocument::readExtraNativeMimeTypes() );
 #endif
 
+    bool success = true;
     dialog->setMimeFilter( mimeFilter );
     if (dialog->exec() == QDialog::Accepted) {
         KUrl url(dialog->selectedUrl());
@@ -455,13 +459,18 @@ void KoPAView::importDocument()
             KoPAPastePage paste( d->doc,d->activePage );
             if ( ! paste.paste( d->doc->documentType(), &data ) ) {
                 KMessageBox::error(0, i18n("Could not import\n%1", url.pathOrUrl()));
+                success = false;
             }
         }
         else {
             KMessageBox::error(0, i18n("Could not import\n%1", url.pathOrUrl()));
+            success = false;
         }
     }
     delete dialog;
+    koDocument()->endMacro();
+    if (!success)
+        koDocument()->undoStack()->undo(); // remove our macro command.
 }
 
 void KoPAView::viewSnapToGrid(bool snap)
@@ -485,18 +494,24 @@ void KoPAView::editPaste()
 
 void KoPAView::pagePaste()
 {
+    koDocument()->beginMacro(i18n("Paste Page"));
     const QMimeData * data = QApplication::clipboard()->mimeData();
 
     KoOdf::DocumentType documentTypes[] = { KoOdf::Graphics, KoOdf::Presentation };
 
+    bool pastedOne = false;
     for ( unsigned int i = 0; i < sizeof( documentTypes ) / sizeof( KoOdf::DocumentType ); ++i )
     {
         if ( data->hasFormat( KoOdf::mimeType( documentTypes[i] ) ) ) {
+            pastedOne = true;
             KoPAPastePage paste( d->doc, d->activePage );
             paste.paste( documentTypes[i], data );
             break;
         }
     }
+    koDocument()->endMacro();
+    if (!pastedOne)
+        koDocument()->undoStack()->undo(); // remove our macro command.
 }
 
 void KoPAView::editDeleteSelection()
--
1.6.0.4

---

_______________________________________________
koffice-devel mailing list
koffice-devel@...
https://mail.kde.org/mailman/listinfo/koffice-devel

Hello,

>
> Additionally, I used the patch from Thorsten and based a fix on it. I
> committed it to trunk.
>

can you please revert that patch. It has nothing to do with what I proposed.
It does not fix the problem. It still creates an undo command for every loaded
text shape and this is wrong.  

My patch makes sure no undo command is created during loading of a shape.

Can we please go fix the bug and not try to work around it?

Thorsten
_______________________________________________
koffice-devel mailing list
koffice-devel@...
https://mail.kde.org/mailman/listinfo/koffice-devel

On Tue October 27 2009, Thomas Zander wrote:

> On Monday 26. October 2009 21.43.48 Thorsten Zachmann wrote:
> > today Thomas and I had the following discussion about if the patch should
> > go into koffice 2.1 or not. We really like to know what the other think
> > about it.
>
> As suggested by Thorsten I made a patch that solves the user problem using
> beginMacro/endMacro. I attached the patch and I've tried it. Seems to do
>  the job.
> This also seems to fix a bug in KPresenter that loading a document creates
> an undo command called "Paste Slide", which is a bit confusing.
> I have not found any bugs in behavior, Thorsten, do you see any?

No command called Paste Slide is generated on loading a document for me. Can
you paste a backtrace that shows who creates the command.

As said during the irc discussion with this method it is not possible to have
different command names for for pasting one or pasting multiple pages.

Note there are different command names generated if pasted in kivio or pasted
in kpresenter which you patch does not do. This could be done much simpler if
done inside KoPastePage.

Thorsten

_______________________________________________
koffice-devel mailing list
koffice-devel@...
https://mail.kde.org/mailman/listinfo/koffice-devel

On Tuesday 27. October 2009 06.49.48 Thorsten Zachmann wrote:
> > This also seems to fix a bug in KPresenter that loading a document
> > creates an undo command called "Paste Slide", which is a bit confusing.
> > I have not found any bugs in behavior, Thorsten, do you see any?
[]
>
> As said during the irc discussion with this method it is not possible to
> have different command names for for pasting one or pasting multiple
> pages.

How do I reproduce that problem? I don't see how the user can get more
than one page on the clipboard to paste.

>Note there are different command names generated if pasted in kivio or
> pasted in kpresenter which you patch does not do. This could be done
much
> simpler if done inside KoPastePage.

Sure, but we were talking about a fix for 2.1 and kivio is not part of 2.1

So, is this a good patch for 2.1?
--
Thomas Zander
_______________________________________________
koffice-devel mailing list
koffice-devel@...
https://mail.kde.org/mailman/listinfo/koffice-devel

Le Tuesday 27 October 2009 08:18:59, Thomas Zander a écrit :

> On Tuesday 27. October 2009 06.49.48 Thorsten Zachmann wrote:
> > > This also seems to fix a bug in KPresenter that loading a document
> > > creates an undo command called "Paste Slide", which is a bit confusing.
> > > I have not found any bugs in behavior, Thorsten, do you see any?
>
> []
>
> > As said during the irc discussion with this method it is not possible to
> > have different command names for for pasting one or pasting multiple
> > pages.
>
> How do I reproduce that problem? I don't see how the user can get more
> than one page on the clipboard to paste.
>
> >Note there are different command names generated if pasted in kivio or
> > pasted in kpresenter which you patch does not do. This could be done
>
> much
>
> > simpler if done inside KoPastePage.
>
> Sure, but we were talking about a fix for 2.1 and kivio is not part of 2.1
>
> So, is this a good patch for 2.1?
>

Just to add my 2 cents to the discussion, and also some background info as to
the decisions leading to the current design:

- the textShape is using QTextDocuments for the editing and displaying of
text.There is one QTextDocument per TextShape but several shapes can work on
the same QTextDocument.
- editing of the QTextDocument is done via the QTextCursor. Editing actions
will generate an undo command within the QTextDocument. Some actions are
"grouped" with the previous one (and no undo command are created). These undo
command are not accessible to the outside. When an undo command is created
QTextDocument will emit a signal. These undo commands are pushed on a
"QTextDocument" internal undoStack.
- our applications have an "application level" undoStack where we can push
QUndoCommand derived commands. They have nothing to do with the above
mentioned QTextDocument undo commands.
- QTextDocument commands are undone/redone with QTextDocument::undo (::redo)

It is therefore of the utmost importance that sync is kept between the two
stacks, specifically, no QTextDocument undo command should be left without a
corresponding application level undo command.


Good now, how did we do this (prior to KoTextEditor):
The textTool was responsible for this. When the textTool would be active on a
shape, it would listen to the signal of the QTextDocument of that shape and
create "application level" undo commands on the stack. These "application
level" undo command would call QTextDocument::undo or redo in their own undo
or redo methods.
And there lied a big hole in the design. The textTool could listen (when
active on a shape) only to one QTextDocument at a time. Should anything modify
a QTextDocument which the tool would not listen to, we would loose sync
between the application level undo stack and the QTextDocuments undo stacks.
This anything could be a script, another tool, a plug-in,...

In order to solve this problem the KoTextEditor was created. There is one
KoTextEditor per QTextDocument. The KoTextEditor is used to modify the
QTextDocument (instead of the QTextCursor), but more importantly, the
KoTextEditor is in charge of listening to the QTextDocument undoCreated signal
and keeps the application level and the QTextDocument level undoStacks in
sync. Since there is one KoTextEditor per QTextDocument we ensure that no
QTextDocument undo command is left without a corresponding application level
undo command.

So where do Thorsten's problem comes from?
The pastePage action in KoPageApp is using the shape loading mechanism.
The loading of a textShape creates some (a lot of) QTextDocument undo
commands. With the previous design we didn't have this problem because of the
hole. The textTool was not active on the new QTextDocument, therefore nothing
was created on the application level undoStack.
In this particular case it didn't matter that the two stacks were not in sync,
because there was no prior QTextDocument undoCommand to access and the
application level undoStack being empty, we would never be able to reach those
created QTextDocument undo commands.

With our present design, the KoTextEditor is created together with the
QTextDocument and listens to the signal straight away, as it should and when
loading modifies the QTextDocument, corresponding QUndoCommands are pushed on
the application level undoStack.
This creates a problem at initial loading of a KOffice document. This was
solved by clearing the application level undoStack at the end of KoDocument
loading mechanism.
However, this fix does not solve Thorsten problem.

The fix for Thorsten problem he and I both thought about (in parallel) was to
"disable" the creation of application level undo commands at the beginning of
the textShape::load and re-enable it at the end of it. This equates to
recreate the hole we had in the previous design. I think it should be OK
because the modifications are done on the newly created QTextDocument. The
fact that we were not hit by that hole in KPresenter with the previous design
makes me confident that we are not loosing sync on an already in-use
undoStack. However, I am not 100% sure that this solution is really safe in
all use cases.

The solution Thomas has proposed is that all the created "application level"
undo commands are treated by the application level undoStack as ONE undo
command. This is achieved by the beginMacro/endMacro. I can't think why
Thorsten is experiencing one undocommand per shape from what I have seen in
the patch (but I haven't personally tried). However, this solution doesn't
allow to have different names (singular/plural, "paste slide" "paste page"
depending on the type of page) to the command in KPresenter.

The handling of that particular case has however to my mind nothing to do with
KoTextEditor being immature or not finished or what not. The rule is that no
QTextDocument undo command should be ignored. The special case of
loading/initializing needs to be handled, which means that eventually not
creating an application level undoCommand has to be a conscious decision, not
a result of a hole in the design.

My opinion on this is that for long term a solution blocking the creation of
application level undo command at loading/initializing of a textShape is the
desirable solution. So Thorsten patch should definitely be applied on trunk.
For the branch, I tend to think it wouldn't create problems but the cautious
route would be to apply Thomas' solution at least for 2.1.0. I am sure that we
can reach creation of only one command for the whole paste, and my opinion is
that for KPresenter an action called "Paste Slides" wouldn't be too
detrimental to the user experience (even if just one slide is pasted).

I hope I haven't bored anyone to death.

Pierre


_______________________________________________
koffice-devel mailing list
koffice-devel@...
https://mail.kde.org/mailman/listinfo/koffice-devel

Hello,

On Tue October 27 2009, Pierre Stirnweiss wrote:
> I hope I haven't bored anyone to death.

thanks for you very detailed explanation.

One of the concerns you raised in your mail is that the the stacks are not in
sync.
 
> The pastePage action in KoPageApp is using the shape loading mechanism.
> The loading of a textShape creates some (a lot of) QTextDocument undo
> commands. With the previous design we didn't have this problem because of
>  the  hole. The textTool was not active on the new QTextDocument, therefore
>  nothing was created on the application level undoStack.
> In this particular case it didn't matter that the two stacks were not in
>  sync,  because there was no prior QTextDocument undoCommand to access and
>  the application level undoStack being empty, we would never be able to
>  reach those created QTextDocument undo commands.

I read some more API docs and came up with the attached patch that keeps the
undo stack of the QTextDocment and the application in sync by not creating
commands while the text shape is loaded. By this no more commands are created
during loading which fixes the bug.

If there are no objections I will apply the patch to trunk. For 2.1 I will
come up with a different patch based on the begin/endMacro for kopageapp. This
works but does not allow to have different command names which is why I would
have preferred to fix the bug instead of working around it.

Thorsten

[no_commands_on_load2.diff]

---

Index: kpresenter/part/KPrPlaceholderTextStrategy.cpp
===================================================================
--- kpresenter/part/KPrPlaceholderTextStrategy.cpp (revision 1040921)
+++ kpresenter/part/KPrPlaceholderTextStrategy.cpp (working copy)
@@ -144,7 +144,7 @@ bool KPrPlaceholderTextStrategy::loadOdf
         m_textShape = factory->createDefaultShapeAndInit( context.dataCenterMap() );
 
         KoTextShapeData * shapeData = qobject_cast<KoTextShapeData*>(  m_textShape->userData() );
-        KoTextDocument( shapeData->document() ).textEditor()->beginEditBlock();
+        shapeData->document()->setUndoRedoEnabled(false);
 
         QTextDocument * document = shapeData->document();
         QTextCursor cursor( document );
@@ -153,7 +153,7 @@ bool KPrPlaceholderTextStrategy::loadOdf
         cursor.insertText( text() );
         shapeData->foul();
         m_paragraphStyle = paragraphStyle.clone();
-        KoTextDocument( shapeData->document() ).textEditor()->endEditBlock();
+        shapeData->document()->setUndoRedoEnabled(true);
     }
 
     styleStack.restore();
Index: plugins/textshape/TextShape.cpp
===================================================================
--- plugins/textshape/TextShape.cpp (revision 1040921)
+++ plugins/textshape/TextShape.cpp (working copy)
@@ -329,7 +329,7 @@ void TextShape::saveOdf(KoShapeSavingCon
 
 bool TextShape::loadOdf(const KoXmlElement &element, KoShapeLoadingContext &context)
 {
-    KoTextDocument(m_textShapeData->document()).textEditor()->beginEditBlock();
+    m_textShapeData->document()->setUndoRedoEnabled(false);
     loadOdfAttributes(element, context, OdfAllAttributes);
 
     // load the (text) style of the frame
@@ -363,7 +363,7 @@ bool TextShape::loadOdf(const KoXmlEleme
     }
 
     bool answer = loadOdfFrame(element, context);
-    KoTextDocument(m_textShapeData->document()).textEditor()->endEditBlock();
+    m_textShapeData->document()->setUndoRedoEnabled(true);
     return answer;
 }
 

---

_______________________________________________
koffice-devel mailing list
koffice-devel@...
https://mail.kde.org/mailman/listinfo/koffice-devel

If there are no objections I will apply the patch to trunk. For 2.1 I will
come up with a different patch based on the begin/endMacro for kopageapp. This
works but does not allow to have different command names which is why I would
have preferred to fix the bug instead of working around it.

As far as I can tell, this is not possible because when setUndoRedoEnable(false) is called, it also clears up the QTextDocument undoStack. Which means that all the undoCommands on the application stack, prior to your paste command won't have their corresponding QTextDocument undoStack commands anymore. And I don't think that clearing the undo history on the application each time a paste command is used is really desirable.

As I said, I think that "recreating" the hole in the loading of the shape shouldn't be an issue (because I think it would have been an issue with the previous design too).
So I think the best course of action would be to apply your first solution to trunk and give it some good testing. To be absolutly honest, I think the absolute best would be to find a way to link all those commands to the main PasteCommand one (with beginMacro/endMacro) and still be able to decide for the name first.

Meanwhile, for 2.1, in order to play safe, the other solution is better. If recreating the hole is the only possibility and proves harmless, we could always backport it for 2.1.1.

Pierre

_______________________________________________
koffice-devel mailing list
koffice-devel@...
https://mail.kde.org/mailman/listinfo/koffice-devel

On Wed October 28 2009, Pierre Stirnweiss wrote:

> > If there are no objections I will apply the patch to trunk. For 2.1 I
> > will come up with a different patch based on the begin/endMacro for
> > kopageapp. This
> > works but does not allow to have different command names which is why I
> > would
> > have preferred to fix the bug instead of working around it.
>
> As far as I can tell, this is not possible because when
> setUndoRedoEnable(false) is called, it also clears up the QTextDocument
> undoStack. Which means that all the undoCommands on the application stack,
> prior to your paste command won't have their corresponding QTextDocument
> undoStack commands anymore. And I don't think that clearing the undo
>  history on the application each time a paste command is used is really
>  desirable.
>

That should be no problem as load on the shape is only called during loading
the shape and not while it already has some content. So there should be no
commands in the QTextDocument stack which will be undone by the application
after loading.

> Meanwhile, for 2.1, in order to play safe, the other solution is better. If
> recreating the hole is the only possibility and proves harmless, we could
> always backport it for 2.1.1.

Agreed.

Thorsten
_______________________________________________
koffice-devel mailing list
koffice-devel@...
https://mail.kde.org/mailman/listinfo/koffice-devel

On Wednesday 28. October 2009 09.18.09 Thorsten Zachmann wrote:
[]

Thanks guys for looking at all solutions and coming to a good conclusion.  I
really like that :)

The proposed solution of disabling the qtextdocument stack in loadOdf() looks
good to me. Great catch.
--
Thomas Zander
_______________________________________________
koffice-devel mailing list
koffice-devel@...
https://mail.kde.org/mailman/listinfo/koffice-devel

That should be no problem as load on the shape is only called during loading
the shape and not while it already has some content. So there should be no
commands in the QTextDocument stack which will be undone by the application
after loading.

Very true. That would clear the undoStack of the new QTextDocument. The other QTextDocuments and their corresponding KoTextEditor shouldn't be affected.
It is actually a pretty elegant solution.
 

Pierre

_______________________________________________
koffice-devel mailing list
koffice-devel@...
https://mail.kde.org/mailman/listinfo/koffice-devel

On Wed October 28 2009, Thorsten Zachmann wrote:
> If there are no objections I will apply the patch to trunk. For 2.1 I will
> come up with a different patch based on the begin/endMacro for kopageapp.
>  This  works but does not allow to have different command names which is
>  why I would have preferred to fix the bug instead of working around it.
>

Can someone please review the attached patch to not create a lot of commands
while pasting a slide. I like to apply to koffice 2.1 branch to get this fixed
before 2.1 is released.

Thorsten

[dont_create_commands.diff]

---

Index: KoPAPastePage.cpp
===================================================================
--- KoPAPastePage.cpp (revision 1042108)
+++ KoPAPastePage.cpp (working copy)
@@ -42,6 +42,12 @@ KoPAPastePage::KoPAPastePage( KoPADocume
 
 bool KoPAPastePage::process( const KoXmlElement & body, KoOdfReadStore & odfStore )
 {
+    if ( m_doc->pageType() == KoPageApp::Slide ) {
+        m_doc->beginMacro( i18n( "Paste Slides" ) );
+    }
+    else {
+        m_doc->beginMacro( i18n( "Paste Pages" ) );
+    }
     KoOdfLoadingContext loadingContext( odfStore.styles(), odfStore.store(), m_doc->componentData() );
     KoPALoadingContext paContext( loadingContext, m_doc->dataCenterMap() );
 
@@ -147,6 +153,7 @@ bool KoPAPastePage::process( const KoXml
     }
 
     m_doc->addCommand( cmd );
+    m_doc->endMacro();
 
     return true;
 }

---

_______________________________________________
koffice-devel mailing list
koffice-devel@...
https://mail.kde.org/mailman/listinfo/koffice-devel

On Sunday 01 November 2009, Thorsten Zachmann wrote:

> On Wed October 28 2009, Thorsten Zachmann wrote:
> > If there are no objections I will apply the patch to trunk. For 2.1 I
> > will come up with a different patch based on the begin/endMacro for
> > kopageapp. This  works but does not allow to have different command names
> > which is why I would have preferred to fix the bug instead of working
> > around it.
>
> Can someone please review the attached patch to not create a lot of
>  commands while pasting a slide. I like to apply to koffice 2.1 branch to
>  get this fixed before 2.1 is released.

Looks ok to me.

--
Boudewijn Rempt | http://www.valdyas.org
_______________________________________________
koffice-devel mailing list
koffice-devel@...
https://mail.kde.org/mailman/listinfo/koffice-devel