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

Switch to CodeMirror 4.6.0 #6221

Merged
merged 11 commits into from Oct 13, 2014

Conversation

Projects
None yet
7 participants
@Carreau
Member

Carreau commented Jul 27, 2014

Update to CodeMirror 4

That mean multiple cursor ! Yeaaah.

On the dark side, we have to painfully recode ipythongfm and ipython mode (I'll appreciate help) . (Done)

A few things have changed, in particular event.stop() does not exist, and I had to comment the goLineUp and goLineDown in kerboardmanager as returning true/falseseem not to be the way to go anymore to prevent CM from handeling the events ( @ellisonbg your input on that )

Completer does not intercept up/down anymore...

Todo:

  • update to cm 4.6
  • fix completer
  • search for "somethingSelected" add "or MultipleCursors" (like completion)
  • fix shift tab 4 time select previsous cell
  • fix tests.
  • remove deprecated logic since CM 2.x
  • completer : insert at all cursors.
  • autohighlight (Carreau#10)

But globally it seem to work.

@Carreau Carreau added this to the 3.0 milestone Jul 27, 2014

@ellisonbg

This comment has been minimized.

Member

ellisonbg commented Jul 27, 2014

How is your thesis coming ;-)

On Sun, Jul 27, 2014 at 12:44 PM, Matthias Bussonnier <
notifications@github.com> wrote:

Update to CodeMirror 4

That mean multiple cursor ! Yeaaah.

On the dark side, we have to painfully recode ipythongfm and ipython mode
(I'll appreciate help) .

A few things have changed, in particular event.stop() does not exist, and
I had to comment the goLineUp and goLineDown in kerboardmanager as
returning true/falseseem not to be the way to go anymore to prevent CM
from handeling the events ( @ellisonbg https://github.com/ellisonbg
your input on that )

Completer does not intercept up/down anymore...

But globally it seem to work.

You can merge this Pull Request by running

git pull https://github.com/Carreau/ipython cm4

Or view, comment on, or merge it at:

#6221
Commit Summary

  • working loading codemirror
  • working codemirror !
  • fix completer tooltip and double linedown

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#6221.

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

@Carreau

This comment has been minimized.

Member

Carreau commented Jul 27, 2014

@Carreau

This comment has been minimized.

Member

Carreau commented Jul 27, 2014

(defense on Sept. 16th, if you are around)

@damianavila

This comment has been minimized.

Contributor

damianavila commented Jul 27, 2014

Can we make comments on your Thesis too? Interesting stuff... you have awaken a little bit my biological background... just a little bit... jeje

<script src="{{ static_url("components/codemirror/mode/markdown/markdown.js") }}" charset="utf-8"></script>
<script src="{{ static_url("components/codemirror/mode/python/python.js") }}" charset="utf-8"></script>
<script src="{{ static_url("notebook/js/codemirror-ipython.js") }}" charset="utf-8"></script>
<script src="{{ static_url("notebook/js/codemirror-ipythongfm.js") }}" charset="utf-8"></script>

This comment has been minimized.

@jdfreder

jdfreder Jul 28, 2014

Member

cheers this is great!

@@ -156,7 +156,7 @@ define([
return false;
} else {
var cm = cell.code_mirror;
cm.execCommand('goLineDown');
//cm.execCommand('goLineDown');

This comment has been minimized.

@jdfreder

jdfreder Jul 28, 2014

Member

Why not just delete the else block entirely?

This comment has been minimized.

@Carreau

Carreau Jul 28, 2014

Member

I don't want to remove goLineUp/Down before I understand why it works :-p

Envoyé de mon iPhone

Le 28 juil. 2014 à 04:45, Jonathan Frederic notifications@github.com a écrit :

In IPython/html/static/notebook/js/keyboardmanager.js:

@@ -156,7 +156,7 @@ define([
return false;
} else {
var cm = cell.code_mirror;

  •                    cm.execCommand('goLineDown');
    
  •                    //cm.execCommand('goLineDown');
    
    Why not just delete the else block entirely?


Reply to this email directly or view it on GitHub.

CodeMirror.defineMode("ipythongfm", function(config, parserConfig) {
//console.log('defining ipython gfm');

This comment has been minimized.

@jdfreder

jdfreder Jul 28, 2014

Member

Don't forget to remove these debug comments before merge.

This comment has been minimized.

@Carreau

Carreau Jul 28, 2014

Member

Anyway I'll hate to rewrite Ipythongfm and Ipython from scratch :-( so Pr far from ready, and it depends on FA PR and Momentjs PR (at least the Ipython component part)

Envoyé de mon iPhone

Le 28 juil. 2014 à 04:47, Jonathan Frederic notifications@github.com a écrit :

In IPython/html/static/notebook/js/codemirror-ipythongfm.js:

     CodeMirror.defineMode("ipythongfm", function(config, parserConfig) {
  • //console.log('defining ipython gfm');
    Don't forget to remove these debug comments before merge.


Reply to this email directly or view it on GitHub.

@Carreau

This comment has been minimized.

Member

Carreau commented Jul 28, 2014

Of course you can comment on my phd, you can even open PRs :-)

Envoyé de mon iPhone

Le 28 juil. 2014 à 00:10, Damián Avila notifications@github.com a écrit :

Can we make comments on your Thesis too? Interesting stuff... you have awaken a little bit my biological background... just a little bit... jeje


Reply to this email directly or view it on GitHub.

@Carreau

This comment has been minimized.

Member

Carreau commented Jul 28, 2014

ipython and ipython-gfm mode restored.

@@ -36,10 +41,17 @@ require([
notificationarea,
savewidget,
keyboardmanager,
config
config,
custom,

This comment has been minimized.

@jdfreder

jdfreder Jul 28, 2014

Member

Two things here:

  • Custom.js should be loaded as the last module
  • CodeMirror is only used by Cell and it's derived classes, therefor it should be required in Cell not in main.
@Carreau

This comment has been minimized.

Member

Carreau commented Jul 31, 2014

@ellisonbg I'm assigning you to take a look at the keyboard stuff with new CodeMirror.
I'll be mostly busy for the next few days in trying to get my computer fixed.

@Carreau

This comment has been minimized.

Member

Carreau commented Aug 5, 2014

@ellisonbg I'm sorry to disturb you, but you tend to return true/false everywhere in your keyboard manager, and it bubble all the way up to... nothing. Is there any reason to return true/false ?

@Carreau

This comment has been minimized.

Member

Carreau commented Aug 5, 2014

Hum, apparently is is not possible to prevent codemirror to do action and up/down arrowkeys, but it does work on other chars. So, unless we listen for codemirror event also, there is no way to get exactly the old behavior at top and/or bottom of the cells.

@ellisonbg

This comment has been minimized.

Member

ellisonbg commented Aug 6, 2014

I thought we were returning true/false and eventually returning that from the jquery event handler - that determines how other handlers should treat the event. If that is not happening, something odd is going on.

Can we submit a bug to CM about the up/down keys? It is pretty important to have the same behavior.

@Carreau

This comment has been minimized.

Member

Carreau commented Aug 6, 2014

I thought we were returning true/false and eventually returning that from the jquery event handler

I'll re-check, there was a lot of layers in the code.

Can we submit a bug to CM about the up/down keys? It is pretty important to have the same behavior.

No, we capture event on "$(document)", and event bubble up. Thus KeyboadManager handler is the last to treat the event. There is no reason for CodeMirror to react on that as it batched all its operations before.
Marijnh will respond to you that codemirror is already triggerring event "beforeChange" if you want to cancel them.

Thinking about it, we might be able to go around that using our code, but it will be quite tough IMHO in case like @ivanov vimception mode.

@jdfreder

This comment has been minimized.

Member

jdfreder commented Aug 14, 2014

Hey @Carreau what's the status of this?

@Carreau

This comment has been minimized.

Member

Carreau commented Aug 14, 2014

Still away until this week-end.

Envoyé de mon iPhone

Le 14 août 2014 à 20:58, Jonathan Frederic notifications@github.com a écrit :

Hey @Carreau what's the status of this?


Reply to this email directly or view it on GitHub.

@Carreau

This comment has been minimized.

Member

Carreau commented Aug 17, 2014

@ellisonbg I would still like you input on how the keyboard handler is supposed to work.

@ellisonbg

This comment has been minimized.

Member

ellisonbg commented Aug 18, 2014

Ok

Sent from my iPhone

On Aug 17, 2014, at 8:40 AM, Matthias Bussonnier notifications@github.com wrote:

@ellisonbg I would still like you input on how the keyboard handler is supposed to work.


Reply to this email directly or view it on GitHub.

@Carreau Carreau force-pushed the Carreau:cm4 branch from 451cc9e to 35fc3dd Aug 23, 2014

@Carreau

This comment has been minimized.

Member

Carreau commented Aug 23, 2014

rebased and switched to codemirror 4.5

@Carreau Carreau changed the title from Cm4 to Switch to CodeMirror 4.5.0 Aug 23, 2014

@Carreau Carreau force-pushed the Carreau:cm4 branch from 35fc3dd to de4fc21 Sep 20, 2014

@SylvainCorlay

This comment has been minimized.

Member

SylvainCorlay commented Oct 10, 2014

I am using this with no problem so far besides the editor is not defined that Min corrected.

@Carreau

This comment has been minimized.

Member

Carreau commented Oct 10, 2014

Some cleaning update, I'll rebase later as it does not merge cleanly.

Carreau and others added some commits Jul 27, 2014

Update to codemirror 4
Update to codemirror 4.6 and update most notebook to work wiht it
this include keyevent that where triggerd twice and the re-writing of
ipython-gfm anf ipython mode to work with require.
use require to load CodeMirror modes
CM's loadmode addon only works if you *don't* load CodeMirror itself with require.

@Carreau Carreau force-pushed the Carreau:cm4 branch from 70f1fef to b725fa9 Oct 10, 2014

@minrk

This comment has been minimized.

Member

minrk commented Oct 13, 2014

I've been using this for a couple of days, and I haven't seen any more issues. 👍 to merge and get more in-the-wild testing in master.

@minrk minrk added Ready To Merge and removed -Needs review labels Oct 13, 2014

@damianavila

This comment has been minimized.

Contributor

damianavila commented Oct 13, 2014

I agree, w/o any major issues since last rebase...

@jdfreder

This comment has been minimized.

Member

jdfreder commented Oct 13, 2014

I just checked out the latest, and can also 👍 that it still works well here.

@jdfreder

This comment has been minimized.

Member

jdfreder commented Oct 13, 2014

Thanks @Carreau !

jdfreder added a commit that referenced this pull request Oct 13, 2014

Merge pull request #6221 from Carreau/cm4
Switch to CodeMirror 4.6.0

@jdfreder jdfreder merged commit 098e950 into ipython:master Oct 13, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@jdfreder

This comment has been minimized.

Member

jdfreder commented Oct 13, 2014

For people interested in using the Sublime Text keyMap with the IPython Notebook (including select similar, ctrl-d or cmd-d on a Mac), add the following Javascript code to your custom.js:

require(["codemirror/keymap/sublime", "notebook/js/cell"], function(sublime_keymap, cell) {
    cell.Cell.options_default.cm_config.keyMap = 'sublime';
});

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

@Carreau Carreau deleted the Carreau:cm4 branch Dec 15, 2014

@sglyon

This comment has been minimized.

sglyon commented Mar 6, 2015

@jdfreder, I tried adding this inside $([IPython.events]).on('app_initialized.NotebookApp', function(){ in my custom.js, but had no luck.

Am I supposed to put is somewhere different?

@Carreau

This comment has been minimized.

Member

Carreau commented Mar 6, 2015

$([IPython.events])

Will not be define you need to

require([
        'base/js/namespace',
        'base/js/events'
    ], function(IPython, events) {

.....

});
@minrk

This comment has been minimized.

Member

minrk commented Mar 6, 2015

Yes, though it's a bug that $([IPython.events]) isn't defined.

@jdfreder

This comment has been minimized.

Member

jdfreder commented Mar 10, 2015

That snippet sits in my custom.js just as I pasted it, no reference to the events object. Require.js is enough.

@sglyon

This comment has been minimized.

sglyon commented Mar 10, 2015

Excellent. I have put it at the top level of my custom.js and it works.

Thanks.

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