Bug 74490 – Only EditCommandComposition should implement unapply and reapply

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

Right now EditCommand, SimpleEditCommand, and CompositeEditCommand all implement apply, unapply, and reapply methods. However this doesn't make much sense since CompositeEditCommand's unapply/reapply are never called and EditCommandComposition's unapply & reapply can simply involve SimpleEditCommand's doUnapply and doReapply (we can disable delete button at the top-level command; not need to re-enable them after unapplying/reapplying each simple edit command).

Furthermore, it makes zero sense for EditCommandComposition for have apply or doApply. We should get rid of them.

[Ryosuke Niwa]

Created attachment 119392 [details]
Patch

[Ryosuke Niwa]

I'm very unhappy about having to rename EditCommand to UndoManagerEntry but it seems this is inevitable because edit commands now create EditCommandComposition for undo/redo instead of themselves being undoable/redoable. It's semantically wrong for us to register an edit command for undo/redo.

An alternative approach is either replace EditCommand with EditCommandComposition or rename EditCommandComposition to UndoManagerEntry. I'm fine with either approach.

[Early Warning System Bot]

Comment on attachment 119392 [details]
Patch

Attachment 119392 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10910178

[Ryosuke Niwa]

Note I didn't update Wx code because Wx port overrides CompositeEditCommand and do other fancy stuff with edit commands. Per IRC discussion, I'll let Kevin take care of it after the fact.

[Ryosuke Niwa]

Comment on attachment 119392 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119392&action=review

> Source/WebKit/qt/WebCoreSupport/EditorClientQt.cpp:240
> -void EditorClientQt::registerCommandForUndo(WTF::PassRefPtr<WebCore::EditCommand> cmd)
> +void EditorClientQt::registerCommandForUndo(WTF::PassRefPtr<WebCore::UndoManagerEntry> entry)

Apparently I forgot to update EditorClientQt.h.

[Ryosuke Niwa]

Created attachment 119401 [details]
Fixed Qt and Win builds

[Early Warning System Bot]

Comment on attachment 119401 [details]
Fixed Qt and Win builds

Attachment 119401 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10914157

[Ryosuke Niwa]

Comment on attachment 119401 [details]
Fixed Qt and Win builds

View in context: https://bugs.webkit.org/attachment.cgi?id=119401&action=review

> Source/WebKit/qt/WebCoreSupport/EditCommandQt.h:23
> -#include <EditCommand.h>
> +#include <UndoManagerEntry.h>

I don't understand why Qt's giving the following error given that I'm including UndoManagerEntry.h here:

In file included from ../../../Source/WebKit/qt/WebCoreSupport/EditCommandQt.cpp:21:
../../../Source/WebKit/qt/WebCoreSupport/EditCommandQt.h:34: error: expected ')' before '<' token
../../../Source/WebKit/qt/WebCoreSupport/EditCommandQt.h:44: error: ISO C++ forbids declaration of 'RefPtr' with no type
../../../Source/WebKit/qt/WebCoreSupport/EditCommandQt.h:44: error: invalid use of '::'
../../../Source/WebKit/qt/WebCoreSupport/EditCommandQt.h:44: error: expected ';' before '<' token
../../../Source/WebKit/qt/WebCoreSupport/EditCommandQt.cpp:26: error: expected ')' before '<' token
../../../Source/WebKit/qt/WebCoreSupport/EditCommandQt.cpp: In member function 'virtual void EditCommandQt::redo()':
../../../Source/WebKit/qt/WebCoreSupport/EditCommandQt.cpp:51: error: 'm_cmd' was not declared in this scope
../../../Source/WebKit/qt/WebCoreSupport/EditCommandQt.cpp: In member function 'virtual void EditCommandQt::undo()':
../../../Source/WebKit/qt/WebCoreSupport/EditCommandQt.cpp:58: error: 'm_cmd' was not declared in this scope
distcc[26227] ERROR: compile ../../../Source/WebKit/qt/WebCoreSupport/EditCommandQt.cpp on localhosFailed to run "['Tools/Scripts/build-webkit', '--release', '--qt', '--makeargs="-j20"']" exit_code: 2

[Darin Adler]

I do not thing “undo manager entry” is a good name. I do think that one undoable step should be a class, maybe named “undo step”, but “manager entry” is computer jargon disaster!

[Kevin Ollivier]

(In reply to comment #9)
> I do not thing “undo manager entry” is a good name. I do think that one undoable step should be a class, maybe named “undo step”, but “manager entry” is computer jargon disaster!

What about EditActionUndo, or EditActionApplier?

[Darin Adler]

I like a name like “undoable action” or “undo step” or “undoable operation” or something like that. I don’t like “edit action undo” as much because the phrase doesn’t even sound like a noun phrase. I don’t like “edit action applier” because I in the phrase “edit action” the word edit sounds like a verb, not a noun.

[Kevin Ollivier]

(In reply to comment #11)
> I like a name like “undoable action” or “undo step” or “undoable operation” or something like that. I don’t like “edit action undo” as much because the phrase doesn’t even sound like a noun phrase. I don’t like “edit action applier” because I in the phrase “edit action” the word edit sounds like a verb, not a noun.

I think UndoableAction sounds good. :)

[Ryosuke Niwa]

(In reply to comment #9)
> I do not thing “undo manager entry” is a good name. I do think that one undoable step should be a class, maybe named “undo step”, but “manager entry” is computer jargon disaster!

But the thing that manages undo stack is called UndoManager:
http://developer.apple.com/library/mac/#documentation/Cocoa/Reference/Foundation/Classes/NSUndoManager_Class/Reference/Reference.html

(In reply to comment #11)
> I like a name like “undoable action” or “undo step” or “undoable operation” or something like that. I don’t like “edit action undo” as much because the phrase doesn’t even sound like a noun phrase. I don’t like “edit action applier” because I in the phrase “edit action” the word edit sounds like a verb, not a noun.

I've considered names like UndoItem, UndoableAction, etc... but they all don't sound quite right since they're also used for redos. We could name it like UndoRedoItem but that sounds ad-hoc.

[Kevin Ollivier]

> (In reply to comment #11)
> > I like a name like “undoable action” or “undo step” or “undoable operation” or something like that. I don’t like “edit action undo” as much because the phrase doesn’t even sound like a noun phrase. I don’t like “edit action applier” because I in the phrase “edit action” the word edit sounds like a verb, not a noun.
> 
> I've considered names like UndoItem, UndoableAction, etc... but they all don't sound quite right since they're also used for redos. We could name it like UndoRedoItem but that sounds ad-hoc.

I do see the problem with undo itself, but I think that undoable suggests that it is inherently redoable as well, though maybe that's just how I tend to look at it... If you can undo something, you usually can also redo it. 

I think for myself, the reason I prefer something like UndoableAction is that UndoManagerEntry sounds like a simple data item in a list. I wouldn't think from the name that it actually would do the undo / redo itself, I would think that it would just provide the data needed for the UndoManager to perform those actions.

[Ryosuke Niwa]

(In reply to comment #14)
> I do see the problem with undo itself, but I think that undoable suggests that it is inherently redoable as well, though maybe that's just how I tend to look at it... If you can undo something, you usually can also redo it. 
> 
> I think for myself, the reason I prefer something like UndoableAction is that UndoManagerEntry sounds like a simple data item in a list. I wouldn't think from the name that it actually would do the undo / redo itself, I would think that it would just provide the data needed for the UndoManager to perform those actions.

Fair enough. Let's go with UndoableAction then.

[Ryosuke Niwa]

hm... UndoableAction might be problematic due to its having editingAction(). action.editingAction makes little sense. Undo step, though, sounds too low-level since one of these entries may consist of multiple SimpleEditCommand.

How about UndoItem? Or UndoableCommand?

[Ryosuke Niwa]

.net framework 4 calls it UndoUnit: http://msdn.microsoft.com/en-us/library/system.activities.presentation.undounit.aspx

QtUndoManager uses QAction: http://doc.qt.nokia.com/solutions/4/qtundo/qtundomanager.html

Java calls it UndoableEdit: http://docs.oracle.com/javase/6/docs/api/javax/swing/undo/UndoManager.html

Cocoa's UndoManager doesn't give it an explicit name (it says undoable/redoable "operation" in some places):
http://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/UndoArchitecture/UndoArchitecture.html

Visual Studio's UndoManager interface calls it UndoableAction:
http://msdn.microsoft.com/en-us/library/microsoft.visualstudio.modeling.undomanager.aspx

ActionScript 3.0 has IOperation:
http://help.adobe.com/en_US/FlashPlatform/reference/actionscript/3/flashx/undo/UndoManager.html

[Kevin Ollivier]

(In reply to comment #16)
> hm... UndoableAction might be problematic due to its having editingAction(). action.editingAction makes little sense. Undo step, though, sounds too low-level since one of these entries may consist of multiple SimpleEditCommand.
> 
> How about UndoItem? Or UndoableCommand?

Perhaps UndoItem is the best one to avoid using a term too similar to Action in the class name. I do like Java's UndoableEdit as well, but then we end up with edit.editingAction() syntax. :(

[Ryosuke Niwa]

(In reply to comment #18)
> Perhaps UndoItem is the best one to avoid using a term too similar to Action in the class name. I do like Java's UndoableEdit as well, but then we end up with edit.editingAction() syntax. :(

Yeah, but I don't like the fact "item" is such a generic word; no better than entry. "Edit" is a horrible noun though.

[Ryosuke Niwa]

I think we can live with UndoableAction if we renamed editingAction to actionType since we have to rename either editingAction or EditAction anyway.

[Ryosuke Niwa]

Hm... thinking more carefully, UndoableAction sounds like it's an action that can be undone and should have apply in addition to unapply/reapply.

UndoStep is starting to grow on me. I think "step" is a better word than "item" or "unit".

[Kevin Ollivier]

(In reply to comment #21)
> Hm... thinking more carefully, UndoableAction sounds like it's an action that can be undone and should have apply in addition to unapply/reapply.
> 
> UndoStep is starting to grow on me. I think "step" is a better word than "item" or "unit".

Yeah, maybe this is the best choice.

[Eric Seidel (no email)]

Comment on attachment 119401 [details]
Fixed Qt and Win builds

View in context: https://bugs.webkit.org/attachment.cgi?id=119401&action=review

Seems reasonable.  Obviously you need to fix Qt.

> Source/WebCore/editing/CompositeEditCommand.cpp:-203
> -    RefPtr<CompositeEditCommand> command = prpCommand;

I'm not sure this is a negative pattern we should be removing.

>> Source/WebKit/qt/WebCoreSupport/EditCommandQt.h:23
>> +#include <UndoManagerEntry.h>
> 
> I don't understand why Qt's giving the following error given that I'm including UndoManagerEntry.h here:
> 
> In file included from ../../../Source/WebKit/qt/WebCoreSupport/EditCommandQt.cpp:21:
> ../../../Source/WebKit/qt/WebCoreSupport/EditCommandQt.h:34: error: expected ')' before '<' token
> ../../../Source/WebKit/qt/WebCoreSupport/EditCommandQt.h:44: error: ISO C++ forbids declaration of 'RefPtr' with no type
> ../../../Source/WebKit/qt/WebCoreSupport/EditCommandQt.h:44: error: invalid use of '::'
> ../../../Source/WebKit/qt/WebCoreSupport/EditCommandQt.h:44: error: expected ';' before '<' token
> ../../../Source/WebKit/qt/WebCoreSupport/EditCommandQt.cpp:26: error: expected ')' before '<' token
> ../../../Source/WebKit/qt/WebCoreSupport/EditCommandQt.cpp: In member function 'virtual void EditCommandQt::redo()':
> ../../../Source/WebKit/qt/WebCoreSupport/EditCommandQt.cpp:51: error: 'm_cmd' was not declared in this scope
> ../../../Source/WebKit/qt/WebCoreSupport/EditCommandQt.cpp: In member function 'virtual void EditCommandQt::undo()':
> ../../../Source/WebKit/qt/WebCoreSupport/EditCommandQt.cpp:58: error: 'm_cmd' was not declared in this scope
> distcc[26227] ERROR: compile ../../../Source/WebKit/qt/WebCoreSupport/EditCommandQt.cpp on localhosFailed to run "['Tools/Scripts/build-webkit', '--release', '--qt', '--makeargs="-j20"']" exit_code: 2

Include cycle?

[Ryosuke Niwa]

Created attachment 119659 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 119659 [details]
Patch for landing

Clearing cq+ temporarily to wait for Qt EWS.

[Ryosuke Niwa]

Thanks for the review, Eric.

I've renamed UndoManagerEntry to UndoStep per discussion with Darin & Kevin.

[Ryosuke Niwa]

Committed r103104: <http://trac.webkit.org/changeset/103104>

[Mark Rowe (bdash)]

This broke the build because it removed headers from WebCore.framework that are used from within WebKit.

[Ryosuke Niwa]

(In reply to comment #28)
> This broke the build because it removed headers from WebCore.framework that are used from within WebKit.

You mean EditCommand.h? I thought I've replaced all of #include by UndoStep.h

[Mark Rowe (bdash)]

The failures mention CompositeEditCommand.h.

[Mark Rowe (bdash)]

When making code changes that remove headers I’d strongly suggest deleting all built frameworks from your build directory. It will prevent stale headers from being picked up so that you can verify that you have removed all uses of them, but won’t require a full rebuild like nuking the entire build directory would.

[Ryosuke Niwa]

(In reply to comment #30)
> The failures mention CompositeEditCommand.h.

I can't find any file in Source/WebKit that includes CompositeEditCommand.h

[Ryosuke Niwa]

Of course, except files in wx port.

[Mark Rowe (bdash)]

This is the sort of issue that grep isn’t really suitable to diagnose.  No files in WebKit include it directly. However, WebFrame.mm includes one or more files from WebCore that in turn attempt to include CompositeEditCommand.h.

[Ryosuke Niwa]

(In reply to comment #34)
> This is the sort of issue that grep isn’t really suitable to diagnose.  No files in WebKit include it directly. However, WebFrame.mm includes one or more files from WebCore that in turn attempt to include CompositeEditCommand.h.

Oh it's probably ReplaceSelectionCommand.h.  I thought I removed it in http://trac.webkit.org/changeset/102627 :( I guess that removal got lost in somewhere.

[Mark Rowe (bdash)]

The following headers are in WebCore.framework/PrivateHeaders and include CompositeEditCommand.h:

ApplyBlockElementCommand.h
DeleteSelectionCommand.h
MoveSelectionCommand.h
ReplaceSelectionCommand.h
TypingCommand.h

Any and all of these will cause similar build failures if used.

[Mark Rowe (bdash)]

Do you want me to file a new bug tracking this or are you going to fix it quickly? Given that the build is broken I’d appreciate either a quick fix or the change to be rolled out.

[Ryosuke Niwa]

(In reply to comment #37)
> Do you want me to file a new bug tracking this or are you going to fix it quickly? Given that the build is broken I’d appreciate either a quick fix or the change to be rolled out.

I'll try to fix it ASAP. Doing a clean rebuild to reproduce the build failure at the moment. Sorry for the failure again :(

[Mark Rowe (bdash)]

As I mentioned earlier, a clean rebuild shouldn’t be necessary to see this. Deleting WebCore.framework from the build directory should be sufficient and should not result in WebCore rebuilding at all (though it may relink).

[Ryosuke Niwa]

Oops, it appears that _insertParagraphSeparatorInQuotedContent is calling TypingCommand::insertParagraphSeparatorInQuotedContent :( I'll have to add CompositeEditCommand.h back to the framework.

[Ryosuke Niwa]

Build fix landed in http://trac.webkit.org/changeset/103141; filed https://bugs.webkit.org/show_bug.cgi?id=74778 to remove these *Command.h dependencies from WebKit code.

[Mark Rowe (bdash)]

Thanks!