Commenting function. #1790
Commenting function. #1790
Conversation
|
Hi, Thanks for the Pull Request ! Usually, we try not to directly modify Also, usually, we prefere to have different fix in different PR, it allows us to merge one fix even if we don't agree (yet) on the implementation of another one. pinging @ellisonbg as he should be consulted for the notebook. |
| @@ -95,6 +95,10 @@ var IPython = (function (IPython) { | |||
| $(this).dialog('close'); | |||
| } | |||
| } | |||
| }).keyup(function(e){ | |||
| if( e.keyCode == 13 ){ | |||
There was a problem hiding this comment.
Can you document what this does in a comment. Also rather than using hard coded number like 13 can you use the constants in IPython.utils.js? Actually I think those haven't been merged yet, but will be soon - so maybe let's wait on merging this until that other PR (#1711) is merged.
There was a problem hiding this comment.
I had the same reflex, about IPython.utils.js , i think we should merged it to have feedback as fast as possible.
|
I agree with @Carreau that we should not directly make changes to CodeMirror in our code base. Can you submit this upstream to CodeMirror and then we can update from them? |
|
Hi. Okay, i will open another one when will be ready or reopen this. |
|
@Carreau What if i will use codecell.js for comment/uncomment function? |
|
I am fine if you want to add the commenting feature to code cell. On Wed, May 30, 2012 at 10:49 AM, ak3n
Brian E. Granger |
|
I'm also fine if the commenting code is in codecell. |
|
So, i will wait for #1711. |
|
OK #1711 is in. Can you use the keycodes in |
| @@ -153,6 +153,16 @@ var IPython = (function (IPython) { | |||
| } else { | |||
| return false; | |||
| }; | |||
| } else if (event.keyCode === 191 && event.ctrlKey && event.type == 'keydown') { | |||
| // if Ctrl+/ is pressed then comment/uncomment selected lines. | |||
| for (i = editor.getCursor(true).line;i < editor.getCursor(false).line + 1; i++) { | |||
There was a problem hiding this comment.
When I try to use this code I get an error that i is not defined. We need a var i somewhere.
|
I'll just need to try it on a non english keyboard (slash is accessed by pressing SHIFT on french for example), and then the logic might fail. Otherwise look ok. |
|
It seem to don't care wether shift is pressed or not, so the shortcut for english keyboard is both (C-/ and C-Shift-?). Also, you can't comment lines if you already have comments ... toggle with I would go for a logic :
What do you think ? Also, try not to "merge" upstream/master, otherwise the history get messy. If it don't apply cleanly, we prefere a rebase, don't hesitate to ask for help if you need. Thanks. Pinging @ellisonbg to have his thought on that. |
|
I agree that when some lines are commented and others are not and the user attempts to comment/uncomment the section, you should look at what is done on the first line. If it is uncommented, comment the block and vice-versa. I also notice that the selection logic is wonky. When I use Shift and arrow keys to select a block of text and then comment that block, The line after the block is also affected. I think the selection logic is pulling the trailing |
No description provided.