#460 Make the Resource Viewer editable as a proof-of-concept Scripture editor

[FoolRunning]

Note that to test this, the WEB project needs to be editable.

---

This change is 

[tjcouch-sil]

Awesome!! Thanks! Note: Tim and I discussed that he will look into putting this work in the ParatextPDP as well though that is probably not easily testable for the time being. Will probably need to test in #448 or something

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

[lyonsil]

How does one make a project editable?

The USFM data provider is a good place to start. I wonder if it would be good to apply these changes to ParatextProjectStorageInterpreter.cs, SetProjectData at the same time. I expect the USFM data provider to go away not long after the tech demo. It was just a placeholder to start.

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

---

c-sharp/NetworkObjects/UsfmDataProvider.cs line 99 at r1 (raw file):

    }

    public ResponseToRequest SetUsx(VerseRef vref, string newUsx)

I'm sensing the potential benefit of some USX helper classes. This is more of an FYI for the future than any change that is needed now, but I think it would be nice to have something that understands the inner details of USX separate from the data provider class itself.

[tjcouch-sil]

Awesome! Thanks for the hard work getting this updated! Just one thing that is probably a typo.

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

---

extensions/src/usfm-data-provider/index.d.ts line 209 at r2 (raw file):

     * @returns unsubscriber function (run to unsubscribe from listening for updates)
     */
    subscribeVerseUSX(

I think this should be subscribeChapterUSX, right?

[lyonsil]

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

---

c-sharp/Projects/ProjectStorageInterpreter.cs line 58 at r2 (raw file):

                return GetProjectData(dataScope);
            case "setProjectData":
                var setProjectReturn = SetProjectData(dataScope, args[1]!.ToJsonString());

It looks like you changed all these to pass through JsonNodes instead of strings, but then you just call ToString() on the other side. This also requires pulling in more using statements, mixing both System.Text.Json and Newtonsoft.Json in the same file. This has caused bugs for us in the past because there are some classes with identical names, creating some ambiguity about what classes you're actually pulling in when working with JSON. Since you aren't actually using the JsonNode properties or functions on the other side, please revert these changes and pass strings instead of the JsonNode objects.

[lyonsil]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @FoolRunning)

---

c-sharp/Projects/ParatextProjectStorageInterpreter.cs line 1 at r2 (raw file):

using System.Net.Http.Json;

Why is this being included? If we can keep the number of JSON-related imports down, that would be nice.

---

c-sharp/Projects/ParatextProjectStorageInterpreter.cs line 276 at r2 (raw file):

    /// <summary>
    /// Converts usfm to usx, but does not annotate

What are the implications of "does not annotate"? Should we file an "Ideas" issue to improve our USX transformations in the future?

[lyonsil]

Reviewed 1 of 5 files at r2.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @FoolRunning)

[FoolRunning]

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

---

c-sharp/Projects/ParatextProjectStorageInterpreter.cs line 1 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Why is this being included? If we can keep the number of JSON-related imports down, that would be nice.

See my comment below on why this was deemed necessary.

---

c-sharp/Projects/ParatextProjectStorageInterpreter.cs line 276 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

What are the implications of "does not annotate"? Should we file an "Ideas" issue to improve our USX transformations in the future?

The comment was copied from the Paratext code. It, indeed, makes no sense here.

---

c-sharp/Projects/ProjectStorageInterpreter.cs line 58 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

It looks like you changed all these to pass through JsonNodes instead of strings, but then you just call ToString() on the other side. This also requires pulling in more using statements, mixing both System.Text.Json and Newtonsoft.Json in the same file. This has caused bugs for us in the past because there are some classes with identical names, creating some ambiguity about what classes you're actually pulling in when working with JSON. Since you aren't actually using the JsonNode properties or functions on the other side, please revert these changes and pass strings instead of the JsonNode objects.

This was done on purpose since the previous ToJsonString call that was passed to the SetProjectData method is the thing that escapes all of the USX data (i.e. not a good thing to pass into SetProjectData). I chose to pass in the whole JsonNode so that the various handlers can determine how the argument should be processed. If I, instead, changed the ToJsonString to be ToString, this would mean that we'd have to re-parse the JSON string if it was actually a JSON object that was needed in the handler.

---

extensions/src/usfm-data-provider/index.d.ts line 209 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I think this should be subscribeChapterUSX, right?

Yep. Good catch.

[lyonsil]

Reviewed 2 of 4 files at r3, all commit messages.
Reviewable status: 7 of 9 files reviewed, 5 unresolved discussions (waiting on @FoolRunning and @tjcouch-sil)

---

c-sharp/NetworkObjects/UsfmDataProvider.cs line 72 at r3 (raw file):

    }

    private ResponseToRequest GetUsx(string args)

There is already a GetUsx below. To help avoid confusion between identical function names, can we either call this something else or rename the other function?

---

c-sharp/Projects/ParatextProjectStorageInterpreter.cs line 1 at r2 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

See my comment below on why this was deemed necessary.

This is not the same as JsonNode which you reference below. That's from the System.Text.Json references. Why System.Net.Http.Json, too? That's 3 different JSON libraries in one file. Do you really need System.Net.Http.Json?

---

c-sharp/Projects/ProjectStorageInterpreter.cs line 58 at r2 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

This was done on purpose since the previous ToJsonString call that was passed to the SetProjectData method is the thing that escapes all of the USX data (i.e. not a good thing to pass into SetProjectData). I chose to pass in the whole JsonNode so that the various handlers can determine how the argument should be processed. If I, instead, changed the ToJsonString to be ToString, this would mean that we'd have to re-parse the JSON string if it was actually a JSON object that was needed in the handler.

It's not a good thing to have both Newtonsoft.Json and System.Text.Json in the same file for reasons I mentioned. Can you just change the ToJsonString calls to ToString so we don't have to add the other references by passong JsonNodes? Also, are you sure that the change to ToString from ToJsonString didn't effect any of the other, existing functions?

[lyonsil]

Reviewable status: 7 of 9 files reviewed, 5 unresolved discussions (waiting on @FoolRunning and @tjcouch-sil)

---

c-sharp/NetworkObjects/UsfmDataProvider.cs line 72 at r3 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

There is already a GetUsx below. To help avoid confusion between identical function names, can we either call this something else or rename the other function?

Same with SetUsx.

[FoolRunning]

Reviewable status: 6 of 9 files reviewed, 3 unresolved discussions (waiting on @lyonsil and @tjcouch-sil)

---

c-sharp/NetworkObjects/UsfmDataProvider.cs line 72 at r3 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Same with SetUsx.

Done

---

c-sharp/Projects/ParatextProjectStorageInterpreter.cs line 1 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

This is not the same as JsonNode which you reference below. That's from the System.Text.Json references. Why System.Net.Http.Json, too? That's 3 different JSON libraries in one file. Do you really need System.Net.Http.Json?

Didn't notice that. Removed.

---

c-sharp/Projects/ProjectStorageInterpreter.cs line 58 at r2 (raw file):

Can you just change the ToJsonString calls to ToString so we don't have to add the other references by passong JsonNodes?

Fine.

Also, are you sure that the change to ToString from ToJsonString didn't effect any of the other, existing functions?

As long as the node is actually a value that is a string, this is best. However, this will fail to work if we ever have a handler that needs a JSON object since I think ToString will fail for a non-value node.

[lyonsil]

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tjcouch-sil)

[tjcouch-sil]

thank you! Sorry I was holding up the PR - I didn't notice til now

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

---

c-sharp/Projects/ProjectStorageInterpreter.cs line 58 at r2 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

Can you just change the ToJsonString calls to ToString so we don't have to add the other references by passong JsonNodes?

Fine.

Also, are you sure that the change to ToString from ToJsonString didn't effect any of the other, existing functions?

As long as the node is actually a value that is a string, this is best. However, this will fail to work if we ever have a handler that needs a JSON object since I think ToString will fail for a non-value node.

That sounds like a problem - I imagine we will want to do JSON when we make the USJ endpoints. But I'd rather get this in and worry about it later honestly 😬