Bug 31400 - Web Inspector: Support Ctrl+P, Ctrl+N, and other Readline keyboard shortcuts in the Console
: Web Inspector: Support Ctrl+P, Ctrl+N, and other Readline keyboard shortcuts ...
Status: RESOLVED FIXED
: WebKit
Web Inspector (Deprecated)
: 528+ (Nightly build)
: Macintosh Intel Mac OS X 10.6
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-11-12 03:17 PST by
Modified: 2009-11-21 19:40 PST (History)


Attachments
[PATCH] Ctrl+P, Ctrl+N, Ctrl+U for Macs Only (5.19 KB, patch)
2009-11-20 23:01 PST, Joseph Pecoraro
timothy: review-
Review Patch | Details | Formatted Diff | Diff
[PATCH] Ctrl+P, Ctrl+N, Ctrl+U for Macs Only (5.68 KB, patch)
2009-11-21 07:26 PST, Joseph Pecoraro
no flags Review Patch | Details | Formatted Diff | Diff
[PATCH] Ctrl+P, Ctrl+N, Ctrl+U, Arrow Keys (10.97 KB, patch)
2009-11-21 09:01 PST, Joseph Pecoraro
no flags Review Patch | Details | Formatted Diff | Diff
[PATCH] Ctrl+P, Ctrl+N, Ctrl+U, Arrow Keys (10.96 KB, patch)
2009-11-21 09:16 PST, Joseph Pecoraro
timothy: review-
Review Patch | Details | Formatted Diff | Diff
[PATCH] Ctrl+P, Ctrl+N, and Arrow Keys (9.25 KB, patch)
2009-11-21 19:33 PST, Joseph Pecoraro
timothy: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-11-12 03:17:44 PST
My muscle memory keeps trying ctrl-p to access history which totally fails to do anything at all.  Would it be possible to map ctrl-p and ctrl-n to up and down in the console to mimic the behaviour of Terminal.app?
------- Comment #1 From 2009-11-20 23:00:04 PST -------
These are well known Readline keyboard shortcuts. Terminal.app Supports a number of them. Here is a general list:
http://www.bigsmoke.us/readline/shortcuts

keishi mentioned on IRC:
> I think it's good for os x but I'm wasn't sure about Windows.
> I'm not sure. I was wondering if it had any conflicts when i read it.
> all the readline key bindings don't work in Windows.
> http://en.wikipedia.org/wiki/Control_key seems like Ctrl+P Ctrl+N
> wouldn't have conflicts in the console. I like the readline key bindings
> and I want this, but I just wonder if it's natural for Windows users.
------- Comment #2 From 2009-11-20 23:01:22 PST -------
Created an attachment (id=43644) [details]
[PATCH] Ctrl+P, Ctrl+N, Ctrl+U for Macs Only

Also fixes modifier keys (Shift/Alt/Meta/Control) from re-triggering the type ahead.
------- Comment #3 From 2009-11-20 23:06:56 PST -------
I'm not sure the best way to handle this on Windows, maybe the meta key would work (Windows key).
------- Comment #4 From 2009-11-20 23:29:22 PST -------
(From update of attachment 43644 [details])

> +                // fall through to default
> +            case "U+004E": // Ctrl+N = Next

Are you sure this falls through to the default? I thought it fell through to the next case.
------- Comment #5 From 2009-11-21 00:21:45 PST -------
(From update of attachment 43644 [details])
The switch needs fixed.

Example:

switch(true) {
  case false:
    alert('false');
  case true:
    alert('true');
  case false:
    alert('false');
  case false:
    alert('false');
  default:
    alert('default');
}
------- Comment #6 From 2009-11-21 07:11:52 PST -------
(In reply to comment #4)
> > +                // fall through to default
> > +            case "U+004E": // Ctrl+N = Next
> 
> Are you sure this falls through to the default? I thought it fell through to
> the next case.

Correct, it falls through to the next case, which then falls through to the next case, and eventually will end up at default. This cases a few extra conditions to be checked, so maybe I should just refactor this anyway.
------- Comment #7 From 2009-11-21 07:26:13 PST -------
Created an attachment (id=43648) [details]
[PATCH] Ctrl+P, Ctrl+N, Ctrl+U for Macs Only

More explicit behavior in the switch. No fall throughs.

The only thing I am worried about is the O(n) behavior of deleting nodes in clearBeforeCaret. Even though my tests showed it was immediate for a rather large paste, I still think there should be a better way to delete a range of nodes in 1 operation, rather then n. Any pointers?
------- Comment #8 From 2009-11-21 09:01:27 PST -------
Created an attachment (id=43651) [details]
[PATCH] Ctrl+P, Ctrl+N, Ctrl+U, Arrow Keys

Included arrow keys for multiline here because bug 30553 seems to have diverted its attention to Snippets.
------- Comment #9 From 2009-11-21 09:08:30 PST -------
Video Showing Arrow Traversals so you can get a feel:
http://screencast.com/t/MTBlMGZiNjA
------- Comment #10 From 2009-11-21 09:16:34 PST -------
Created an attachment (id=43652) [details]
[PATCH] Ctrl+P, Ctrl+N, Ctrl+U, Arrow Keys

Removed a "+1" I had added that shouldn't have been there and caused bad behavior.
------- Comment #11 From 2009-11-21 19:10:18 PST -------
(From update of attachment 43652 [details])
> +        this.clearAutoComplete(true);
> +        if (this.isCaretAtEndOfPrompt()) {
> +            this.element.removeChildren();
> +            this.element.appendChild(document.createElement("br"));
> +            this.moveCaretToEndOfPrompt();
> +            return;
> +        }
> +
> +        while (focusNode.previousSibling)
> +            this.element.removeChild(focusNode.previousSibling);
> +        focusNode.textContent = focusNode.textContent.substring(selection.focusOffset);
> +        if (this.element.textContent.length === 0)
> +            this.element.appendChild(document.createElement("br"));
> +
> +        var newRange = document.createRange();
> +        newRange.setStart(focusNode, 0);
> +        newRange.setEnd(focusNode, 0);
> +
> +        selection.removeAllRanges();
> +        selection.addRange(newRange);

This can be simplified. Just make a DOM range that starts at the beginning and ends at the focusNode and focusOffset. Then call removeContents. This should leave you with a good caret selection too. You might need the <br> still if totally empty.

Even better:

    this.element.execCommand("DeleteToBeginningOfLine")

But looking at it, I don't think DeleteToBeginningOfLine is allowed from execCommand! Maybe we can change it to be "supported" not just "supportedFromMenuOrKeyBinding"?

http://trac.webkit.org/browser/trunk/WebCore/editing/EditorCommand.cpp#L1313

> +            selectionRange.setStart(this.element.firstChild, firstNewlineIndex);
> +            selectionRange.setEnd(this.element.firstChild, firstNewlineIndex);

Is it true the first child is always a text node that contains this index?
------- Comment #12 From 2009-11-21 19:33:13 PST -------
Created an attachment (id=43669) [details]
[PATCH] Ctrl+P, Ctrl+N, and Arrow Keys

Removed Ctrl+U based on chat in IRC, because It should act like Ctrl+K and put the content into the Kill Ring (usable with Ctrl+Y), but we cannot do that and it may be confusing.
------- Comment #13 From 2009-11-21 19:40:26 PST -------
Landed in http://trac.webkit.org/changeset/51291
r51291 = 5e12adf30e89cc9f952fb8cd7b39a16731aadb2b

I will look into this.element.execCommand("DeleteToBeginningOfLine") when I find time. I'll close this bug.