Bug 15969 – Eliminate Editor::deleteRange()

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

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

> - 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.

[Darin Adler]

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

[Alexey Proskuryakov]

> 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.