#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).
Attachments (17)
Change History (58)
Changed 13 years ago by
Changed 13 years ago by
Converts template structure to an inheritance based tree. Apply this patch alone to sagenb repo.
comment:1 Changed 13 years ago by
- 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 13 years ago by
comment:3 Changed 13 years ago by
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 13 years ago by
Rebased on #7650 and its dependencies. Apply this patch alone to sagenb repo.
Changed 13 years ago by
Rebased on #7650 and its dependencies. Apply this patch alone to sagenb repo.
Changed 13 years ago by
Updates source_code.html
and adds styling for it. Also adds a warning to the .css files.
comment:4 Changed 13 years ago by
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
- 4050:
- Doctests:
- Failures:
challenge.py
,template.py
,worksheet.py
. - Memory-leak(?):
cell.py
.
- Failures:
comment:5 Changed 13 years ago by
- "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 13 years ago by
- Minor: Message at top of
main.sass
should also refer to thereadme.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 13 years ago by
- Actually, 4050 gives the same error (i.e., not a failure) as above.
js.py
does not compressmain.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?
comment:8 Changed 13 years ago by
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 13 years ago by
- It seems there are two
account_recovery.html
pages.
comment:10 Changed 13 years ago by
Actually, Graph.plot's partition
option may be a better choice for coloring vertices. Anyway, have fun!
comment:11 Changed 13 years ago by
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 13 years ago by
Sure, no problem.
comment:13 Changed 13 years ago by
By the way, the dependency graph is *awesome*. You may want to put it in the wiki somewhere, for Sage Notebook development tools.
comment:14 Changed 13 years ago by
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?
comment:15 Changed 13 years ago by
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 fromsagenb/tests/*
, i.e., put them all in anotebook_test_case.SeleniumTestCase
. Then we may be able to reusetest_*
for purezope.testbrowser
tests --- "just" write a correspondingnotebook_test_case.ZTTestCase
.
comment:16 follow-up: ↓ 20 Changed 13 years ago by
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
?
comment:17 Changed 13 years ago by
- It's not possible to delete compute cells from docbrowser worksheets, it seems.
comment:18 Changed 13 years ago by
Reminder: Use self-closing <input />
tags in HTML files. WebKit browsers will complain about extra </input>
s.
comment:19 follow-up: ↓ 22 Changed 13 years ago by
Can we delete twist.Reset_css
?
comment:20 in reply to: ↑ 16 ; follow-up: ↓ 24 Changed 13 years ago by
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) intemplate.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 13 years ago by
Should NotebookSettingsPage.render
return a HTMLResponse
, instead of a Response
?
comment:22 in reply to: ↑ 19 ; follow-up: ↓ 23 Changed 13 years ago by
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 13 years ago by
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.
comment:24 in reply to: ↑ 20 Changed 13 years ago by
- 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 seeSass::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 generatedmain.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 13 years ago by
comment:26 Changed 13 years ago by
Yes, it was missing. This patch fixes that problem.
comment:27 Changed 13 years ago by
Positive review from me. If you agree, please update the status.
comment:28 Changed 13 years ago by
Actually, I just noticed some problems with $(document).ready()
. I'm working on V10.
comment:29 Changed 13 years ago by
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 thatnotebook_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...
comment:30 Changed 13 years ago by
V11 is attached.
comment:31 follow-up: ↓ 32 Changed 13 years ago by
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:32 in reply to: ↑ 31 Changed 13 years ago by
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: ↓ 34 Changed 13 years ago by
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
comment:34 in reply to: ↑ 33 Changed 13 years ago by
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: ↓ 36 Changed 13 years ago by
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 13 years ago by
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
fromtd.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.
comment:37 Changed 13 years ago by
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 13 years ago by
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 13 years ago by
- Status changed from needs_review to positive_review
Alright, signing this off.
comment:40 Changed 13 years ago by
- Merged in set to sagenb-0.6
- Resolution set to fixed
- Status changed from positive_review to closed
comment:41 Changed 13 years ago by
- Milestone changed from sage-feature to sage-4.3.1
Preliminary work. Worksheet pages not yet done.