Hook in web view context into web view menu command call

[tjcouch-sil]

• Added web view id to web view menu commands
• Moved projectId to saved web view definition
• Spread saved web view definition into web view props instead of just updatable props and run function to access
• Allowed getting saved web view definitions from web view service
• Exercised this stuff with hello world extension
  • Moved web view service projectId onto web view definition
  • Added web view menu items to the hello world project web view to delete the project or open a viewer for that project

Resolves #866

Note: this PR builds on #895. Please get that in before this one. Thanks!

---

This change is 

[lyonsil]

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

---

extensions/src/hello-world/contributions/localizedStrings.json line 9 at r1 (raw file):

      "%mainMenu_createNewHelloWorldProject%": "Create New Hello World Project",
      "%mainMenu_deleteHelloWorldProject%": "Delete Hello World Project",
      "%helloWorld_webViewMenu_project%": "Project",

Are there any simple cases like "Project" and "View" where you'd want to use some of the fallback options to use other localized strings? Seems like %mainMenu_project% is what you might want to either use directly instead of %helloWorld_webViewMenu_project% or fall back if there isn't another localized provided.

---

extensions/src/hello-world/contributions/menus.json line 6 at r1 (raw file):

    "groups": {
      "helloWorld.helloWorldProjects": {
        "order": 1.5,

I was thinking that it might be nice to either establish a convention where an extension uses the same decimal value in all orders to avoid collisions with other extensions OR we make a change to the menu service to automatically calculate a decimal value through some hash code to avoid collisions. I think the convention might be better to give us more determinism between extensions. What do you think? If you agree with the convention, maybe update the order to be something like 1.8765 here, 1000.8765 for helloWorld.projectSubmenu, 1.8765 for mainMenu_openHelloWorldProject, etc.

---

extensions/src/hello-world/src/main.ts line 217 at r1 (raw file):

    try {
      return await papi.storage.readUserData(context.executionToken, allProjectDataStorageKey);
    } catch {

Is there a reason to try/catch here instead of just letting the error flow through if it happens?

---

extensions/src/hello-world/src/main.ts line 391 at r1 (raw file):

    await helloWorldPdpefPromise,
    await helloWorldProjectWebViewProviderPromise,
    await helloWorldProjectViewerProviderPromise,

I was looking at all these sequential awaits and thinking about whether this is the pattern we'd want others to use. Do you think we should use a different pattern that leverages Promise.all to demonstrate better error handling?

---

src/renderer/services/web-view.service-host.ts line 640 at r1 (raw file):

  // If update properties aren't specified, keep the original values
  WEBVIEW_DEFINITION_UPDATABLE_PROPERTY_KEYS.forEach((key) => {
    if (key in updateInfo && updatedWebViewDefinition[key] !== updateInfo[key]) {

One of these values is projectIds, and a different array will always not be equal even if the array contents are the same. Do we need to check for that here?

[tjcouch-sil]

Reviewable status: 21 of 28 files reviewed, 5 unresolved discussions (waiting on @lyonsil)

---

extensions/src/hello-world/contributions/localizedStrings.json line 9 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Are there any simple cases like "Project" and "View" where you'd want to use some of the fallback options to use other localized strings? Seems like %mainMenu_project% is what you might want to either use directly instead of %helloWorld_webViewMenu_project% or fall back if there isn't another localized provided.

Ah probably a good call. I put a couple fallbacks in! I do wonder if it would be good to make a new issue for a couple things, though:

1. localized string browser webview - maybe we can add an endpoint to the localized string service for querying it or something. Search string, pagination, etc. I can't imagine it would take very long to do all this. Just thinking it is rather challenging to find other appropriate localized strings right now even for me as a core dev who knows where all the files are.
2. no-fallback mode - I realized I don't think there's any way to tell which strings are using fallbacks in the UI. I'm thinking a no-fallback mode (maybe just a setting) would be helpful for ui translators who want to go in and play around with the ui to figure out which strings are high priority to translate

---

extensions/src/hello-world/contributions/menus.json line 6 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I was thinking that it might be nice to either establish a convention where an extension uses the same decimal value in all orders to avoid collisions with other extensions OR we make a change to the menu service to automatically calculate a decimal value through some hash code to avoid collisions. I think the convention might be better to give us more determinism between extensions. What do you think? If you agree with the convention, maybe update the order to be something like 1.8765 here, 1000.8765 for helloWorld.projectSubmenu, 1.8765 for mainMenu_openHelloWorldProject, etc.

Maybe we could alphabetize by extension name hash and then let order be individual to each extension...? I dunno; I had a note down to talk to you about this too haha it is very annoying. Another option we could consider is to let the order be like it is now but do some kind of collision avoidance (like hash alphabetize) instead of rejecting menus when there are collisions. My biggest issue after having used it a bit is that the contribution is rejected if the order already exists, and it's nearly impossible to know if another extension has the same order as yours.

---

extensions/src/hello-world/src/main.ts line 217 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Is there a reason to try/catch here instead of just letting the error flow through if it happens?

readUserData throws if the file doesn't exist. I suppose we could change that if we want! I guess undefined could take its place since an empty text file would probably return an empty string. I dunno what an empty binary file would return though🤔 maybe an empty Buffer?

---

extensions/src/hello-world/src/main.ts line 391 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I was looking at all these sequential awaits and thinking about whether this is the pattern we'd want others to use. Do you think we should use a different pattern that leverages Promise.all to demonstrate better error handling?

I think it would probably be worth taking some time to consider various options and going with something that will serve us well longer-term. This is a mess.

One thing I have considered is having people add to the registrations a tuple [registrationPromise: Promise<Unsubscriber>, name: string]. Then we could do nice Promise.all and make everything all nice and report errors in a way that is understandable. Then we could also use that name when running the unsubscribers when deactivating the extension instead of just telling them "hey the unsubscriber at index 2 (which by the way doesn't actually match your index 2 because we add deactivate as the 0th index and maybe some other stuff before your registrations) is throwing". But I'd say there are a number of options to consider, and I don't know that there is an obvious best solution right offhand.

Maybe we can file an issue and consider further when we can?

---

src/renderer/services/web-view.service-host.ts line 640 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

One of these values is projectIds, and a different array will always not be equal even if the array contents are the same. Do we need to check for that here?

I decided to go with the way React does it, which is reference equality ===. I don't think it really matters much offhand. I'd say I'm more concerned that people would push to the array or something without actually updating the array than I am concerned that they would get an extra meaningless update, but I guess they'll probably notice it doesn't work if they just push. Hopefully, people will be used to immutable state because of React and will treat this right 😬

Would also be interested to hear your opinion on if it makes sense to have two different fields projectId?: string and projectIds?: string[] or if we should just have one projectIds?: string | string[] or even just projectIds?: string[] in web view definitions. Part of me just wants one place to get the information about relevant project ids. But I do like being able to specify that you only deal with one project id at all. I dunno

[tjcouch-sil]

Reviewable status: 21 of 28 files reviewed, 5 unresolved discussions (waiting on @lyonsil)

---

src/renderer/services/web-view.service-host.ts line 640 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I decided to go with the way React does it, which is reference equality ===. I don't think it really matters much offhand. I'd say I'm more concerned that people would push to the array or something without actually updating the array than I am concerned that they would get an extra meaningless update, but I guess they'll probably notice it doesn't work if they just push. Hopefully, people will be used to immutable state because of React and will treat this right 😬

Would also be interested to hear your opinion on if it makes sense to have two different fields projectId?: string and projectIds?: string[] or if we should just have one projectIds?: string | string[] or even just projectIds?: string[] in web view definitions. Part of me just wants one place to get the information about relevant project ids. But I do like being able to specify that you only deal with one project id at all. I dunno

While implementing these changes in the various extensions, I noticed a couple things:

• It's really nice not to have to determine if the field is a string or an array literally everywhere I use it
• It might be nice in some cases (like in the text collection) to have projectIds and then have one special projectId. In the case of the text collection, projectIds is all the projects open in verse view on the left and projectId is the specific project that's open in chapter view on the right. So menu commands for the text collection would work on the project open in chapter view. Pretty nice

Maybe it is worth having two fields :p

[lyonsil]

extensions/src/hello-world/src/main.ts line 217 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

readUserData throws if the file doesn't exist. I suppose we could change that if we want! I guess undefined could take its place since an empty text file would probably return an empty string. I dunno what an empty binary file would return though🤔 maybe an empty Buffer?

It looks like this all bubbles up from fs.promises.readFile. It seems like we would want to have clearer ways to determine kinds of failures. "File not found" vs other exceptions (e.g., "device I/O error", "invalid execution token", etc.). Maybe this is just part of whatever we do with enhancing the storage API.

[lyonsil]

All my comments are resolved well enough. I answered one of your questions and am OK whichever approach you'd prefer to take.

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

---

extensions/src/hello-world/contributions/localizedStrings.json line 9 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Ah probably a good call. I put a couple fallbacks in! I do wonder if it would be good to make a new issue for a couple things, though:

1. localized string browser webview - maybe we can add an endpoint to the localized string service for querying it or something. Search string, pagination, etc. I can't imagine it would take very long to do all this. Just thinking it is rather challenging to find other appropriate localized strings right now even for me as a core dev who knows where all the files are.
2. no-fallback mode - I realized I don't think there's any way to tell which strings are using fallbacks in the UI. I'm thinking a no-fallback mode (maybe just a setting) would be helpful for ui translators who want to go in and play around with the ui to figure out which strings are high priority to translate

Good ideas - those probably would be helpful.

---

extensions/src/hello-world/contributions/menus.json line 6 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Maybe we could alphabetize by extension name hash and then let order be individual to each extension...? I dunno; I had a note down to talk to you about this too haha it is very annoying. Another option we could consider is to let the order be like it is now but do some kind of collision avoidance (like hash alphabetize) instead of rejecting menus when there are collisions. My biggest issue after having used it a bit is that the contribution is rejected if the order already exists, and it's nearly impossible to know if another extension has the same order as yours.

My main hesitation with using a hash to settle ordering is that you can't deterministically set order how you'd like it to be. There will always be some seeming randomness to ordering.

Resolving this for now, but I do think we'll need to do something.

---

extensions/src/hello-world/src/main.ts line 217 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

It looks like this all bubbles up from fs.promises.readFile. It seems like we would want to have clearer ways to determine kinds of failures. "File not found" vs other exceptions (e.g., "device I/O error", "invalid execution token", etc.). Maybe this is just part of whatever we do with enhancing the storage API.

Added comment to #898

---

extensions/src/hello-world/src/main.ts line 391 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I think it would probably be worth taking some time to consider various options and going with something that will serve us well longer-term. This is a mess.

One thing I have considered is having people add to the registrations a tuple [registrationPromise: Promise<Unsubscriber>, name: string]. Then we could do nice Promise.all and make everything all nice and report errors in a way that is understandable. Then we could also use that name when running the unsubscribers when deactivating the extension instead of just telling them "hey the unsubscriber at index 2 (which by the way doesn't actually match your index 2 because we add deactivate as the 0th index and maybe some other stuff before your registrations) is throwing". But I'd say there are a number of options to consider, and I don't know that there is an obvious best solution right offhand.

Maybe we can file an issue and consider further when we can?

#905

---

src/renderer/services/web-view.service-host.ts line 640 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

While implementing these changes in the various extensions, I noticed a couple things:

• It's really nice not to have to determine if the field is a string or an array literally everywhere I use it
• It might be nice in some cases (like in the text collection) to have projectIds and then have one special projectId. In the case of the text collection, projectIds is all the projects open in verse view on the left and projectId is the specific project that's open in chapter view on the right. So menu commands for the text collection would work on the project open in chapter view. Pretty nice

Maybe it is worth having two fields :p

Yeah it's a little weird to have 2 fields. I think it would be a little cleaner to just have the array. If you want to make one of values in the array special, just specify it's the first item in the array. No need to make two different variables. But it's not a huge deal from my point of view. It just leaves things a little ambiguous (e.g., if I only have 1 project ID, should I also create an array with 1 item?, what does it mean as a general statement if I set both variables?, does it ever make sense to have the single project ID set to a value that isn't included in the array of project ID values?).

[tjcouch-sil]

Reviewable status: complete! all files reviewed, all discussions resolved

---

extensions/src/hello-world/contributions/localizedStrings.json line 9 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Good ideas - those probably would be helpful.

#909
#910

Think these would have any priority to speak of? Or forget about them as they fade into obscurity in the depths of the IDeaths Bucket?

---

extensions/src/hello-world/contributions/menus.json line 6 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

My main hesitation with using a hash to settle ordering is that you can't deterministically set order how you'd like it to be. There will always be some seeming randomness to ordering.

Resolving this for now, but I do think we'll need to do something.

#911 - uh oh! Look at that issue number! I guess it's time to call the police!

---

extensions/src/hello-world/src/main.ts line 391 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

#905

Thanks!

---

src/renderer/services/web-view.service-host.ts line 640 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Yeah it's a little weird to have 2 fields. I think it would be a little cleaner to just have the array. If you want to make one of values in the array special, just specify it's the first item in the array. No need to make two different variables. But it's not a huge deal from my point of view. It just leaves things a little ambiguous (e.g., if I only have 1 project ID, should I also create an array with 1 item?, what does it mean as a general statement if I set both variables?, does it ever make sense to have the single project ID set to a value that isn't included in the array of project ID values?).

We discussed and decided to go with just projectIds. Having both doesn't have clear inherent meaning.

[tjcouch-sil]

Reviewable status: complete! all files reviewed, all discussions resolved

---

src/renderer/services/web-view.service-host.ts line 640 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

We discussed and decided to go with just projectIds. Having both doesn't have clear inherent meaning.

Sadly, I encountered enough annoyances and weird things while implementing that I'm starting to be convinced it may be best to keep both fields again... Would you mind taking a look at my writeup and letting me know what you think please? Thanks! I will say I think having projectId not in projectIds is my weakest and least intuitive case of all. Basically I just said "don't worry about it", but that probably isn't a great answer haha I guess we could constrain projectId to be one of projectIds if projectIds is defined, but I don't know that I really want to work that hard to make sure that is the case haha might not really be worth worrying about.

I kinda wonder if it would be worth changing the name projectId to primaryProjectId, but I dunno if it really matters. Might be a bit confusing either way. Maybe also renaming projectIds to allProjectIds and forcing primaryProjectId into allProjectIds if it isn't there would make it a bit more self-explanatory, but it's still a weird data model to have these two independent fields that have interconnected meaning. Maybe just sticking with the simpler case of not worrying about it is best...?

[lyonsil]

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

---

extensions/src/hello-world/contributions/localizedStrings.json line 9 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

#909
#910

Think these would have any priority to speak of? Or forget about them as they fade into obscurity in the depths of the IDeaths Bucket?

I added them to the Internationalization epic. I think that's probably the best way to track/prioritize them appropriately over time. Right now they are not high enough priority to do anything with IMO, but when we look back at what we can/might do for different areas it is good to recognize the things we discovered about an epic after creating all the original tickets for it.

---

extensions/src/hello-world/contributions/menus.json line 6 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

#911 - uh oh! Look at that issue number! I guess it's time to call the police!

I added this to the Menus epic.

---

src/renderer/services/web-view.service-host.ts line 640 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Sadly, I encountered enough annoyances and weird things while implementing that I'm starting to be convinced it may be best to keep both fields again... Would you mind taking a look at my writeup and letting me know what you think please? Thanks! I will say I think having projectId not in projectIds is my weakest and least intuitive case of all. Basically I just said "don't worry about it", but that probably isn't a great answer haha I guess we could constrain projectId to be one of projectIds if projectIds is defined, but I don't know that I really want to work that hard to make sure that is the case haha might not really be worth worrying about.

I kinda wonder if it would be worth changing the name projectId to primaryProjectId, but I dunno if it really matters. Might be a bit confusing either way. Maybe also renaming projectIds to allProjectIds and forcing primaryProjectId into allProjectIds if it isn't there would make it a bit more self-explanatory, but it's still a weird data model to have these two independent fields that have interconnected meaning. Maybe just sticking with the simpler case of not worrying about it is best...?

I read through your writeup and understand the concerns. I think my concern with projectId and projectIds might come down to the fact that variable names projectId and projectIds aren't quite as useless as calling them string and strings, but it isn't far from there.

Perhaps if we change their names to something like selectedProjectId and projectIdsToOpen that might clarify things better? Honestly if we can't come up with unambiguous meanings that will be obvious to devs, cover all the cases we have in mind clearly, and don't leave other cases without good solutions, then it seems like we're thinking about this incorrectly. Like maybe each webview just defines its own custom context object type with whatever fields it wants that its own commands understand and expect. Then everyone doesn't have to figure out their own interpretations of things and isn't locked into just these one or two values. They just write commands that expect to work with the specific object type they define.

I guess where I am with things is that we just have projectId and leave it up to anything that needs multiple projects to sort it out some other way or we come up with something that is less ambiguous than having two variables with nearly identical names and purposes. It seems like the case of having a single project ID is common enough that we can just lock that in, but anything else starts to get custom based on the extension/webview.

[tjcouch-sil]

Reviewable status: 23 of 28 files reviewed, 1 unresolved discussion (waiting on @lyonsil)

---

extensions/src/hello-world/contributions/localizedStrings.json line 9 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I added them to the Internationalization epic. I think that's probably the best way to track/prioritize them appropriately over time. Right now they are not high enough priority to do anything with IMO, but when we look back at what we can/might do for different areas it is good to recognize the things we discovered about an epic after creating all the original tickets for it.

Sounds good. Thanks!

---

extensions/src/hello-world/contributions/menus.json line 6 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I added this to the Menus epic.

Thanks!

---

src/renderer/services/web-view.service-host.ts line 640 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I read through your writeup and understand the concerns. I think my concern with projectId and projectIds might come down to the fact that variable names projectId and projectIds aren't quite as useless as calling them string and strings, but it isn't far from there.

Perhaps if we change their names to something like selectedProjectId and projectIdsToOpen that might clarify things better? Honestly if we can't come up with unambiguous meanings that will be obvious to devs, cover all the cases we have in mind clearly, and don't leave other cases without good solutions, then it seems like we're thinking about this incorrectly. Like maybe each webview just defines its own custom context object type with whatever fields it wants that its own commands understand and expect. Then everyone doesn't have to figure out their own interpretations of things and isn't locked into just these one or two values. They just write commands that expect to work with the specific object type they define.

I guess where I am with things is that we just have projectId and leave it up to anything that needs multiple projects to sort it out some other way or we come up with something that is less ambiguous than having two variables with nearly identical names and purposes. It seems like the case of having a single project ID is common enough that we can just lock that in, but anything else starts to get custom based on the extension/webview.

Oh haha I hadn't read your comment yet and didn't realize you had already suggested just having projectId 😂 anyway I went ahead and removed projectIds.

[lyonsil]

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

[Sebastian-ubs]

Reviewable status: complete! all files reviewed, all discussions resolved

---

extensions/src/hello-world/contributions/menus.json line 6 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Thanks!

Is there a knowledge / decision document somewhere about this menu structure? I had in mind we decided that an extension cannot plugin to any location in the menu, but maybe I am wrongly remembering things here or not getting the problem you are discussing...

[tjcouch-sil]

Reviewable status: complete! all files reviewed, all discussions resolved

---

extensions/src/hello-world/contributions/menus.json line 6 at r1 (raw file):

Previously, Sebastian-ubs wrote…

Is there a knowledge / decision document somewhere about this menu structure? I had in mind we decided that an extension cannot plugin to any location in the menu, but maybe I am wrongly remembering things here or not getting the problem you are discussing...

We discussed the menu design extensively in meetings long ago as it sounds like you remember. During those meetings (unfortunately, I'm not sure if it's documented anywhere), the consensus shifted from no extensibility into other menus to allowing extensibility into other menus.

Here are some comments on the menu design doc that indicate the shift from before our meetings to after our meetings. Dunno if there's much more than that. Maybe on another document:

• https://docs.google.com/document/d/1N9BVj5w1b3H90Sjjc1lRN2zbalL-beDHmh4jxxExtiw/edit?disco=AAAA4GzM2-o
• https://docs.google.com/document/d/1N9BVj5w1b3H90Sjjc1lRN2zbalL-beDHmh4jxxExtiw/edit?disco=AAAA3cCR3IA (note the "cannot" in the original comment text vs the "can" in the document text)
• https://docs.google.com/document/d/1N9BVj5w1b3H90Sjjc1lRN2zbalL-beDHmh4jxxExtiw/edit?disco=AAAA2eUYKN8