Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ReST support in notebook frontend #873

Closed
wants to merge 1 commit into from
Closed

ReST support in notebook frontend #873

wants to merge 1 commit into from

Conversation

jjehannet
Copy link

Hello,

I'm working on the integration of the notebook app into the Logilab's framework named CubicWeb.

http://www.cubicweb.org

Here a first patch about minor changes to enable ReST format in frontend.

Cheers from Logilab !

@minrk
Copy link
Member

minrk commented Oct 14, 2011

Thanks! This is a good start.

We do have plans for rst support, but I think it will have be rendered whole document at a time. The reason rst cells have been disabled is that if cells are rendered on their own, they do not behave as expected. For instance, section levels will not be respected across cells, cross-cell links will not resolve, etc.

@jjehannet
Copy link
Author

Ok, I see. @minrk: could you put your requirements in a new ticket, please ? So, I could maybe provide useful code then.

In a second time, I hope to provide some changes to make the ajax call configurable to connect to a different rendering server (instead of tornado by default)

@minrk
Copy link
Member

minrk commented Oct 17, 2011

Sure thing, I'll try to describe what we need.

@minrk
Copy link
Member

minrk commented Oct 17, 2011

My recollections of our ReST discussions are now in #888.

@fperez
Copy link
Member

fperez commented Nov 20, 2011

@jjehannet, I've also added a number of further points on this to #888 from a recent discussion we had on email... Thanks again for pitching in, this is a hugely important feature so we will get to it; it's just that it's also non-trivial so it will deserve (and require) a bit of design/thought up front.

@fperez
Copy link
Member

fperez commented Nov 26, 2011

@jjehannet, I've played with the code and it's indeed a great start, but as @minrk pointed out, we'll need to back off from rendering reST for now, because it's simply too easy to trigger a docutils error. For example, right now if you have one cell with:

.. _example:

An example
========

blah...

and another with:

as we see in the example_ above, blah...

the second one produces a "Docutils System Messages" result with an "unknown target name" for the "example" target.

So I think that in the long run, we'll probably want a solution that does:

  • per-cell rendering with manually controlled docutils errors, so that unresolved references are just left alone. This would render basic things, in-cell references and simple markup, but leave the rest as-is.
  • have a button/option that forces a render of the whole document and updates all the cells. This may not be completely trivial to write, as the default html writer in docutils just returns a whole blob, and we'll need a way to get per-cell output to put it back in place.
  • export to reST on-disk for production of full-quality sphinx-based html and pdf.

Now, it seems there's already useful code in this PR, so we could start merging things piece by piece, as the full effort will be a big job that will take some time. But in its current form we can't merge it quite yet. I see when I run the notebook it's dumping the html out to the terminal, but I don't know where that's coming from, since your code doesn't have printing. Perhaps those were in the old reST code that @ellisonbg was prototyping? In any case, they should be silenced and turned into logging calls at best, so they aren't seen in normal usage.

So question to everyone participating: what do we do with this specific PR? Do we try to polish it and merge it in as a first step on the road towards reST support, or do you think we need to start over with a different approach? I just want to reach a conclusion with this one before it starts having merge conflicts...

@ellisonbg
Copy link
Member

As we add reST cells we need to move to a model where the cell type is selected by a select menu/box rather than clicking on a button. This will require substantial refactoring but will more easily open the door to adding other cells types, such as the heading cells @fperez discusses. I guess I am 50/50 on simply aborting this PR. But, if we do allow it to live on, we need to remove the reST rendering that is there now.

@fperez
Copy link
Member

fperez commented Nov 29, 2011

@jjehannet, would you wan to rebase and refactor it to remove the rendering part, so we merge this bit only as an incremental first step towards the model @ellisonbg describes? Or do you prefer to close this PR and work from scratch on the whole problem as discussed above and in more detail in #888?

@fperez
Copy link
Member

fperez commented Nov 29, 2011

I should have said more explicitly that in any event, a rebase is needed right now, because it currently doesn't apply cleanly anymore.

@jjehannet
Copy link
Author

@fperez: Sorry, I'm afraid I will only refresh the patch for now. I hope to work on it tomorrow.
I will rebase the whole thing (and try to fix the console output as well).

IMO, there is another approach: instead of using just one cell content, we could try to rendering the whole document every time and redispatch cell output afterwards. Not very elegant (!) but we could test a prototype before rejecting.

@ellisonbg
Copy link
Member

If we render the entire document to HTML, there is no way to extract
the individual cells output from the HTML output. I looked at this
during the summer and the problem is that the HTML generated by
docutils is highly nested. This nesting itself depends on the various
heading levels found in the original reST source. Also, it is simply
too costly to render and update the entire notebook each time a single
cell is changed.

On Wed, Nov 30, 2011 at 10:47 AM, Julien Jehannet
reply@reply.github.com
wrote:

@fperez: Sorry, I'm afraid I will only refresh the patch for now. I hope to work on it tomorrow.
I will rebase the whole thing (and try to fix the console output as well).

IMO, there is another approach: instead of using just one cell content, we could try to rendering the whole document every time and redispatch cell output afterwards. Not very elegant (!) but we could test a prototype before rejecting.


Reply to this email directly or view it on GitHub:
#873 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@fperez
Copy link
Member

fperez commented Nov 30, 2011

On Wed, Nov 30, 2011 at 10:57 AM, Brian E. Granger
reply@reply.github.com
wrote:

Also, it is simply
too costly to render and update the entire notebook each time a single
cell is changed.

Just to say that I concur with this view, Julien. I think it would be
OK to have an optional mode that does a full render, as long as it's :

  • off by default
  • you can solve the problem of cell extraction Brian pointed out.

So users who are running smaller documents on a fast/local network
can enable it. But we're already getting bug reports from users with
large notebooks (unrelated to reST), so it's clear that people are
using the notebook to write very large files. In that case, doing a
full round-trip to the server on every single cell would be
catastrophic, so such a feature would necessarily have to be off by
default.

@ellisonbg
Copy link
Member

Even if we could get this working and it was off by default, I don't
think it should be

On Wed, Nov 30, 2011 at 12:00 PM, Fernando Perez
reply@reply.github.com
wrote:

On Wed, Nov 30, 2011 at 10:57 AM, Brian E. Granger
reply@reply.github.com
wrote:

Also, it is simply
too costly to render and update the entire notebook each time a single
cell is changed.

Just to say that I concur with this view, Julien.  I think it would be
OK to have an optional mode that does a full render, as long as it's :

  • off by default
  • you can solve the problem of cell extraction Brian pointed out.

Even if we could get this working and it was off by default it adds
significant code complexity and server load that is not worth it. It
would also mean that the entire page has to be re-MathJax rendered
after a single cell changes. For notebooks with many formulas, this
would be quite problematic. I would rather stick to our plan of:

  • Cells are not rendered individually upon edits.
  • There is a button that can be used to render the entire document in
    a new tab/window.

So users who  are running smaller documents on a fast/local network
can enable it.  But we're already getting bug reports from users with
large notebooks (unrelated to reST), so it's clear that people are
using the notebook to write very large files.  In that case, doing a
full round-trip to the server on every single cell would be
catastrophic, so such a feature would necessarily have to be off by
default.


Reply to this email directly or view it on GitHub:
#873 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@ellisonbg
Copy link
Member

RST support is being added in PR #1331.

@ellisonbg ellisonbg closed this Jan 27, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants