Use menu contributions to build menus

[lyonsil]

This enables taking menu documents contributed from extensions, localizing them, and passing them into the toolbar main menu.

---

This change is 

[lyonsil]

assets/localization/eng.json line 9 at r2 (raw file):

  "mainMenu_help": "Help",
  "mainMenu_layout": "Layout",
  "mainMenu_openHelloWorldProject": "Open Hello World Project",

Note that we don't have a way for extensions to contribute localizations yet, so I added this to the main repo for now.

[tjcouch-sil]

Awesome!! Thanks for the hard work on this. I'm so excited to have our first contributions in!!!

Reviewed 32 of 35 files at r1, 7 of 10 files at r2, 3 of 3 files at r3, 10 of 10 files at r4, 8 of 8 files at r5, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @lyonsil)

---

lib/platform-bible-react/src/components/toolbar.component.tsx line 8 at r4 (raw file):

import GridMenu from './grid-menu.component';
import './toolbar.component.css';
import usePromise from '../hooks/use-promise.hook';

Please use a path alias :)

---

lib/platform-bible-react/src/components/toolbar.component.tsx line 76 at r4 (raw file):

  );

  const [menuData] = usePromise(

I'd like to discuss ways to refactor this code so we aren't doing this. I think it would be more appropriate to pass the menu data down as a prop. To enable the shift stuff to work properly, I'd say we also should make isOpen and onChangeIsOpen props (onChangeIsOpen can do something that will allow the parent to determine if you're holding shift, I suppose, or just pass up if you're holding shift if you want to do it that way). Then the parent can pass down the menu data that is appropriate, and we are using expected react patterns.

I also lean toward having the parent manage when the menu data should change (only when closed) instead of in here since it is expected that prop changes lead to updating the dom based on the prop changes. Even now, changing menuProvider while the menu is open would cause the menu data to change.

Of course, all of this can be done in a follow-up if we don't want to wait to get this in.

---

lib/platform-bible-utils/src/menus.model.ts line 2 at r4 (raw file):

//----------------------------------------------------------------------------------------------
// NOTE: If you change any of the types, make sure the JSON schema at the end of this file gets

Do any of these changes necessitate a change to the JSON schema? Just a reminder in case; no action needed unless you think of something

---

src/extension-host/data/menu.data.json line 39 at r4 (raw file):

      },
      {
        "label": "%mainMenu_openResourceViewer%",

Shouldn't this be contributed by the resource viewer?

---

src/extension-host/services/extension.service.ts line 754 at r4 (raw file):

    const { webViewMenus } = currentMenus;
    Object.entries(webViewMenus).forEach(([webViewType]) => {
      menuDocumentCombiner.deleteContribution(webViewType);

It loos like this deletion of contributions by web view type isn't parallel to the addition of contributions by extension name. Should we keep an array of contribution names to remove here? Just go ahead and remove all contributions? Other?

---

src/extension-host/services/extension.service.ts line 762 at r4 (raw file):

      if (!extension.menus) return;
      try {
        const menuJson = await nodeFS.readFileText(joinUriPaths(extension.dirUri, extension.menus));

I wonder if we should make some kind of utility in here or somewhere that makes sure the path can't escape the extension's folder. Maybe we should put a TODO otherwise? File an issue? Other?

---

src/extension-host/services/extension.service.ts line 771 at r4 (raw file):

  );

  menuDataService.rebuildMenus();

Looks like this is currently an un-awaited promise. Would you mind wrapping it in an IIFE with a try/catch or whatever seems appropriate to you?

---

src/extension-host/services/menu-data.service-host.ts line 44 at r4 (raw file):

  }

  async rebuildMenus(): Promise<void> {

Is this intended to be able to be called on the network? I believe it is currently exposed on the network since it is on the engine. If you want to be able to rebuild just from the extension host, you probably can access it in a similar way to how you access the menuDocumentCombiner in the extension service.

---

src/extension-host/services/menu-data.service-host.ts line 47 at r4 (raw file):

    const currentMenus = await menuDocumentCombiner.getCurrentMenus();
    if (!currentMenus || currentMenus.mainMenu === this.mainMenu) return;
    this.#loadAllMenuData(currentMenus);

I think you need to notifyUpdate here to let subscribers know about the update to the data, right?

---

src/extension-host/services/menu-data.service-host.ts line 92 at r4 (raw file):

}

function getMenuDataObject(): Localized<PlatformMenus> {

Looks like this is probably here to handle null and undefined appropriately according to our deserialize. However, I see you're using menuDataObject directly in other places. Think it would be ok to use this instead? Or just make another const that is this deserialized version or something (I imagine probably caching the result would be best)

---

src/renderer/components/platform-bible-menu.data.ts line 122 at r4 (raw file):

): Promise<Localized<MultiColumnMenu>> {
  if (!isSupportAndDevelopment) return menuDataService.getMainMenu();
  const supportMenu = new AsyncVariable<Localized<MultiColumnMenu>>('supportMenu');

I think I understand you did this to wrap the support menu in a promise. If so, alternatively I believe you can just make the function async and return the support menu directly or return Promise.resolve(supportMenu) :)

---

src/renderer/components/platform-bible-toolbar.tsx line 12 at r4 (raw file):

};

export default function PlatformBibleToolbar() {

Regarding reworking the menu retrieval stuff, in here, you could use useData(menuData.dataProviderName) to get the menu data and keep up with any updates that come out in the future instead of passing a function that retrieves it asynchronously when the menu opens. Then to prevent it from updating when the menu is open, I suppose you could keep a useState of the menu stuff and have a useEffect that updates the state to equal the retrieved menu data when the menu is closed.

I think doing it this way follows normal react data flow patterns and prevents us from having to disable hook dependency array checking. I'd say both of those things give a strong case for needing a rework since those things are probably very prone to lead to bugs.

---

src/shared/services/menu-data.service-model.ts line 63 at r4 (raw file):

   * @returns Unsubscriber function
   */
  setMainMenu(

This was here to add JSDocs to the automatic setMainMenu function signature from IDataProvider<MenuDataDataTypes> (same with setWebviewMenu. I think it may be valuable to have, but it's also fine to leave it out. Maybe a bit more precise jsdoc description would be a bit clearer.

Anyway this is also why the getMainMenu signature with mainMenuType was there as well - just providing JSDoc for the IDataProvider<MenuDataDataTypes> function. Don't have to keep that either if you don't think it would be helpful. I think removing the useless parameter on getMainMenu adds an overload; I dunno if the JSDoc displays on both.

---

src/shared/services/menu-data.service-model.ts line 76 at r4 (raw file):

   */
  subscribeMainMenu(
    mainMenuType: undefined,

Unfortunately I believe this parameter currently needs to be here for the subscribe function to work. It needs the first parameter to be the selector, which is basically nothing in this case. I think the description may have been a bit misleading.

Maybe we could rework selectors so you can just not pass a selector, but I imagine that's probably something to consider another day.

---

src/shared/utils/menu-document-combiner.ts line 165 at r4 (raw file):

    const column = columns[columnName as ReferencedItem];
    if (!!column && typeof column === 'object')
      strings = strings.concat([removePercentSigns(column.label)]);

I'd argue it's the localization service's job to handle the percent signs - maybe worth discussing a bit and then making an issue?

[lyonsil]

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @tjcouch-sil)

---

lib/platform-bible-react/src/components/toolbar.component.tsx line 8 at r4 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Please use a path alias :)

What alias? There is nothing under paths in tsconfig.json within this lib. I can add something there, but this is the first component that I can see in this lib that is referencing one of our custom hooks.

---

lib/platform-bible-react/src/components/toolbar.component.tsx line 76 at r4 (raw file):
At a high level, I don't want this task to be about redesigning this component. The current design seems to work OK with a menuProvider as it is now. If redesigning helps achieve some other goal, then let's tackle it when we're looking at that other goal. The whole "shift menu" design might not be what we want with UX, anyway. I know Paratext does it now, but it's not easily discoverable and perhaps should be rethought.

I can't think of a reason we would want/need to change the menuProvider once things are running. All the dynamism around inputs is being handled in the document combiner. Do you have something in mind?

I also lean toward having the parent manage when the menu data should change
I'm not sure if that's desirable long-term. That is, I think it is quite possible/likely that things that contribute menus may need to be dynamic in what they contribute, and the owner of the menu might not know (or shouldn't have to know) whenever a contributor has a change. Since those changes only matter when a menu is reopened, it's reasonable to avoid having anything be the "master tracking owner of when a menu should change" and just say "reevaluate what goes in a menu when it is opened." That's what the spec said, I believe.

I guess at a high level you're suggesting a "push" model, and in this case I think a "pull" model makes more sense given that we will have to have dynamic menus (e.g., deciding what items are enabled/disabled at any given point, displaying menu items based on which windows are open, etc.) and those who are contributing to menus can't always reasonably push.

---

lib/platform-bible-utils/src/menus.model.ts line 2 at r4 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Do any of these changes necessitate a change to the JSON schema? Just a reminder in case; no action needed unless you think of something

Not in this case, no. These changes came about because:

1. Tom pointed out something that was really a nuanced difference between TS types and JSON schema syntax/meaning.
2. I didn't do a good enough job in the first ticket about distinguishing between pre-localized types and post-localized types for menus.

None of my changes are meant to be an actual, structural change to the shape of the JSON documents. Instead they are a better reflection of TS types related to the JSON document contents and what we do with them.

---

src/extension-host/data/menu.data.json line 39 at r4 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Shouldn't this be contributed by the resource viewer?

Yes, it would make more sense. I'll move it.

---

src/extension-host/services/extension.service.ts line 754 at r4 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

It loos like this deletion of contributions by web view type isn't parallel to the addition of contributions by extension name. Should we keep an array of contribution names to remove here? Just go ahead and remove all contributions? Other?

At this point, the only menu contributors should be 1) the starting document from the platform, and 2) the documents provided by each extension. If we ever get to the point where there are other contributors, as well, then we will need this logic to be more detailed.

One complication to using a different list of things to remove is that we kind of need to know what the extensions WERE, not just what they are now. If someone deactivated an extension, we have no way of knowing that here.

---

src/extension-host/services/extension.service.ts line 762 at r4 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I wonder if we should make some kind of utility in here or somewhere that makes sure the path can't escape the extension's folder. Maybe we should put a TODO otherwise? File an issue? Other?

We could do it, but even if someone did point to something else what would we be worried about happening? The build-in JSON parser parsing some malicious JSON blob that wasn't distributed with a malicious or buggy extension? I'm not sure what the risk is here that it would mitigate.

---

src/extension-host/services/extension.service.ts line 771 at r4 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Looks like this is currently an un-awaited promise. Would you mind wrapping it in an IIFE with a try/catch or whatever seems appropriate to you?

Good catch. I just added an await.

---

src/extension-host/services/menu-data.service-host.ts line 44 at r4 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Is this intended to be able to be called on the network? I believe it is currently exposed on the network since it is on the engine. If you want to be able to rebuild just from the extension host, you probably can access it in a similar way to how you access the menuDocumentCombiner in the extension service.

I went back and forth on this. This design makes it more straightforward for extensions to be able to adjust their menu contributions later if/when we get to that point, but honestly I would be fine switching to the other approach. Do you have a preference?

---

src/extension-host/services/menu-data.service-host.ts line 47 at r4 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I think you need to notifyUpdate here to let subscribers know about the update to the data, right?

Done.

---

src/extension-host/services/menu-data.service-host.ts line 92 at r4 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Looks like this is probably here to handle null and undefined appropriately according to our deserialize. However, I see you're using menuDataObject directly in other places. Think it would be ok to use this instead? Or just make another const that is this deserialized version or something (I imagine probably caching the result would be best)

I just removed this function. It was created prior to the menu document combiner which does JSON schema validation. This function was, from what I could see, a very simple form of validation. At this point it doesn't really provide unique value. If the starting menu document is not valid, the menu combiner will throw separately.

---

src/renderer/components/platform-bible-menu.data.ts line 122 at r4 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I think I understand you did this to wrap the support menu in a promise. If so, alternatively I believe you can just make the function async and return the support menu directly or return Promise.resolve(supportMenu) :)

Yeah, you're right. While I was working on it, this function kept bouncing back and forth between async and not async. I guess I got lost in all the switching.

---

src/renderer/components/platform-bible-toolbar.tsx line 12 at r4 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Regarding reworking the menu retrieval stuff, in here, you could use useData(menuData.dataProviderName) to get the menu data and keep up with any updates that come out in the future instead of passing a function that retrieves it asynchronously when the menu opens. Then to prevent it from updating when the menu is open, I suppose you could keep a useState of the menu stuff and have a useEffect that updates the state to equal the retrieved menu data when the menu is closed.

I think doing it this way follows normal react data flow patterns and prevents us from having to disable hook dependency array checking. I'd say both of those things give a strong case for needing a rework since those things are probably very prone to lead to bugs.

I was trying to leave existing services and components alone as much as possible. Having said that, we could make these changes. I'm not sure we'll get much value out of it, though. I think it's possible we will end up with situations where contributed menu objects change once we need dynamic menu items. (That's why copyDocuments is false in the MenuDocumentCombiner constructor.) There is no way to know when an object changes, though, so a typical notification scheme can't be relied upon. Since menus are only shown rarely during the lifetime of a process, rebuilding the menus when someone wants to open a menu seemed reasonable.

We could enable the set functions in the data provider to do something, but that would kind of be confusing because the service itself shouldn't expose the setcalls on the network due to a lack of authentication. Instead what we would really need to do once we need dynamic menus is expose it only on the extension host and integrate with the activation token.

Perhaps to say this in another way, I don't think we can follow the normal react data flow pattern here without making the system a lot more complicated.

---

src/shared/services/menu-data.service-model.ts line 63 at r4 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

This was here to add JSDocs to the automatic setMainMenu function signature from IDataProvider<MenuDataDataTypes> (same with setWebviewMenu. I think it may be valuable to have, but it's also fine to leave it out. Maybe a bit more precise jsdoc description would be a bit clearer.

Anyway this is also why the getMainMenu signature with mainMenuType was there as well - just providing JSDoc for the IDataProvider<MenuDataDataTypes> function. Don't have to keep that either if you don't think it would be helpful. I think removing the useless parameter on getMainMenu adds an overload; I dunno if the JSDoc displays on both.

Well, this is weird. When I was doing my development, the set functions only showed up in Intellisense when they were defined in this interface. If I removed them, then they were gone from the menu data service calls. Now when I look that is no longer the case. I don't know if this was build related or something else, but I removed things that I didn't want to have show up in Intellisense because they make no sense.

Honestly I was pretty happy with what I was seeing. I wish we could hide the set calls and the getMainMenu call with a parameter because they no have meaning or value. Since I can't really do that, though, I'm reverting these deletions.

---

src/shared/services/menu-data.service-model.ts line 76 at r4 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Unfortunately I believe this parameter currently needs to be here for the subscribe function to work. It needs the first parameter to be the selector, which is basically nothing in this case. I think the description may have been a bit misleading.

Maybe we could rework selectors so you can just not pass a selector, but I imagine that's probably something to consider another day.

This was backed out with the other deletions per my previous comment in this file.

---

src/shared/utils/menu-document-combiner.ts line 165 at r4 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I'd argue it's the localization service's job to handle the percent signs - maybe worth discussing a bit and then making an issue?

The placeholder localization service isn't doing it, so I did it here. I could see it making sense for the localization service to take care of this, though.

[lyonsil]

Reviewable status: 39 of 46 files reviewed, 10 unresolved discussions (waiting on @tjcouch-sil)

---

src/extension-host/services/menu-data.service-host.ts line 44 at r4 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I went back and forth on this. This design makes it more straightforward for extensions to be able to adjust their menu contributions later if/when we get to that point, but honestly I would be fine switching to the other approach. Do you have a preference?

Actually after thinking about this one further, when dynamic menus come into play the renderer will probably need to call this. That means I should leave it on the network object.

[tjcouch-sil]

Reviewed 5 of 7 files at r6, all commit messages.
Reviewable status: 43 of 46 files reviewed, 4 unresolved discussions

---

lib/platform-bible-react/src/components/toolbar.component.tsx line 8 at r4 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

What alias? There is nothing under paths in tsconfig.json within this lib. I can add something there, but this is the first component that I can see in this lib that is referencing one of our custom hooks.

Oh yep, sorry. No alias needed. I need to pay closer attention to which file we're in.

---

src/extension-host/services/menu-data.service-host.ts line 92 at r4 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I just removed this function. It was created prior to the menu document combiner which does JSON schema validation. This function was, from what I could see, a very simple form of validation. At this point it doesn't really provide unique value. If the starting menu document is not valid, the menu combiner will throw separately.

Does the menu combiner make sure anything that's null is transformed to undefined? I imagine that was probably the intent of this function because I don't think this function is really buying much else since menuDataObject is already parsed JSON.

Maybe it doesn't matter, and we can just wait until a bug comes up (if ever) to deal with this problem. I'll unblock, and you can do something further if you want. Don't feel like you have to.

---

src/shared/services/menu-data.service-model.ts line 63 at r4 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Well, this is weird. When I was doing my development, the set functions only showed up in Intellisense when they were defined in this interface. If I removed them, then they were gone from the menu data service calls. Now when I look that is no longer the case. I don't know if this was build related or something else, but I removed things that I didn't want to have show up in Intellisense because they make no sense.

Honestly I was pretty happy with what I was seeing. I wish we could hide the set calls and the getMainMenu call with a parameter because they no have meaning or value. Since I can't really do that, though, I'm reverting these deletions.

Yeah, that would be nice to have! Sorry for the mix-ups :p

[tjcouch-sil]

Reviewed 1 of 35 files at r1, 2 of 7 files at r6.
Reviewable status: all files reviewed, 2 unresolved discussions

[lyonsil]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tjcouch-sil)

---

src/extension-host/services/extension.service.ts line 762 at r4 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

We could do it, but even if someone did point to something else what would we be worried about happening? The build-in JSON parser parsing some malicious JSON blob that wasn't distributed with a malicious or buggy extension? I'm not sure what the risk is here that it would mitigate.

Added a TODO here and in the node fs code

---

src/extension-host/services/menu-data.service-host.ts line 92 at r4 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Does the menu combiner make sure anything that's null is transformed to undefined? I imagine that was probably the intent of this function because I don't think this function is really buying much else since menuDataObject is already parsed JSON.

Maybe it doesn't matter, and we can just wait until a bug comes up (if ever) to deal with this problem. I'll unblock, and you can do something further if you want. Don't feel like you have to.

The combiner does not explicitly do this, no. However, I'd be hard pressed to come up with a situation where someone accidentally contributes null on a property but otherwise has a valid menu contribution that passes schema validation and the other checks.

[lyonsil]

Reviewable status: 45 of 47 files reviewed, 1 unresolved discussion (waiting on @tjcouch-sil)

---

src/extension-host/services/extension.service.ts line 754 at r4 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

At this point, the only menu contributors should be 1) the starting document from the platform, and 2) the documents provided by each extension. If we ever get to the point where there are other contributors, as well, then we will need this logic to be more detailed.

One complication to using a different list of things to remove is that we kind of need to know what the extensions WERE, not just what they are now. If someone deactivated an extension, we have no way of knowing that here.

My mistake here - you were right I was removed the wrong contributions. Fixed

[tjcouch-sil]

thanks!! So exciting!

Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

---

src/shared/utils/menu-document-combiner.ts line 165 at r4 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

The placeholder localization service isn't doing it, so I did it here. I could see it making sense for the localization service to take care of this, though.

I believe we discussed an issue for this - this is just a reminder :)

[lyonsil]

src/shared/utils/menu-document-combiner.ts line 165 at r4 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I believe we discussed an issue for this - this is just a reminder :)

#797