Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#7786 closed enhancement (fixed)

Restructure templates to idiomatic Jinja

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

Description

By idiomatic Jinja, I mean

  • Inheritance instead of inclusion
  • Arguments required are only the needed models to produce the required view [1]

This will make editing the templates easier, and will allow for a more consistent interface (by specifying base templates for each section).

[1] Model-View-Controller

Attachments (17)

trac_7783-sage-scripts.patch (684 bytes) - added by timdumol 10 years ago.
Preliminary work. Worksheet pages not yet done.
trac_7786-template-jinja-idiomatic.patch (211.0 KB) - added by timdumol 10 years ago.
Converts template structure to an inheritance based tree. Apply this patch alone to sagenb repo.
trac_7786-template-jinja-idiomatic.2.patch (211.6 KB) - added by timdumol 10 years ago.
Rebased on #7650 and its dependencies. Apply this patch alone to sagenb repo.
trac_7786-template-jinja-idiomatic.2,patch (211.6 KB) - added by timdumol 10 years ago.
Rebased on #7650 and its dependencies. Apply this patch alone to sagenb repo.
trac_7786-template-jinja-idiomatic.3.patch (217.3 KB) - added by timdumol 10 years ago.
Updates source_code.html and adds styling for it. Also adds a warning to the .css files.
trac_7786-template-jinja-idiomatic.4.patch (217.3 KB) - added by timdumol 10 years ago.
Adds missing div_wrap argument to template call at Cell.html()
jinja_template_deps.html (2.2 KB) - added by mpatel 10 years ago.
Simple Jinja template dependency graph worksheet. Not a patch.
trac_7786-template-jinja-idiomatic.5.patch (215.7 KB) - added by timdumol 10 years ago.
Fixes the issues pointed out (Se failures, etc.)
trac_7786-template-jinja-idiomatic.6.patch (259.9 KB) - added by mpatel 10 years ago.
New test options. Should fix Se + doc tests. Replaces previous.
trac_7786-template-jinja-idiomatic.7.patch (261.5 KB) - added by mpatel 10 years ago.
Conditional main.js compression. See js.py. Replaces previous.
trac_7786-template-jinja-idiomatic.8.patch (263.9 KB) - added by mpatel 10 years ago.
Further small fixes. Replaces previous.
trac_7786-template-jinja-idiomatic.9.patch (261.6 KB) - added by timdumol 10 years ago.
Adds missing file _topbar.sass
trac_7786-template-jinja-idiomatic.10.patch (266.3 KB) - added by mpatel 10 years ago.
DOM ready / load event timing fixes. Replaces previous.
trac_7786-template-jinja-idiomatic.11.patch (267.5 KB) - added by mpatel 10 years ago.
Cache HTML for published worksheets. Replaces previous.
trac_7786-template-jinja-idiomatic.12.patch (267.5 KB) - added by mpatel 10 years ago.
Fix text cell typesetting. Replaces previous.
trac_7786-template-jinja-idiomatic.13.patch (267.5 KB) - added by mpatel 10 years ago.
Fix HTML reference manual build. Replaces previous.
trac_7786-template-jinja-idiomatic.14.patch (262.7 KB) - added by timdumol 10 years ago.
Rebased on #7650 reviewer patch.

Change History (58)

Changed 10 years ago by timdumol

Preliminary work. Worksheet pages not yet done.

Changed 10 years ago by timdumol

Converts template structure to an inheritance based tree. Apply this patch alone to sagenb repo.

comment:1 Changed 10 years ago by timdumol

  • Cc mpatel was added
  • Status changed from new to needs_review

This is a big patch that makes the template structure more consistent by using inheritance. It also removes a lot of non-semantic markup (<br />, <hr />, etc.). Please note that there are some visual changes (font stack, bar below banner at login.html, headers instead of bold tags at login.html, etc.). I hope they're not too major. Kindly note any changes that are too big and may require consultation at the mailing list.

comment:2 Changed 10 years ago by timdumol

Note: Depends on #7269, #7650, and dependencies.

comment:3 Changed 10 years ago by mpatel

  • Authors set to Tim Dumol

I can only look at the patch right now, but I think these are much-needed and appreciated changes to SageNB. I think we should make merging this ticket (plus its dependencies) a priority. I'll try to start reviewing the patch soon.

As the merge nears, I'll rebase #7666 and its descendants, as necessary.

Changed 10 years ago by timdumol

Rebased on #7650 and its dependencies. Apply this patch alone to sagenb repo.

Changed 10 years ago by timdumol

Rebased on #7650 and its dependencies. Apply this patch alone to sagenb repo.

Changed 10 years ago by timdumol

Updates source_code.html and adds styling for it. Also adds a warning to the .css files.

Changed 10 years ago by timdumol

Adds missing div_wrap argument to template call at Cell.html()

comment:4 Changed 10 years ago by mpatel

V4 applies cleanly to 4.3.1.alpha0 + #7650's latest. With this configuration, I see

  • Se test errors:
    • 4050: Exception: Timed out after 30000ms
    • 7433: Exception: ERROR: Element link=Sign out not found
    • 3960, 7433, 7428, 7444, searching_for_worksheets: Exception: ERROR: Element link=Worksheet not found
  • Doctests:
    • Failures: challenge.py, template.py, worksheet.py.
    • Memory-leak(?): cell.py.

comment:5 Changed 10 years ago by mpatel

  • "Home" page contains "Toggle" link.
  • Printed/published worksheets: Click bar is visible (on hover). Clicking adds a cell, which appears to be editable.
  • Upon opening a worksheet: all existing input cells appear to have the same size.
  • Docbrowser worksheets:
    • Extra clickbar at top.
    • Shift-click is not disabled.

comment:6 Changed 10 years ago by mpatel

  • Minor: Message at top of main.sass should also refer to the readme.txt (cf. test_report.sass.
  • Just Se test 4050 fails with
    trac_7843-cell_listdir.patch
    trac_7844-notebook_address.patch
    trac_7847-empty_trash_ie_ff.patch
    trac_7846-no_CODE_PY_symlinks.patch
    trac_7650-sagenb_doctesting_v3.patch
    trac_7786-template-jinja-idiomatic.4.patch
    

!

comment:7 Changed 10 years ago by mpatel

  • Actually, 4050 gives the same error (i.e., not a failure) as above.
  • js.py does not compress main.js.
  • 'Error:' --> 'Error: ' on the Sign in page.
  • Ratings:
    • Unrated worksheets are rated -1.0, it seems.
    • A worksheet's ratings page shows "rating[0]" under User and "rating[1]" under "Rating."
    • Out of curiosity: Is there a maximum comment length?

Changed 10 years ago by mpatel

Simple Jinja template dependency graph worksheet. Not a patch.

comment:8 Changed 10 years ago by mpatel

The attached worksheet (just paste it into an "Edit" window) generates a template dependency graph in Sage. I'm sure there are many improvements to make, but I hope it's a useful start. By the way, the last non-empty cell requires Graphviz's dot.

comment:9 Changed 10 years ago by mpatel

  • It seems there are two account_recovery.html pages.

comment:10 Changed 10 years ago by mpatel

Actually, Graph.plot's partition option may be a better choice for coloring vertices. Anyway, have fun!

comment:11 Changed 10 years ago by mpatel

Hi Tim -- Do you mind if I make a separate, commuting patch here of just changes under sagenb/testing? If it's OK, I'd like to incorporate some changes I made at #7666, including running a test(s) by name (e.g., rt.run_any(['4088', 'test_3711'], verbosity=1)) and making it easier to test other browsers (e.g., first steps toward enabling Selenium Grid). Actually getting the tests to pass in the other browsers --- I found this very tedious / time-consuming --- we can leave for another ticket.

comment:12 Changed 10 years ago by timdumol

Sure, no problem.

comment:13 Changed 10 years ago by timdumol

By the way, the dependency graph is *awesome*. You may want to put it in the wiki somewhere, for Sage Notebook development tools.

Changed 10 years ago by timdumol

Fixes the issues pointed out (Se failures, etc.)

comment:14 Changed 10 years ago by timdumol

The doctest failures are not yet fixed. I am having trouble with the Se tests: after a recent kernel upgrade, it seems that shutil.rmdir() fails silently, and os.path.exists reports the directory as gone, but is actually still there. I can only think of a kludgy fix of sleeping a while and looping until it can be deleted. Any ideas?

Changed 10 years ago by mpatel

New test options. Should fix Se + doc tests. Replaces previous.

comment:15 Changed 10 years ago by mpatel

V6 might help. I've tested just FF 3.5.6 on Linux, so far. Examples:

import sagenb.testing.run_tests as rt

# Selenium.
rt.setup_tests(environment='*firefox3 /usr/lib64/firefox-3.5.6/firefox')
rt.run_any()

# Selenium Grid.
envs = [
    '*firefox',
#    '*googlechrome',
    '*iexplore',
    '*opera',
#    '*safari'
    ]

for e in envs:
    rt.setup_tests('192.168.50.99', False, e)
    name = 'report_' + e.split()[0][1:] + '.html'
    rt.run_any(make_report=True, report_filename=name)

For other tickets:

  • Parallel testing.
  • Abstract away all self.selenium calls from sagenb/tests/*, i.e., put them all in a notebook_test_case.SeleniumTestCase. Then we may be able to reuse test_* for pure zope.testbrowser tests --- "just" write a corresponding notebook_test_case.ZTTestCase.

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

Of course, please feel free to make changes!

On the dependency graph: Thanks! Maybe we should include parts of the code (wrapped in a try-except block) in template.py?

Changed 10 years ago by mpatel

Conditional main.js compression. See js.py. Replaces previous.

comment:17 Changed 10 years ago by mpatel

  • It's not possible to delete compute cells from docbrowser worksheets, it seems.

comment:18 Changed 10 years ago by mpatel

Reminder: Use self-closing <input /> tags in HTML files. WebKit browsers will complain about extra </input>s.

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

Can we delete twist.Reset_css?

comment:20 in reply to: ↑ 16 ; follow-up: Changed 10 years ago by timdumol

Replying to mpatel:

Of course, please feel free to make changes!

On the dependency graph: Thanks! Maybe we should include parts of the code (wrapped in a try-except block) in template.py?

It doesn't sound like it can be used by a non-developer, so I don't think it's worth including into the package. That's just my opinion though.

comment:21 Changed 10 years ago by mpatel

Should NotebookSettingsPage.render return a HTMLResponse, instead of a Response?

comment:22 in reply to: ↑ 19 ; follow-up: Changed 10 years ago by timdumol

Replying to mpatel:

Can we delete twist.Reset_css?

Yes, we can.

Should NotebookSettingsPage?.render return a HTMLResponse, instead of a Response?

Yes.

But I think this patch has way too many changes in it as is. The clean up on twist.py should probably be spun off to another ticket.

comment:23 in reply to: ↑ 22 Changed 10 years ago by mpatel

Replying to timdumol:

But I think this patch has way too many changes in it as is. The clean up on twist.py should probably be spun off to another ticket.

Sounds good. I need to do this anyway for public / remote interacts.

Changed 10 years ago by mpatel

Further small fixes. Replaces previous.

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

  • Reviewers set to Mitesh Patel

V8 should cover the problems above, except for deleting docbrowser cells, which is a known "problem" --- we don't save changes to these worksheets. Anyway, to the extent it counts, my review is positive, except:

  • Is _topbar.sass missing? When compiling, I see
    Sass::SyntaxError on line 29 of /.../sass/src/main.sass: File to import not found or unreadable: topbar.sass.
    
    If I comment out the @import, the generated main.css seems to be missing topbar directives.

By the way, I removed template_error.html and base_popup.html, since they don't appear to be used. I also put set div_wrap = true in cell.html (when printing). I apologize for going too far, in case I have.

comment:25 Changed 10 years ago by mpatel

  • Authors changed from Tim Dumol to Tim Dumol, Mitesh Patel

Changed 10 years ago by timdumol

Adds missing file _topbar.sass

comment:26 Changed 10 years ago by timdumol

Yes, it was missing. This patch fixes that problem.

comment:27 Changed 10 years ago by mpatel

Positive review from me. If you agree, please update the status.

comment:28 Changed 10 years ago by mpatel

Actually, I just noticed some problems with $(document).ready(). I'm working on V10.

Changed 10 years ago by mpatel

DOM ready / load event timing fixes. Replaces previous.

comment:29 Changed 10 years ago by mpatel

In V10, I replaced $(document).ready() in a few places with either synchronous evaluation or $(window).load() (in particular, $(document).load() does not always work). The main reason is timing --- the "DOM ready" event can fire too early for certain notebook initializations. For example, evaluate

import time
time.sleep(20)
print 'foo'

and reload the worksheet. Published interacts are another, forthcoming example...

I noticed that Worksheet.html_cell_list in V9

  • Referred to a non-existent template published_worksheet.html.
  • Is really no longer used. The lone remaining call, in twist.py sends refreshed HTML that notebook_lib.js ignores.

I've replaced the call with an empty string. However, the main problem is that HTML for published worksheets is now no longer cached by the server. Or am I mistaken?

I'll try to fix this in V11...

Changed 10 years ago by mpatel

Cache HTML for published worksheets. Replaces previous.

comment:30 Changed 10 years ago by mpatel

V11 is attached.

comment:31 follow-up: 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.

Changed 10 years ago by mpatel

Fix text cell typesetting. Replaces previous.

comment:32 in reply to: ↑ 31 Changed 10 years ago by mpatel

Replying to 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.

V12 should fix this.

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

I just noticed that Sphinx fails to build the reference manual. The "problem" may be in template.py:

Sphinx error:
'utf8' codec can't decode bytes in position 746-749: invalid data

Changed 10 years ago by mpatel

Fix HTML reference manual build. Replaces previous.

comment:34 in reply to: ↑ 33 Changed 10 years ago by mpatel

Replying to mpatel:

I just noticed that Sphinx fails to build the reference manual. The "problem" may be in template.py:

V13 should fix this.

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

It seems the worksheet/cell layout is less compact than before. It's probably easier to fix this at #7666.

comment:36 in reply to: ↑ 35 Changed 10 years ago by mpatel

Replying to mpatel:

It seems the worksheet/cell layout is less compact than before. It's probably easier to fix this at #7666.

Or the same? Anyway, possibilities for a completely different ticket:

  • Remove &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; from td.cell_number for cells with no output.
  • Put the green "cell is running" bar adjacent to the red "cell is not evaluated" line. The latter is actually a border, but we could insert a fixed-width div element here. We could make this a status-area for the whole cell (input + output), add a right-click menu (e.g., for select / insert / move / delete operations, etc.), an on-hover toolbar, etc.

Changed 10 years ago by timdumol

Rebased on #7650 reviewer patch.

comment:37 Changed 10 years ago by timdumol

My patch series before up to this patch:

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

comment:38 Changed 10 years ago by mpatel

Tim -- Can you confirm the review (unless you want to get a third opinion)? At this stage, I suggest we create new tickets to fix problems (if any) discovered later.

comment:39 Changed 10 years ago by timdumol

  • Status changed from needs_review to positive_review

Alright, signing this off.

comment:40 Changed 10 years ago by timdumol

  • Merged in set to sagenb-0.6
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:41 Changed 10 years ago by mvngu

  • Milestone changed from sage-feature to sage-4.3.1
Note: See TracTickets for help on using tickets.