On 8/4/2015 8:13 PM, Semyon Sadetsky wrote:
On 8/4/2015 6:17 PM, Alexander Scherbatiy wrote:
On 8/4/2015 3:13 PM, Semyon Sadetsky wrote:
On 8/4/2015 2:03 PM, Alexander Scherbatiy wrote:
On 8/4/2015 12:32 PM, Semyon Sadetsky wrote:
On 8/4/2015 11:47 AM, Alexander Scherbatiy wrote:
On 8/3/2015 4:19 PM, Semyon Sadetsky wrote:
On 8/3/2015 3:12 PM, Alexander Scherbatiy wrote:
On 7/31/2015 9:44 AM, Semyon Sadetsky wrote:
Good question. But I did not add a concrete class.
The problem is that UndoManager provided by JDK wants to be
serialized but undoable objects knows nothing about that. The
contract between UndoManager and undoable is
UndoableEditListener which only notifies UndoManager to add a
new edit. AbstractDocument does not care about the specific
UndoManager implementation and it can contain plenty different
UndoableEditListener. That is the current API approach.
If our specific UndoManager wants to be serialized it should
also take into account that the undoable it controls may
require serialization. For that it needs undoable's
synchronization monitor and AbstractDocument can provide it
using writeLock()/writeUnlock() methods. I assumed that in the
first turn UndoManger should work well with JDK undoables than
to serve as a general implementation. Also I tried to
preserve the current API.
And your suggestion is to change the existing
UndoableEditListener API by introducing synchronization
methods in it. Am I correctly understand you?
What I said is that UndoManager can be used not only by
AbstractDocument but also in other classes which can have the
same synchronization problems.
There should be a way to solve these problems without
storing links of external classes inside the UndoManager.
As well as AbstractDocument can use another undo managers. It
can be addressed to both parties. They need each others locks to
serialize changes without deadlock.
AbstarctDocument is related to UndoableEditListener as one to
many that means a lock should be taken for each undo manager
before the document change.
Undo manager does not have any methods to get its lock because
it is an UndoableEditListener implementation. AbstarctDocument
has API to receive its lock.
Do you still propose to fix the issue on AbstractDocument side?
Yes.
Could you clarify how do you see such fix?
Put an UndoableEdit/UndoableEditEvent/necessary information
to a queue instead of firing the undoable edit event under the
write lock. Do not read the queue under the write lock. The queue
allows to preserve the order of UndoableEdit's adding to an
UndoManager.
Is not it the same as the previous attempt to fix this issue (see
8030118 )?
8030118 fix does a strange thing like firing InsertUpdate
document event out of the lock. Do not do that.
Document change event need to be fired under write lock because
the change to the document should be atomic. Queue of changes is
undo manager's responsibility not the document.
And such queue in the AbstractDocument would require complex
coordination with all its undo managers queues. What if undo
called on undo manager during the doc's queue processing? The
right undo/redo requests and edit events order need to be
preserved in this case and it would be too complex or we would
have to change the concept and make AbstractDocument to maintain
its undo/redo history internally instead of external undo managers.
It only needs to pass undoable edits in the right order from
abstract document to the UndoManager.
Consider the scenario: UndoManager executes undo/redo before it
receives the undoable edits. As result it will undo not the last
edit but intermediate and it will crash because the document state
is changed and intermediate the undoable edit cannot be applied to
the final document state.
This is a good point. But this does not work neither with the
current behavior nor with your proposed fix.
Consider the following scenario:
-----------------------
document.insertString("AAA") // "AAA" UndoableEdit is added to
the UndoManager
document.insertString("BBB")
writeLock();
handleInsertString();
// a user press undo, the "AAA" UndoableEdit is selected in
UndoManager but not executed, because of the write lock
fireUndoableEditUpdate("BBB") // UndoManager is waiting
for the "AAA" UndoableEdit execution
writeUnlock() // "AAA" UndoableEdit is executed instead of
"BBB"
// "BBB" UndoableEdit is added to
the UndoManager
-----------------------
It will work after the fix. When undo() method is called it will be
blocked on the document lock until the edit is done in another thread.
Then undo() will acquire the document lock and call editToBeUndone()
method which will return the actual last edit added with addEdit()
during the undoable callback execution.
Is it possible to use the same undo manager with several abstract
documents? If so, how are you going to map a document lock with the
document undoable edit without querying it?
There is a mistake in your scenario steps: fireUndoableEditUpdate()
is called before the freeing the lock (see
AbstractDocument.handleInsertString() method).
Yet another argument do not do this from the user experience: if
user starts a long edit operation and press undo after that he
expects when the long edit is finished it will be rolled back
immediately.
It is not true. The first process adds his undo edit to the
UndoManager. While a user trying to press undo the second long
process can be started.
That is what led to this issue because when undo is in progress
document writing should be allowed.
Sorry but I didn't see why is "It not true"? Then what is your
expectation when you press undo button while edit is not finished
yet and there is no way to abort it?
It would be good if it works as you described. But it does not
work in this way with or without your fix.
undo() action has writeLock in AbstractDocument and because of
it is always executed after insert string action.
If a user sees that undo is available, he can call it but the
second long insertString process can start earlier and acquire the
writeLock.
That is what we are going to fix. And this does work after this fix.
Undo call will be blocked by the long edit until the last is done
without any deadlocks. And when edit is done undo() will acquire the
lock and prevent any new edits until undo() is done. Please provide a
scenario when in your opinion it does not wok.
The first process starts for 5 minutes. When it is finished a user
sees that he can press undo. While he is pressing undo button, the
second long process starts for 10 minutes and acquire the write lock.
The user presses undo but he needs to wait 10 more minutes until the
second process is finished.
So undo should be executed after the edit is fully performed
because the corresponding UndoableEdit which undos this edit can
be produced only after the edit is done.
I think at first we need to look on the situation externally
rather than concentrate on implementation questions like in which
class do references go.
Yes, please look on this situation from a user point of view
which wants to implement simple Java Painter.
But could you describe this scenario? Just steps when this simple
Painter fails under the proposed fix?I
Note, if this Painter's content is not an AbstarctDocument it will
work as before the fix.
Any application that uses UndoManager and wants to have the same
synchronization (have the same lock both for UndoableEdit adding and
undo() method execution) will have the same deadlock problems.
As I have already written:
---------------
Consider someone writes Java Painter application where it is
possible to draw lines and images and uses UndoManager for undo/redo
actions.
He might want that it was possible to work with copied images. He
can get lock on ctrl+v action, process an image, prepare UndoableEdit
and notify the UndoManager.
He also can use lock/unlock in the undo action to have a
consistent state with the processed image. If someone calls undo
action during the image processing and gets a deadlock does it mean
that link from Java Painter need to be added to the UndoManager?
---------------
Still do not understand the steps for your Painter scenario. A link
(reference?) can be added if it is required to implement
functionality. If the content is not an AbstarctDocument it may be
required to implement custom UndoManager to support the same behavior.
What is the difference between the AbstractDocument and other
classes (in Swing or user defined)? Do you mean that the UndoManager is
intended only to be used with AbstractDocument and it shouldn't be used
in other cases where undo/redo actions are required for non text data?
Thanks,
Alexandr.
I don't see a contradiction here, could you point on it more precisely?
Thanks,
Alexandr.
Thanks,
Alexandr.
Thanks,
Alexandr.
--Semyon
Thanks,
Alexandr.
--Semyon
On 7/30/2015 5:27 PM, Alexander Scherbatiy wrote:
Consider someone writes Java Painter application where it
is possible to draw lines and images and uses UndoManager for
undo/redo actions.
He might want that it was possible to work with copied
images. He can get lock on ctrl+v action, process an image,
prepare UndoableEdit and notify the UndoManager.
He also can use lock/unlock in the undo action to have a
consistent state with the processed image. If someone calls
undo action during the image processing and gets a deadlock
does it mean that link from Java Painter need to be added to
the UndoManager?
Thanks,
Alexandr.
It looks like AbstractDocument violates UndoManager
synchronization contract when it both use lock to work with
UndoManager and in the implemented undo() method.
Thanks,
Alexandr.
--Semyon
