Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Checks: Add "multiplexer" so that multiple web views can configure check service #1232

Closed
lyonsil opened this issue Oct 21, 2024 · 6 comments · Fixed by #1319
Closed

Checks: Add "multiplexer" so that multiple web views can configure check service #1232

lyonsil opened this issue Oct 21, 2024 · 6 comments · Fixed by #1319
Assignees

Comments

@lyonsil
Copy link
Member

lyonsil commented Oct 21, 2024

#1222 implies multiple UIs will be configuring the check service, so we need a way for them all to communicate without overwriting settings from each other.

@lyonsil lyonsil added the needs design Waiting on further design decisions and clarification label Oct 21, 2024
@lyonsil lyonsil moved this to 📥 For Consideration in Paranext Oct 21, 2024
@lyonsil lyonsil added this to Paranext Oct 21, 2024
@CraigNisbett CraigNisbett moved this from 📥 For Consideration to 🎬 Product Backlog in Paranext Oct 24, 2024
@tjcouch-sil
Copy link
Member

tjcouch-sil commented Nov 4, 2024

I propose the following:

  • Change the check service so its apis revolve around uses of checks/ranges/projects. The check service is the multiplexer that determines what combinations of checks/ranges/projects need to be run. No more enable/disable checks and ranges endpoints...? Do we need them anymore?

And one of the following:

  1. Change data provider subscriptions so subscribing actually informs the data provider of subscribes and unsubscribes so it can tell what data is being used (I guess we could also add a way to send an identifier to inform the data provider of who is using the data). Use this to determine what checks/ranges/projects need to be run. This would possibly also be useful in other contexts as well.
  2. Add apis to the check service so consumers of checks inform the check service when they want to use its data and when they are done using it or want to change what they're using. Pretty much the same as item 1 but kinda a shortcut.

In either case, we will probably need to tune some things up like closing web views so they actually run the unsubscribers. I don't expect they do now.

EDIT: I was linking to issue #1 on accident, but I meant to refer to item 1. Fixed

@irahopkinson
Copy link
Contributor

Is it too early to consider using YJS docs for these settings? You can observe that doc for changes. And it would be moving in the right direction so that collab just works.

@lyonsil
Copy link
Member Author

lyonsil commented Nov 5, 2024

This is primarily about configuring the check service, not storing settings. Setting storage will play a part in where things get persisted, but for the most part we're trying to decide how to take (currently) anonymous data subscribers for check results and make it so each can independently turn on/off checks on a per project and scripture range basis.

I chatted with TJ in Discord, and the central idea suggested (as I understand it, though it isn't explicitly called out here) is making the anonymous subscribers no longer be anonymous. Every subscriber will have an ID. I think having an ID here probably makes sense, and the existing check service can manage subscriptions based on those IDs.

Having said all this, I'm not sure I agree with dropping all the other endpoints. Instead it seems to me that the selectors (which are basically ignored now) can start to be used and be based on the subscription IDs.

@CraigNisbett CraigNisbett removed the needs design Waiting on further design decisions and clarification label Nov 8, 2024
@lyonsil
Copy link
Member Author

lyonsil commented Nov 8, 2024

I think we can fix #1171 as part of this issue because we're going to have to be reworking the relevant part of the code anyway.

@jolierabideau
Copy link
Contributor

May also be able to fix #1244 as part of this issue

@lyonsil lyonsil self-assigned this Nov 8, 2024
@katherinejensen00 katherinejensen00 moved this from 🎬 Product Backlog to 🔖 ToDo in Paranext Nov 13, 2024
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Paranext Nov 22, 2024
@roopa0222
Copy link

Test Repo steps:
I got the latest code and verified that multiple projects can have checks configured on different books and ranges.

  1. Project > Open Scripture Editor > zzz6
  2. Set BCV to Exodus 1:1
  3. zzz6 > Configure Checks..
  4. Check Capitalization and Chapter/Verse Numbers check and get the subscription ID from the console log
    5.zzz6 > Show Check Results.. and provide the subscription ID
  5. Confirmed that all the results related to zzz6 are displayed
  6. Repeat the same with project zzz5.
  7. Set the BCV to Mattew 1:1
  8. zzz5 > Configure Checks..
  9. Check Capitalization and Punctuation check and get the subscription ID from the console log
    11.zzz5 > Show Check Results.. and provide the subscription id
  10. Confirm that the Results are displayed only for the book of Matthew.

Image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

6 participants