Switch from using settings registry to a signal for notebook collapsing behavior in ToC

[marthacryan]

References

The collapsing behavior for notebooks in the recently-merged ToC extension has raised many concerns (see jupyterlab/jupyterlab-toc#123). In the future, less confusing collapsing behavior could/should be added, but this would require some work on the notebook collapsing behavior / API, so as a patch, (before ToC was merged into core) a ToC configuration was added to the advanced settings that allowed users to enable users to disable / enable the feature as necessary. However, after discussion in dev meetings and (a little bit) on jupyterlab/team-compass#63, it was decided that the ToC extension should instead add a signal that fires when the collapse button is pressed to allow external extensions to add the original collapsing behavior while waiting for the changes to be made that would enable a cleaner implementation of collapsing notebooks in the ToC.

Code changes

See notes below on implementation questions I have. Removes all references to the setting registry and adds a signal that is fired when the "collapse" button is pressed so that external extensions can add the functionality if necessary. To access the signal from an extension, the widget would need to require the ITableOfContentsRegistry token in their extension, then use the registry API (i.e. TableOfContentsRegistry.find) to access the OptionsManager for the notebook ToC generator, which has a field for the collapseSignal. The signal gives a Cell widget as an argument.

User-facing changes

This adds back in the "collapse" button (which was disabled along with the collapsing behavior by default) so the ToC looks like this for notebooks:

Users will be able to collapse sections within the ToC widget, but the notebook will not be collapsed. So for example, if the button on the top entry of the ToC were pressed, the ToC would look like this:

Notes

• Not sure about the use of signals here - is calling emit from this function proper usage of signals?
• Want to discuss naming of the updateAndCollapse function - couldn't think of a good name for it.

[jupyterlab-dev-mode]

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link:

[kgryte]

@marthacryan We probably don't want the signal to be buried in the options manager. That class is an internal implementation detail.

Instead, I might suggest making the ToC generators signal emitters where the registry can listen (within the activate function) to events from the various generators (Markdown, notebook, etc) and re-broadcast. In short, I suggest that we should strive for more generality here.

The signal can then provide: 1) the ToC type (e.g., notebook, markdown, etc), 2) the cellRef, and 3) whether the ToC heading is collapsed.

Currently, you only provide the cellRef, but, TMK, that won't be enough info, as we need the collapsed state to determine whether to, e.g., collapse or expand a notebook section. That collapsed state was previously provided.

Maybe @jasongrout can shed some further light on what the current suggested practice is for 3rd party extensions to listen to signals from core extensions in order to drive some sort of 3rd party behavior.

[marthacryan]

Code updates

Adds a collapseSignal field in the TableOfContentsRegistry that listens to the collapseSignal of every generator (it checks whether a given generator has the signal in the add function of the registry). Extensions would similarly require the ITableOfContentsRegistry, but this way, they can access the signal with just tocRegistry.collapseSignal. Also creates an interface ICollapseSignalArgs for the arguments for this signal, with fields collapsedState, header, and tocType.

@kgryte I kept the initialization of the signal in the options manager because the call to emit is currently in the options manager class, and from what I've gathered, when calling emit, the signal field should be private. Let me know if you think I should implement that a different way though!

[blink1073]

I think the API as-is is in keeping with the general patterns in the code base.

[blink1073]

Looks good overall! Just a couple code style suggestions.

[blink1073]

Are these likely to change over time, i.e. should this be a typed object that is passed?

[marthacryan]

I changed it to taking ICollapseChangedArgs, do you think that's a good solution?

[kgryte]

Do we need to have a plan to update the external ToC extension to follow the changes introduced in this PR? And how should that be coordinated to ensure smooth integration? And how should that affect the inclusion timeline of this PR?

Maybe this is not a problem, but I am curious as to how the external ToC extension can coexist with the core ToC extension until the external ToC extension is updated. If users currently depend on external ToC behavior in JLab v2.x (e.g., for collapsing notebook sections), and this PR makes it into JLab v2.2, will the two ToCs conflict?

If they can coexist, then may be confusing for external ToC users to know which ToC offers which behavior.

I might suggest holding off on enabling ToC in core until v3.0 in order to ensure peaceful coexistence.

Also, not clear to me atm what the implications are, and the next steps are, for building on and coexisting with ToC in core.

[blink1073]

This looks great, thanks!

[blink1073]

I had to drop from the meeting today, did we decide whether to include this in 2.2?

[marthacryan]

@blink1073 We decided to include this in a 3.0.0-alpha release which will be released today as well.

@jasongrout @saulshanabrook Should I update the version of toc / toc-extension as part of this PR, or would that just happen as part of the release process?

[saulshanabrook]

@jasongrout @saulshanabrook Should I update the version of toc / toc-extension as part of this PR, or would that just happen as part of the release process?

I believe it will be updated as part of the release process!

[saulshanabrook]

Merging this in for the 3.0.0-alpha release!