editor strategy for batching/merging transactions needs some work

VERIFIED FIXED in M12

Status

()

Core
Editor
P3
normal
VERIFIED FIXED
19 years ago
17 years ago

People

(Reporter: buster, Assigned: Joe Francis)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

19 years ago
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.]
(Reporter)

Updated

19 years ago
Summary: editor's use of GetChildNodes needs auditing → editor strategy for batching/merging transactions needs some work
(Assignee)

Updated

19 years ago
Status: NEW → ASSIGNED
Target Milestone: M12
(Assignee)

Comment 1

19 years ago
accepting bug / m12
(Assignee)

Updated

19 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 19 years ago
Resolution: --- → FIXED
(Assignee)

Comment 2

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

Comment 3

19 years ago
I can't verify this one....jfrancis/buster, can you mark this verified-fixed..
thanks!
(Assignee)

Updated

19 years ago
Status: RESOLVED → VERIFIED
(Assignee)

Comment 4

19 years ago
verified, per demo to rickg yesterday ;-)
You need to log in before you can comment on or make changes to this bug.