WebView API

[irahopkinson]

• Save and load dock layout
• Add WebView tabs to Dock

---

This change is 

[tjcouch-sil]

Thanks for the hard work on this!! Looking forward to having nicer web views!

Reviewed 19 of 19 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @irahopkinson)

---

.prettierignore line 32 at r2 (raw file):

# vite output
dist

Would it be clearer to make this extensions/dist? Probably not a big deal.

---

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

export type WebViewContents = WebViewContentsReact | WebViewContentsHtml;

export const TYPE_WEBVIEW = 'webview';

Might be best to keep casing consistent here - 'webView'

[FoolRunning]

Reviewed 19 of 19 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @irahopkinson)

---

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

  ['tab', createTabPanel],
  [TYPE_WEBVIEW, createWebViewPanel],
]);

I was expecting that, as part of this task, this map would no longer be hard-coded - i.e. the types and functions would be supplied by the extensions. It worries me that this still seems hard-coded. How are extensions registering their own types?

---

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

 * @param savedTabInfo Data that is to be used to create the new tab (comes from rc-dock, typically from disk)
 */
function loadTab(savedTabInfo: SavedTabInfo): TabData & SavedTabInfo {

I feel like I'm missing something. How does a loaded tab get hooked up back to the extension that created it? I expected a bunch of code changes in this file (and other files) to handle extensions providing the web views, but I'm not seeing anything related to that. 🤔

---

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

 */
function onLayoutChange(newLayout: LayoutBase): void {
  localStorage.setItem(DOCK_LAYOUT_KEY, JSON.stringify(newLayout));

Does this get called for small changes like changing the splitters or resizing the window? If so, it might be best to only save during shutdown or via user request.

---

src/shared/utils/papi-util.ts line 213 at r2 (raw file):

export function deserializeRequestType(requestType: string): RequestType {
  const [category, ...directiveParts] = requestType.split(REQUEST_TYPE_SEPARATOR);
  const directive = directiveParts.join(REQUEST_TYPE_SEPARATOR);

Probably worth thinking about doing something more efficient here since this gets called for each request:
https://stackoverflow.com/a/2878746/4953232

[FoolRunning]

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

---

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

Previously, FoolRunning (Tim Steenwyk) wrote…

I was expecting that, as part of this task, this map would no longer be hard-coded - i.e. the types and functions would be supplied by the extensions. It worries me that this still seems hard-coded. How are extensions registering their own types?

TJ explained that you both talked and decided to push that part off to another task.

---

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

Previously, FoolRunning (Tim Steenwyk) wrote…

I feel like I'm missing something. How does a loaded tab get hooked up back to the extension that created it? I expected a bunch of code changes in this file (and other files) to handle extensions providing the web views, but I'm not seeing anything related to that. 🤔

TJ explained that you both talked and decided to push that part off to another task.

[irahopkinson]

I've made 2 changes. I'll merge main in once everyone is happy with it as is and then it will need another review before merge.

Reviewable status: 16 of 19 files reviewed, 1 unresolved discussion (waiting on @FoolRunning and @tjcouch-sil)

---

.prettierignore line 32 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Would it be clearer to make this extensions/dist? Probably not a big deal.

I did think about this but dist is a common output folder name and should be excluded in general. The comment is to clarify the reason for it being added in the first place rather than specifically to only be for that. I've updated the comment to make that clearer.

---

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

Previously, FoolRunning (Tim Steenwyk) wrote…

Does this get called for small changes like changing the splitters or resizing the window? If so, it might be best to only save during shutdown or via user request.

Yes it does since those are all layout changes.

I would argue against "via user request" on UX grounds. Saving of the layout is not something the user should ever have to concern themselves about - it just works™.

It's a little bit tricky to assure you can always get to a save on an electron/web shutdown and I'd prefer all my previous layout adjustments where saved even if it doesn't get to a shutdown, e.g. because of a crash - crashing is bad enough for a user without not remembering all my changes! I would also argue it's premature optimisation since anecdotally I think localStorage is fast. I thought I'd better check my assumptions here so I googled it. It's really fast! - see https://gomakethings.com/how-fast-is-vanilla-js-localstorage/.

Do note the TODO in JSDoc above. There is a 3rd argument to the onLayoutChange method that we could filter saves on. I'll likely investigate that in a follow-up PR.

---

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

Previously, tjcouch-sil (TJ Couch) wrote…

Might be best to keep casing consistent here - 'webView'

Done.

---

src/shared/utils/papi-util.ts line 213 at r2 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

Probably worth thinking about doing something more efficient here since this gets called for each request:
https://stackoverflow.com/a/2878746/4953232

Actually at the moment, it's not used in requests at all - only serializeRequestType is used. And before this PR the code was actually broken as it only returned the catergory and the derective was always undefined. In this PR it's only called by deserializeTabId which in turn is only called twice for each WebView at setup.

However, I do agree it can probably be written more efficiently which is one of the main reasons I wrote the tests for it (which I think cover all our requirements). Now someone can refactor it and have confidence it still does everything required. I've moved it forward but didn't feel the need to go the whole way given its current usage. If you still feel strongly about it I can fix it in a follow-up PR.

[tjcouch-sil]

thanks for the hard work on this!

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @FoolRunning)

[irahopkinson]

@FoolRunning main is now merged in and this is ready for final review.

[FoolRunning]

Reviewable status: 19 of 20 files reviewed, 1 unresolved discussion (waiting on @irahopkinson and @tjcouch-sil)

---

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

Saving of the layout is not something the user should ever have to concern themselves about - it just works™.

Actually, this is actually a feature that UX will ask for (since you can do it in Paratext 9). The user can save their current layout to a file and then load that file to restore a particular layout.

As far as the crashing is concerned, I agree that we should do something, but having it save for every movement of the splitter seems overkill. Paratext 9 saves every 5 minutes and that is usually good enough. And another thought, just because saving it to cached storage is fast, that doesn't mean that running the saving on the docking framework is fast (i.e. creating the object to be saved). However, I guess we can wait to see if it becomes an issue for now.

[irahopkinson]

Reviewable status: 19 of 20 files reviewed, 1 unresolved discussion (waiting on @FoolRunning and @tjcouch-sil)

---

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

Actually, this is actually a feature that UX will ask for (since you can do it in Paratext 9). The user can save their current layout to a file and then load that file to restore a particular layout.

For sure, but saving a layout for a latter load is a different feature to this. This is just remembering how you modify your current layout.

And another thought, just because saving it to cached storage is fast, that doesn't mean that running the saving on the docking framework is fast (i.e. creating the object to be saved).

I'm not sure why you think this would be slow. It doesn't have to create the layout object since that already exists in rc-dock, it just has to get it (and even if it did have to create it I'm not sure why that would be slow either). If any of that is slow we have much bigger problems. Maybe I didn't understand your point?

However, I guess we can wait to see if it becomes an issue for now.

If you are OK to merge this as is please change your disposition (bottom-right) - this is the last blocking item. If not please tell me how you would like it changed.

[FoolRunning]

Reviewed 3 of 3 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @irahopkinson)

---

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

Previously, irahopkinson (Ira Hopkinson) wrote…

Actually, this is actually a feature that UX will ask for (since you can do it in Paratext 9). The user can save their current layout to a file and then load that file to restore a particular layout.

For sure, but saving a layout for a latter load is a different feature to this. This is just remembering how you modify your current layout.

And another thought, just because saving it to cached storage is fast, that doesn't mean that running the saving on the docking framework is fast (i.e. creating the object to be saved).

I'm not sure why you think this would be slow. It doesn't have to create the layout object since that already exists in rc-dock, it just has to get it (and even if it did have to create it I'm not sure why that would be slow either). If any of that is slow we have much bigger problems. Maybe I didn't understand your point?

However, I guess we can wait to see if it becomes an issue for now.

If you are OK to merge this as is please change your disposition (bottom-right) - this is the last blocking item. If not please tell me how you would like it changed.

I was originally hoping to still see this changed (why it was a blocking comment), but it is true that the object creation is not really an issue so I guess it's fine for now. I'll stop holding this up.

[lyonsil]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @irahopkinson)