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.
Created attachment 119392 [details] Patch
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 on attachment 119392 [details] Patch Attachment 119392 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10910178
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 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.
Created attachment 119401 [details] Fixed Qt and Win builds
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
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
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!
(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?
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.
(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. :)
(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.
> (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.
(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.
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?
.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
(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. :(
(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.
I think we can live with UndoableAction if we renamed editingAction to actionType since we have to rename either editingAction or EditAction anyway.
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".
(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 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?
Created attachment 119659 [details] Patch for landing
Comment on attachment 119659 [details] Patch for landing Clearing cq+ temporarily to wait for Qt EWS.
Thanks for the review, Eric. I've renamed UndoManagerEntry to UndoStep per discussion with Darin & Kevin.
Committed r103104: <http://trac.webkit.org/changeset/103104>
This broke the build because it removed headers from WebCore.framework that are used from within WebKit.
(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
The failures mention CompositeEditCommand.h.
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.
(In reply to comment #30) > The failures mention CompositeEditCommand.h. I can't find any file in Source/WebKit that includes CompositeEditCommand.h
Of course, except files in wx port.
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.
(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.
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.
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.
(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 :(
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).
Oops, it appears that _insertParagraphSeparatorInQuotedContent is calling TypingCommand::insertParagraphSeparatorInQuotedContent :( I'll have to add CompositeEditCommand.h back to the framework.
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.
Thanks!