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

Implementation of same-day reviews in 1, 5, and 10 minute intervals #569

Draft
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

psaunderualberta
Copy link

This PR implements the ability to review a card in 1 minute or 10 minute intervals, (partially) addressing issues #502 and #503. These minute intervals are hardcoded and so an interval of 2.5 minutes, for example, is not possible. Several major changes were required to implement such a feature. These include:

  1. Changing the 'interval attribute of a flashcard' from days to minutes
  2. Changing the format of the review date to include hours, minutes, & seconds
  3. Tracking flashcards in a heap rather than an array, using a custom comparison function (Deck.comparator in flashcard-modal.tsx)
    1. Required addition of heap.js NPM package.
  4. Changing content of the "Card" interface.

There are several topics I would like cleared up before I open this PR for review & approval. One thing I'm unclear on is the use of sibling cards. I would appreciate if other maintainers could explain the point of tracking sibling cards. Specifically, what's the point of lines 811-819 in flashcard-modal.tsx:

                // look for first unscheduled sibling
                const pickedCard: Card = this.newFlashcards[pickedCardIdx];
                let idx = pickedCardIdx;
                while (idx >= 0 && pickedCard.siblings.includes(this.newFlashcards[idx])) {
                    if (!this.newFlashcards[idx].isDue) {
                        modal.currentCardIdx = idx;
                    }
                    idx--;
                }

I also understand that it may be too early in the roadmap to implement, as the scheduling algorithm is being changed and that might want to be merged first. My hope in making this pull request is to get this feature implemented before or soon after the winter semester starts. I greatly enjoyed using this plugin in the fall semester, but found myself falling back on Anki due to the lack of same-day reviews. Having all flashcards in one place would make my own life and the lives of other users much easier.

Still TODO before approval:

  • Revert build location to build/main.js.
  • Include other languages for updated constant strings
  • Implement option to not randomize card review order. Currently, program forces randomization of flashcards
  • Update scheduling.test.ts to use minutes
  • Testing with multiple nested decks & flashcards.
  • Testing with sibling cards (what are these)
  • Ensuring ability to review notes has not changed.

Let me know if anyone has questions or concerns about this PR! I'd be happy to address them.

@psaunderualberta psaunderualberta changed the title Implementation of same-day reviews for 1 minute & 10 minutes. Implementation of same-day reviews in 1 minute & 10 minute intervals Dec 31, 2022
@AB1908
Copy link
Contributor

AB1908 commented Jun 6, 2023

One thing I'm unclear on is the use of sibling cards. I would appreciate if other maintainers could explain the point of tracking sibling cards.

Sibling cards are mostly from clozes AFAIK and they are used to track which cards are related so that they don't show up in the same review session. I think this also applies to reversed cards so that we don't end up testing ourselves when we already know the answer.

@psaunderualberta psaunderualberta changed the title Implementation of same-day reviews in 1 minute & 10 minute intervals Implementation of same-day reviews in 1, 5, and 10 minute intervals Jun 15, 2023
@psaunderualberta
Copy link
Author

Testing with scheduling.ts now uses minutes instead of days, achieves 100% branch & statement coverage.

@psaunderualberta
Copy link
Author

psaunderualberta commented Jun 15, 2023

@st3v3nmw Should this PR be put off until e2e testing is implemented by myself or others? With such a drastic change to the method of reviewing cards, I'm hesitant to leave it to manual testing to ensure confidence. This is especially true seeing as I didn't know what sibling cards are, and so there's likely other edge cases / card types that I'm not even aware are testable.

@AB1908
Copy link
Contributor

AB1908 commented Jun 15, 2023

You can release a beta version and ask users to opt in. See BRAT.

@AB1908
Copy link
Contributor

AB1908 commented Jun 15, 2023

E2E is very difficult in Obsidian last I tried so unit tests are probably your best approach followed by manual testing.

@psaunderualberta
Copy link
Author

psaunderualberta commented Jun 17, 2023

Okay, I've set up the first release for Beta testing here. How would I go about asking users to opt-in? I don't think at-ing everying following the repo is a great idea.

@AB1908
Copy link
Contributor

AB1908 commented Jun 18, 2023

You could post it on Discord and ask for people to try it out. I'll probably end up doing that myself for my rewrite.

@psaunderualberta
Copy link
Author

I'm happy to announce that this feature is now available as a Beta. The feature is still in early stages, and I'm looking for beta users to help validate functionality / try and break it. Specifically, there are several types of flashcards (i.e. Clozes, Card contexts, images, etc.) that I don't use. Therefore, it would be much appreciated if someone who uses those types regularly can vet the feature's functionality. Of course, being in the beta stage, there will likely be bugs. Please be sure to back up your vault before using this feature, or just make a separate vault to experiment with.

Special thanks to AB1908 for all their help in getting the feature ready for release as a beta.

The plugin must be installed manually or from BRAT with this link: https://github.com/psaunderualberta/obsidian-spaced-repetition/. Installing should work, but please message me if it doesn't.

Copy link
Owner

@st3v3nmw st3v3nmw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thanks for the contribution 😄!

Sorry the underlying code has gone through a major refactor recently. I'm not sure if you're still available to help resolve the merge conflicts.

Stephen,

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use pnpm instead

@psaunderualberta
Copy link
Author

Hi Stephen,

I'm currently in the midst of my Master's degree, so it is unlikely I'll get to fixing the merge conflicts during October or November. However, I have no finals this semester (yay!) and so I should be able to get to it in December. Let me know if anything changes before then.

Thanks,
Paul

@notno
Copy link

notno commented Sep 15, 2024

Can I ask what the status of this is? Is there anything I could do to support?

@psaunderualberta
Copy link
Author

Can I ask what the status of this is? Is there anything I could do to support?

I haven't worked on this in a little while, but seeing as it's Hacktoberfest I'll probably have a look at it & see if it makes sense to continue working on this branch. Given the amount of time that's elapsed since I last touched it, I might just restart development on a different branch to avoid any lingering bugs from something I forgot to change. Once that's done, I might have a better idea of what needs to change & I can get back to you with any help that I'd need.

@notno
Copy link

notno commented Oct 19, 2024

Once that's done, I might have a better idea of what needs to change & I can get back to you with any help that I'd need.

Sounds good,fingers crossed that it's a smooth merge

@notno
Copy link

notno commented Nov 3, 2024

@psaunderualberta I'm looking at the codebase and seeing how much has changed since you worked on this, so I'm going to give it a fresh go and see what I can do. Would be happy to step back if you're committed to working on this, though :)

@psaunderualberta
Copy link
Author

@psaunderualberta I'm looking at the codebase and seeing how much has changed since you worked on this, so I'm going to give it a fresh go and see what I can do. Would be happy to step back if you're committed to working on this, though :)

Thanks, I got way too busy during October to dive into this. Let me know if you need some help or would like to offload some tasks, as otherwise it seems you're more interested in working on it than myself.

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

Successfully merging this pull request may close these issues.

4 participants