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

RST and heading cells #1331

Merged
merged 17 commits into from Jan 30, 2012
Merged

RST and heading cells #1331

merged 17 commits into from Jan 30, 2012

Conversation

ellisonbg
Copy link
Member

Added RST and heading cells to the notebook. The basic implementation is done, but there are a few questions to resolve:

  • Should we implement a plaintext cell that could be used for RST/latex or whatever the user wants.
  • Do we require a notebook to only have a particular type of text cell (RST, Markdown).
  • I am still working out some bugs in the saving on heading cells.

We also need to implement a UI element that clearly shows the type of the currently selected cell. I would like to put that in a separate PR on some related UI work. Export/import to .py files should work. Anyone bored and want to implement RST export? :)

@fperez
Copy link
Member

fperez commented Jan 27, 2012

Looking great! A few comments:

  • plaintext: probably a good idea, so there's a way for people to more easily implement other transformations onto their notebooks.
  • I think we can leave for now both markdown and rst. Honestly for lots of stuff markdown is just easier to type...
  • UI element: My vote would be for a dropdown menu in the (yet to be written) toolbar, that both shows the current style and lets you select it. Most word processors and lyx work this way, and I think it's a good solution. It gives immediate visual feedback and makes changing styles easily, so it's efficient use of the screen space used.
  • keybindings: I'd make C-m-<1>..<6> for heading-1..6.

Awesome job! I haven't reviewed the code yet, just playing with the functionality for now.

@ellisonbg
Copy link
Member Author

  • For the plaintext cell, would you just call it that and get away with the notion of a RST cell altogether.
  • for the UI element, I agree that something in the toolbar is in order and I think your idea is a good one. Are you OK with that going in as a separate PR?
  • Keybindings: these are already in place, let me know if they are not working.

@fperez
Copy link
Member

fperez commented Jan 27, 2012

Hey,

On Thu, Jan 26, 2012 at 11:23 PM, Brian E. Granger
reply@reply.github.com
wrote:

  • For the plaintext cell, would you just call it that and get away with the notion of a RST cell altogether.

Well, I'm wondering if it doesn't make sense to have both: rst cells
would be meant to be post-processed by docutils/sphinx, etc. While
plaintext ones would be left alone by all exporters (but users could
write their own tools that do something special to them).

  • for the UI element, I agree that something in the toolbar is in order and I think your idea is a good one.  Are you OK with that going in as a separate PR?

Oh sure, that's totally for later. It's fine if for now there's not
much UI feedback on the style, I think the fact that they render
visibly differently is OK for the time being.

  • Keybindings: these are already in place, let me know if they are not working.

Ah, I didn't realize because they weren't listed in the keybindings
help screen. Which btw, is starting to get long in the vertical
direction. I wonder if we should make it two-columns, like the gmail
one (type ? while in the message list view to see theirs).

But they seem to work fine, which is awesome!

@ellisonbg
Copy link
Member Author

  • I think we should have one plain text cell type. The implementation of latex/RST/plaintext in the notebook UI and the nbformat would be identical. It would be very redundant to have all of these cell types.
  • Should we increment the nbformat version? Notebooks with the new heading and RST cells cannot be opened by earlier versions of the notebook. Earlier notebooks open fine with the new code. I hate to have to do this, but we probably should.

@minrk
Copy link
Member

minrk commented Jan 28, 2012

Can I ask why, in general, RST is abbreviated (and as the unconventional RST no less), and others are not? It seems like the menu item should say 'reStructuredText', or at least use the more common 'reST' abbreviation if that's too long for some reason. Abbreviations in menu items are generally a bad idea.

And yes, I think we do need to increment the notebook version if you are adding things that aren't sensible in previous versions. We also should add all the planned cell metadata (UUIDs, etc.) in the next revision, and ideally the hash mechanism Stefan would need for his plan to reduce the enormous network traffic on save, which will be critical once remote notebook servers of anything beyond trivial notebooks become common.

@ellisonbg
Copy link
Member Author

On Fri, Jan 27, 2012 at 5:49 PM, Min RK
reply@reply.github.com
wrote:

Can I ask why, in general, RST is abbreviated (and as the unconventional RST no less), and others are not?  It seems like the menu item should say 'reStructuredText', or at least use the more common 'reST' abbreviation if that's too long for some reason.  Abbreviations in menu items are generally a bad idea.

Hehe, good question. I am completely unsatisfied with the traditional
spellings and abbreviations of restructured text. I picked RST
because it is a reasonable linear combination of the the filename
extension and acronym. From the Wikipedia page:

"reStructuredText is sometimes abbreviated as RST, ReST, or reST"

I suppose "reStructuredText" is the best non-abbreviated version and I
don't mind going with that....But I do think we should only have a
plaintext cell and not separate ones for latex/rest/plaintext, which
eliminates this decision altogether.

And yes, I think we do need to increment the notebook version if you are adding things that aren't sensible in previous versions.  We also should add all the planned cell metadata (UUIDs, etc.) in the next revision, and ideally the hash mechanism Stefan would need for his plan to reduce the enormous network traffic on save, which will be critical once remote notebook servers of anything beyond trivial notebooks become common.

OK I will increment the nbformat version in this PR. I agree that we
should revisit the nbformat before we release to see if there are
other things we need to add in. But let's take one thing at a time
and focus on the reST and heading cells in this PR.


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

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

@minrk
Copy link
Member

minrk commented Jan 28, 2012

But I do think we should only have a
plaintext cell and not separate ones for latex/rest/plaintext, which
eliminates this decision altogether.

So there's just one cell type for 'untransformed'? Then I think it should definitely not be called RST. How about calling it 'raw'?

But let's take one thing at a time and focus on the reST and heading cells in this PR.

Yes, I didn't mean that this should all be part of the same PR, I was just making the point that since we have additions planned that already require incrementing the format, we don't need to try to fit this change into the previous version since there will never be a release with this change and not a new format.

@ellisonbg
Copy link
Member Author

So there's just one cell type for 'untransformed'?  Then I think it should definitely not be called RST, then.  How about calling it 'raw'?

I have not implemented this yet, but I was going to call these cells
"Plaintext".

Yes, I didn't mean that this should all be part of the same PR, I was just making the point that since we have additions planned that already require incrementing the format, we don't need to try to fit this change into the previous version since there will never be a release with this change and not a new format.

Ahh OK this sounds good.


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

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

@fperez
Copy link
Member

fperez commented Jan 28, 2012

I'm OK with leaving just one plaintext cell type for now. We can always revisit that later if we find a compelling need for both plaintext and explicit reST cells for some reason.

@ellisonbg, it seems that once you update the cell name in the menu, this one is almost ready to go, right? The only thing I see missing is that the new cell types will need to be added to the drop-list in the new toolbar, which got merged in the meantime.

@fperez fperez mentioned this pull request Jan 29, 2012
@fperez
Copy link
Member

fperez commented Jan 29, 2012

BTW, I suggest we wait on merging this one until #1332 is in, to avoid creating that conflict again which @astraw has already had to manually disentangle twice. That one seems more or less good to go, I just don't want to merge it without Brian having a chance to look at it first in its final state.

@ellisonbg
Copy link
Member Author

More work is needed on this one before it is merged, #1332 should be
merged before this one.

On Sun, Jan 29, 2012 at 12:09 PM, Fernando Perez
reply@reply.github.com
wrote:

BTW, I suggest we wait on merging this one until #1332 is in, to avoid creating that conflict again which @astraw has already had to manually disentangle twice.  That one seems more or less good to go, I just don't want to merge it without Brian having a chance to look at it first in its final state.


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

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

@fperez
Copy link
Member

fperez commented Jan 29, 2012

OK, I just merged #1332 and there are no conflicts here, which is good. It also seems to work fine, the only changes I see still pending are:

  • Change the RST label, and have a keybinding for this cell type.
  • List the cell type keybindings in the help popup.
  • Add the new cell types to the dropdown menu in the toolbar. That would be IMO sufficient UI feedback for this. I imagine later on we could for example add something in the editing mode where a small area atop the cell itself shows its type, so that even without the toolbar one could directly see the type of a cell when editing it. But this is just thinking out loud for the future; as far as I'm concerned once the dropdown menu lists all cell types, we're good for now.

Did I miss anything?

ellisonbg added a commit that referenced this pull request Jan 30, 2012
Added plaintext and heading cells to the notebook UI and nbformat.

In the process we have updated the nbformat to v3 and integrated these new cell types into the new toolbar.
@ellisonbg ellisonbg merged commit 069f64e into ipython:master Jan 30, 2012
@minrk
Copy link
Member

minrk commented Jan 31, 2012

I'm not sure this was ready for merge, because the notebook format version handling was not fully fleshed out. For instance, if I open an existing notebook and add rst/heading cells to it, and save it, it remains notebook format v2, which is inaccurate.

@ellisonbg
Copy link
Member Author

@minrk: Hmm, I thought I tested everything pretty well, but looks like there is a bug. Do you have an example of a notebook that behaves in this way, or is it completely general?

@ellisonbg
Copy link
Member Author

What if you open the notebook and resave without adding any new cell types?

On Mon, Jan 30, 2012 at 6:27 PM, Min RK
reply@reply.github.com
wrote:

I'm not sure this was ready for merge, because the notebook format version handling was not fully fleshed out.  For instance, if I open an existing notebook and add rst/heading cells to it, and save it, it remains notebook format v2, which is inaccurate.


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

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

@ellisonbg
Copy link
Member Author

Also, can you do an extra notebook reload or two. The nbformat used
to save a notebook is set in the Javascript code and if there is
caching you would see this behavior.

On Mon, Jan 30, 2012 at 6:34 PM, Brian Granger ellisonbg@gmail.com wrote:

What if you open the notebook and resave without adding any new cell types?

On Mon, Jan 30, 2012 at 6:27 PM, Min RK
reply@reply.github.com
wrote:

I'm not sure this was ready for merge, because the notebook format version handling was not fully fleshed out.  For instance, if I open an existing notebook and add rst/heading cells to it, and save it, it remains notebook format v2, which is inaccurate.


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

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

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

@minrk
Copy link
Member

minrk commented Jan 31, 2012

Hm, could have been my mistake being too hasty. Do we really want to be silently upgrading users' notebooks to a version incompatible with the stable release, with no indication that this is happening at all?

@Carreau
Copy link
Member

Carreau commented Jan 31, 2012

You should send a message on the dev list that notebook version is bumped, hopefully I do have versionning of my .ipynb files
And the notebook example might have been updated, but they still speak of a "Left Panel" by the way.

@fperez
Copy link
Member

fperez commented Jan 31, 2012

Yes, let's be careful with this in the future. This very nearly bit me yesterday, as I was writing (using master) a set of notebooks for a lecture at Berkeley, that were meant to be used by a bunch of students using EPD. Fortunately I hadn't pulled during the day, but it could have led to an awkward scramble to disentangle a version mess in front of the whole class (they were meant to follow me along).

So let's try to be pretty strict from now on about announcing clearly whenever we bump the notebook version number so at least it doesn't happen silently under the hood.

And I think that whenever a notebook gets opened that's in an older format, we should pop up immediately a dialog saying something like:

"This notebook was saved in an older format X, note that the first time you save it, it will be written in the new format Y. If you do not want this to happen, reopen it with the older version of IPython used to create it."

or similar. We don't want to say that on save, b/c by that point the user may have already done a bunch of work, so I think that warning on open is the right approach.

I'm at the airport about to board a flight, so I'll be offline at least until tomorrow; but I think pinging the lists (both -dev and -user) soon explaining this would be a good idea.

@ellisonbg
Copy link
Member Author

A few points:

  • We have always been handling notebook conversions in this manner -
    the only difference is that not many people ever had v1 notebooks
    (mostly a few core devs) and we converted our notebooks long ago and
    never released the v1 code.
  • We can add a dialog/notification when a notebook is converted to a
    new version...
  • But, we can't give the user the option to not convert. The reason
    is that by the time the notebook has gotten to the client/JS it has
    already been converted. The client/JS code only knows how to handle
    the current version of the nbformat and all conversions are done
    server side before passing the notebook to the client.
  • I will add the dialog and some other code to handle these issues in
    a separate PR.
  • I will also notify the list about these changes.

On Tue, Jan 31, 2012 at 5:09 AM, Fernando Perez
reply@reply.github.com
wrote:

Yes, let's be careful with this in the future.   This very nearly bit me yesterday, as I was writing (using master) a set of notebooks for a lecture at Berkeley, that were meant to be used by a bunch of students using EPD.  Fortunately I hadn't pulled during the day, but it could have led to an awkward scramble to disentangle a version mess in front of the whole class (they were meant to follow me along).

So let's try to be pretty strict from now on about announcing clearly whenever we bump the notebook version number so at least it doesn't happen silently under the hood.

And I think that whenever a notebook gets opened that's in an older format, we should pop up immediately a dialog saying something like:

"This notebook was saved in an older format X, note that the first time you save it, it will be written in the new format Y.  If you do not want this to happen, reopen it with the older version of IPython used to create it."

or similar.  We don't want to say that on save, b/c by that point the user may have already done a bunch of work, so I think that warning on open is the right approach.

I'm at the airport about to board a flight, so I'll be offline at least until tomorrow; but I think pinging the lists (both -dev and -user) soon explaining this would  be a good idea.


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

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

@minrk
Copy link
Member

minrk commented Jan 31, 2012

  • We have always been handling notebook conversions in this manner -
    the only difference is that not many people ever had v1 notebooks
    (mostly a few core devs) and we converted our notebooks long ago and
    never released the v1 code.

Right, and this is why this is the first real notebook increment. The notebook format was v2 before it was even in master, I think, and certainly well before anybody other than you and Fernando were using it for real.

  • We can add a dialog/notification when a notebook is converted to a new version...

Yes, I think we must do this. It is terrible practice to silently upgrade in a destructive way, which we are doing in master now. Information is preserved, but compatibility is destroyed.

  • But, we can't give the user the option to not convert. The reason
    is that by the time the notebook has gotten to the client/JS it has
    already been converted. The client/JS code only knows how to handle
    the current version of the nbformat and all conversions are done
    server side before passing the notebook to the client.

I think that's fair. We just want to avoid destroying their ability to collaborate with 0.12 users without giving them a chance to take some precautions. Probably just offering to: save / cancel / duplicate old version before save should do it. If we want a 'compatibility mode', we would need to define a v3->v2 conversion on the server side, which would of course not be lossless as we are adding new metadata and cell types, but would still probably be useful.

  • I will add the dialog and some other code to handle these issues in a separate PR.
  • I will also notify the list about these changes.

Great, thanks!

@fperez
Copy link
Member

fperez commented Feb 2, 2012

Just to say that I think the dialog as added is fine. As long as the user has a way to back off from the destructive write (not everybody has backups or uses version control), that's OK. So I think in the future (there will be more instances of this) we can keep this policy: notify both user/dev lists, and add a similar dialog that at least lets people back off manually before they clobber the on-disk version of their notebooks.

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Added plaintext and heading cells to the notebook UI and nbformat.

In the process we have updated the nbformat to v3 and integrated these new cell types into the new toolbar.
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