Select Scripture when you click a check result #1345
Conversation
There was a problem hiding this comment.
Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @tjcouch-sil)
extensions/src/platform-scripture/src/checking-results-list.web-view.tsx line 218 at r1 (raw file):
if (!selectedRow || !editorWebViewController) return; logger.info(JSON.stringify(selectedRow));
I'm guessing this was accidentally left in from debugging. This should probably at least move to logger.debug and add some more context if we're going to leave it in.
extensions/src/platform-scripture/src/checking-results-list.web-view.tsx line 237 at r1 (raw file):
offset: startOffset, }; let end: ScriptureLocation = { ...start, offset: start.offset ?? 0 };
@irahopkinson We probably need some clarification on the right way to specify a SelectionRange if the end isn't known/specified. It would be nice if the default was to automatically select to the end of verse, but either way I can see us potentially ending up with something unexpected/not useful if offsets are identical in the start and end of the range selection.
extensions/src/platform-scripture-editor/src/platform-scripture-editor.web-view.tsx line 105 at r1 (raw file):
switch (method) { case 'selectRange': logger.info(`selectRange targetScrRef ${serialize(targetScrRef)} ${serialize(range)}`);
Another case that should be logger.debug?
extensions/src/platform-scripture-editor/src/platform-scripture-editor.web-view.tsx line 108 at r1 (raw file):
if (compareScrRefs(scrRef, targetScrRef) !== 0) { // Need to update scr ref. Give it some time before setting the range
What does give it some time mean here exactly? Should we be awaiting something?
extensions/src/platform-scripture-editor/src/types/platform-scripture-editor.d.ts line 2 at r1 (raw file):
declare module 'platform-scripture-editor' { import type { SelectionRange } from 'shared-react/annotation/selection.model';
Looks like ts is unhappy about these new type references for some reaosn
extensions/src/platform-scripture-editor/src/types/platform-scripture-editor.d.ts line 19 at r1 (raw file):
/** * Position in Scripture. See {@link CheckLocation} for more information as this is mostly a
I'm wondering if we need to have a cleanup ticket where we settle on exactly where we use ScriptureReference and where we use VerseRef. They seem redundant, and I know we've used VerseRef back and forth with the .NET process, so I'm not sure what the cross-process concern is exactly.
extensions/src/platform-scripture-editor/src/types/platform-scripture-editor.d.ts line 25 at r1 (raw file):
* Also added `bookNum` and `chapterNum` to the `jsonPath` result */ export type ScriptureLocation =
I wonder if we ought to define a type like this in platform-scripture and have checks use it instead of defining a separate type.
extensions/src/platform-scripture-editor/src/main.ts line 175 at r1 (raw file):
async selectRange(range) { try { logger.info(
Similar to my other comment, I'm guessing we don't want to leave this as-is in production code. Maybe change to logger.debug?
tjcouch-sil
force-pushed
the
454-check-results-navigation
branch
from
1454d82 to
4bb2b0b
Compare
There was a problem hiding this comment.
Reviewable status: 2 of 6 files reviewed, 8 unresolved discussions (waiting on @irahopkinson and @lyonsil)
extensions/src/platform-scripture/src/checking-results-list.web-view.tsx line 218 at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
I'm guessing this was accidentally left in from debugging. This should probably at least move to
logger.debugand add some more context if we're going to leave it in.
Removed - thanks!
extensions/src/platform-scripture/src/checking-results-list.web-view.tsx line 237 at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
@irahopkinson We probably need some clarification on the right way to specify a
SelectionRangeif the end isn't known/specified. It would be nice if the default was to automatically select to the end of verse, but either way I can see us potentially ending up with something unexpected/not useful if offsets are identical in the start and end of the range selection.
FYI I believe the built-in browser range always has a start and end. They have the same node and offset if there is nothing selected. Feels a bit less ambiguous to me.
Side note, the browser selection has anchor and focus instead of start and end so it is directional. Interesting
Selection - Web APIs | MDN
Range - Web APIs | MDN
extensions/src/platform-scripture-editor/src/main.ts line 175 at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
Similar to my other comment, I'm guessing we don't want to leave this as-is in production code. Maybe change to
logger.debug?
Got it, thanks!
extensions/src/platform-scripture-editor/src/platform-scripture-editor.web-view.tsx line 105 at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
Another case that should be
logger.debug?
Got it, thanks!
extensions/src/platform-scripture-editor/src/platform-scripture-editor.web-view.tsx line 108 at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
What does
give it some timemean here exactly? Should we be awaiting something?
Updated! Hopefully that clarifies things :)
extensions/src/platform-scripture-editor/src/types/platform-scripture-editor.d.ts line 2 at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
Looks like
tsis unhappy about these new type references for some reaosn
Yep, fixed. Thanks!
extensions/src/platform-scripture-editor/src/types/platform-scripture-editor.d.ts line 19 at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
I'm wondering if we need to have a cleanup ticket where we settle on exactly where we use
ScriptureReferenceand where we useVerseRef. They seem redundant, and I know we've usedVerseRefback and forth with the .NET process, so I'm not sure what the cross-process concern is exactly.
VerseRef is a class, and classes don't rehydrate into classes across processes but just stay as an object that has whatever the class's toJSON returns. We could possibly craft into our serialize and deserialize a way to rehydrate specific hard-coded classes like VerseRef; I made an issue about that somewhere a long time ago.
ScriptureReference is a simple object with the three specific properties. No fanciness needed. I think I remember that C#'s VerseRef can deserialize ScriptureReferences, but I dunno about serializing into ScriptureReferences; I just don't remember if we write those properties or not.
Anyway, probably wouldn't hurt to make an issue for considering how to clean these up.
extensions/src/platform-scripture-editor/src/types/platform-scripture-editor.d.ts line 25 at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
I wonder if we ought to define a type like this in
platform-scriptureand have checks use it instead of defining a separate type.
I would be interested in that! I figured maybe we could reconsider all the checks types at once sometime in a follow-up issue for fixing the offsets and such. Does that seem reasonable, or do you want to do something sooner?
Maybe we could add it to this issue #1215 ?
There was a problem hiding this comment.
Reviewed 2 of 4 files at r2, all commit messages.
Reviewable status: 4 of 6 files reviewed, 2 unresolved discussions (waiting on @irahopkinson and @tjcouch-sil)
extensions/src/platform-scripture-editor/src/platform-scripture-editor.web-view.tsx line 108 at r1 (raw file):
Previously, tjcouch-sil (TJ Couch) wrote…
Updated! Hopefully that clarifies things :)
Yep, thanks!
extensions/src/platform-scripture-editor/src/types/platform-scripture-editor.d.ts line 19 at r1 (raw file):
Previously, tjcouch-sil (TJ Couch) wrote…
VerseRefis a class, and classes don't rehydrate into classes across processes but just stay as an object that has whatever the class'stoJSONreturns. We could possibly craft into ourserializeanddeserializea way to rehydrate specific hard-coded classes likeVerseRef; I made an issue about that somewhere a long time ago.
ScriptureReferenceis a simple object with the three specific properties. No fanciness needed. I think I remember that C#'sVerseRefcan deserializeScriptureReferences, but I dunno about serializing intoScriptureReferences; I just don't remember if we write those properties or not.Anyway, probably wouldn't hurt to make an issue for considering how to clean these up.
Interestingly I realized yesterday afternoon that the rehydration problem with VerseRef is what is causing #1171. I should have a change to deserialize coming up shortly for that purpose.
Having said this, it still seems error prone to have two types that mean basically the same thing but aren't the same. When should we use VerseRef and when should we use ScriptureReference? It seems like we're going to end up with a mishmash in the end. It's our own constructed version of undefined and null.
extensions/src/platform-scripture-editor/src/types/platform-scripture-editor.d.ts line 25 at r1 (raw file):
Previously, tjcouch-sil (TJ Couch) wrote…
I would be interested in that! I figured maybe we could reconsider all the checks types at once sometime in a follow-up issue for fixing the offsets and such. Does that seem reasonable, or do you want to do something sooner?
Maybe we could add it to this issue #1215 ?
I updated the description of that issue to include looking at similar types.
There was a problem hiding this comment.
Reviewed 2 of 4 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @irahopkinson and @tjcouch-sil)
extensions/src/platform-scripture/src/checking-results-list.web-view.tsx line 237 at r1 (raw file):
Previously, tjcouch-sil (TJ Couch) wrote…
FYI I believe the built-in browser range always has a start and end. They have the same node and offset if there is nothing selected. Feels a bit less ambiguous to me.
Side note, the browser selection has anchor and focus instead of start and end so it is directional. Interesting
It would be nice to get @irahopkinson 's thoughts here before merging if possible.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @irahopkinson and @lyonsil)
extensions/src/platform-scripture-editor/src/types/platform-scripture-editor.d.ts line 19 at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
Interestingly I realized yesterday afternoon that the rehydration problem with
VerseRefis what is causing #1171. I should have a change todeserializecoming up shortly for that purpose.Having said this, it still seems error prone to have two types that mean basically the same thing but aren't the same. When should we use
VerseRefand when should we useScriptureReference? It seems like we're going to end up with a mishmash in the end. It's our own constructed version ofundefinedandnull.
Interesting!
Here's the issue I was talking about - apparently not about classes in general but about one specific class that doesn't even exist yet XD #56
Yes, agreed; we already have the angst between these two types. It would be nice to have it tightened up.
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved
Resolves #454
WebViewControllers for Scripture editorsselectRangethat you pass USJ or USFM range specification, and it will select that location in the ScriptureWebViewProvidertoWebViewFactoryKnown issues:
This change is