Opened 10 years ago

Closed 10 years ago

#7666 closed enhancement (fixed)

Alphanumeric cell IDs, resize on paste (#2902), ESC ends introspection (#5644), JSLint for notebook_lib.js

Reported by: mpatel Owned by: was
Priority: major Milestone: sage-4.3.1
Component: notebook Keywords:
Cc: was, timdumol, boothby, mhansen Merged in: sagenb-0.6
Authors: Mitesh Patel Reviewers: Tim Dumol
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by mpatel)

This ticket

  • Makes it possible to assign alphanumeric cell IDs.
  • Should subsume
    • #2902 - Resize an input cell after a paste event.
    • #5644 - Make ESC close introspection windows.
  • Runs notebook_lib.js "through" JSLint.

Patch:

Suggested queue, applied to SageNB 0.4.9 + #7269:

trac_7843-cell_listdir.patch
trac_7844-notebook_address.patch
trac_7847-empty_trash_ie_ff.patch
trac_7846-no_CODE_PY_symlinks_v2.patch
trac_7650-sagenb_doctesting_v4.patch    # Possible trivial failed "hunk" here.
trac_7786-template-jinja-idiomatic.12.patch
trac_7871-interact_bgcolor.patch
trac_7858-key_binding_vars_v2.patch
trac_7863-declare_vars_aux_js_v2.patch
trac_7874-typeset_interact_labels.patch
trac_7666-alphanumeric_cell_ids_B5.patch

Or install this spkg:

Attachments (4)

trac_7666-alphanumeric_cell_ids_B5.patch (299.4 KB) - added by timdumol 10 years ago.
Rebase on a new patch set.
trac_7666-reviewer.patch (3.7 KB) - added by timdumol 10 years ago.
A few style tweaks.
trac_7666-alphanumeric_cell_ids_B6.patch (306.8 KB) - added by mpatel 10 years ago.
Combined patch. See comment. Apply only this patch.
trac_7666-alphanumeric_cell_ids_B7.patch (306.8 KB) - added by mpatel 10 years ago.
Combined patch. Apply only this patch.

Download all attachments as: .zip

Change History (37)

comment:1 Changed 10 years ago by mpatel

  • Authors set to Mitesh Patel
  • Cc was added
  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 Changed 10 years ago by mpatel

Please the commit string for a brief summary. Other notes:

  • It seems that auto-indentation was already broken in IE8. V9 does not fix the problem.
  • Possible CSS problem: The rendered completions menu is just two columns (not characters) wide in IE8 and O10.
  • On introspection: I think I've reproduced most of the standard behavior. New: Pressing TAB for explicitly requested docstrings or source code toggles between both.
  • IE8 CPU problem: jsMath seems to be involved, somehow. Try factor(factorial(100)), say, and scrolling up and down.

comment:3 follow-up: Changed 10 years ago by mpatel

A pre-existing bug:

  • "Evaluate All" cells in a worksheet.
  • Evaluate the last cell.
  • A new cell is not automatically inserted in the notebook.
  • Reload to see the new cell.

? It seems the client and server cell lists are out of sync. This might explain the "spontaneously appended cell" phenomenon.

comment:4 Changed 10 years ago by mpatel

In v9, "Evaluate All" can delay the updates of freshly rendered interacts. I can restore their priority (e.g., prepend their IDs to the active_cell_list) here or at #6855.

comment:5 in reply to: ↑ 3 Changed 10 years ago by mpatel

Replying to mpatel:

A pre-existing bug: [...] ? It seems the client and server cell lists are out of sync. This might explain the "spontaneously appended cell" phenomenon.

The "B"-series of patches at #6855 should fix this problem.

comment:6 Changed 10 years ago by mpatel

  • Description modified (diff)

V10 at

is rebased vs. Sage 4.3, SageNB 0.4.8, #7483, #7482, #7514, #7650, #7269.

comment:7 Changed 10 years ago by mpatel

The latest, V13, is rebased vs. #7811.

comment:8 Changed 10 years ago by mpatel

I'll stop pre-emptively rebasing this patch, for now. Just let me know, if/when it's time.

comment:9 Changed 10 years ago by was

mpatel -- fyi, Tom Boothby is going to help get this work into Sage very soon...

comment:10 Changed 10 years ago by mpatel

That's great! I should still rebase the patch and include a few corrections from #6855's "B" series. Remarks:

  • I've made many changes, it seems. Absolutely no offense is intended.
  • It's likely the introspection rewrite is just a first step.
  • The JS functions are not consistently documented. I hope to fix this in a future ticket.
  • It's likely that many Selenium tests on non-FF browsers will now fail. (Selenium is problematic, not everyone is testing them, etc.) I won't fix them here.
  • I think we can write/use a simple JS doctesting framework for non-{DOM, network} functions, but this should be a separate ticket.

comment:11 follow-up: Changed 10 years ago by mpatel

Just a quick note: I'm working on breaking this up into more manageable pieces.

comment:12 in reply to: ↑ 11 Changed 10 years ago by mpatel

Replying to mpatel:

Just a quick note: I'm working on breaking this up into more manageable pieces.

With Tim's permission, I've added the Se test framework changes to #7786.

comment:13 Changed 10 years ago by mpatel

Please see #7858 about setting the Content-Type header for dynamic JavaScript? files.

comment:14 Changed 10 years ago by mpatel

#7863 applies JSLint to the notebook's auxiliary JS files.

comment:15 Changed 10 years ago by mpatel

If I save $\alpha$ in a text cell, then edit the cell again, I see Ë in the editor. Moreover, the HTML source now contains <span> tags, etc.

comment:16 Changed 10 years ago by mpatel

Oops! Wrong ticket.

comment:17 Changed 10 years ago by mpatel

  • Cc timdumol boothby mhansen added
  • Description modified (diff)
  • Summary changed from Alphanumeric cell IDs to Alphanumeric cell IDs, resize on paste (#2902), ESC ends introspection (#5644), JSLint for notebook_lib.js
  • Try giving names/labels to cells in the "Edit" window, e.g.,
    `{{{id=hello|
    factor(factorial(37))
    ///
    }}}`
    
  • JSLint is not a compiler, but it can find bugs. Of course, it helps to start with a reasonably lint-free file! To do: Look into Closure Tools.

comment:18 Changed 10 years ago by mpatel

  • Description modified (diff)

comment:19 Changed 10 years ago by mpatel

Reminder: Set proxify to false prior to merge (if we get that far).

comment:20 follow-up: Changed 10 years ago by rbeezer

mpatel asked if I would test this package. Some observations follow, Firefox 3.5.6 on Kubuntu 9.10.

  1. ESC on introspection seems to be working (a great addition!). First time I hit ESC it didn't seem to have effect, then next tab-completion was weird, then all subsequent uses of ESC worked fine. Opened a new worksheet, and several uses of ESC worked fine. So maybe just once per server session?
  1. Formatted labels are working in interacts, but now I don't see any output. Three different interacts, no output at all. Controls are functional though, so it is not hung.
  1. Was testing resizing cells on paste:

(a) paste into a new cell, hit edit key and the pasted text is gone.

(b) this one took me a while

  • make a new cell, type a space, then type a sentence almost to the far end of the cell.
  • resize browser window by grabbing right edge and dragging left (slowly)
  • resize until just the last word of sentence wraps to a new line.
  • cell doesn't resize and second line is hidden
  • hit edit, cancel changes, see cell is now sized right
  • click into cell, it sizes wrong again

Without a space at the start of the line, sometimes there's no problem. Notice if you keep sliding the window edge, and push two words onto the next line, the sizing comes back to being right.

comment:21 in reply to: ↑ 20 Changed 10 years ago by mpatel

Replying to rbeezer:

mpatel asked if I would test this package. Some observations follow, Firefox 3.5.6 on Kubuntu 9.10.

Thanks for your detailed feedback!

  1. ESC on introspection seems to be working (a great addition!). First time I hit
  2. Formatted labels are working in interacts, but now I don't see any output.

I think the JavaScript compressor may be so aggressive that it's changing the semantics of the code. Could you try this (but don't bother if it's too much trouble):

  • Insert debug_mode = False just before _cache_javascript = None in $SAGE_ROOT/local/lib/python2.6/site-packages/sagenb-0.5-py2.6.egg/sagenb/notebook/js.py.
  • Restart the server, clear the browser's cache, and test ESC and interacts again.
  1. Was testing resizing cells on paste:

(a) paste into a new cell, hit edit key and the pasted text is gone.

This should be easy to fix by also sending the input to the server just after the paste event.

(b) this one took me a while

I've noticed this, too. The server code and JS library use different methods to (re)size a cell. The server does the sizing just before it sends the worksheet to the browser (e.g., on first display, after exiting "Edit" mode, etc.), but the browser resizes in between. I don't know how easy it is to reconcile the methods, but I'll definitely take a closer look.

comment:22 Changed 10 years ago by mpatel

Oops. That should be debug_mode = True.

comment:23 Changed 10 years ago by mpatel

  • Description modified (diff)

V4 should make it so a cell is resized and saved about 0.25 seconds after a paste event -- using no delay at all is too fast, apparently.

I think unifying the client and server resize methods should be a separate ticket. Neither of the current methods is optimal. The server assumes 80-column-wide input cells. The browser suffers from the problem described above.

I'm not sure what to do about the compressor. See #7787 and sage-notebook for potential options. For now, how about adding .min.js file(s), generated offline with the YUI Compressor?

comment:24 Changed 10 years ago by mpatel

  • Description modified (diff)

comment:25 Changed 10 years ago by mpatel

Found the problem: The JavaScriptCompressor converts, in effect,

function foo() {
    return 'hello';
}

to

function foo(){return 
'hello'
;}

But

In JavaScript, a linefeed can be whitespace or it can act as a semicolon. This replaces one ambiguity with another.

In particular, if I execute foo();, the former returns 'hello' and latter returns undefined. I've modified the compressor in V5 so that it does not insert extra '\n's. Of course, we should replace this with an free / open source, stable, Pythonic minifier (see above), when we can. By the way, the YUI Compressor yields

function foo(){return"hello"}

comment:26 Changed 10 years ago by mpatel

  • Description modified (diff)

comment:27 Changed 10 years ago by mpatel

I've updated the spkg to V5, too.

comment:28 Changed 10 years ago by timdumol

  • Status changed from needs_review to positive_review

Excellent work. Cleans up the library quite well.

I've rebased the patch on a new patch queue, but since the reject is just some empty lines, I'll mark this with a positive review.

--- notebook_lib.js
+++ notebook_lib.js
@@ -4376,10 +4676,10 @@ function decode64(input) {
     return output;
 }
 
+
 ///////////////////////////////////////////////////////////////////
 // Trash
 ///////////////////////////////////////////////////////////////////
-
 function empty_trash() {
     /*
     Empties the trash folder, after asking for confirmation.

The new patch queue is:

trac_7650-sagenb_doctesting_v6.patch
trac_7650-reviewer.patch
trac_7648-missing_indent.patch
trac_7663-rstrip_prompt.patch
trac_7847-empty-trash-no-referer.patch
trac_7786-template-jinja-idiomatic.patch
trac_7863-declare_vars_aux_js_v2.patch
trac_7874-typeset_interact_labels.patch
trac_7858-key_binding_vars_v2.patch
trac_7666-alphanumeric_cell_ids_B5.patch

Changed 10 years ago by timdumol

Rebase on a new patch set.

Changed 10 years ago by timdumol

A few style tweaks.

comment:29 Changed 10 years ago by timdumol

I've attached a few additional style tweaks to be applied on top of the patch. Feel free to review it or ignore it.

Changed 10 years ago by mpatel

Combined patch. See comment. Apply only this patch.

comment:30 Changed 10 years ago by mpatel

  • Reviewers set to Tim Dumol

I've attached a combined patch that incorporates most of the reviewer patch. Thanks for further simplifying handle_introspection! If it's OK, I'd like to adhere to Crockford's one-var-statement-per-function rule (see Scope), to minimize false positives from JSLint on its "The Good Parts" setting.

comment:31 Changed 10 years ago by timdumol

I think that the rule is made for functions with less than say, 5, variables. Putting all of those declarations in one line is a tad hard to read.

comment:32 Changed 10 years ago by mpatel

B7 does this

function bar() {
    var foo1, foo2, foo3, foo4, foo5, foo6,
        foo7, foo8, foo9, foo10;
    // ...
}

Changed 10 years ago by mpatel

Combined patch. Apply only this patch.

comment:33 Changed 10 years ago by timdumol

  • Merged in set to sagenb-0.6
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.