Bug 113384

Summary: [GTK] Toggle OverWrite mode when pressing the Insert key
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: New BugsAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: cgarcia, gustavo, mrobinson, svillar
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch mrobinson: review+

Description Sergio Villar Senin 2013-03-27 03:25:40 PDT
[GTK] Toggle OverWrite mode when pressing the Insert key
Comment 1 Sergio Villar Senin 2013-03-27 03:31:20 PDT
Created attachment 195258 [details]
Patch
Comment 2 Martin Robinson 2013-03-27 08:03:44 PDT
Comment on attachment 195258 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195258&action=review

> Source/WebCore/platform/gtk/KeyBindingTranslator.cpp:238
> +    if (event->keyval == GDK_Insert && type == KeyDown) {
> +        commandList.append("OverWrite");
> +        return;
> +    }
> +

Why KeyDown and not KeyPress?
Comment 3 Sergio Villar Senin 2013-03-27 08:45:24 PDT
(In reply to comment #2)
> (From update of attachment 195258 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=195258&action=review
> 
> > Source/WebCore/platform/gtk/KeyBindingTranslator.cpp:238
> > +    if (event->keyval == GDK_Insert && type == KeyDown) {
> > +        commandList.append("OverWrite");
> > +        return;
> > +    }
> > +
> 
> Why KeyDown and not KeyPress?

We don't apparently get KeyPress for the insert key.
Comment 4 Martin Robinson 2013-03-27 09:26:22 PDT
Comment on attachment 195258 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195258&action=review

>>> Source/WebCore/platform/gtk/KeyBindingTranslator.cpp:238
>>> +
>> 
>> Why KeyDown and not KeyPress?
> 
> We don't apparently get KeyPress for the insert key.

Okay, that's reasonable. Another question is why not simply add this to keyPressEntries? By doing this without respect to modifiers you are breaking Shift+Insert.
Comment 5 Sergio Villar Senin 2013-03-27 11:18:16 PDT
(In reply to comment #4)
> (From update of attachment 195258 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=195258&action=review
> 
> >>> Source/WebCore/platform/gtk/KeyBindingTranslator.cpp:238
> >>> +
> >> 
> >> Why KeyDown and not KeyPress?
> > 
> > We don't apparently get KeyPress for the insert key.
> 
> Okay, that's reasonable. Another question is why not simply add this to keyPressEntries? By doing this without respect to modifiers you are breaking Shift+Insert.

I thought about that but the problem with modifiers is that the Insert won't be detected for example if the BloqNum is active (unless we register all the possible combinations). BTW what's shift+insert for?
Comment 6 Martin Robinson 2013-03-27 16:22:00 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 195258 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=195258&action=review
> > 
> > >>> Source/WebCore/platform/gtk/KeyBindingTranslator.cpp:238
> > >>> +
> > >> 
> > >> Why KeyDown and not KeyPress?
> > > 
> > > We don't apparently get KeyPress for the insert key.
> > 
> > Okay, that's reasonable. Another question is why not simply add this to keyPressEntries? By doing this without respect to modifiers you are breaking Shift+Insert.
> 
> I thought about that but the problem with modifiers is that the Insert won't be detected for example if the BloqNum is active (unless we register all the possible combinations). BTW what's shift+insert for?

Is NumLock detected as a modifier? Typically modifiers are keys like Shift and Control that you chord with. Even so, if there is a chord combination that should be active too, you should just add all applicable ones to keyPressEntries. I'm almost certain we don't want to treat Shift+Insert as a switch to overtype mode.
Comment 7 Martin Robinson 2013-03-27 16:23:41 PDT
Looking at this again, instead of adding a special keybinding, you should likely just add your logic to the toggle-overwrite signal! :)

Take a look at toggleOverwriteCallback in that file.
Comment 8 Sergio Villar Senin 2013-04-08 08:56:14 PDT
Created attachment 196860 [details]
Patch
Comment 9 Martin Robinson 2013-04-08 08:59:10 PDT
Comment on attachment 196860 [details]
Patch

This is a lot nicer. :)
Comment 10 Sergio Villar Senin 2013-04-08 09:04:29 PDT
Committed r147916: <http://trac.webkit.org/changeset/147916>