Bug 15969 - Eliminate Editor::deleteRange()
Summary: Eliminate Editor::deleteRange()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.4
: P3 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-11-13 06:58 PST by Alexey Proskuryakov
Modified: 2007-11-17 00:59 PST (History)
0 users

See Also:


Attachments
proposed patch (22.07 KB, patch)
2007-11-13 07:15 PST, Alexey Proskuryakov
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2007-11-13 06:58:43 PST
There are a lot of deleteThisOrThat functions and classes around, too many to my liking. Editor::deleteRange() seems to be designed as a bottleneck/dispatcher for those, but doesn't help in my opinion:
- It's called incorrectly, as it doesn't work with ranges. It works with selections.
- It tries to unify too many different operations - as a result, it has to re-select what is already selected etc. Makes data flow hard to follow.
- It isn't a bottleneck anyway, as many code paths bypass it.

I think we can get rid of it. Patch forthcoming.
Comment 1 Alexey Proskuryakov 2007-11-13 07:15:34 PST
Created attachment 17226 [details]
proposed patch

I did notice that SelectionController was used improperly in one place here, but adding the needed functionality to Selection seemed too involved for this patch.
Comment 2 Alexey Proskuryakov 2007-11-16 12:21:04 PST
> - It tries to unify too many different operations - as a result, it has to
> re-select what is already selected etc. Makes data flow hard to follow.

Also, complicates fixing bugs that require deleting a range that's smaller than selection.
Comment 3 Darin Adler 2007-11-16 20:16:11 PST
Comment on attachment 17226 [details]
proposed patch

+    void yank();
+    void yankAndSelect();
+    void setMark();
+    void deleteToMark();
+    void selectToMark();
+    void swapWithMark();

I was hoping that at some point these would be added to platforms other that Macintosh. The mark ones, for example, don't rely on anything Macintosh-specific -- they just happen to be unused on other platforms at the moment. But people might later want to add key bindings for them, since they mimic emacs.

The yank ones simply rely on the existence of a kill ring, something that we could create a small platform-independent abstraction for, so the kill ring itself would be platform-specific, but the operations could be platform independent.

+    if (Frame* coreFrame = core([self _frame]))
+        coreFrame->editor()->swapWithMark();

A better way to handle these would be to add "SwapWithMark" to the named commands that Editor handles by adding it to the CommandMap. Then swapWithMark: could be added to the list of commands using the WEBCORE_COMMAND macro instead of having hand-written code to forward to a particular named command.

r=me, as-is, but I think this could be made even better
Comment 4 Alexey Proskuryakov 2007-11-17 00:59:42 PST
> A better way to handle these would be to add "SwapWithMark" to the named
> commands that Editor handles by adding it to the CommandMap.

Ah, that would definitely be better - I didn't understand the architecture well enough. I'll keep that in mind when working on editing next time.

Committed revision 27873.