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

Merge develop with main #106

Open
cashpw opened this issue Oct 18, 2022 · 8 comments
Open

Merge develop with main #106

cashpw opened this issue Oct 18, 2022 · 8 comments

Comments

@cashpw
Copy link
Contributor

cashpw commented Oct 18, 2022

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

@l3kn
Copy link
Owner

l3kn commented Oct 18, 2022

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
Copy link
Contributor Author

cashpw commented Oct 18, 2022

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

@l3kn
Copy link
Owner

l3kn commented Nov 30, 2022

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
Copy link
Owner

l3kn commented Nov 30, 2022

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
Copy link
Contributor Author

cashpw commented Nov 30, 2022

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.

#106 (comment)

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.

#106 (comment)

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.

#106 (comment)

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
Copy link
Contributor Author

cashpw commented Dec 3, 2022

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.

#106 (comment)

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

@l3kn
Copy link
Owner

l3kn commented Dec 3, 2022

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
Copy link
Contributor Author

cashpw commented Dec 9, 2022

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

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

No branches or pull requests

2 participants