l3kn / org-fc

Spaced Repetition System for Emacs org-mode
https://www.leonrische.me/fc/index.html
GNU General Public License v3.0
258 stars 31 forks source link

Merge develop with main #106

Open cashpw opened 1 year ago

cashpw commented 1 year ago

Is there a plan for how and when the develop branch will be merged with main?

l3kn commented 1 year ago

I'll get to it when I find the time and motivation to do so. I sent you a mail regarding this a few days ago but maybe the address linked on your website was incorrect.

cashpw commented 1 year ago

Apologies -- it went to spam. I've replied.

l3kn commented 1 year ago

Sorry for the long delay.

I'd like to add a new feature which excludes whole files or all positions of a card from the shuffling before a review and looking through your changes, it seems like the special handling of single/enum cloze cards has been lost.

The design behind this is a bit unfortunate but I'd consider these as distinct from other card types where seeing one position would spoil the answers to the others, and being able to review multiple positions of such cards at once has become an integral part of how I use org-fc.

I think I can fit this into your refactored card / position classes but this raises the question of how we handle the "Author" and "Maintainer" comments of files we both work on. Are you fine with having both our names in files we both worked on? And do you think the "Maintainer" comment is necessary? It's not set in any of the other files.

l3kn commented 1 year ago

I'm also not sure about the "Keywords" and "Homepage" comments, the first seems unnecessary and the latter points to a page that doesn't exist. In addition, the GPL license text is missing.

cashpw commented 1 year ago

I think I can fit this into your refactored card / position classes but this raises the question of how we handle the "Author" and "Maintainer" comments of files we both work on. Are you fine with having both our names in files we both worked on? And do you think the "Maintainer" comment is necessary? It's not set in any of the other files.

https://github.com/l3kn/org-fc/issues/106#issuecomment-1331994933

I'm also not sure about the "Keywords" and "Homepage" comments, the first seems unnecessary and the latter points to a page that doesn't exist. In addition, the GPL license text is missing.

https://github.com/l3kn/org-fc/issues/106#issuecomment-1332001383

That boiler plate was automatically added, or perhaps I copy/pasted it from somewhere. I was blind to it but agree that it should be fixed up before we merge it in. I'll revise it to match the existing comments.

I'd like to add a new feature which excludes whole files or all positions of a card from the shuffling before a review and looking through your changes, it seems like the special handling of single/enum cloze cards has been lost.

The design behind this is a bit unfortunate but I'd consider these as distinct from other card types where seeing one position would spoil the answers to the others, and being able to review multiple positions of such cards at once has become an integral part of how I use org-fc.

https://github.com/l3kn/org-fc/issues/106#issuecomment-1331994933

Oh no! I haven't made much use of those types of cloze cards and didn't notice that they were broken. I agree that the situation you describe will spoil the card during review.

cashpw commented 1 year ago

I'd like to add a new feature which excludes whole files or all positions of a card from the shuffling before a review and looking through your changes, it seems like the special handling of single/enum cloze cards has been lost.

The design behind this is a bit unfortunate but I'd consider these as distinct from other card types where seeing one position would spoil the answers to the others, and being able to review multiple positions of such cards at once has become an integral part of how I use org-fc.

https://github.com/l3kn/org-fc/issues/106#issuecomment-1331994933

Can you elaborate on the specifics of this "special handling"? What is the expected behavior and how does #102 break it?

l3kn commented 1 year ago

One part is this line that is remove and that I can't find in the added code: https://github.com/l3kn/org-fc/pull/102/files#diff-e2e8952e1d5a3888caa7e9c4f18d1ccbd0c7f7af18c6c23c23778eef4b73a3d0L617

The main reason for removing sibling cards (other positions of a card where one position is reviewed) is to avoid “spoilers”. Having reviewed the front->back direction of a double card would make it much easier to review the back->front direction during the same session.

Cloze single / enum cards are special in that reviewing one position doesn't spoil the others (as long as they appear in order, in the case of the enum type).

This is a rare edge case but it's important to me because I'm using cloze-single cards for a form of incremental reading, where during the review of a file (corresponding to a book), I'd like to review all positions of all cards in the order they appear in the file.

As I understand it, this is slightly broken in another way by your changes to how items are shuffled. Currently the positions of a card are zipped with a list of sorted random numbers to ensure later positions appear later in the review, then all positions are sorted by their random number.

The original implementation was messy and evolved over time while I discovered different ways of structuring / reviewing cards that would be useful, but I'm struggling to come up with something more abstract or useful right now.

However that's a whole other issue, so to fix this one, would you be fine with me picking out the new card / position classes from this (with the same header / license as the other org-fc files, including you as the author), possibly throwing in another class for whole files and then fitting that into the current review implementation?

This hierarchy (file - card - position) should make it easier to implement other review ordering / grouping mechanisms in the future.

cashpw commented 1 year ago

Thank you for the detailed response! I'll try to integrate this into my new pull request (#107).