jupyterlab / frontends-team-compass

A repository for team interaction, syncing, and handling meeting notes across the JupyterLab ecosystem.
https://jupyterlab-team-compass.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
59 stars 30 forks source link

Merge JupyterLab-TOC to JupyterLab core #63

Closed lresende closed 3 years ago

lresende commented 4 years ago

As suggested in the TOC repo and discussed on the JupyterLab dev meeting on May 20, we will start the process to move TOC to JupyterLab core.

choldgraf commented 4 years ago

yessssss

afshin commented 4 years ago

:shipit: 💯 👍

jasongrout commented 4 years ago

I support moving TOC to core.

blink1073 commented 4 years ago

I also support moving it to core. I suggested in today's meeting that we bring it in as-is, and then make it not visible by default (to preserve space in the side bar), but easily enabled using a menu item that is backed by a setting.

ian-r-rose commented 4 years ago

Very cool! A few thoughts about the process of migrating into core, and why I have been a bit reticent about it in the past:

  1. There is currently registry pattern somewhat loosely modeled after the rendermime registry. We should make sure that this is exposed to extension authors with the ITableOfContentsRegistry token and the JLab standard @jupyterlab/toc/@jupyterlab/toc-extension division of responsibilities.
  2. There is some awkwardness around the API design for the ToC toolbar. There is no documentation about how to use it, and I'm not sure that there is anybody around who could easily generate it (I don't really understand it at the moment). They deserve to be rethought and cleaned up, whether before or after migration.
  3. The logic around hiding and collapsing notebook cells involves some DOM manipulation of things that don't belong to the extension (that is, notebook cells). This has proven confusing to users. I think there should be some unification with the native JupyterLab collapsing UX and cell metadata, but others have disagreed with me on this (cc @kgryte).
  4. Similarly, the auto-numbering functionality does some DOM manipulation that makes me uncomfortable. We might want to remove that.
  5. The current ToC extension blesses LaTeX documents as one of the doctypes which it knows how to generate a ToC for. Do we want to continue that?
  6. The HTML, Markdown, and LaTeX ToC generators currently have some hand-written best-effort regexes for identifying headers. Obviously, I should never have done this, but here we are! Do we want to revisit that?
  7. The ToC itself could probably use a design/CSS pass (:wave: @tgeorgeux )
  8. The current extension has functionality for previewing markdown and code cells in the ToC itself. While a cool bit of hacking, I kind of feel that it is a bit antithetical to the purpose of a table of contents. Should we retain that?
kgryte commented 4 years ago

As discussed previously (i.e., at the JLab dev sprint), core needs to provide various APIs for collapsing groups of cells. But this was left unresolved.

Not clear to me that ToC should be merged as is. This would be just moving the headache around. The extension is a nice PoC, but I'd advise taking a step back and asking the question: "if we did this in core today, from scratch, how would we implement it and what changes would we need to make to make that happen?" Once answered, can compare to the extension implementation. I am not convinced those two visions will be the same.

As is, the extension basically has to work around limitations in core. These have been detailed and discussed previously on issue threads and in PRs.

I think a larger question for me is: what about the ToC extension makes it an essential part of the lab experience? Yes, the extension is relatively popular, but I would not consider essential. So not clear to me what the criteria is for pulling some things into core and not others. For example, if popularity is the metric, then the Git extension would seem like a prime candidate, but this does not seem "right".

Whether or not ToC is in core, APIs need to be exposed to overcome the current ToC workarounds, particularly in facilitating bidirectional ToC/notebook collapse behavior.

jasongrout commented 4 years ago

If and when we get to merging TOC (or another plugin) to core, and want to preserve git history (and make it easily accessible, unlike git subtree or subtree merges), here's one way to do it:

  1. Install git-filter-repo
  2. Clone the toc repo and use git filter-repo to move the commit history to the appropriate subdirectory:
    git clone git@github.com:jupyterlab/jupyterlab-toc.git
    cd jupyterlab-toc
    git filter-repo --to-subdirectory-filter packages/toc --tag-rename '':'@jupyterlab/toc@'
    cd ..

    Notice that the entire repo is now in the packages/toc directory, and all history happens there.

  3. Clone JupyterLab and merge the history
    git clone git@github.com:jupyterlab/jupyterlab.git
    cd jupyterlab
    git remote add toclocal ../jupyterlab-toc
    git fetch toclocal
    git checkout -b tocmerge
    git merge --allow-unrelated-histories toclocal/master

At this point, it probably makes sense to break up the package into the library and -extension package, integrate it with jlab core, etc.

I like git-filter-repo! It's much nicer and faster than the old git filter-branch!

jasongrout commented 4 years ago

Given the reservations expressed from those that have worked extensively with the code in question, I agree that we need to take a step back and reevaluate.

lresende commented 4 years ago

Thanks for the good feedback from the ones that have more historical info about the extension.

What I see today with this particular extension is that it is very popular on the user community and a lot of the cloud providers that have Jupyter based services deploy this extension by default. Having said that, and probably because it sort of works, it currently does not attract much interest from the dev side and somewhat important features and/or bugs are left unattended for a while (e.g. the collapse notebook feature).

My goal towards incorporating TOC into the core was to have more eyes on the dev side, get some prioritization and help from UX, etc, etc, and also make sure it's always in sync and available to all JLab releases.

From the list above, I feel some are nice to have while others might be blockers. Assuming that the most complained issue was related to collapsable cells and that is now optional and disabled by default, what would be the blocker issues that should be addressed before merging.

lresende commented 4 years ago

We had another discussion on TOC on today's JupyterLab dev meeting and the consensus seems to be that we should start working on the necessary steps to bring TOC to core.

One question that was brought up during the meeting is about collapsable cell feature and that is something we might need further discussion, where we need to have a final decision if we remove the functionality and leave an event to allow users to still have access to implement the capability or if we leave the current functionality disabled. We might opt for the first option as the "clean implementation" requires changes to core that might not come in the near future.

I will start working on moving the extension over the weekend.

bollwyvl commented 4 years ago

Very exciting stuff!

I use toc, generally recommend it, ship it in several full lab builds, and would welcome seeing it, even if in a more primitive form, in core. I have been bit by the dom rewriting, but have also relied it extensively, especially when working on long-form documentation where header depth is far easier to gauge from numbers than visual size. But I've also not contributed to it... So grains of salt.

Some food for thought on integration into some heavier labextensions:

over on jupyterlab-lsp, one of the features I've been looking to get around to is the (hierarchical) symbol message:

https://microsoft.github.io/language-server-protocol/specification#textDocument_documentSymbol

Being able to land this against the toc ui (in core or otherwise) would be great, as I'm trying to move as much stuff as possible into core metaphors (e.g running sidebar).

This would be deferring the symbol discovery (and click target resolution) to a server-side feature, and would be challenging with mixed language documents, to be sure... The way we have to manage notebooks is already... Intricate.

Another extension that could benefit from a collapsible tree browser is jupyterlab-drawio, especially the "full fat" drawio in embed mode. It supports multiple pages, layers, and arbitrary levels of object grouping inside that. individual "stencils" have multiple metadata/parameters/properties inside them. Being able to put editable controls in the toc would be amazing, e.g. move layer up/down, ungroup, etc. For the other direction, e.g group elements, the toolbar, becoming a multi-selection-aware command bar would make a lot of sense.

On Wed, Jun 3, 2020, 13:21 Luciano Resende notifications@github.com wrote:

We had another discussion on TOC on today's JupyterLab dev meeting and the consensus seems to be that we should start working on the necessary steps to bring TOC to core.

One question that was brought up during the meeting is about collapsable cell feature and that is something we might need further discussion, where we need to have a final decision if we remove the functionality and leave an event to allow users to still have access to implement the capability or if we leave the current functionality disabled. We might opt for the first option as the "clean implementation" requires changes to core that might not come in the near future.

I will start working on moving the extension over the weekend.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/jupyterlab/team-compass/issues/63#issuecomment-638338240, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAALCRBG4LXJDOFJLOORRSLRU2BA7ANCNFSM4NGDWY6A .

lresende commented 3 years ago

This is now available in JupyterLab 3.0 master