Skip to content

Commit

Permalink
Switch to overlay webviews for notebooks (#5829)
Browse files Browse the repository at this point in the history
Addresses #5491 

This PR changes how we render html output in notebooks 2.0 by switching
to overlay webviews instead of standard ones.
The reason for this switch was that there is no way to preserve the
content of the webview across visibility changes with standard webviews.
This means we would have to fully re-render every output every time the
user switched to the notebook view.

One benefit of this switch is we were able to remove all the logic
surrounding different webview types being returned from the various
webview services, simplifying the API a good bit.


### QA Notes

- Dynamic plots like hvplot should still show up as expected. 
- You should be able to navigate away from the notebook and back and the
plot should reappear in the same state it was in.
- Plots should never spill out of the notebook containment. (e.g. be
visible when the notebook isnt. This is an annoying problem with overlay
webviews that we've had to deal with before in the plots pane etc..)
  • Loading branch information
nstrayer authored Jan 2, 2025
1 parent 24620f4 commit feb2937
Show file tree
Hide file tree
Showing 14 changed files with 444 additions and 167 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*---------------------------------------------------------------------------------------------
* Copyright (C) 2024 Posit Software, PBC. All rights reserved.
* Licensed under the Elastic License 2.0. See LICENSE.txt for license information.
*--------------------------------------------------------------------------------------------*/

// React.
import React, { PropsWithChildren, createContext, useContext } from 'react';

// Other dependencies.
import { ISettableObservable } from '../../../../base/common/observableInternal/base.js';

/**
* Create the notebook visibility context.
*/
const NotebookVisibilityContext = createContext<ISettableObservable<boolean>>(undefined!);

/**
* Provider component for notebook visibility state
*/
export const NotebookVisibilityProvider = ({
isVisible,
children
}: PropsWithChildren<{ isVisible: ISettableObservable<boolean> }>) => {
return (
<NotebookVisibilityContext.Provider value={isVisible}>
{children}
</NotebookVisibilityContext.Provider>
);
};

/**
* Hook to access the notebook visibility state
* @returns The current visibility state as a boolean
*/
export const useNotebookVisibility = () => {
return useContext(NotebookVisibilityContext)
};
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,16 @@ export function PositronNotebookComponent() {
const notebookInstance = useNotebookInstance();
const notebookCells = useObservedValue(notebookInstance.cells);
const fontStyles = useFontStyles();
const containerRef = React.useRef<HTMLDivElement>(null);

React.useEffect(() => {
notebookInstance.setCellsContainer(containerRef.current);
}, [notebookInstance]);

return (
<div className='positron-notebook' style={{ ...fontStyles }}>
<PositronNotebookHeader notebookInstance={notebookInstance} />
<div className='positron-notebook-cells-container'>
<div className='positron-notebook-cells-container' ref={containerRef}>
{notebookCells?.length ? notebookCells?.map((cell, index) => <>
<NotebookCell key={cell.handleId} cell={cell as PositronNotebookCellGeneral} />
<AddCellButtons index={index + 1} />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ import { NotebookInstanceProvider } from './NotebookInstanceProvider.js';
import { PositronNotebookComponent } from './PositronNotebookComponent.js';
import { ServicesProvider } from './ServicesProvider.js';
import { IEditorGroup, IEditorGroupsService } from '../../../services/editor/common/editorGroupsService.js';
import { IEditorService } from '../../../services/editor/common/editorService.js';
import { IWorkbenchLayoutService } from '../../../services/layout/browser/layoutService.js';
import { PositronNotebookEditorInput } from './PositronNotebookEditorInput.js';
import { ILogService } from '../../../../platform/log/common/log.js';
import { IWebviewService } from '../../webview/browser/webview.js';
Expand All @@ -56,6 +58,7 @@ import { IOpenerService } from '../../../../platform/opener/common/opener.js';
import { INotificationService } from '../../../../platform/notification/common/notification.js';
import { IPositronNotebookOutputWebviewService } from '../../positronOutputWebview/browser/notebookOutputWebviewService.js';
import { IPositronWebviewPreloadService } from '../../../services/positronWebviewPreloads/browser/positronWebviewPreloadService.js';
import { NotebookVisibilityProvider } from './NotebookVisibilityContext.js';


interface NotebookLayoutInfo {
Expand Down Expand Up @@ -124,6 +127,8 @@ export class PositronNotebookEditor extends EditorPane {
@ICommandService private readonly _commandService: ICommandService,
@IOpenerService private readonly _openerService: IOpenerService,
@INotificationService private readonly _notificationService: INotificationService,
@IEditorService private readonly _editorService: IEditorService,
@IWorkbenchLayoutService private readonly _layoutService: IWorkbenchLayoutService,
) {
// Call the base class's constructor.
super(
Expand Down Expand Up @@ -219,11 +224,22 @@ export class PositronNotebookEditor extends EditorPane {
*/
private _size = observableValue<ISize>('size', { width: 0, height: 0 });

/**
* Observable tracking if the editor is currently visible
*/
private readonly _isVisible = observableValue<boolean>('isVisible', false);


// Getter for notebook instance to avoid having to cast the input every time.
get notebookInstance() {
return (this.input as PositronNotebookEditorInput)?.notebookInstance;
}

protected override setEditorVisible(visible: boolean): void {
this._isVisible.set(visible, undefined);
super.setEditorVisible(visible);
}

protected override createEditor(parent: HTMLElement): void {

this._logService.info(this._identifier, 'createEditor');
Expand Down Expand Up @@ -315,14 +331,19 @@ export class PositronNotebookEditor extends EditorPane {
override clearInput(): void {
this._logService.info(this._identifier, 'clearInput');


// Clear the input observable.
this._input = undefined;
if (this.notebookInstance && this._parentDiv) {
this.notebookInstance.detachView();
console.log('isVisible', this._isVisible.get());
}

if (this.notebookInstance) {
this._saveEditorViewState();
this.notebookInstance.detachView();
}

// Clear the input observable.
this._input = undefined;

this._disposeReactRenderer();

// Call the base class's method.
Expand Down Expand Up @@ -444,24 +465,28 @@ export class PositronNotebookEditor extends EditorPane {
const reactRenderer: PositronReactRenderer = this._positronReactRenderer ?? new PositronReactRenderer(this._parentDiv);

reactRenderer.render(
<NotebookInstanceProvider instance={notebookInstance}>
<ServicesProvider services={{
configurationService: this._configurationService,
instantiationService: this._instantiationService,
textModelResolverService: this._textModelResolverService,
webviewService: this._webviewService,
notebookWebviewService: this._notebookWebviewService,
webviewPreloadService: this._positronWebviewPreloadService,
commandService: this._commandService,
logService: this._logService,
openerService: this._openerService,
notificationService: this._notificationService,
sizeObservable: this._size,
scopedContextKeyProviderCallback: container => scopedContextKeyService.createScoped(container)
}}>
<PositronNotebookComponent />
</ServicesProvider>
</NotebookInstanceProvider>
<NotebookVisibilityProvider isVisible={this._isVisible}>
<NotebookInstanceProvider instance={notebookInstance}>
<ServicesProvider services={{
configurationService: this._configurationService,
instantiationService: this._instantiationService,
textModelResolverService: this._textModelResolverService,
webviewService: this._webviewService,
notebookWebviewService: this._notebookWebviewService,
webviewPreloadService: this._positronWebviewPreloadService,
commandService: this._commandService,
logService: this._logService,
openerService: this._openerService,
notificationService: this._notificationService,
sizeObservable: this._size,
scopedContextKeyProviderCallback: container => scopedContextKeyService.createScoped(container),
editorService: this._editorService,
layoutService: this._layoutService
}}>
<PositronNotebookComponent />
</ServicesProvider>
</NotebookInstanceProvider>
</NotebookVisibilityProvider>
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
*--------------------------------------------------------------------------------------------*/

import { Emitter } from '../../../../base/common/event.js';
import { Disposable, DisposableStore } from '../../../../base/common/lifecycle.js';
import { Disposable, DisposableStore, toDisposable } from '../../../../base/common/lifecycle.js';
import { ISettableObservable, observableValue } from '../../../../base/common/observableInternal/base.js';
import { URI } from '../../../../base/common/uri.js';
import { localize } from '../../../../nls.js';
Expand Down Expand Up @@ -109,6 +109,16 @@ export class PositronNotebookInstance extends Disposable implements IPositronNot
*/
private _container: HTMLElement | undefined = undefined;

/**
* The DOM element that contains the cells for the notebook.
*/
private _cellsContainer: HTMLElement | undefined = undefined;

/**
* Disposables for the current cells container event listeners
*/
private readonly _cellsContainerListeners = this._register(new DisposableStore());

/**
* Callback to clear the keyboard navigation listeners. Set when listeners are attached.
*/
Expand Down Expand Up @@ -159,6 +169,12 @@ export class PositronNotebookInstance extends Disposable implements IPositronNot
private readonly _onDidChangeContent = this._register(new Emitter<void>());
readonly onDidChangeContent = this._onDidChangeContent.event;

/**
* Event emitter for when the cells container is scrolled
*/
private readonly _onDidScrollCellsContainer = this._register(new Emitter<void>());
readonly onDidScrollCellsContainer = this._onDidScrollCellsContainer.event;

// =============================================================================================
// #region Public Properties

Expand All @@ -167,6 +183,59 @@ export class PositronNotebookInstance extends Disposable implements IPositronNot
*/
private _id: string;

/**
* The DOM element that contains the cells for the notebook.
*/
get cellsContainer(): HTMLElement | undefined {
return this._cellsContainer;
}

/**
* Sets the DOM element that contains the cells for the notebook.
* @param container The container element to set, or undefined to clear
*/
setCellsContainer(container: HTMLElement | undefined | null): void {
// Clean up any existing listeners
this._cellsContainerListeners.clear();

if (!container) { return; }

this._cellsContainer = container;

// Fire initial scroll event after a small delay to ensure layout has settled
const initialScrollTimeout = setTimeout(() => {
this._onDidScrollCellsContainer.fire();
}, 50);

// Set up scroll listener
const scrollListener = DOM.addDisposableListener(container, 'scroll', () => {
this._onDidScrollCellsContainer.fire();
});

// Set up mutation observer to watch for DOM changes
const observer = new MutationObserver(() => {
// Small delay to let the DOM changes settle
setTimeout(() => {
this._onDidScrollCellsContainer.fire();
}, 0);
});

observer.observe(container, {
childList: true,
subtree: true,
attributes: true,
attributeFilter: ['style', 'class']
});

// Add all the disposables to our store
this._cellsContainerListeners.add(toDisposable(() => clearTimeout(initialScrollTimeout)));
this._cellsContainerListeners.add(scrollListener);
this._cellsContainerListeners.add(toDisposable(() => observer.disconnect()));

// Fire initial scroll event
this._onDidScrollCellsContainer.fire();
}

/**
* User facing cells wrapped in an observerable for the UI to react to changes
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ import { IInstantiationService } from '../../../../platform/instantiation/common
import { ILogService } from '../../../../platform/log/common/log.js';
import { INotificationService } from '../../../../platform/notification/common/notification.js';
import { IOpenerService } from '../../../../platform/opener/common/opener.js';
import { IEditorService } from '../../../services/editor/common/editorService.js';
import { IWorkbenchLayoutService } from '../../../services/layout/browser/layoutService.js';
import { IPositronNotebookOutputWebviewService } from '../../positronOutputWebview/browser/notebookOutputWebviewService.js';
import { IWebviewService } from '../../webview/browser/webview.js';
import { IPositronWebviewPreloadService } from '../../../services/positronWebviewPreloads/browser/positronWebviewPreloadService.js';
Expand Down Expand Up @@ -83,6 +85,16 @@ interface ServiceBundle {
*/
scopedContextKeyProviderCallback: (container: HTMLElement) => IScopedContextKeyService;

/**
* Service for managing active editors and editor state
*/
editorService: IEditorService;

/**
* Service for managing workbench layout and panel positions
*/
layoutService: IWorkbenchLayoutService;

}

/**
Expand Down Expand Up @@ -110,5 +122,3 @@ export function useServices() {
}
return serviceBundle;
}


Loading

0 comments on commit feb2937

Please sign in to comment.