Skip to content
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

Added new extension snippets_menu #827

Merged
merged 6 commits into from Jan 10, 2017
Merged

Added new extension snippets_menu #827

merged 6 commits into from Jan 10, 2017

Conversation

moble
Copy link
Contributor

@moble moble commented Jan 2, 2017

@jcb91 suggested here that jupyter_boilerplate should be moved into this repo. This PR is an attempt to do so. This extension appears to be different from @adrn's snippets extension in several ways:

  1. The menu is placed in the menu bar, rather than the toolbar.
  2. The ability to create nested menus allows for a geometric explosion in the number of snippets available, without much added complexity.
  3. It comes preloaded with a default menu that is intended to be a fairly comprehensive collection of snippets for common modules (or at least, ones that I use).
  4. Insertions are made at the cursor position, rather than insertions of entire cells.
  5. Tooltips show the code that will be inserted.

I've renamed this extension snippets_menu to be close enough to @adrn's extension to prompt comparisons, while also being obviously distinct.

A number of problems should probably be addressed before it is merged — mostly having to do with the fact that I don't understand the order of operations.

  1. Would it be possible to ensure that load_ipython_extension is called only after the usual Jupyter menus have been created (and specifically the Help menu item)? At the moment, I've just thrown in a lot of requires, and it seems to work out, though I'm not sure it always will.

  2. How can a function be written in custom.js that is guaranteed to be called only after load_ipython_extension has finished manipulating the menus? Many of the examples I've written in the readme.md (which are basically copied into examples_for_custom.js) could benefit from this.

  3. Would it be possible to run custom.js, and then check again to ensure that if nothing was added by custom.js, at least the default is added? The use of remove_top_level_snippets_menu_items in my custom examples is pretty hacky, and could be much more elegant if this were possible.

  4. Probably plenty of other problems related to the fact that this is my first JS code, and I'm not really sure how nbextensions are supposed to work...

@moble moble changed the title Migrate moble/jupyter_boilerplate to jupyter_contrib_nbextensions Added new extension snippets_menu (migrating from moble/jupyter_boilerplate) Jan 2, 2017
@moble moble changed the title Added new extension snippets_menu (migrating from moble/jupyter_boilerplate) Added new extension snippets_menu Jan 2, 2017
Originally copied from `zenmode`, these are obviously useless.
@adrn
Copy link
Contributor

adrn commented Jan 3, 2017

Actually I think this is quite a bit nicer than the hack I threw together -- what do you think about renaming this to snippets instead? I'd be happy to remove mine from this repo... That said, I have some opinions about the built-in menus you've added. I opted to make the user adds their own snippets because we all have our own styles / conventions and wants / needs (especially in terms of the complexity of the snippets). Obviously, in terms of complexity, everyone probably agrees there is a floor which is like "don't add things that are so trivial we'd just be reproducing the full set of syntax for the language," but I think everyone draws the complexity threshold above that in different places. For example, I would never want a snippet to drop in an astropy constant because I'd do:

from astropy.constants import G, M_sun
print(G * M_sun)

instead of print(astropy.constants.G * astropy.constants.M_sun).

What do you think about leaving a few built-in snippets as examples of the types of things a user might want to automate (e.g., imports, plotting stuff, class template, etc.), and make it really easy to edit / add snippets? I don't have a specific idea on how to do that because the JS in Jupyter confuses me, but others might have ideas! But in any case, I would vote to not ship with all of the current built-in menus, but I suppose I don't feel very strongly because I can always delete things locally...

cc @jcb91

@jcb91
Copy link
Member

jcb91 commented Jan 3, 2017

As far as differences in the two go:

  1. The menu is placed in the menu bar, rather than the toolbar.

I like the menubar, but others may like the toolbar, or both. We can make this choice configurable through the jupyter_nbextensions_configurator.

  1. The ability to create nested menus allows for a geometric explosion in the number of snippets available, without much added complexity.

Yes, this is neat, let's keep this idea

  1. It comes preloaded with a default menu that is intended to be a fairly comprehensive collection of snippets for common modules (or at least, ones that I use).

So, as @adrn mentions, he doesn't like some of the snippets. I think having a broad selection is quite useful, but again, as @adrn mentions, this is largely a matter of taste. Perhaps we could have another configurable option to add (or not) the more extensive set of builtin snippets. Although the user can certainly edit the json themselves, the configuration option can make it a little more user-friendly for common operations. Not sure, I'd have to think about how to implement that.

  1. Insertions are made at the cursor position, rather than insertions of entire cells.

I like this more than adding whole cells, so would be inclined to have it do that, but then others may prefer cells. Configurable again, I guess 😉

  1. Tooltips show the code that will be inserted.

Neat. Keep this.


make it really easy to edit / add snippets?

Yeah, this is a nice idea, but I'm not sure exactly how to implement it. The configurator doesn't handle json specially at the moment, and even then that wouldn't be the most intuitive for the user. We could build a form for editing snippets, accessible as a menu item, and then use the notebook config API to store the json, I guess, although I'm not immediately clear how the more complex system of menus in the boilerplate nbextension would fit into that model. Something to think about, I guess...


problems to be addressed:

  1. Would it be possible to ensure that load_ipython_extension is called only after the usual Jupyter menus have been created (and specifically the Help menu item)? At the moment, I've just thrown in a lot of requires, and it seems to work out, though I'm not sure it always will.

Yes, the DOM elements are hardcoded parts of the html page, so they should all be present once notebook extensions are loaded. Even the menu's javascript event handlers should also be bound before any extensions are loaded (see menu js instantiation in notebook/js/main.js#L102-L109, and then the nbextension loading later at notebook/js/main.js#L171-L175

  1. How can a function be written in custom.js that is guaranteed to be called only after load_ipython_extension has finished manipulating the menus? Many of the examples I've written in the readme.md (which are basically copied into examples_for_custom.js) could benefit from this.

The only ways I can think of to do this are

  • to define the function in custom.js without actually calling it, then rely on the extension to call it at the appropriate moment
  • to bind the function as a handler for a custom event in custom.js, then have the extension fire that event

But I'm not really sure that it's necessary. I think a better solution will be to store the snippets using the notebook's config API, then load them in the extension.

  1. Would it be possible to run custom.js, and then check again to ensure that if nothing was added by custom.js, at least the default is added? The use of remove_top_level_snippets_menu_items in my custom examples is pretty hacky, and could be much more elegant if this were possible.

Well, the most obvious way to do this is to have the nbextension expose its menus, then you can edit the value directly in custom.js before the load_ipython_extension method is called by the notebook. Alternatively, have the nbextension attempt to use a variable (something like window._boilerplate_menus would work) then, if it's undefined, use the defaults instead.

  1. Probably plenty of other problems related to the fact that this is my first JS code, and I'm not really sure how nbextensions are supposed to work...

We'll find those as we go, I expect 😄

I think the major change to make is where to load the snippet json from, so that we can avoid using custom.js at all. I'll have a think about how to edit/add snippets

@moble
Copy link
Contributor Author

moble commented Jan 4, 2017

I think I've mostly solved the issues brought up above. I feel really strongly that we should keep all of the current snippets as options, but I agree that it needs to be easier to remove unwanted submenus, and to add custom menus. At the moment it requires some JS skills and custom.js to do anything beyond the default. So I've added options to the jupyter_nbextensions_configurator to

  1. Insert as new cells, rather than the default of at the cursor in the current cell.
  2. Include a custom menu at the top of the default menu.
  3. Edit the custom menu (as JSON in a textarea).
  4. Deselect any of the built-in sub-menus (numpy, scipy, etc.), which are all on by default.

I think this is roughly as much customizability as will be possible in this way, since it appears to just use the basic YAML file to create the configuration section — though I'd be happy for anyone to point out ways to really edit things. But I've also restructured things to make the extension easier to customize through custom.js, and I've updated all the examples in the README that explain how to do so. I think I've also figured out how dependencies work, so that all looks good to me.

Things I haven't addressed:

  1. I haven't added an option to put the menu in the toolbar, because I'm not sure if it's possible to have the hierarchical format that's possible from the menu bar. Comments?
  2. I haven't renamed it to snippets as adrn suggested, to leave some time to firm up all our ideas and come to consensus, so I don't step on any toes. :) But if we all agree to that, I have no problem with it.
  3. Maybe something else I'm forgetting?

@adrn. Just to explain my thinking about these huge menus a little more:

I would never want a snippet to drop in an astropy constant because I'd [...never do...] print(astropy.constants.G * astropy.constants.M_sun)

Neither would I, but I like having all those constants listed to remind me of what constants are available, and how exactly they're named — e.g., M_sun or M_Sun or MSun, which are all names in packages I've used. (Actually, I just use the solar mass parameter G*M_sun, because it's known to far higher accuracy than either of its factors, but you get the idea.) Same deal with all those special functions in scipy and sympy. At least when I first wrote this extension, those menus were exhaustive lists of everything in all these modules, which saves me from having to go to the doc pages and click through lots of places to see what was available.

But again, since it's now much easier to deselect sub-menus, I don't think having huge default menus is as much of an issue.

@jcb91
Copy link
Member

jcb91 commented Jan 5, 2017

Nice job @moble, looks good to me :)

I actually started similar work in a fork of the original repo, but haven't pushed anything for anyone to see yet, so you win that one 😉

Incidentally, if you're working on top of your original repo, then we could merge those commits in with some renames to keep the two copies in step, which worked nicely with scratchpad.

I think this is roughly as much customizability as will be possible in this way, since it appears to just use the basic YAML file to create the configuration section — though I'd be happy for anyone to point out ways to really edit things

Yeah, this is about as much as is easy to do without building a full javascript-based editor form. I was thinking of something based on something like https://github.com/vakata/jstree, but it'd be a relatively major hassle, so I think this is good at least for the foreseeable.

  1. I haven't added an option to put the menu in the toolbar, because I'm not sure if it's possible to have the hierarchical format that's possible from the menu bar. Comments?

Yes, I've had that thought too. I'm starting to think that the two are serving different use-cases, and certainly for the toolbar case, it seems less sensible to have a huge set of snippets, and definitely not easy to have a hierarchical structure. Perhaps we'd be better off keeping the simpler case as toolbar-only in a separate nbextension. The other alternative would be to kep them together, but using different config values for their snippet sets, and ignore and submenus in the toolbar. It would be nice to have them both accept the same object structure for menus, at any rate.

  1. I haven't renamed it to snippets as @adrn suggested, to leave some time to firm up all our ideas and come to consensus, so I don't step on any toes. :) But if we all agree to that, I have no problem with it.

Sure, In fact, in light of point 1, it might be better to keep the two separate-though-related

  1. Maybe something else I'm forgetting?

Nothing springs to mind!

@jcb91
Copy link
Member

jcb91 commented Jan 9, 2017

I've put a few alterations on the jcb branch of my fork of the standalone repo, (that is, https://github.com/jcb91/jupyter_boilerplate/tree/jcb). Mainly making things in options configurable. If you think they still need to be accessible, it'd be easy to export the config as part of the options structure (or even merge the two, and just expose one structure). I'm not sure the pre_config_hook is necessary, as anything in custom.js will (or at least can be made to) execute before the nbextension is loaded, but it's not a big deal.

@moble
Copy link
Contributor Author

moble commented Jan 9, 2017

Wow, that's much nicer looking code, alright. Thanks!

There are a few little bugs that I found:

  1. include_custom_menu should default to false for consistency with config.yaml
  2. custom_menu_content's default should have the outermost square brackets removed, since there can be only one custom menu (and for consistency with config.yaml).
  3. This definition of the "Snippets" menu should use sub-menu-direction instead of menu-direction.
  4. You should probably include around here some logic that uses the custom menu options, like this:
            if (cfg.include_custom_menu) {
                var custom_menu_content = JSON.parse(cfg.custom_menu_content);
                console.log(mod_log_prefix,
                            "Inserting custom", custom_menu_content.name, "sub-menu");
                options.menus[0]['sub-menu'].push(custom_menu_content);
            }
  1. The use of Snippets: in the log message is now redundant.
  2. The definition of sub_element is missing a closing double quote. I'm really surprised that didn't show up as an error in the JS console. It made it so that the entire Snippets menu just didn't appear.

@jcb91
Copy link
Member

jcb91 commented Jan 10, 2017

@moble haha good list! Ugh, that's what comes of doing my testing only before a bunch of rebases, amateurish of me. I'll be out of the loop for a few days now, so feel free to mess with that set of changes if you like, or I'll get around to it once I get back :)

@adrn
Copy link
Contributor

adrn commented Jan 10, 2017

I'll check this out after the next iteration - thanks @moble and @jcb91 !

@moble
Copy link
Contributor Author

moble commented Jan 10, 2017

Okay, @jcb91, I've merged your commits and those bugfixes. This is all looking just about ready to go, to me.

@juhasch
Copy link
Member

juhasch commented Jan 10, 2017

Looks good to me. Let's merge.

@juhasch juhasch merged commit e7a805b into ipython-contrib:master Jan 10, 2017
@moble
Copy link
Contributor Author

moble commented Jan 10, 2017

@juhasch Agh, sorry. We were talking about commits made on my repo, which I think we're now mostly happy with. They include a number of commits @jcb91 did to fix things up that haven't made it over here yet. I'm expecting that jcb has some git magic that he can perform to do so...

@jfbercher
Copy link
Member

Yes, @jcb91 has git magics!

Very nice extension, I knew it before and it was already very nice; and it has improved :-)

@juhasch
Copy link
Member

juhasch commented Jan 11, 2017

Sorry, if I was a little fast :-)
Would you like to open a new PR for the updates ow wait for @jcb91 ?

@moble
Copy link
Contributor Author

moble commented Jan 11, 2017

There's no rush from my end, so I'm happy to wait to see what jcb thinks would be easiest.

@jcb91
Copy link
Member

jcb91 commented Jan 13, 2017

Haha hi all. So, given that this is merged already, I think we may as well just add the upstream copy over the top of it, rather than attempting to rip this out of master and risking peoples histories diverging. I'll try to throw together a relevant PR now...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants