Updatecm #1303

Merged
merged 14 commits into from Jan 24, 2012

Conversation

Projects
None yet
4 participants
Owner

ellisonbg commented Jan 20, 2012

This updates CodeMirror and refactors a good bit of the notebook code.

  • Updated CodeMirror to the latest stable release.
  • Fix numerous bugs related to the CM update.
  • Refactored the Cell API and the notebook's cell handling methods.
  • Generalized split/merge to work with all cell types.
  • Generalized "Edit in Ace" to work with all cell types.
  • Loading optimizations: pager starts out hidden, faster loads.
  • Shading added to Markdown and HTML cells when they are being edited.

This branch will require solid usability testing on Safari, FF and Chrome before merging.

Owner

Carreau commented Jan 20, 2012

Chrome 16, Mac 2 blinking cursor:

  • have 2 cell.

  • click in the first

  • edit in ace

  • close

  • click the second

  • first have still the blinking cursor

    100% broken in safari, (full white page, some element on display none)

  ace.js:1SyntaxError: Cannot declare a parameter named 'e' in strict mode 
  mode-python.js:1ReferenceError: Can't find variable: define
  mode-markdown.js:1ReferenceError: Can't find variable: define
  mode-html.js:1ReferenceError: Can't find variable: define
  theme-textmate.js:1ReferenceError: Can't find variable: define
  fulleditwidget.js:32ReferenceError: Can't find variable: ace
Owner

fperez commented Jan 20, 2012

Sorry I can't test in detail now, but I tried to use it for work and noticed a problem, shown here:

<img src="http://i.imgur.com/x5F1i.png" /img>

I got this by going to ace, typing a two-line cell, and back to normal with C-m e. It created one spurious blank cell (the no. 2 cell shouldn't be there) and also truncated my cell at the bottom, which should have two lines. This was on chrome, with multiple page reloads to ensure I had the right JS.

Owner

Carreau commented Jan 20, 2012

Note, ace.js is a minified source, which would gave the sames problems has the minified source of jquery for shipping with debian.

Contributor

fawce commented Jan 20, 2012

testing scope

I only had time to try a subset of the new functionality - merging code cells, editing code cells in ACE. I didn't have time to try the markdown and html cells.

usability nits

  • my first guess for "edit in ace" was under the edit menu...
  • when i went to try merging for the first time, i first tried to multi-select cells. took me a few tries to find the right menu item.
  • I looked at ace as well, and I found it a bit buggy in their kitchen sink demo. What is the benefit of using ACE vs. just styling up Codemirror? You can get line numbers, error/break markers, and anything else in CodeMirror as well.
  • when I switched to edit a cell in ACE, I thought something had gone wrong, like i had lost my other cells. once you are editing in ACE, it is a little scary to click a close button without saving first. maybe you could just plop the ACE editor right into the same div as the original codemirror cell?
  • was there always a BUSY signal in the top right? I love it.
  • refreshing page after working on a new notebook for a while actually creates a new notebook. /new should probably redirect to the new notebook so that /new and opening an existing notebook have the same url context. also good for getting the permalink to the actual notebook

functional bugs

menu header selection bug:

  • click on menu header (e.g. file), header is selected, menu displayed
  • click on same menu hader again, menu disappears but header is still selected
  • clear by selecting an item in the same menu, or picking another menu (e.g. file --> save)
  • quick screen capture: http://screencast.com/t/A9v1OptTki

blank file name accepted for notebook:

  • click notebook title, delete all text
  • save notebook with blank name
  • go to notebook list, your original notebook is gone

first time load of a notebook after upgrading from 0.12, requires a refresh for the javascript

design ideas

  • maybe when a cell is selected, you could add a little tab to the div with extra controls to move it up/down, ACE edit it, merge it
  • multi-selecting with ctrl-mouseclick, or shift+arrow keys like you do in excel would be cool. maybe this would work well with the tool tab idea above. or maybe there's a hotkey to merge the cells that are selected
Owner

ellisonbg commented Jan 22, 2012

@fperez: yes this is a bug and I think I know how to fix it. Related to all of the CodeMirror issues I am having.

@Carreau: yes, we will need to add the unminified source.

@fawce:

  • We should better understand why Ace is better or not. CodeMirror has improved a lot so I am not sure it makes sense to have a Ace mode. Also, we could make the editor a dialog which would show the original notebook cells in the background.
  • Maybe name the "Close" button "Return to Notebook" or something?
  • Only one Ace editor is allowed on the page, which makes it difficult to inject into the cells.
  • We should have multiple select.
  • The bugs in the Menu are on the jQueryUI side of things. We are using beta quality code and I expect it to improve over time.
Owner

fperez commented Jan 22, 2012

@ellisonbg, glad you can reproduce it. Let me know and I can keep on testing...

Contributor

fawce commented Jan 23, 2012

@ellisonbg - Return to Notebook sounds good to me.

ellisonbg added some commits Jan 17, 2012

@ellisonbg ellisonbg Updating to CodeMirror 2.2, latest stable release. 7d2dff1
@ellisonbg ellisonbg Fixing bugs that have shown up since updating CM to 2.2. 911dba0
@ellisonbg ellisonbg Work on the base Cell API.
* Added set_text/get_text in favor of get_code/set_code and
  get_source/set_source.
* Added methods to the base class (get_text, set_text, refresh,
  edit, render, toJSON, fromJSON).
f790de5
@ellisonbg ellisonbg Updating cell logic.
* Added placeholder to the base Cell class.
* Removed the \u0000 char at the beginning of the TextCell
  placeholder that was there for a CodeMirror bug workaround.
* Other refactoring of Cell related Notebook methods.
5281dc6
@ellisonbg ellisonbg Refactoring of the notebooks cell management.
* Cell insertion code completely rewritten. We now have two methods
  rather than 9: insert_cell_above, insert_cell_below.
* Renaming methods to be more consistent.
* Creating new methods to provide a uniform API.
* Minor bug fixes.
fd82b35
@ellisonbg ellisonbg Lots of small notebook improvements.
* Merge/split works for all cell types.
* Notebook.select won't select an already selected cell.
* Bugs in markdown cell editing fixed.
* border-box-sizing used for Markdown cells to get correct css boxes.
* Shading/border added to Markdown cell editor.
357e3e6
@ellisonbg ellisonbg Ace editor now works with Markdown and HTML cells with proper modes. cbd3102
@ellisonbg ellisonbg Start the pager out collapsed. 9ad2978
@ellisonbg ellisonbg Optimizing notebook loading. 197288e
@ellisonbg ellisonbg Fixing bugs in CodeMirror refreshing. 5caff73
Owner

ellisonbg commented Jan 23, 2012

OK I think I have fixed the main bugs, please test this branch on all platforms/all browsers.

Owner

ellisonbg commented Jan 23, 2012

The bug on Safari is with Ace. It looks like there is a fix for this, but we need to decide if we want to keep using Ace.

ellisonbg referenced this pull request Jan 23, 2012

Closed

Insertcell #1314

Owner

ellisonbg commented Jan 24, 2012

I am going to merge this. I am pretty sure I have resolved all of the non-Ace related bugs, which are also in master. I have 3 other branches whose progress depends on this branch and things are getting impossible to manage and move forward. We will need to figure out the Ace stuff in a separate branch.

@ellisonbg ellisonbg Fixing auto-indent issues in CodeMirror config.
* Block-level indent/dedent works.
* Tab in docstrings works.
4e3ae12

@ellisonbg ellisonbg added a commit that referenced this pull request Jan 24, 2012

@ellisonbg ellisonbg Merge pull request #1303 from ellisonbg/updatecm
This updates CodeMirror and refactors a good bit of the notebook code related to it.

* Updated CodeMirror to the latest stable release.
* Fix numerous bugs related to the CM update.
* Refactored the Cell API and the notebook's cell handling methods.
* Generalized split/merge to work with all cell types.
* Generalized "Edit in Ace" to work with all cell types.
* Loading optimizations: pager starts out hidden, faster loads.
* Shading added to Markdown and HTML cells when they are being edited.
* This branch will require solid usability testing on Safari, FF and Chrome before merging.
* Fixed a number of CM related bugs.
c585349

@ellisonbg ellisonbg merged commit c585349 into ipython:master Jan 24, 2012

Owner

Carreau commented Jan 24, 2012

Question,
I've just seen that you now have each notebook saved date, but for loaded notebook, it is the date/time at which it was loaded. Is it planed to changed to the "Real" date of saving ? Or at least the start with a "Loded on January..." then changed to "last saved..." ?

Otherwise splitting/merging cell is really cool, but I tend to search for it in the 'Cell' menu.
still completly broken with safari...

Owner

Carreau commented Jan 24, 2012

Replacing all use strict in IPython/frontend/html/notebook/static/ace/ace.js by empty string seem to work fir safari.
One of them might be 'leaking' into global namespace

Owner

ellisonbg commented Jan 24, 2012

Yes, the fix is easy, but I think we are going to move away from Ace...

On Tue, Jan 24, 2012 at 2:27 AM, Bussonnier Matthias
reply@reply.github.com
wrote:

Replacing all use strict in IPython/frontend/html/notebook/static/ace/ace.js by empty string seem to work fir safari.
One of them might be 'leaking' into global namespace


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

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

@mattvonrocketstein mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

@ellisonbg ellisonbg Merge pull request #1303 from ellisonbg/updatecm
This updates CodeMirror and refactors a good bit of the notebook code related to it.

* Updated CodeMirror to the latest stable release.
* Fix numerous bugs related to the CM update.
* Refactored the Cell API and the notebook's cell handling methods.
* Generalized split/merge to work with all cell types.
* Generalized "Edit in Ace" to work with all cell types.
* Loading optimizations: pager starts out hidden, faster loads.
* Shading added to Markdown and HTML cells when they are being edited.
* This branch will require solid usability testing on Safari, FF and Chrome before merging.
* Fixed a number of CM related bugs.
89c8576
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment