New Tooltip, New Completer and JS Refactor #1711
Conversation
|
OK thanks for creating this branch. I am going to be working in it directly over the next few days. |
|
@Carreau how do you want to work on this branch so we don't step on each other. I could spend a day or so working on my parts and reviewing your and then you could work on yours. How does that sound? |
|
@ellisonbg, I can also push on my repository and send you PRs. |
|
Please let me know when you have pushed your changes so I can start to On Tue, May 8, 2012 at 10:11 AM, Bussonnier Matthias
Brian E. Granger |
|
Pushed 2h ago. Sorry, it starting to be late here, won't work on it for at least 18h. |
|
Awesome thanks for your work! On Tue, May 8, 2012 at 11:59 AM, Bussonnier Matthias
Brian E. Granger |
|
Thanks to you for the new JS interface to communicate with the kernel, it's great, and for all the notebook in general. |
|
@minrk, OK I have added the options to |
| // callbacks = { | ||
| // 'complete_reply': complete_reply_callback | ||
| // } | ||
| // |
There was a problem hiding this comment.
I was wondering, as there is only one argument for the callbacks, could we not directly build it ourself and use complete_reply_callback as an argument of the function ? We can even support both by looking at wether the third argument given to the function has a complete_reply key or not...
Same for object_info_request I think
There was a problem hiding this comment.
The reason that I went with this is that JavaScript doesn't support
optional keyword arguments. So for functions like execute that take
multiple optional callbacks, there really isn't a way to do something
like:
def execute(code, execute_reply=None, cell=None, output=None, ...)
Some of the other methods doesn't need multiple callbacks, but I
wanted to make a uniform interface, so I went with a callback struct.
On Wed, May 9, 2012 at 9:02 AM, Bussonnier Matthias
reply@reply.github.com
wrote:
return msg.header.msg_id;
};
- Kernel.prototype.complete = function (line, cursor_pos) {
- Kernel.prototype.complete = function (line, cursor_pos, callbacks) {
- // When calling this method pass a callbacks structure of the form:
- //
- // callbacks = {
- // 'complete_reply': complete_reply_callback
- // }
- //
I was wondering, as there is only one argument for the
callbacks, could we not directly build it ourself and usecomplete_reply_callbackas an argument of the function ? We can even support both by looking at wether the third argument given to the function has acomplete_replykey or not...Same for
object_info_requestI think
Reply to this email directly or view it on GitHub:
https://github.com/ipython/ipython/pull/1711/files#r795774
Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com
|
@Carreau some feedback on the tooltip: First, some related to the visual consistency:
Some related to the different modes:
|
Changed, cf screenshot
Tested on Chrome or safari osx I guess... on firefox and other OS you get a scrollbar in those 15 pixels if the text is longer than the pager The scroll bar is hidden when tooltip is not expanded so I can remove those 15px then, but I have to add them it is expanded, otherwise it overlaps and clicking on the 'cross+scrollbar' get weird and have unexpected behaviour. I havent found any good ways to 'detect' the scrollbar.
Sure, when the clock is there you can continue typing and the pager don't dismiss for 10s (I call it sticky mode) Unless of course, I ask for tooltip on another function.
So do I keep "sticky mode" and "15px" ? |
|
Pager problem updated on my branch with same name as this one. |
|
On Wed, May 9, 2012 at 11:13 AM, Bussonnier Matthias
Great, I like this look much better.
Ahh, I see then leave it as is.
Ahh, OK is there a way we could indicate to the user what the
OK great.
Yes, I think so, unless we wanted to make sticky mode on by default.
Brian E. Granger |
|
OK I tested out your branch and the tooltip looks pretty good. The behavior is pretty smooth and intuitive and the visual look is clean. When you are ready, can you submit a PR against this branch and I will merge it in. I will have further comments coming though. |
|
Also, please check |
| * have some strange ccs to have the scroll bar on | ||
| * the left of the left with fix button on the top right of the tooltip | ||
| */ | ||
| @-moz-keyframes fadeOut { |
There was a problem hiding this comment.
jQuery/jQueryUI have the ability to animate css properties, can we use that instead of this css? I think it will give us better cross browser coverage. If you don't think it makes sense that is fine, but at least look at it.
There was a problem hiding this comment.
Well when supported css3 is native, so much faster and less load on JS VM. And as tooltip fading is not that important I thought it was a good trade not o have it if not supported... I can try with jquery
For security reasons, the kernel should not be started until after the notebook content is completely loaded and on the page. This prevents people from creating notebooks that run nasty code on the users machine at load time. In order to implement this, we had to create a CodeCell.set_kernel method that allows the kernel attribute of a CodeCell to be set at a later time. This also fixes some error messages we were seeing related to the kernel's channels not being setup properly when a send was attempted.
This reverts commit 8af188d.
it seem that show and hide methods fron tooltip where collinding with jquery, sometime blocking the notebook. Move from css fadeIn/Out to jQuery fade In/Out, and replace addRemoveClass by fadeIn/fadeOut
check that the cell the tooltip is giving focus back is the currently selected cell
When kernel is died and restarted, or restarted while it is in the busy state, message "(Busy)" on the window title is not updated. This problem is fixed by updating document title when restarting.
This allows code outside notebook.js to set the dirty flag, but doesn't require that code depend on notebook.js.
|
OK, I have done everything talked about in this review. I think it is ready for merging - this a huge, long-lived branch that should get merged sooner rather than later. Please let me know if you see anything else that needs work. |
|
OK, I'm merging this: at this point, delaying it any further only increases the risk of nasty conflicts, and we're hitting the point of diminishing returns. We can always (and we will) keep improving after it's merged. A huge thank you and kudos to @Carreau and @ellisonbg for the amount of effort you guys put into this, and for the great teamwork here. Thanks also to @tkf and everyone else who pitched in with code, feedback, etc. It's a real pleasure to work on a project with a team like this. On to the widgets!!!!!!!!!! |
New Tooltip, New Completer and JS Refactor. This is a major reworking of lots of notebook client code, both introducing new features and allowing certain things to be done more cleanly: - New graphical tooltip with keyboard control: successive presses of the TAB key will expand it, pin it for 10 s (a clock icon is shown), and send its content to the bottom help pager (which is now resizable). - Completer has been refactored into a new class and now can analyze the current cell to support completions on objects that don't exist in the kernel yet (because the cell hasn't been executed). - All the client-side JavaScript has been decoupled to ease reuse of parts of the machinery without having to build a full-blown notebook. This will make it much easier to communicate with an IPython kernel from existing web pages and to integrate single cells into other sites, without loading the full notebook document-like UI. - This refactoring also enables the possibility of writing dynamic javascript widgets that are returned from Python code and that present an interactive view to the user, with callbacks in Javascript executing calls to the Kernel. This will enable many interactive elements to be added by users in notebooks. An example of this capability has been provided as a proof of concept in `docs/examples/widgets` that lets you directly communicate with one or more parallel engines, acting as a mini-console for parallel debugging and introspection. Closes #1498.
New Tooltip, New Completer and JS Refactor. This is a major reworking of lots of notebook client code, both introducing new features and allowing certain things to be done more cleanly: - New graphical tooltip with keyboard control: successive presses of the TAB key will expand it, pin it for 10 s (a clock icon is shown), and send its content to the bottom help pager (which is now resizable). - Completer has been refactored into a new class and now can analyze the current cell to support completions on objects that don't exist in the kernel yet (because the cell hasn't been executed). - All the client-side JavaScript has been decoupled to ease reuse of parts of the machinery without having to build a full-blown notebook. This will make it much easier to communicate with an IPython kernel from existing web pages and to integrate single cells into other sites, without loading the full notebook document-like UI. - This refactoring also enables the possibility of writing dynamic javascript widgets that are returned from Python code and that present an interactive view to the user, with callbacks in Javascript executing calls to the Kernel. This will enable many interactive elements to be added by users in notebooks. An example of this capability has been provided as a proof of concept in `docs/examples/widgets` that lets you directly communicate with one or more parallel engines, acting as a mini-console for parallel debugging and introspection. Closes ipython#1498.
On request of @ellisonbg, I pushed a branch that regroups 2 others PR : #1509 (New tooltip and Completer) #1697 (JS refactor) that were conflicting and both changing some logic.
This makes the 2 compatible and working... It still need some spring cleaning to be sure there is no orphan methods, useless code or simplification possible due to new JS refactoring.