Bug 74490 - Only EditCommandComposition should implement unapply and reapply
: Only EditCommandComposition should implement unapply and reapply
Status: RESOLVED FIXED
: WebKit
HTML Editing
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 74249
: 74748 74754 74778
  Show dependency treegraph
 
Reported: 2011-12-14 00:51 PST by
Modified: 2011-12-16 20:45 PST (History)


Attachments
Patch (100.09 KB, patch)
2011-12-15 00:56 PST, Ryosuke Niwa
no flags Review Patch | Details | Formatted Diff | Diff
Fixed Qt and Win builds (100.88 KB, patch)
2011-12-15 02:19 PST, Ryosuke Niwa
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (99.39 KB, patch)
2011-12-16 13:15 PST, Ryosuke Niwa
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-12-14 00:51:18 PST
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.
------- Comment #1 From 2011-12-15 00:56:36 PST -------
Created an attachment (id=119392) [details]
Patch
------- Comment #2 From 2011-12-15 01:01:18 PST -------
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.
------- Comment #3 From 2011-12-15 01:01:54 PST -------
(From update of attachment 119392 [details])
Attachment 119392 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10910178
------- Comment #4 From 2011-12-15 01:03:18 PST -------
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.
------- Comment #5 From 2011-12-15 01:06:14 PST -------
(From update of attachment 119392 [details])
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.
------- Comment #6 From 2011-12-15 02:19:04 PST -------
Created an attachment (id=119401) [details]
Fixed Qt and Win builds
------- Comment #7 From 2011-12-15 02:27:06 PST -------
(From update of attachment 119401 [details])
Attachment 119401 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10914157
------- Comment #8 From 2011-12-15 02:48:25 PST -------
(From update of attachment 119401 [details])
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
------- Comment #9 From 2011-12-15 09:15:22 PST -------
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!
------- Comment #10 From 2011-12-15 09:29:04 PST -------
(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?
------- Comment #11 From 2011-12-15 09:30:46 PST -------
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.
------- Comment #12 From 2011-12-15 09:42:10 PST -------
(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. :)
------- Comment #13 From 2011-12-15 09:53:31 PST -------
(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.
------- Comment #14 From 2011-12-15 11:17:00 PST -------
> (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.
------- Comment #15 From 2011-12-15 11:36:58 PST -------
(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.
------- Comment #16 From 2011-12-15 20:40:17 PST -------
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?
------- Comment #17 From 2011-12-15 22:21:47 PST -------
.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
------- Comment #18 From 2011-12-15 23:41:47 PST -------
(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. :(
------- Comment #19 From 2011-12-15 23:49:38 PST -------
(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.
------- Comment #20 From 2011-12-15 23:51:30 PST -------
I think we can live with UndoableAction if we renamed editingAction to actionType since we have to rename either editingAction or EditAction anyway.
------- Comment #21 From 2011-12-16 00:00:03 PST -------
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".
------- Comment #22 From 2011-12-16 12:08:55 PST -------
(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.
------- Comment #23 From 2011-12-16 12:42:38 PST -------
(From update of attachment 119401 [details])
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?
------- Comment #24 From 2011-12-16 13:15:47 PST -------
Created an attachment (id=119659) [details]
Patch for landing
------- Comment #25 From 2011-12-16 13:18:01 PST -------
(From update of attachment 119659 [details])
Clearing cq+ temporarily to wait for Qt EWS.
------- Comment #26 From 2011-12-16 13:20:31 PST -------
Thanks for the review, Eric.

I've renamed UndoManagerEntry to UndoStep per discussion with Darin & Kevin.
------- Comment #27 From 2011-12-16 14:05:06 PST -------
Committed r103104: <http://trac.webkit.org/changeset/103104>
------- Comment #28 From 2011-12-16 19:52:17 PST -------
This broke the build because it removed headers from WebCore.framework that are used from within WebKit.
------- Comment #29 From 2011-12-16 19:57:56 PST -------
(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
------- Comment #30 From 2011-12-16 19:59:28 PST -------
The failures mention CompositeEditCommand.h.
------- Comment #31 From 2011-12-16 20:01:10 PST -------
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.
------- Comment #32 From 2011-12-16 20:04:24 PST -------
(In reply to comment #30)
> The failures mention CompositeEditCommand.h.

I can't find any file in Source/WebKit that includes CompositeEditCommand.h
------- Comment #33 From 2011-12-16 20:04:42 PST -------
Of course, except files in wx port.
------- Comment #34 From 2011-12-16 20:08:16 PST -------
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.
------- Comment #35 From 2011-12-16 20:12:33 PST -------
(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.
------- Comment #36 From 2011-12-16 20:20:42 PST -------
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.
------- Comment #37 From 2011-12-16 20:21:25 PST -------
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.
------- Comment #38 From 2011-12-16 20:26:05 PST -------
(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 :(
------- Comment #39 From 2011-12-16 20:29:30 PST -------
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).
------- Comment #40 From 2011-12-16 20:30:27 PST -------
Oops, it appears that _insertParagraphSeparatorInQuotedContent is calling TypingCommand::insertParagraphSeparatorInQuotedContent :( I'll have to add CompositeEditCommand.h back to the framework.
------- Comment #41 From 2011-12-16 20:45:04 PST -------
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.
------- Comment #42 From 2011-12-16 20:45:42 PST -------
Thanks!