TextTool commands

Hello,

I am looking into the TextTool because I would like to work
on a track changes 
functionnality for KWord, which could then lead to
collaboration 
functionnality.

After a quick look it seems that:
- there is some inconsistency in the handling of the
different text editing 
functions (through selectionHandler for formating or
directly for text 
inserting stuff)
- the undo/redo framework is dealt with through a generic
QUndoCommand derived 
class that then forward the calls to the QDocument
undo/redo.

It would seem better to have:
- a consistent way of doing text editing operations (that
could eventually be 
exposed as actions). These actions/commands could feed the
track change 
framework (a flake tool?)
- undo/redo functions that would also feed the track change
framework and not 
loose information (it seems to cause problems for the inline
text stuff).

It seems the QDocument undo/redo is not fit for the job as
there is no way to 
have detailed information on the exact operation that has
been undone or 
redone. Furthermore, it looses information of the custom
data assossiated 
with a textblock, which seems to be an interesting way for
track change info.

So my proposal is (after having reached illumination when
looking at the 
KoShapes commands):
- to have all the text editing operations as commands
(inheriting 
QUndoCommand). These would all reside in a
shapes/text/commands directory.
- these commands save the additionnal information (like user
data,..) trough 
undo/redo operations.

I have done a quick trial for the bold operation. It work as
follow:
- the TextTool has got a slot for the bold action
- the slot creates a new TextBoldCommand and adds it
directly to the canvas
- the constructor of the command stores the text formatting
about to be 
changed with a TextFragmentBackup object
- this info is used for the undo.
This bypasses the QDocument undo/redo but allows more
flexibility.

it still is a bit rough on the edges (does not update the
action buttons,...) 
and your comments are more than welcomed.

attached is the svn diff file.

_______________________________________________
koffice-devel mailing list
koffice-develkde.org
h
ttps://mail.kde.org/mailman/listinfo/koffice-devel

On Thursday 22 March 2007 21:32, Pierre Stirnweiss wrote:
> I am looking into the TextTool because I would like to
work on a track
> changes functionnality for KWord, which could then lead
to collaboration
> functionnality.
[]
> I have done a quick trial for the bold operation. It
work as follow:
> - the TextTool has got a slot for the bold action
> - the slot creates a new TextBoldCommand and adds it
directly to the canvas
> - the constructor of the command stores the text
formatting about to be
> changed with a TextFragmentBackup object
> - this info is used for the undo.
> This bypasses the QDocument undo/redo but allows more
flexibility.

Not using the QTextDocument undo/redo is not possible; as
that would require 
us to emulate all the features of components like the
QTextCursor etc.
For example; pressing enter inserts a 'n' but that also
inherits the 
properties from the previous paragraph.
I really don't want to see code appearing in koffice to do
that.

Did you look into the kotext/KoTextEditingPlugin.h ?
Its rather new, and does not have much API docs, but the
idea is that you get 
a call every time a word is changed via the tool.

Additionally you may be interrested in the
QTextDocument::contentsChange 
signal.

Hope that helps.
-- 
Thomas Zander

_______________________________________________
koffice-devel mailing list
koffice-develkde.org
h
ttps://mail.kde.org/mailman/listinfo/koffice-devel

On Monday 26 March 2007 13:49:20 Thomas Zander wrote:

>
> > With regards to the KoTextEditingPlugin, it seems
that the notifications
> > would not happen on formating changes. The
contentsChange signal seem to
> > be emited on format change though.
>
> Yes, I think you want to use the signal.
>
After a couple of experimentations and a lot of reading the
code (both of 
TextTool,... and QTextDocument and the like), here is were I
stand for the 
track change at which point I need some feedback/help:

- The problem with the signal is that it only tells you
something happened. It 
is prety much useless when you want to know what happened
(which is paramount 
to properly track the changes with undo/redo). There is no
way (I can think 
of) to know from the signal if the change is due to an
action being 
undone/redone or to a new action (the pos and nbr of char
affected is not 
enough).


- the editing actions are of two types: the one which
generates undo/redo 
(inserting/deleting text, applying formating on a selection,
...) and the one 
which does not (changing the format of the cursor with no
selection).

- the editing actions which do not generate an undo/redo do
not need to be 
tracked as their influence on the document will only appear
after another 
action which generates an undo/redo is done (expl. changing
the format of the 
cursor (with no selection) will not influence the document
until at least 
one char is then inserted)

- the track change needs to be informed when an editing
action is applied 
(redo) and cancelled (undo). It needs to know exactly what
has been 
done/undone (string, format,...)

- the track change needs to merge its actions in sync with
the undo/redo 
stack. This is currently my strongest headache.

- changes can be accepted or rejected. These are actions
that can be 
undone/redone.
- accepting a change do not change the document content. It
just removes the 
changes from the track change and consider the changes as
original content
- on the other hand, rejecting a change changes the document
content (or 
formating). this particular change should not be tracked.


- ideally what I think would be great from Qt is the
following: any "editing" 
action on a QTextCursor would instead of being void, return
an id of the 
undo/redo command it has created or merged with (or -1 if it
was not 
an undo/redo). Signals (docChanged, undo/redo, ...) would
pass this id (or 
index). That way keeping the two in sync would be really
easy.

Would it be possible to include this in Qt (I would be happy
to do it if there 
is a chance of it being included)? Otherwise a workaround
needs to be found.

All the ones I have thought about so far mimic the QTextDoc
undo/redo 
(creating a "tracked object" for each editing
actions and merging them the 
same way as QTextDoc does).

I welcome some new line of thoughts cause I am a bit stuck
there.
What I need is:
- a way to uniquely identify changes that are made on the
doc and have a link 
to the corresponding command.
- a way to know reliably when a command has been merged and
with which command 
of the document stack
- a way to know which command has been undone/redone.

_______________________________________________
koffice-devel mailing list
koffice-develkde.org
h
ttps://mail.kde.org/mailman/listinfo/koffice-devel

On Sunday 08 April 2007 23:42, Pierre Stirnweiss wrote:
> > Yes, I think you want to use the signal.
>
> After a couple of experimentations and a lot of reading
the code (both
> of TextTool,... and QTextDocument and the like), here
is were I stand
> for the track change at which point I need some
feedback/help:
>
> - The problem with the signal is that it only tells you
something
> happened. It is prety much useless when you want to
know what happened
> (which is paramount to properly track the changes with
undo/redo).
> There is no way (I can think of) to know from the
signal if the change
> is due to an action being undone/redone or to a new
action (the pos and
> nbr of char affected is not enough).

What I'm curious about is why you are so fucussed on the
undo / redo 
actions integration?
I really don't see the connection between track changes and
commands.

Maybe you have looked at OOWriter which seems to behave like
this; but I 
have to tell you, its an entirely unsettling experience to
have each 
correction become a separate tracked change.

Why I don't see the tracking and undo commands as connected
is due to what 
I think the end result should look like.  So lets see if we
actually are 
talking about the same feature here;

Albert finished a document and sends it to Bonny, his
secretary. She will 
look over the document and fix any language, grammar and
other problems 
before the document is send to the client.  B. Changes words
like 'then' 
to 'than' and she adds some sentences and she removes some
sentences.
When B is finished she saves the document and sends it to A
for him to 
look at.  A will then check which changes have been made to
see if what 
he meant to say has not been lost.


When I type a new sentence, I tend to make some typos, which
I correct by 
pressing backspace and then I keep on typing.  In all word
processors 
that amounts to 3 undo commands.  1 for the part typed, 1
for the 
removed, 1 for the continued typing.
A has no interrest in reading how many rewriting attempts B
made in 
rewriting one of his more crummy sentences. He just wants
one remove and 
one add for such a sentence. Not 10 partiallies.

Also another issue with your idea to combine track changes
with the undo 
stack;  the stack is not endless. Command objects are
automatically 
deleted after there are, say, 30 on the stack.  I don't want
that to 
cause us to loose tracked changes ;)


So, from my perspective the track changes code would do
something like the 
following;
* it monitors all changes by listening to the signal on
QTextDocument
* it ignores all zero-length changes.
* textual changes made are recorded. If text is added, then
we can just 
copy it from the document. If text is removed we might want
to do 
a 'undo', 'fetch text', 'redo' cycle to figure out what that
was.
* Text changes are merged with already made changes so the
above example 
of 3 undo commands will really be 1 change in the tracked
changes.
* When the user presses undo, he changes text.  This is just
a tracked 
change that gets merged with the other changes. No special
treatment 
needed.

You referenced the side of Albert; when he gets the document
from Bonny 
and can either accept or reject the changes.
This has to be done without any commands being present as
the undo stack 
is not stored in a document.

So, technically;
A text tool has an action to switch on track changes.
A text tool has a variable; a ChangeTracker class. This
class is listening 
to the document signal.
When changes are made the ChangeTracker registers them and
merges changes.

Hope that helps.
-- 
Thomas Zander

_______________________________________________
koffice-devel mailing list
koffice-develkde.org
h
ttps://mail.kde.org/mailman/listinfo/koffice-devel

On Monday 09 April 2007 08:22:10 Thomas Zander wrote:
> On Sunday 08 April 2007 23:42, Pierre Stirnweiss
wrote:
> > > Yes, I think you want to use the signal.
> >
>
> What I'm curious about is why you are so fucussed on
the undo / redo
> actions integration?
> I really don't see the connection between track changes
and commands.
>
I don't want them to be integrated. I need the track change
to be aware of the 
fact that undo has been done (that one is easy) and what
exactly the editing 
action this undo was (this is were the problem lies, because
of merging). 
This is needed to cancel/remove/... the particular tracked
change 
corresponding to the editing action that created this undo
command.


> Maybe you have looked at OOWriter which seems to behave
like this; but I
> have to tell you, its an entirely unsettling experience
to have each
> correction become a separate tracked change.

Actually no, I haven't looked at how OOWriter is doing it. I
have actually 
used this function on MS Word a couple of years ago (on
Office XP). The thing 
might not be perfect on MS Word (in particular when doing
some nested 
operations and how the info was presented to the user) but
overall I found it 
was pretty intuitive as far as the tracking of the changes
is concerned.

>
> Why I don't see the tracking and undo commands as
connected is due to what
> I think the end result should look like.  So lets see
if we actually are
> talking about the same feature here;
>
> Albert finished a document and sends it to Bonny, his
secretary. She will
> look over the document and fix any language, grammar
and other problems
> before the document is send to the client.  B. Changes
words like 'then'
> to 'than' and she adds some sentences and she removes
some sentences.
> When B is finished she saves the document and sends it
to A for him to
> look at.  A will then check which changes have been
made to see if what
> he meant to say has not been lost.

Yes we are talking about the same thing there.

>
>
> When I type a new sentence, I tend to make some typos,
which I correct by
> pressing backspace and then I keep on typing.  In all
word processors
> that amounts to 3 undo commands.  1 for the part typed,
1 for the
> removed, 1 for the continued typing.

Yes, both for the undo/redo and for the tracked change, 
these should be merged. I am not advocating to integrate the
two together but 
they should be consistent in their merging behaviour.

The problem in your example is if B uses undo instead of
backspace to correct 
what was done (perhaps unlikly, but still possible, for the
typing but 
probable if it was some formating), neither the 
contentsChange signal nor the undo/redo signals are detailed
enough.

> So, from my perspective the track changes code would do
something like the
> following;
> * it monitors all changes by listening to the signal on
QTextDocument

As I said, this signal is, to my mind, just not detailed
enough:

B puts the cursor at pos 10
- deleting the char and undoing the delete will trigger
contentsChange(pos=10, 
char del=0, char added=1) for the undo
- typing a single letter will also trigger
contentsChange(pos=10, char del =0, 
char added=1)
- formating (expl bold) the 10th char will trigger the exact
same signal
- undoing this formatting or adding another formating
(italic) to it will also 
trigger this signal with the same args.

so listening to the signal on QTextDocument is not enough.
>
>
> * it ignores all zero-length changes.
yes, these are the ones I refered to as not undo/redo
actions

> * textual changes made are recorded. If text is added,
then we can just
> copy it from the document. If text is removed we might
want to do
> a 'undo', 'fetch text', 'redo' cycle to figure out what
that was.
> * Text changes are merged with already made changes so
the above example
> of 3 undo commands will really be 1 change in the
tracked changes.

this is what I was talking about when I said I would need to
mimic the merging 
of the undo/redo stack of the QTextDocument (<> from
the canvas stack). I was 
looking for a "cleaner" solution (the suggestion
for Qt notifying of a unique 
ID for each editing "commands"). It would work as
follow:

when a change is made on the QTextCursor (insert,
formatting,...) which 
creates an "undo command" internal to
QTextDocument (the "internal" 
QTextUndoCommand) a unique, persistent ID would get
generated and passed back 
to the caller (instead of returning void). If the command
was merged, the ID 
of the action it has been merged into would be passed back.
The caller could notify the ID together with the exact
change that was done to 
the the changeTracker. Undoing/redoing this internal action
would send a 
signal with this unique ID. The changeTracker could listen
to this and flag 
the corresponding change as being there or not. On saving,
the "active" 
changes would be saved using the OASIS syntax.

> * When the user presses undo, he changes text.  This is
just a tracked
> change that gets merged with the other changes. No
special treatment
> needed.
>
once again here, the documentChanges signal is not verbose
enough.

Hence my initial idea of having each undo/redo command (the
ones created on 
the KWCanvas stack) notify the trackChange of the actions.
The problem there 
is that these are created as a result of the UndoAvailable
signal. This one 
is not sent if an editing action was merged into the
previous one (typing two 
letters in a row).

> You referenced the side of Albert; when he gets the
document from Bonny
> and can either accept or reject the changes.
> This has to be done without any commands being present
as the undo stack
> is not stored in a document.
>
The accept action in itself should be a "command"
because you want Albert to 
be able to change his mind and after having accepted the
change just press 
undo (whether the KWCanvas undo/redo or a specific
undo/redo).

> So, technically;
> A text tool has an action to switch on track changes.
yes

> A text tool has a variable; a ChangeTracker class. This
class is listening
> to the document signal.
listening to the changes is where I have a problem.

> When changes are made the ChangeTracker registers them
and merges changes.
yes.

Pierre
_______________________________________________
koffice-devel mailing list
koffice-develkde.org
h
ttps://mail.kde.org/mailman/listinfo/koffice-devel

On Monday 09 April 2007 20:32, Pierre Stirnweiss wrote:
> > When I type a new sentence, I tend to make some
typos, which I
> > correct by pressing backspace and then I keep on
typing.  In all word
> > processors that amounts to 3 undo commands.  1 for
the part typed, 1
> > for the removed, 1 for the continued typing.
>
> Yes, both for the undo/redo and for the tracked
change,
> these should be merged. I am not advocating to
integrate the two
> together but they should be consistent in their merging
behaviour.

To be clear; that this creates 3 undo actions is perfectly
fine IMO. And 
should not change.

-- 
Thomas Zander

_______________________________________________
koffice-devel mailing list
koffice-develkde.org
h
ttps://mail.kde.org/mailman/listinfo/koffice-devel

On Monday 09 April 2007 21:03:29 Thomas Zander wrote:
> On Monday 09 April 2007 20:32, Pierre Stirnweiss
wrote:
> > > When I type a new sentence, I tend to make
some typos, which I
> > > correct by pressing backspace and then I keep
on typing.  In all word
> > > processors that amounts to 3 undo commands.
 1 for the part typed, 1
> > > for the removed, 1 for the continued typing.
> >
> > Yes, both for the undo/redo and for the tracked
change,
> > these should be merged. I am not advocating to
integrate the two
> > together but they should be consistent in their
merging behaviour.
>
> To be clear; that this creates 3 undo actions is
perfectly fine IMO. And
> should not change.

we agree, i was refering to all the individual letter
typings
_______________________________________________
koffice-devel mailing list
koffice-develkde.org
h
ttps://mail.kde.org/mailman/listinfo/koffice-devel

On Monday 09 April 2007 20:32, Pierre Stirnweiss wrote:
> As I said, this signal is, to my mind, just not
detailed enough:

Well, I tried to explain that it isn't. And I'm quite unsure
why you say 
it is.
So, to avoid any more timeloss I just coded a working
example that shows 
the before and after state of the text on each user action
using debug 
output.  Works great! 

All that is missing still is to create a 'database' of
changes and the 
ability to merge all the changes.

Hope that helps.
-- 
Thomas Zander

_______________________________________________
koffice-devel mailing list
koffice-develkde.org
h
ttps://mail.kde.org/mailman/listinfo/koffice-devel

On Monday 09 April 2007 21:46:04 Thomas Zander wrote:
> On Monday 09 April 2007 20:32, Pierre Stirnweiss
wrote:
> > As I said, this signal is, to my mind, just not
detailed enough:
>
> Well, I tried to explain that it isn't. And I'm quite
unsure why you say
> it is.

To my point of view there is a difference between having:
- these are the changes (explicitely) and
- changes happened there, figure out what they were.

The first one is to me more robust and more elegant.

> So, to avoid any more timeloss I just coded a working
example that shows
> the before and after state of the text on each user
action using debug
> output

For me doing a undo/redo cycle to find out what was changed
is a workaround. 
It might be better that the one I was considering but it is
by no mean exempt 
of drawbacks:
- an undo/redo cycle seem to trigger two repaint which might
be noticeable by 
the user if there is some heavy inline object.
- this cycle triggers the usual signals (contentsChange,
undo/redo,...). We 
have therefore to inform all the listeners that they should
ignore these 
signals (done with the
"m_tool->m_allowAddUndoCommand = false", and
the 
disconnect). 
This is acceptable now because we know what is listening to
these signals, but 
should we had other functionnality later that would use
them, this will turn 
out to be a headache. Or worse, should Qt change in a way
that creates some 
internal stuff which would listen to these signals and we're
in trouble.

> .  Works great!  

Actually it works great as long as you don't hit undo. Which
is what I tried 
to explain. The signal contentsChange following an undo will
not, on its own, 
allow you to know it resulted from an undo operation. After
an undo, with 
your idea, to find out what changed you need to do a
redo/undo instead of a 
undo/redo.

Just try the following: open a new doc, enable the tracker,
type a few letters 
and hit undo the result will be:
- ""
+ "" 
instead of 
- "some letters you typed"
+ ""

So, if you use the workaround listening to the
contentsChange signal, you need 
to notify the tracker somehow that it should do a redo/undo
instead of the 
undo/redo (which should also take into account the case of
one undo triggers 
several changes (so called grouped actions, like replacing
text...)


So since I don't want to waste anymore of your time and
don't want to 
jeopardize the existing text shape, is it OK for me to
create a alttext shape 
which would be a place I could experiment. Its compilation
would depend on a 
cmake variable, disabled by default?

Thanks for the time you already spent on this,

Pierre
_______________________________________________
koffice-devel mailing list
koffice-develkde.org
h
ttps://mail.kde.org/mailman/listinfo/koffice-devel

On Wednesday 11 April 2007 21:16, Pierre Stirnweiss wrote:
> On Monday 09 April 2007 21:46:04 Thomas Zander wrote:
> > On Monday 09 April 2007 20:32, Pierre Stirnweiss
wrote:
> > > As I said, this signal is, to my mind, just
not detailed enough:
> >
> > Well, I tried to explain that it isn't. And I'm
quite unsure why you
> > say it is.
>
> To my point of view there is a difference between
having:
> - these are the changes (explicitely) and
> - changes happened there, figure out what they were.
>
> The first one is to me more robust and more elegant.

More robust, true.
more elegant? Well, thats only true if you don't need to
implement loads 
of actions and duplicate (and keep uptodate) functionality
from Qt.
Then the elegant very quickly evaporates.
So, I'm sure you see problems with the approach I described
here, and I'm 
certainly open to other suggestions.  But the suggestion you
made earlier 
just does not scale and is far from what I call elegant.

> > So, to avoid any more timeloss I just coded a
working example that
> > shows the before and after state of the text on
each user action
> > using debug output
>
> For me doing a undo/redo cycle to find out what was
changed is a
> workaround. It might be better that the one I was
considering but it is
> by no mean exempt of drawbacks:
> - an undo/redo cycle seem to trigger two repaint which
might be
> noticeable by the user if there is some heavy inline
object.

I"m not sure how you determined there are multiple
repaints. It would 
surprise me as the design I made for relayout allows this
kind of thing 
to happen.  I don't see multiple repaints here.

> - this cycle triggers the usual signals
(contentsChange,
> undo/redo,...). We have therefore to inform all the
listeners that they
> should ignore these signals (done with the
> "m_tool->m_allowAddUndoCommand = false",
and the disconnect).

Not 'listeners' but one listener. The definition of a text
tool is that it 
is code that handles input.  And there is only one tool
active at any 
time. Again, definition.

In short; I'd hate to design for a usecase that I find
unlikely to happen. 
And surely to be unwanted currently.

> > .  Works great! 
>
> Actually it works great as long as you don't hit undo.


Good catch!
Not a hard problem to work around. I can take a look if you
want.


Look, I understand you want an elegant solution that works
great.  We all 
do. You made some proposals based on commands that I have
severe doubts 
about. For a lot of reasons.  I don't think its worth
researching it 
further, but you are always free to do as you want. Show me
code that 
works is a great way to proof me wrong ;)
In absence of a truly great solution I think the one I coded
has more 
potential and, well, there actually is code that works in
most cases 
already.


> So since I don't want to waste anymore of your time and
don't want to
> jeopardize the existing text shape, is it OK for me to
create a alttext
> shape which would be a place I could experiment. Its
compilation would
> depend on a cmake variable, disabled by default?

I'm not sure why you ask me. Its open source and you can do
whatever you 
want with the code.  (well almost ;)
Or are you in possesion of an svn account and want to work
in koffice SVN?
Hmm, better either use a branch or one of the distributed
source control 
systems that don't require you to put stuff in KDE svn.

Have a good evening!
-- 
Thomas Zander

_______________________________________________
koffice-devel mailing list
koffice-develkde.org
h
ttps://mail.kde.org/mailman/listinfo/koffice-devel