Notebook bug fix branch #1361

Merged
merged 11 commits into from Feb 2, 2012

Projects

None yet

6 participants

@ellisonbg
Member

I am using this branch to fix a number of bugs in the notebook. This branch is not done yet, but the changes are significant so testing should begin now.

Addresses: #1344, #1337, #1339, #1348, #1359

ellisonbg added some commits Feb 1, 2012
@ellisonbg ellisonbg Major refactoring of saving, notification.
* Refactored the save widget so that the notebook doesn't depend
  on it.  Now the notebook emits events and the save widget
  observes those events.
* Created a new event system for all IPython events (events.js).
  We should start to use this to allow our classes to be loosely
  coupled.
* Created a new notification widget that should be used for all
  notifications. Uses new event system.
* Removed the kernel status widget.
* All kernel status message use new event/notification system.
* The print notebook view works again.
ed25637
@ellisonbg ellisonbg Fixing minor issues with nbformat.
* Test suite passes.
* nbformat put into a single location for easy update.
981f2ab
@ellisonbg ellisonbg Fixing #1348 to allow Ctrl-M M to work w/o releasing Ctrl. 648d2de
ellisonbg added some commits Feb 1, 2012
@ellisonbg ellisonbg Fixing Shift-Enter bug in text cells.
Hitting Shift-Enter on an already rendered text cell should simply
move past it. It was causing the cell to enter edit mode. This
is not fixed.
1be7dfb
@ellisonbg ellisonbg Fixing #1359. We are not using itex mode in sympyprinting.
Previuosly we were using itex=True in the sympy latex printer.
I don't know why where were doing this but we should use regular
printing mode and put the $$ around the equations ourselves.
5cf294c
@ellisonbg ellisonbg Fixing #1337. Tooltip stops TAB from being handled by others.
Pressing TAB to get the tooltip "range(TAB" was inserting a TAB.
To get rid of this we are now telling CodeMirror to ignore it
and also stopping the event from bubbling up the DOM.
07435e3
@ellisonbg ellisonbg commented on the diff Feb 1, 2012
IPython/frontend/html/notebook/static/js/codecell.js
@@ -123,6 +123,10 @@ var IPython = (function (IPython) {
return false;
} else if ((pre_cursor.substr(-1) === "("|| pre_cursor.substr(-1) === " ") && tooltip_on_tab ) {
that.request_tooltip_after_time(pre_cursor,0);
+ // Prevent the event from bubbling up.
@ellisonbg
ellisonbg Feb 1, 2012 Member

@carreau and @fperez: can you both test the tooltip behavior to make sure that TAB is not being inserted when the tooltip is brought up.

ellisonbg added some commits Feb 1, 2012
@ellisonbg ellisonbg Updating CodeMirror to c813c94 to fix #1344. ecc950e
@ellisonbg ellisonbg Making keyboard shortcut for showing line numbers consistent. de28679
@ellisonbg ellisonbg Proper error handling for nbformat versions in client code.
1. Users are warned when an older nbformat notebook is converted.
2. The notebook notifies the user when a notebook that is too new
   is attempted to load.

To test these things, create a simple notebook. Then change its
nbformat field by hand to 2 (to test 1) and then 4 (to test 2).
cddb82e
@ellisonbg
Member

OK, this is ready for review. @fperez, @Carreau, @takluyver, @minrk can you test this, the changes are significant and many bugs are fixed (list above).

@fperez
Member
fperez commented Feb 2, 2012

Looking great so far, all but the mathjax bug seem completely gone; for that one I still see a slight glitch, best explained with an image:

<img src="http://img94.imageshack.us/img94/3061/selection001x.png" /img>

The dollar signs shouldn't be there, though I don't know if that is somehow a sympy issue instead. I just want to point it out for now.

I haven't reviewed the code yet, but behavior-wise this is major progress, thanks a lot!

@Carreau
Member
Carreau commented Feb 2, 2012

Won't have time in the next 24h ... see maybe tomorow. i'ts on my to-do list.

@miham
miham commented Feb 2, 2012

I'm not seeing the issue @fperez is having with dollar signs. I'm using Firefox 9.0.1, Sympy 0.7.1 and I started the notebook with $ python2 ipython.py notebook.

@miham
miham commented Feb 2, 2012

When I run notebook in Epiphany 3.2.1 browser, $ BROWSER=epiphany python2 ipython.py notebook, and open a new notebook, I get the following error:

Websocket connection to
ws://127.0.0.1:8888/kernels/9645b519-b50f-4adf-aeca-5f08b012003d could not
be established. You will NOT be able to run code. Your browser may not be
compatible with the websocket version in the server, or if the url does not
look right, there could be an error in the server's configuration.

And I am indeed not able to run code - I entered 3+3 in the input cell and
nothing happened. The URL of the new notebook is http://127.0.0.1:8888/new.

Epiphany is great for IPython notebooks because it has a web application mode which makes IPython notebook behave like any other (native desktop) application you have installed - it appears in dash, application lists, etc.

Edit: Notebook doesn't work in Epiphany in master too. I am pretty sure that it worked a couple of days ago though.

@takluyver
Member

Miha, the notebook will only work in modern browsers which support
websockets. We recommend using recent versions of Firefox or Chrome. This
requirement isn't going to change on our end, so it won't work with
Epiphany until Epiphany gets websocket support.

@ellisonbg
Member

@fperez: I don't see this issue with the dollar signs either. This was on both Chrome and FF on Mac OS X. I am wondering if it is a MathJax version issue. Of what vintage is your MathJax - mine is late summer.

@fperez
Member
fperez commented Feb 2, 2012

Mmh, that's quite odd. Just to be safe, I've re-downloaded mathjax with a fresh copy from their site and re-installed locally, and I've tested both Ffox and Chrome. I see those $ signs in both browsers (after multiple javascript reloads).

It's really bizarre. On the one hand I am not using sympy so much that this is a huge deal for me (I hadn't seen the issue in the first place), but on the other, if I see the problem, chances are someone else out there will too. So it really would be better to get to the bottom of it.

It almost looks as if we were inserting extra $ signs... If I manually call Math(latex(a)), it works fine:

Image Hosted by ImageShack.us
Uploaded with Shutter

@ellisonbg
Member

On Thu, Feb 2, 2012 at 9:02 AM, Fernando Perez
reply@reply.github.com
wrote:

Mmh, that's quite odd.  Just to be safe, I've re-downloaded mathjax with a fresh copy from their site and re-installed locally, and I've tested both Ffox and Chrome.  I see those $ signs in both browsers (after multiple javascript reloads).

Hmm, that is a puzzle.

It's really bizarre.  On the one hand I am not using sympy so much that this is a huge deal for me (I hadn't seen the issue in the first place), but on the other, if I see the problem, chances are someone else out there will too.  So it really would be better to get to the bottom of it.

Yes, what about other types of sympy expressions? Is this problem
localized to root?

It almost looks as if we were inserting extra $ signs... If I manually call Math(latex(a)), it works fine:

Yes, that is what I am thinking. I will give you some JS code to
enter into the console to look at that.

Image Hosted by ImageShack.us
Uploaded with Shutter


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

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

@ellisonbg
Member

@fperez: here is the code to type into the console:

IPython.notebook.get_cell(1).outputs[0]

This assumes that the cell with the output is cell 1 (0-indexed) and that there is one output. It will give you a JS object you can introspect. There will be a latex attribute that has the raw text that was returned by the server.

@fperez
Member
fperez commented Feb 2, 2012

Yes, what about other types of sympy expressions? Is this problem localized to root?

OK, the problem is actually in all sympy expressions:

Image Hosted by ImageShack.us
Uploaded with Shutter

I double-checked, and with master it's OK, no spurious $ signs anywhere. They only show up with this branch.

@fperez
Member
fperez commented Feb 2, 2012

Yup, I just typed sqrt(2) into a cell after loading sympy/sympyprinting, and got, using your JS snippet, this in the object's latex field:

latex: "$$$\sqrt{2}$$$"

so there seem to be indeed spurious $ in there.

@ellisonbg
Member

OK, now let's track down where the extra $ is coming from. In the
sympy printing extension, we call:

def print_latex(o):
"""A function to generate the latex representation of sympy expressions."""
s = latex(o, mode='plain')
s = s.replace('\dag','\dagger')
return '$$%s$$' % s

You can see where we are adding the double pair if $$. You can call
the latex function (it is sympy.latex) in the notebook to see if it is
adding $:

latex(3**(S(1)/4))

Try that on a few expressions - it comes back with no $ on my system.
What a funny problem...

On Thu, Feb 2, 2012 at 9:22 AM, Fernando Perez
reply@reply.github.com
wrote:

Yup, I just typed sqrt(2) into a cell after loading sympy/sympyprinting, and got, using your JS snippet, this in the object's latex field:

latex: "$$$\sqrt{2}$$$"

so there seem to be indeed spurious $ in there.


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

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

@ellisonbg
Member

@fperez: let's pull up IRC and figure this out.

On Thu, Feb 2, 2012 at 9:26 AM, Brian Granger ellisonbg@gmail.com wrote:

OK, now let's track down where the extra $ is coming from.  In the
sympy printing extension, we call:

def print_latex(o):
   """A function to generate the latex representation of sympy expressions."""
   s = latex(o, mode='plain')
   s = s.replace('\dag','\dagger')
   return '$$%s$$' % s

You can see where we are adding the double pair if $$.  You can call
the latex function (it is sympy.latex) in the notebook to see if it is
adding $:

latex(3**(S(1)/4))

Try that on a few expressions - it comes back with no $ on my system.
What a funny problem...

On Thu, Feb 2, 2012 at 9:22 AM, Fernando Perez
reply@reply.github.com
wrote:

Yup, I just typed sqrt(2) into a cell after loading sympy/sympyprinting, and got, using your JS snippet, this in the object's latex field:

latex: "$$$\sqrt{2}$$$"

so there seem to be indeed spurious $ in there.


Reply to this email directly or view it on GitHub:
#1361 (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

@fperez
Member
fperez commented Feb 2, 2012

Will hop on irc now... The problem may be a sympy version issue...

ellisonbg added some commits Feb 2, 2012
@ellisonbg ellisonbg Related to #1359. Removing extra $ that older versions sympy add.
Older versions of sympy put in an extra $ when using
latex(e, mode='plain'). We are removing those before we add our
own to make sure it works.
8247cb1
@ellisonbg ellisonbg Changes to Math class and added Latex class.
* Math now adds $$ if the user doesn't provide them.
* Latex is like Math, but doesn't add delimiters.
4aa7390
@fperez
Member
fperez commented Feb 2, 2012

Just to confirm for the PR record, that with the updates discussed on IRC to Math and the new Latex display object, this puppy is good to go.

@miham
miham commented Feb 2, 2012

Miha, the notebook will only work in modern browsers which support websockets. We recommend using recent
versions of Firefox or Chrome. This requirement isn't going to change on our end, so it won't work with
Epiphany until Epiphany gets websocket support.

Just for the record: It turns out that the Tornado upgrade (2.1.1 -> 2.2) broke notebooks in Epiphany. With tornado 2.1.1 both master and nbissues branches work fine in Epiphany. I sent mail to Epiphany mailing list and i hope that they figure out why Tornado 2.2 (which was released on jan 30) is causing problems.

@fperez fperez commented on the diff Feb 2, 2012
IPython/frontend/html/notebook/static/js/notebook.js
// Persistance and loading
+ Notebook.prototype.get_notebook_id = function () {
@fperez
fperez Feb 2, 2012 Member

Is it really necessary to have explicit getters and setters? I'm just curious: in Java/C++ they are ubiquitous, in Python rarely used. I just don't know if JS really requires them, and what the issues may be with explicitly setting the attribute from the outside. Basically, does the extra indirection layer really buy us anything?

@ellisonbg
ellisonbg Feb 2, 2012 Member

I am not sure what the usual patterns are in Javascript. It is surely more like Python than Java/C++. In this case, it was a gut feel that I wanted a layer of abstraction, in case I needed to ever change how the attribute was handled. I haven't done many of these though and I guess you could argue that it is not needed yet.

@minrk
minrk Feb 2, 2012 Member

I think the principal reason is that there is no equivalent of @property. If it seems like changing the notebook name might have side effects (even if it doesn't start out with any), it should be a setter because you can't turn an attribute into a property without changing the API, unlike in Python.

I think it's a minor style issue really, but that's the usual motivation.

@fperez
fperez Feb 2, 2012 Member

OK, it's no big deal and I don't have any major objection, really. Since we're all finding our way through JS, I'm sure we'll learn more and will find our own style over time. I think it's fine to leave these as-is for now, following your instinct.

I'll merge now then, since we're good on review and you updated the example nb, which was the last point in my mind. Thanks for the great job, this fixes a number of key issues!

@fperez fperez merged commit 54c3e06 into ipython:master Feb 2, 2012
@fperez
Member
fperez commented Feb 2, 2012

Merged, thanks @ellisonbg!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment