Bug 74490 - Only EditCommandComposition should implement unapply and reapply
Summary: Only EditCommandComposition should implement unapply and reapply
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on: 74249
Blocks: 74748 74754 74778
  Show dependency treegraph
 
Reported: 2011-12-14 00:51 PST by Ryosuke Niwa
Modified: 2011-12-16 20:45 PST (History)
16 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 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 Ryosuke Niwa 2011-12-15 00:56:36 PST
Created attachment 119392 [details]
Patch
Comment 2 Ryosuke Niwa 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 Early Warning System Bot 2011-12-15 01:01:54 PST
Comment on attachment 119392 [details]
Patch

Attachment 119392 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10910178
Comment 4 Ryosuke Niwa 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 Ryosuke Niwa 2011-12-15 01:06:14 PST
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.
Comment 6 Ryosuke Niwa 2011-12-15 02:19:04 PST
Created attachment 119401 [details]
Fixed Qt and Win builds
Comment 7 Early Warning System Bot 2011-12-15 02:27:06 PST
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 8 Ryosuke Niwa 2011-12-15 02:48:25 PST
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
Comment 9 Darin Adler 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 Kevin Ollivier 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 Darin Adler 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 Kevin Ollivier 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 Ryosuke Niwa 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 Kevin Ollivier 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 Ryosuke Niwa 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 Ryosuke Niwa 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 18 Kevin Ollivier 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 Ryosuke Niwa 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 Ryosuke Niwa 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 Ryosuke Niwa 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 Kevin Ollivier 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 Eric Seidel (no email) 2011-12-16 12:42:38 PST
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?
Comment 24 Ryosuke Niwa 2011-12-16 13:15:47 PST
Created attachment 119659 [details]
Patch for landing
Comment 25 Ryosuke Niwa 2011-12-16 13:18:01 PST
Comment on attachment 119659 [details]
Patch for landing

Clearing cq+ temporarily to wait for Qt EWS.
Comment 26 Ryosuke Niwa 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 Ryosuke Niwa 2011-12-16 14:05:06 PST
Committed r103104: <http://trac.webkit.org/changeset/103104>
Comment 28 Mark Rowe (bdash) 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 Ryosuke Niwa 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 Mark Rowe (bdash) 2011-12-16 19:59:28 PST
The failures mention CompositeEditCommand.h.
Comment 31 Mark Rowe (bdash) 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 Ryosuke Niwa 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 Ryosuke Niwa 2011-12-16 20:04:42 PST
Of course, except files in wx port.
Comment 34 Mark Rowe (bdash) 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 Ryosuke Niwa 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 Mark Rowe (bdash) 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 Mark Rowe (bdash) 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 Ryosuke Niwa 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 Mark Rowe (bdash) 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 Ryosuke Niwa 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 Ryosuke Niwa 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 Mark Rowe (bdash) 2011-12-16 20:45:42 PST
Thanks!