14623 - editor strategy for batching/merging transactions needs some work

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P3)

Description

[buster]

3) I've been overusing batching.  Batching is evil.  I used to think it
was
such a clear blue sky - being able to nest batches made a lot of
stuff so much
easier to write.  But batching absolutely, positively
breaks any kind of
merging.  You can't use it inside of any code that
might be doing anything that
 is part of a mergable operation.  Most
things we do in response to typing is
supposed to merge: hence, no
batching in any of our typing execution path.


The right solution here is the placeHolderTxn, which was Steve's
original
answer to the problem of merging together a bunch of disparate
transactions
under the auspices of one operation, while still allowing
that overall
operation to merge with a like one executed later.

  However, it's a pain to
carry around a pointer to a placeHolderTxn
everywhere the rules code goes and
then fill it in the first time we
discover that yes, this operation is going to
 have _some_ transactions
in it and not be empty.  There are a lot of places
that can be the
_first_ place to make actual content changes in response to
typing.
Deletion of the bogus node is one.  Splitting of mailcites is one.

Actual text insertion is one.  Creating inline style nodes is one.  I
bet I'm
forgetting some, and I bet there will be more later.  Having all
of these
having to call some "check to see if the placeHolderTxn is null
and if so make
one" code (everyplace they might first make a content
change) is pretty darn
ugly.

  I would prefer that we create one of these placeHolderTxns at the

start of a mergable operation (EditorKeyPress() would probably be the
right
place) and then at the end of the operation be able to detect
whether anything
actually got done.  If nothing did, somehow the
transaction should vanish.  I
assume we need an addition to the
transaction manager here: something to say
"hey, we know we told you to
do that, and you did, and now it's on your undo
stack, but we've since
realized that transaction did _nothing_ and we want you
to forget about
it".  Otherwise when the user does undo, there will be an
"empty" undo
operation.

  Although I've fixed a lot of undo problems this
weekend without taking
this step, it's going to be broken in places until we
either do this, do
something smarter that I havne't thought of but you
hopefully will, or
write lots of ugly code everywhere to check the status of
any existing
placeHolderTxn.  [An example of broken undo still left is undoing
typing
that has returns in it.  The results are pretty hideous right now.]

Comment 1

[Joe Francis]

accepting bug / m12

Comment 2

[Joe Francis]

this is fixed - undo now works like you would expect.  long live undo

Comment 3

[sujay]

I can't verify this one....jfrancis/buster, can you mark this verified-fixed..
thanks!

Comment 4

[Joe Francis]

verified, per demo to rickg yesterday ;-)