|
Description
Jasper Bryant-Greene
2007-08-22 17:57:07 PDT
Created attachment 16083 [details]
adds missing keypress cases to EditorClientGtk::handleKeypress
Comment on attachment 16083 [details] adds missing keypress cases to EditorClientGtk::handleKeypress This patch looks fine (except for the lack of a ChangeLog entry (see <http://webkit.org/coding/contributing.html>)), but I'd much rather see us reduce code duplication rather than increase it. (In reply to comment #0) > The QT code is basically identical (neither the GTK nor QT code is specific to > that platform) and a way should be found to share the GTK and QT code for this, > but I'm leaving that as a separate issue. I think Windows currently has the most complete code for this. We should work on moving WebView::interpretKeyEvent and WebView::handleEditingKeyboardEvent down into WebCore. Adam, that seems fairly risky to undertake, and i also don't believe it would be overly compatible with mac given the excitingly different IM model it uses. My complaint would be the dramatic formatting change :D I think in fact, the general theme of this patch is correct, but we should have another bug along the lines of "move interpretkeyevent logic into webcore" although that is basically not possible for mac, at least for now. Created attachment 16086 [details]
updated patch with more keypress cases and a ChangeLog entry
Patching this has exposed another issue; please see bug #15058 also. Comment on attachment 16086 [details]
updated patch with more keypress cases and a ChangeLog entry
+ case VK_C:
+ case VK_X:
+ frame->editor()->execCommand("Copy");
+ break;
isn't ctrl-x meant to be cut?
And what about forward/back word/paragraph?
Comment on attachment 16086 [details]
updated patch with more keypress cases and a ChangeLog entry
Looks fine, you might as well do the word/paragraph-fu though, and the changelog should include the files/methods that are touched.
You should be able to just run the prepare-Changelog script and it should just magically work
(In reply to comment #3) > Adam, that seems fairly risky to undertake, and i also don't believe it would > be overly compatible with mac given the excitingly different IM model it uses. I'm not advocating that the Mac code be altered. The Mac code is very different from the Windows, Qt, and (now) Gtk+ code, and even if it weren't this isn't a good time to be mucking around with it. What I am saying is that, as far as I can tell, Windows, Qt, and Gtk+ all want the same implementation of EditorClient::handleKeypress. Windows has the most complete implementation (including handling many key combinations not handled currently by Qt). I think it makes far more sense for us to spend the time to make the Windows implementation sharable rather than for Gtk+ to duplicate all the Qt code (which, as I said, isn't even a complete implementation). Created attachment 16087 [details]
updated patch to implement back/forward word/paragraph and home/end while selecting, and removed duplication
As discussed on IRC, Ctrl-X is replaced with copy when the content is not editable. QT does this too.
I've updated the patch to support:
* back/forward word/paragraph
* shift-home/end to select to start/end of document
Also, I've removed a lot of duplication from the switch statements and made the method have just one exit point. Hope the refactoring is OK.
I'm happy to take a look at the Windows code and make it shareable. I could submit a patch as an alternative to this. Created attachment 16088 [details]
boolean was not named according to code style guidelines
Comment on attachment 16087 [details]
updated patch to implement back/forward word/paragraph and home/end while selecting, and removed duplication
Okay, i think i agree with Adam's subsequent comment, I think we just need to move WebView::interpretKeyEvent and WebView::handleEditingKeyboardEvent into shared code... it might even be possible to move it into WebCore...
possibly in EventHandler surrounded by a #if !PLATFORM(MAC) block
So
EventHandler::handleEditingKeyboardEvent(KeyboardEvent*) would do the current WebView::handleEditingKeyboardEvent behaviour on all non-mac platforms. and on mac it would call the editing client
(In reply to comment #13) > EventHandler::handleEditingKeyboardEvent(KeyboardEvent*) would do the current > WebView::handleEditingKeyboardEvent behaviour on all non-mac platforms. and on > mac it would call the editing client I think it might make more sense to put the shared code in Editor. Right now the control flow is: EventTargetNode::defaultEventHandler EventHandler::defaultKeyboardEventHandler Editor::handleKeypress EditorClient::handleKeypress WebView::handleEditingKeyboardEvent Editor::{insertText,execCommand} I don't think it makes sense to bounce back to EventHandler when the EventHandler has already asked the Editor to handle the event. I think that Editor::handleKeypress should do the work that WebView::handleKeyboardEditingEvent does right now (though on Mac I suppose it should continue to just call up to the EditorClient). Perhaps we can make EditorClient::handleKeypress be a Mac-only method? Ah whoops, i thought it did come from EventHandler -- yes it should be part of Editor, not EventHandler. I did consider saying that EditorClient::handleKeyPress should be mac only but decided that was too obvious :p One concern is what should happen if different platforms allow different keys to be bound to those actions? (In reply to comment #15) > I did consider saying that EditorClient::handleKeyPress should be mac only but > decided that was too obvious :p I love stating the obvious. > One concern is what should happen if different platforms allow different keys > to be bound to those actions? If that is needed we can make a the current WebView::interpretKeyEvent be a platform-specific Editor method. For now it seems like Qt and Gtk+ want to follow the Windows conventions. (In reply to comment #16) > (In reply to comment #15) > > One concern is what should happen if different platforms allow different keys > > to be bound to those actions? > > If that is needed we can make a the current WebView::interpretKeyEvent be a > platform-specific Editor method. For now it seems like Qt and Gtk+ want to > follow the Windows conventions. > True, sounds good then I think we can move the existing implementation of Editor::handleKeypress into EditorMac.mm, and then put the new implementation in Editor.cpp surrounded by #if !PLATFORM(MAC)/#endif Created attachment 16090 [details]
consolidates windows, qt and gtk keypress handling
OK, here's a patch implementing the refactoring described above. I've tested it on GTK, and it works well in my tests. Appreciate comments.
Comment on attachment 16090 [details]
consolidates windows, qt and gtk keypress handling
This needs to have a correct changelog for gtk
You also need to have correct copyright info in Editor{Mac}.cpp
Add that, and i would say it's an r+
However it will at least need to be tested on Mac and Webkit/Win prior to landing.
I can do this tomorrow.
Created attachment 16093 [details]
fixed changelog and copyright info
Fixed up the ChangeLog and added copyright notices.
Comment on attachment 16093 [details]
fixed changelog and copyright info
+static const KeyEntry keyEntries[] = {
I think we might as well move this whole array inside interpretKeyEvent, since it's only used in there.
+const char* Editor::interpretKeyEvent(const KeyboardEvent* evt)
Since we're touching this code anyway, I think it would be good to make this a static helper function instead of a method on Editor, and perhaps give it a better name like "commandForEvent".
+#if !PLATFORM(MAC)
+ const char* interpretKeyEvent(const KeyboardEvent* evt);
void handleKeypress(KeyboardEvent*);
+#endif
handleKeypress should not be within the #if !PLATFORM(MAC) block. handleKeypress exists on all platforms, but it has two implementations, one in Editor.cpp, and one in EditorMac.mm.
You should remove the declarations of WebView::interpretKeyEvent and WebView::handleEditingKeyboardEvent from WebView.h.
It would also be good to be slightly more explicit in your ChangeLogs by saying that the code was moved from WebView to Editor.
Also, you should set the review flag to ? when uploading a patch so that people know it needs review. Created attachment 16102 [details]
updated patch based on suggestions, also changed characters to keycodes
Thanks for your suggestions. I've updated the patch based on them.
Also, I've changed the entries in keyEntries that were specified using a character to keycodes, as the character doesn't work on GTK and I don't think it would have worked on Qt either. For example 'X' has changed to VK_X.
I need verification that this specific change hasn't broken Windows keypress handling, though.
Comment on attachment 16102 [details]
updated patch based on suggestions, also changed characters to keycodes
Looks good to me, just testing mac/windows builds.
Adam, you have any comments?
Comment on attachment 16102 [details]
updated patch based on suggestions, also changed characters to keycodes
This causes a build failure on mac as KeyboardCodes.h does not exist on mac. This can be fixed by an ifdef around the include
Mac also fails due due selectionForEvent not being available in EditorMac -- i suspect moving the Mac impl of Editor::handleKeypress into an ifdef'd block in Editor.cpp, or remove the static modifier in Editor.cpp
On windows KeyboardCodes.h does not exit (ifdef fix again), and WebEditorClient requires interpretKeyEvent -- which might be difficult to work around... perhaps make it accesible from outside webcore? :-/
Created attachment 16103 [details]
(should have) fixed build failures on windows and mac
I've made selectionForEvent non-static and put a prototype in EditorMac.mm.
commandForKeyEvent is now a static member of Editor.
KeyboardCodes.h has been surrounded with #if !PLATFORM(MAC) && !PLATFORM(WIN) ... #endif, however this file is identical on GTK/Qt/WX and so I will submit a subsequent patch to resolve this duplication.
Comment on attachment 16103 [details]
(should have) fixed build failures on windows and mac
Okay looks good, once more with the mac/windows testing though.
Created attachment 16104 [details]
#include <windows.h> when building on windows, for VK_*
You should set the review flag to '?'
VK_{I,B,C,X,V,...} don't exist in windows, however VK_{letter} should be equal to letter, eg VK_C == 'C', so you should be able to just use 'I', 'B', 'C', etc which should get rid of the need for KeyboardCodes.h and windows.h (which gains nothing).
You also need to modify WebEditorClient.cpp to include <WebCore/Editor.h> for windows.
Created attachment 17715 [details]
events for home and end keys
This patch adds more functionality to home and end keys. For example, you can select the whole contents of the page using shift+end or shift+home.
Same way works when in editable elements such as INPUT.
Comment on attachment 17715 [details]
events for home and end keys
r=me
Spaces, not tabs in the ChangeLog next time please.
We're probably going to end up using the GTK+ keybindings system for better integration rather than copying the Win code. The necessary patches in this bug have been landed. Closing. |