Download DBL resources data provider

[rolfheij-sil]

Note that installing/updating/removing resources (using the 'Get' button on the front-end) does not work without the proper DBL credentials in place, provided by DblResourcePasswordProvider.

This change is 

[rolfheij-sil]

Reviewable status: 0 of 7 files reviewed, 3 unresolved discussions

---

c-sharp/Projects/DigitalBibleLibrary/DblDownloadableDataProvider.cs line 89 at r1 (raw file):

                resource.FullName,
                resource.BestLanguageName,
                resource.Size,

For some reason this is always 0. I don't think a valid value is provided, ever.

---

c-sharp/Projects/DigitalBibleLibrary/DblDownloadableDataProvider.cs line 114 at r1 (raw file):

            return true;

        installableResource.Install();

This does return a boolean, but it doesn't tell us if the installation succeeded or not. It tells something about if extra fonts are required for this resource or not or something.

---

c-sharp/Projects/DigitalBibleLibrary/DblDownloadableDataProvider.cs line 138 at r1 (raw file):

        ScrTextCollection.DeleteProject(
            installableResource.ExistingScrText ?? installableResource.ExistingDictionary
        // Note that we don't get any info telling if uninstalling succeeded or failed

It returns void.

[lyonsil]

Looks good overall. I noted a few small things to consider.

Reviewed 6 of 7 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @rolfheij-sil)

---

c-sharp/Projects/DigitalBibleLibrary/DblDownloadableDataProvider.cs line 89 at r1 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

For some reason this is always 0. I don't think a valid value is provided, ever.

That is strange. I see several places in the PT9 code where the size is obtained, but most of those are looking at file sizes. Perhaps the size is only filled in after you download a resource and see how big it actually is? Probably not a blocking concern for now.

---

c-sharp/Projects/DigitalBibleLibrary/DblDownloadableDataProvider.cs line 138 at r1 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

It returns void.

Similar to the previous method, should we look after the refresh to ensure the project no longer exists? I could see an argument either way.

---

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

      group: 'paratext.sendReceive',
      order: 1,
      command: 'paratextBibleDownloadResources.openDownloadResources',

This looks like a duplicate with the menu item from menu.data.json. I must be missing something. As long as it isn't duplicated, ignore this. menu.data.json has localized strings, too, while this one does not. That's partly why I wonder if this one is even used.

---

c-sharp/Projects/DigitalBibleLibrary/DblDownloadableDataProvider.cs line 9 at r2 (raw file):

internal class DblResourcesDataProvider(PapiClient papiClient)
    : NetworkObjects.DataProvider("platformBibleDownloadResources.dblResourcesProvider", papiClient)

In general, we've been trying to use platform or an extension name for things to the left of the period. I know we started doing that after other things already existed, but if we can stick to that going forward it would be nice. Are you added a platformBibleDownloadResources extension later that is meant to expose this? If not, could we stick with platform?

---

c-sharp/Projects/DigitalBibleLibrary/DblDownloadableDataProvider.cs line 106 at r2 (raw file):

        if (!TryFindResource(DBLEntryUid, out var installableResource))
        {
            throw new Exception($"Resource not available from DBL");

Should this be provided by the localization service? Do you use this later in UI to be presented to users or just for logging? If it's only logging this is fine as-is. The same comment applies to other exceptions thrown in this file.

---

c-sharp/Projects/DigitalBibleLibrary/DblDownloadableDataProvider.cs line 115 at r2 (raw file):

        // Note that we don't get any info telling if the installation succeeded or failed
        installableResource.Install();

Are there cases where it could fail that you know of? If failing would always throw an exception, we're probably fine as-is. If failing could happen silently, do we need to see if the resource exists after the refresh below? It might be worth asking the PT9 team if they know of a way to get the real size for resources. I assume it would be nice to display the size before people download resources in case they have a slow internet connection or very little HD space available.

---

c-sharp/Projects/DigitalBibleLibrary/DblDownloadableDataProvider.cs line 118 at r2 (raw file):

        ScrTextCollection.RefreshScrTexts();
        SendDataUpdateEvent("DblResources", "DBL resources data updated");

Since the DblResources data type could be evented here or in the method after this, it would be good to define DblResources as a string const and reference it in both places.

---

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

        "group": "platform.projectResources",
        "order": 1,
        "command": "paratextBibleDownloadResources.openDownloadResources"

Should this whole menu item only exist in PT10S? It seems like we're pointing to a comment here that doesn't exist in P.B, so this menu item will fail to do anything. Same with the other updated command.

[lyonsil]

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

---

c-sharp/Projects/DigitalBibleLibrary/DblDownloadableDataProvider.cs line 9 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

In general, we've been trying to use platform or an extension name for things to the left of the period. I know we started doing that after other things already existed, but if we can stick to that going forward it would be nice. Are you added a platformBibleDownloadResources extension later that is meant to expose this? If not, could we stick with platform?

I see how this is referenced in your other PR. I think this calling thisplatform.dblResourcesProvider is more aligned with what we're trying to do with naming of network objects. @tjcouch-sil feel free to disagree.

---

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

Previously, lyonsil (Matt Lyons) wrote…

Should this whole menu item only exist in PT10S? It seems like we're pointing to a comment here that doesn't exist in P.B, so this menu item will fail to do anything. Same with the other updated command.

After looking at your other PR, I do think this menu item shouldn't exist in P.B and should move to that PR unless we're going to have a different command for P.B.

[rolfheij-sil]

Thanks for reviewing!

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

---

c-sharp/Projects/DigitalBibleLibrary/DblDownloadableDataProvider.cs line 89 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

That is strange. I see several places in the PT9 code where the size is obtained, but most of those are looking at file sizes. Perhaps the size is only filled in after you download a resource and see how big it actually is? Probably not a blocking concern for now.

I've checked the PT9 source code, and it seems like the Size isn't being set on the DBL resources. I see it's set for Marble resources, by looking at the filesize on disk, so that would only be available after downloading. We could probably make something similar, but that still wouldn't give us any info before downloading a resource.

---

c-sharp/Projects/DigitalBibleLibrary/DblDownloadableDataProvider.cs line 138 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Similar to the previous method, should we look after the refresh to ensure the project no longer exists? I could see an argument either way.

Done.

---

c-sharp/Projects/DigitalBibleLibrary/DblDownloadableDataProvider.cs line 9 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I see how this is referenced in your other PR. I think this calling thisplatform.dblResourcesProvider is more aligned with what we're trying to do with naming of network objects. @tjcouch-sil feel free to disagree.

Oops, I meant to call it paratextBibleDownloadResources.something. That's the naming pattern that the other extensions in the Paratext interal extensions repo also follow (paratextBibleMarketplaceand paratextBibleSendReceive). Are you okay with that?

---

c-sharp/Projects/DigitalBibleLibrary/DblDownloadableDataProvider.cs line 106 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Should this be provided by the localization service? Do you use this later in UI to be presented to users or just for logging? If it's only logging this is fine as-is. The same comment applies to other exceptions thrown in this file.

It would probably be good to let the user know something went wrong. Are we able to catch this on the front-end? Are we even able to present this anywhere except for in a development environment?

---

c-sharp/Projects/DigitalBibleLibrary/DblDownloadableDataProvider.cs line 115 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Are there cases where it could fail that you know of? If failing would always throw an exception, we're probably fine as-is. If failing could happen silently, do we need to see if the resource exists after the refresh below? It might be worth asking the PT9 team if they know of a way to get the real size for resources. I assume it would be nice to display the size before people download resources in case they have a slow internet connection or very little HD space available.

Yes, there are some resources that refuse to install, and no feedback is shown. I don't know how to handle those for now.
Installation is handled in a try-catch block in InstallableResource.cs line 293 and further. When installation fails an exception is thrown and handled right there.

I'll post a question about getting the size of the resources on the PT9 discord server.

---

c-sharp/Projects/DigitalBibleLibrary/DblDownloadableDataProvider.cs line 118 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Since the DblResources data type could be evented here or in the method after this, it would be good to define DblResources as a string const and reference it in both places.

Done.

---

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

Previously, lyonsil (Matt Lyons) wrote…

After looking at your other PR, I do think this menu item shouldn't exist in P.B and should move to that PR unless we're going to have a different command for P.B.

Sounds good. Since it was here already I assumed that's where it needed to be, but I agree it makes more sense to move it to the extension

---

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

Previously, lyonsil (Matt Lyons) wrote…

This looks like a duplicate with the menu item from menu.data.json. I must be missing something. As long as it isn't duplicated, ignore this. menu.data.json has localized strings, too, while this one does not. That's partly why I wonder if this one is even used.

I think this menu is shown in developer mode or something. But I'll rip it out of this one too.

[lyonsil]

Thanks for the update! That resolved most of my questions.

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @rolfheij-sil)

---

c-sharp/Projects/DigitalBibleLibrary/DblDownloadableDataProvider.cs line 89 at r1 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

I've checked the PT9 source code, and it seems like the Size isn't being set on the DBL resources. I see it's set for Marble resources, by looking at the filesize on disk, so that would only be available after downloading. We could probably make something similar, but that still wouldn't give us any info before downloading a resource.

Yeah, that's weird. I would think you'd want to know more about the size before you download than after, but it is what it is, I guess. We can always change this later if we need to.

---

c-sharp/Projects/DigitalBibleLibrary/DblDownloadableDataProvider.cs line 138 at r1 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

Done.

Thanks - similar to the previous method, I'm not sure if returning a bool is useful now.

---

c-sharp/Projects/DigitalBibleLibrary/DblDownloadableDataProvider.cs line 9 at r2 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

Oops, I meant to call it paratextBibleDownloadResources.something. That's the naming pattern that the other extensions in the Paratext interal extensions repo also follow (paratextBibleMarketplaceand paratextBibleSendReceive). Are you okay with that?

Sure, that's fine since there is an extension we're creating with that name. Since we don't have a way for our extensions to contribute to our C# DLL, putting it in this repo seems reasonable.

---

c-sharp/Projects/DigitalBibleLibrary/DblDownloadableDataProvider.cs line 106 at r2 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

It would probably be good to let the user know something went wrong. Are we able to catch this on the front-end? Are we even able to present this anywhere except for in a development environment?

This will get sent over the websocket as a JSON-RPC Error object. Then the message provided here will be rethrown in an error from the network service in the TS code. So, if you wrap your network calls in try/catch, you can get this exact error message in your code and display it if you would like to do so. I'm pretty sure TJ did it for a few of his calls. If you're going to do that, though, you'll want to lookup a localized error message string here.

---

c-sharp/Projects/DigitalBibleLibrary/DblDownloadableDataProvider.cs line 115 at r2 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

Yes, there are some resources that refuse to install, and no feedback is shown. I don't know how to handle those for now.
Installation is handled in a try-catch block in InstallableResource.cs line 293 and further. When installation fails an exception is thrown and handled right there.

I'll post a question about getting the size of the resources on the PT9 discord server.

Thanks for the update! Is there a point to the function returning a bool? It seems like we either throw or return true.

---

c-sharp/Projects/DigitalBibleLibrary/DblDownloadableDataProvider.cs line 118 at r2 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

Done.

Thanks!

---

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

Previously, rolfheij-sil (Rolf Heij) wrote…

Sounds good. Since it was here already I assumed that's where it needed to be, but I agree it makes more sense to move it to the extension

Thanks!

---

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

Previously, rolfheij-sil (Rolf Heij) wrote…

I think this menu is shown in developer mode or something. But I'll rip it out of this one too.

Thanks!

---

c-sharp/Projects/DigitalBibleLibrary/DblDownloadableDataProvider.cs line 135 at r3 (raw file):

        }

#pragma warning disable CS8602 // Dereference of a possibly null reference.

This definitely works, but you could get away with not disabling the warning and not having a redundant null check if you change TryFindResource to the following:

    private void FindResource(
        string dblEntryUid,
        string messageToThrowIfNotFound,
        out InstallableResource resource
    )
    {
        resource =
            _resources?.FirstOrDefault(r => r.DBLEntryUid.Id == dblEntryUid)
            ?? throw new Exception(messageToThrowIfNotFound);
    }

Then your calls to the function would look like this:

        FindResource(DBLEntryUid, $"Resource not available from DBL", out var installableResource);

It would be a little more compact and avoid using #pragmas. Again, though, this code is fine if you prefer it.

[lyonsil]

To be clear, I still have some open items, but everything remaining is optional from my standpoint. The most important one is probably getting localized strings if you want to use the exception messages inside of the UI.

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

[rolfheij-sil]

Thanks again! I was able to process all the feedback, including localization

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lyonsil)

---

c-sharp/Projects/DigitalBibleLibrary/DblDownloadableDataProvider.cs line 138 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Thanks - similar to the previous method, I'm not sure if returning a bool is useful now.

Fixed.

---

c-sharp/Projects/DigitalBibleLibrary/DblDownloadableDataProvider.cs line 106 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

This will get sent over the websocket as a JSON-RPC Error object. Then the message provided here will be rethrown in an error from the network service in the TS code. So, if you wrap your network calls in try/catch, you can get this exact error message in your code and display it if you would like to do so. I'm pretty sure TJ did it for a few of his calls. If you're going to do that, though, you'll want to lookup a localized error message string here.

I've added the try-catch blocks for install and uninstall on the TS side, but I don't know how to handle it for the getDblResources data provider. Or does the data provider have a way to handle things like that in itself?

---

c-sharp/Projects/DigitalBibleLibrary/DblDownloadableDataProvider.cs line 115 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Thanks for the update! Is there a point to the function returning a bool? It seems like we either throw or return true.

It would return false in some cases earlier, but throwing seems more reasonable. It returns void now.

---

c-sharp/Projects/DigitalBibleLibrary/DblDownloadableDataProvider.cs line 135 at r3 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

This definitely works, but you could get away with not disabling the warning and not having a redundant null check if you change TryFindResource to the following:

    private void FindResource(
        string dblEntryUid,
        string messageToThrowIfNotFound,
        out InstallableResource resource
    )
    {
        resource =
            _resources?.FirstOrDefault(r => r.DBLEntryUid.Id == dblEntryUid)
            ?? throw new Exception(messageToThrowIfNotFound);
    }

Then your calls to the function would look like this:

        FindResource(DBLEntryUid, $"Resource not available from DBL", out var installableResource);

It would be a little more compact and avoid using #pragmas. Again, though, this code is fine if you prefer it.

Nice. Done.

[lyonsil]

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

---

c-sharp/Projects/DigitalBibleLibrary/DblDownloadableDataProvider.cs line 106 at r2 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

I've added the try-catch blocks for install and uninstall on the TS side, but I don't know how to handle it for the getDblResources data provider. Or does the data provider have a way to handle things like that in itself?

Ah - good point. Data providers automatically try to grab new data when an update event is emitted. They have their own try/catch (from data-provider.service.ts):

      } catch (e) {
        const selectorDetails = JSON.stringify(selector) ?? '<undefined>';
        logger.warn(
          `Tried to retrieve data after an update event for ${dataType} with selector ${selectorDetails.substring(0, 120)}, but it threw. ${e}`,
        );
      }

So you're not going to see the error messages come back from get calls on data providers when the data updates. But you should still see the errors for the direct calls to things like InstallDblResource and UninstallDblResource since there isn't code that automatically wraps those like the get calls.