Closed Bug 922812 Opened 6 years ago Closed 6 years ago

Add a SLIME-style "compile-defun" command

Categories

(DevTools Graveyard :: Scratchpad, defect, P2)

x86
macOS
defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 28

People

(Reporter: fitzgen, Assigned: fitzgen)

Details

Attachments

(1 file, 6 obsolete files)

From my mailing list thread:

We could do some coolness like SLIME[0]'s compile-defun command, which
(translated into the JS world) compiles and evaluates all the code that
your cursor is at (or is selected, or whatever) up until the top level.
Take for example the following source

  function foo () {
    return 'foo'; // 1
  }

  function dodo() {
    return 5;
  }

If your cursor was on the line with the "// 1" comment, it would
re-evaluate the foo function, but not the dodo function.

However in this example:

  function bar() {
    function nested() {
      return 1; // 2
    }
    return nested();
  }

  function qux() {
    return void 0;
  }

If your cursor was on the line with the "// 2" comment, it would
re-evaluate the bar function (which includes re-evaluating the nested
function). The qux function is not re-evaluated.

It shouldn't be too hard to implement with the machinery we have in
place at this time.

Yes, I know you can do an equivalent thing right now by highlighting a
top level function by hand and doing an eval, but that is way too
difficult/manual to be useful. This should be its own keyboard shortcut
for ease of use, or else it is completely useless.
Basically, I think we just need to record the line/col of the cursor, and inside the Program AST node's list of statements, find the one with the range that includes the cursor, and evaluate only the code associated with that statement.
Totally. Emacs has eval-defun, which is essentially this.
Although... wouldn't it be better if we could sustain the illusion that the code was *always* live?
(In reply to Jim Blandy :jimb from comment #3)
> Although... wouldn't it be better if we could sustain the illusion that the
> code was *always* live?

Yes, but I think we can leave that for bug 771339
One thing that I believe the selection + evaluate behavior gives the user is an explicit control over what's happening. There's feedback there as well.

If we implement something like eval-defun on a keystroke, I think it might be good UX to show the highlight before the eval takes place. Even if it's just a flash selection to indicate which block of code got re-evaluated.
Assignee: nobody → nfitzgerald
Priority: -- → P2
Assignee: nfitzgerald → past
Status: NEW → ASSIGNED
Assignee: past → nfitzgerald
Here is a small patch that highlights the evaluated code per comment 5.
Assignee: nfitzgerald → past
Assignee: past → nfitzgerald
New version that doesn't touch the stock CM stylesheet.
Attachment #8334210 - Attachment is obsolete: true
Assignee: nfitzgerald → past
Attached patch wip 1 (obsolete) — Splinter Review
doesn't highlight text (about to integrate panos' other patch)

only does FunctionDeclarations, not VariableDeclarations that happen to be functions.
Attachment #8334107 - Attachment is obsolete: true
Now with support for both light and dark themes.
Attachment #8334216 - Attachment is obsolete: true
Attached patch wip 2 - with highlighting (obsolete) — Splinter Review
Attachment #8334219 - Attachment is obsolete: true
Attachment #8334233 - Attachment is obsolete: true
Assignee: past → nfitzgerald
boom baby
Attachment #8334242 - Attachment is obsolete: true
Attachment #8334868 - Flags: review?(past)
Comment on attachment 8334868 [details] [diff] [review]
bug-922812-eval-statement.patch

Review of attachment 8334868 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/scratchpad/scratchpad.js
@@ +36,5 @@
>  const promise   = require("sdk/core/promise");
>  const Telemetry = require("devtools/shared/telemetry");
>  const Editor    = require("devtools/sourceeditor/editor");
>  const TargetFactory = require("devtools/framework/target").TargetFactory;
> +const acornLoose = require("acorn_loose");

Leftover!

::: browser/devtools/scratchpad/test/browser_scratchpad_eval_func.js
@@ +11,5 @@
> +    gBrowser.selectedBrowser.removeEventListener("load", onLoad, true);
> +    openScratchpad(runTests);
> +  }, true);
> +
> +  content.location = "data:text/html;charset=utf8,test Scratchpad pretty print.";

s/pretty print/evaluate current function/
Attachment #8334868 - Flags: review?(past) → review+
https://hg.mozilla.org/mozilla-central/rev/e7e1fdf75d8c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28
Keywords: verifyme
Tested with Firefox 28 beta 1 on Mac OS X 10.8.5 and Windows 7 32bit. When pressing Ctrl/Cmd+E
* if there are no errors in the code, the right function is highlighted.
* if there are errors in the code, nothing gets highlighted, and the error comment is displayed although the error was in a function that was not supposed to get evaluated (e.g. another function lower on the page and that nothing to do with the one selected for evaluation).

Please let me know if this behavior is by design and if there is anything else that I'm supposed to see happen here.
Flags: needinfo?(nfitzgerald)
Keywords: verifyme
(In reply to Ioana Budnar, QA [:ioana] from comment #16)
> Tested with Firefox 28 beta 1 on Mac OS X 10.8.5 and Windows 7 32bit. When
> pressing Ctrl/Cmd+E
> * if there are no errors in the code, the right function is highlighted.
> * if there are errors in the code, nothing gets highlighted, and the error
> comment is displayed although the error was in a function that was not
> supposed to get evaluated (e.g. another function lower on the page and that
> nothing to do with the one selected for evaluation).
> 
> Please let me know if this behavior is by design and if there is anything
> else that I'm supposed to see happen here.

Assuming by "errors in the code" you meant compile-time syntax errors, then yes that is expected because Reflect.jsm (the parser we are using) does not have an error tolerant parser; the whole source must be valid for anything to parse.

If by "errors in the code" you meant some kind of run-time error, then that is unexpected.
Flags: needinfo?(nfitzgerald)
(In reply to Nick Fitzgerald [:fitzgen] [Ðoge:D6jomNp59N9TVfgc538HU3RswwmwZwcrd7] from comment #17)
> Assuming by "errors in the code" you meant compile-time syntax errors, then
> yes that is expected because Reflect.jsm (the parser we are using) does not
> have an error tolerant parser; the whole source must be valid for anything
> to parse.
> 
> If by "errors in the code" you meant some kind of run-time error, then that
> is unexpected.

Yes, I did mean compile-time syntax errors. It works fine with runtime errors for me.
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.