[prev in list] [next in list] [prev in thread] [next in thread] 

List:       koffice-devel
Subject:    Re: Patch to fix multiple undo command on pasting page in kpresenter
From:       Pierre Stirnweiss <pstirnweiss () googlemail ! com>
Date:       2009-10-27 20:24:51
Message-ID: 200910272124.51320.pierre.stirnweiss_gen () gadz ! org
[Download RAW message or body]

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@kde.org
https://mail.kde.org/mailman/listinfo/koffice-devel

[prev in list] [next in list] [prev in thread] [next in thread]