Create WebViewProvider API, allowing webviews to persist across refreshes #225

tjcouch-sil · 2023-06-07T23:33:08Z

Reworked loading and saving tabs so every tab is responsible for its own serialization and deserialization
Split tab ids into id and tabType
Tightened up layout-related types for better TypeScript support
Moved saving and loading layout to web view service
Created web view provider service that handles web view providers specifically. Each provider is distinct from other providers by having a unique webViewType, which is how the contents of webviews is determined
- This service is a weird split off from web view service, so I made it internal and not affect extension developers. Seems like a helpful distinction, though. Would love input on any thoughts regarding reworking it. Maybe make a singleton class in web view service? Dunno; not sure it's even worth it right now. Might be best to make an issue if it even warrants it
Reworked addWebView so you provide just a webViewType, layout info, and some options. Then the appropriate WebViewProvider takes care of actually making the web view
onDidAddWebView event no longer carries content or styles so there's not so much unnecessary talk all over the network over webviews
Added a button on the (newly renamed) People webview that opens a new People webview :)

Also created PR on rc-dock for the patch included in this PR: ticlo/rc-dock#208

Resolves #177

This change is

…ith addWebView

…ebView

tjcouch-sil

Oops - this is supposed to be a draft PR. Sorry! Thanks for previewing and giving feedback :)

Reviewable status: 0 of 25 files reviewed, all discussions resolved

irahopkinson · 2023-06-08T00:00:45Z

There is a Convert to draft link button top-right in the reviewers section of the side-bar - hard to spot though.

irahopkinson

I've run out of time to look at everything today but I'll publish what I have so far. Do note there are some tests failing which is why the GitHub checks aren't happy yet.

Reviewed 15 of 25 files at r1, all commit messages.
Reviewable status: 15 of 25 files reviewed, 8 unresolved discussions (waiting on @tjcouch-sil)

extensions/lib/hello-world/hello-world.ts line 43 at r1 (raw file):

const reactWebViewType = 'hello-world.react';

const reactWebViewProvider: IWebViewProvider = {

BTW this is the 3rd copy of this code. What about supplying a webViewProviderFactory function or is this likely to be different for other WebViews?

extensions/lib/hello-world/hello-world.ts line 93 at r1 (raw file):

  // anywhere; it just has to match `webViewType`. See `hello-someone.ts` for an example of keeping
  // an existing webview that was specifically created by `hello-someone`.
  papi.webViews.addWebView(htmlWebViewType, undefined, { existingId: '*' });

BTW how about making this the default options in addWebView, then this call (and the one below) would simplify nicely. A unique WebView is likely the default, right?

Code quote:

{ existingId: '*' }

src/shared/data/web-view.model.ts line 158 at r1 (raw file):

export type AddWebViewOptions = {
  /**
   * If provided, requests from the web view provider an existing existing WebView with this id

This sentence needs some revising to clarify meaning.

src/shared/models/web-view-provider.model.ts line 22 at r1 (raw file):

  getWebView(
    serializedWebView: WebViewDefinitionSerialized,
    addWebViewOptions: AddWebViewOptions,

BTW it appears this argument is optional (at least in hello-someone.ts and hello-world.ts)?

src/shared/models/web-view-provider.model.ts line 31 at r1 (raw file):

    CanHaveOnDidDispose<IWebViewProvider> {}

// What the papi returns on register. Basically a layer over DisposableNetworkObject

Err... how can it be called DisposableWebViewProvider when it omits dispose?

src/shared/services/web-view-provider.service.ts line 49 at r1 (raw file):

 * it unless something else in the existing process is subscribed to it.
 */
function hasKnown(webViewType: string): boolean {

BTW I haven't figured out yet why you choose a standard function or an arrow function. Above you have 2 arrow functions - one tiny and one longer. Here it's also tiny but a function. What's the criteria you use for choosing between them?

src/shared/services/web-view-provider.service.ts line 78 at r1 (raw file):

  // Set up the WebView provider to be a network object so other processes can use it
  const disposableWebViewProvider = (await networkObjectService.set(

BTW you don't need to us as here but instead:

  const disposableWebViewProvider: DisposableWebViewProvider = await networkObjectService.set(
    webViewProviderObjectId,
    webViewProvider,
  );

Perhaps this amounts to the same thing but is slightly cleaner without extra brackets.

src/shared/services/web-view.service.ts line 123 at r1 (raw file):

  const webViewDefinitionCloned: Omit<WebViewDefinition, 'content'> &
    Partial<Pick<WebViewDefinition, 'content'>> &
    Partial<Pick<WebViewDefinitionReact, 'styles'>> = { ...webViewDefinition };

BTW the spread opertator won't do a deep clone but I guess that's all you need here, right?

irahopkinson

Reviewed 10 of 25 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 35 unresolved discussions (waiting on @tjcouch-sil)

src/renderer/components/web-view.component.tsx line 57 at r2 (raw file):

}

export default function loadWebViewPanel(savedTabInfo: SavedTabInfo): TabInfo {

This is really loading a WebView tab. Panel has a different meaning in rc-dock so this could be confusing to use panel here. How about loadWebViewTab?

Same for saveWebViewPanel below.

Code quote:

loadWebViewPanel

src/renderer/components/web-view.component.tsx line 57 at r2 (raw file):

}

export default function loadWebViewPanel(savedTabInfo: SavedTabInfo): TabInfo {

BTW I'm not sure if this should remain the default export. I don't have a strong opinion but my instinct is to remove the default now we have other valid exports and we load all 3 in the only place we import them. This is our next item to discuss for styles but this seems a clearer example than some of the other edge cases we need to discuss. Feel free to leave it as is if you feel differently.

Code quote:

default

src/renderer/components/web-view.component.tsx line 71 at r2 (raw file):

    // deserialize the web view. It will asynchronously go get the content and re-run this function
    if (!data.content && data.content !== '')
      (async () => {

The anonymous self-calling function seems a little funky here. How about a named async function that we pass in data to it but don't await when we call it here? E.g. if (!data.content && data.content !== '') getWebViewContent(data);

src/renderer/components/docking/paranext-dock-layout.component.tsx line 62 at r2 (raw file):

const tabLoaderMap = new Map<TabType, TabLoader>([
  [TAB_TYPE_ABOUT, loadAboutPanel],

I think these should all be renamed load<feature>Tab.

src/renderer/components/docking/paranext-dock-layout.component.tsx line 76 at r2 (raw file):

function loadSavedTabInfo(tabInfo: SavedTabInfo): TabInfo {
  const tabLoader = tabLoaderMap.get(tabInfo.tabType);
  if (!tabLoader) return createErrorTab(`No tab loader for tabType '${tabInfo.tabType}'`);

BTW this should be renamed to match the new pattern of loadErrorTab.

src/renderer/components/docking/paranext-dock-layout.component.tsx line 87 at r2 (raw file):

}

type RCDockTabInfo = TabData & Omit<TabInfo, 'title'> & { tabInfoTitle: string };

BTW types should probably be grouped together at the top of the file after imports and before consts. It makes it easier to find and keep them organised and to refactor them into .model.ts files if they are needed elsewhere.

src/renderer/components/docking/paranext-dock-layout.component.tsx line 103 at r2 (raw file):

  return {
    ...tabInfo,
    tabInfoTitle: tabInfo.title,

BTW would it be better to already have it saved this way inside tabInfo so there is no special case here for title and would also simplify things in the saveTab function?

src/renderer/components/docking/paranext-dock-layout.component.test.ts line 77 at r2 (raw file):

      expect(() => loadTab(savedTabInfoNone)).toThrow();
      expect(() => loadTab(savedTabInfoNoId)).toThrow();

These 2 added expects don't add anything to the tests other than more running time. Revert the changes other than the as. If you want to check that no tabType causes a throw then add a new it block and define an id so it gets through the first throw check that this it block tests.

src/renderer/components/docking/paranext-dock-layout.component.test.ts line 87 at r2 (raw file):

      const event: AddWebViewEvent = { webView: {} as WebViewProps, layout: { type: 'tab' } };

      expect(() => addWebViewToDock(event, dockLayout)).toThrow();

In my VSCode this line is complaining that "Expected 3 arguments, but got 2".

src/renderer/components/docking/paranext-dock-layout.component.test.ts line 95 at r2 (raw file):

      const dockLayout = instance(mockDockLayout);
      const event: AddWebViewEvent = {
        webView: { id: 'myId', webViewType: 'test', content: '' },

content is not defined in AddWebViewEvent - you can probably just remove it.

src/renderer/components/docking/paranext-dock-layout.component.test.ts line 99 at r2 (raw file):

      };

      expect(() => addWebViewToDock(event, dockLayout)).toThrow();

In my VSCode this line is complaining that "Expected 3 arguments, but got 2".

src/renderer/components/docking/paranext-dock-layout.component.test.ts line 111 at r2 (raw file):

      const dockLayout = instance(mockDockLayout);
      const event: AddWebViewEvent = {
        webView: { id: 'myId', webViewType: 'test', content: '' },

content is not defined in AddWebViewEvent - you can probably just remove it.

src/renderer/components/docking/paranext-dock-layout.component.test.ts line 115 at r2 (raw file):

      };

      expect(() => addWebViewToDock(event, dockLayout)).toThrow();

In my VSCode this line is complaining that "Expected 3 arguments, but got 2".

src/shared/data/web-view.model.ts line 109 at r2 (raw file):

 * {@link WebViewDefinition}s and verify any existing properties on the WebViews.
 */
export type WebViewDefinitionSerialized =

BTW maybe a better name is SavedWebViewDefinition?

src/shared/services/web-view.service.ts line 111 at r2 (raw file):

}

export function saveTabInfoBase(tabInfo: TabInfo): SavedTabInfo {

This might be better named savedTabInfoBase (added a 'd') since it doesn't actually do any saving but returns saved info.

src/shared/services/web-view.service.ts line 147 at r2 (raw file):

let papiDockLayoutVar = createDockLayoutAsyncVar();

export type OnLayoutChangeEventInternal = {

BTW this is not really internal in the traditional sense since it's exported. It's really the arguments for rc-dock's onLayoutChange method (unless thats what you mean by 'internal')?

src/shared/services/web-view.service.ts line 157 at r2 (raw file):

  onLayoutChange: PapiEvent<OnLayoutChangeEventInternal>;
  addWebViewToDock: (webView: WebViewProps, layout: Layout) => void;
  testLayout: LayoutBase;

BTW it feels like this should be called something else unless you expect it to simply be removed later on?

src/shared/services/web-view.service.ts line 160 at r2 (raw file):

};

const DOCK_LAYOUT_KEY = 'dock-saved-layout';

This should be at the top of the file along with the other consts below the imports.

src/shared/services/web-view.service.ts line 174 at r2 (raw file):

}

async function saveLayout(layout?: LayoutBase): Promise<void> {

I know you talked about moving this here when I was working on the rc-dock stuff but I think I need some help understanding the need since it highly complicates things and moves it out of it's natural encapsulation.

src/shared/services/web-view.service.ts line 175 at r2 (raw file):

async function saveLayout(layout?: LayoutBase): Promise<void> {
  const currentLayout = layout || (await papiDockLayoutVar.promise).dockLayout.saveLayout();

I can't figure out when layout would be optional since the only call to this is in onLayoutChange and it's pasing a value in there. If you are meaning that the value could be undefined then it would be better to declare that rather then make the argument optional (I know they amount to almost the same thing but they read differently). And even then the calling function says it is defined so maybe it needs fixing there too?

src/shared/services/web-view.service.ts line 179 at r2 (raw file):

}

async function loadLayout(layout?: LayoutBase): Promise<void> {

Conversely loadLayout doesn't ever appear to be called with an argument. If so why is it declared? Probably remove the argument.

src/shared/services/web-view.service.ts line 187 at r2 (raw file):

    // A layout was provided, meaning this is a layout change. Since `dockLayout.loadLayout` doesn't
    // run `onLayoutChange`, we run it manually
    await onLayoutChange({ newLayout: layoutToLoad });

This will never get called since the layout argument is never provided. Remove the if block. (BTW I think you will find that the onLayoutChange from rc-dock DOES get called after it runs it's loadTab method.)

src/shared/services/web-view.service.ts line 213 at r2 (raw file):

  loadLayout();

  // Return an unsubscriber ot unregister this dock layout. The primary situation in which I see

typo: 'ot'

Code quote:

ot

src/shared/services/web-view.service.ts line 245 at r2 (raw file):

 * @returns promise that resolves nothing if we successfully handled the webView
 */
export const addWebView = async (

The use of this function has changed. Perhaps rename to showWebView or getWebView (this name is already used in the provider so maybe it makes sense here also or not)?

src/shared/services/web-view.service.ts line 248 at r2 (raw file):

  webViewType: WebViewType,
  layout: Layout = { type: 'tab' },
  options: AddWebViewOptions = {},

BTW could we default this to the single WebView usage, i.e. { existingId: '*' } ?

src/shared/services/web-view.service.ts line 250 at r2 (raw file):

  options: AddWebViewOptions = {},
): Promise<WebViewId | undefined> => {
  const optionsDef = addWebViewOptionsDefaults(options);

BTW prefer unabbreviated names from our style guide. Perhaps use optionDefaults or optionsDefaulted?

src/shared/services/web-view.service.ts line 312 at r2 (raw file):

            // Find any webview with the specified webViewType
            return (item.data as WebViewDefinition).webViewType === webViewType;

BTW this will only ever find the first one of that type. Should we allow the use of '*' if there is more than one of that type? I guess that would be programmer error to use that with more than one available?

…existingId to represent 'any', not 'all'

tjcouch-sil

Reviewable status: all files reviewed, 27 unresolved discussions (waiting on @irahopkinson)

extensions/lib/hello-world/hello-world.ts line 43 at r1 (raw file):