l3kn / org-fc

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

[Feature] modifying cards while in review session #34

Closed natask closed 4 years ago

natask commented 4 years ago

Modifying a card during a review session is pivotal to updating cards fluidly as needed. Now that org-fc uses modes instead of hydras, this is a realizable goal. org fc implements the review session with two modes. evil's mapping in these two modes is not conducive to easily modifying text. There are two approaches, one is to change the default org fc command keymaps to keymaps that don't conflict with inputting text in evil mode or creating a function that disables org-fc mode until edits are complete.

The former would be clunky to use unless mapped onto a modifier key in which case it is a search for keybindings that are not needed for typing in text.

The later can be implemented and attached to a key shortcut accessible on all modes because it need to both be disabled and enabled and enabling needs that the key shortcut is accessible outside of the mode. This can be simplified by including a general org-fc mode that is always on during an org-fc-review session and map the key shortcut to that. This will have two shortcuts, one for each mode, that org-fc uses but it is possible to reduce to on shortcut and increase user experience by added logic. The logic is as follows. check if there is a mode that is currently enabled, if there is toggle that off and if not toggle the last toggled mode. This guarantees only that the most relevant org-fc internal mode is toggled.

Proof of concept

(defcustom last-org-fc-minor-mode nil
  "Last org fc minor mode that was enabled. Used to figure out logic."
  :group 'org-fc
  :type 'string)

(defvar org-fc-review-mode-map
  (let ((map (make-sparse-keymap)))
    (define-key map (kbd "C-c f") 'org-fc-review-toggle-to-edit)
    map)
  "Keymap for `org-fc-review-mode'.")

(define-minor-mode org-fc-review-mode

  :init-value nil
  :lighter " fc-rev"
  :keymap org-fc-review-mode-map
  :group 'org-fc
  (unless (and (eq major-mode 'org-mode) org-fc-review--current-session)
      (org-fc-review-mode -1)))

(defun org-fc-review-toggle-to-edit ()
  "Function for `org-fc-review-mode' that toggles minor modes."
    (interactive )
    (when org-fc-review-mode
    (if org-fc-review-flip-mode
      (progn 
       (org-fc-review-flip-mode -1)
       (message "[C-c f] Resume")
       (setq last-org-fc-minor-mode "flip")
       )
    (if org-fc-review-rate-mode
        (progn
         (org-fc-review-rate-mode -1)
         (message "[C-c f] Resume")
         (setq last-org-fc-minor-mode "rate")
         )
    (if (and (eq major-mode 'org-mode) org-fc-review--current-session)
     (pcase  last-org-fc-minor-mode
       ("flip" (org-fc-review-flip-mode 1))
       ("rate" (org-fc-review-rate-mode 1))
     ))
     ))))
l3kn commented 4 years ago

This is a feature I've wanted for a long time, I'm just not sure how to best implement it.

If I understand it correctly, this doesn't show any of the hidden parts of the buffer. Is that the behavior you'd prefer?

I was thinking of a kind of "resume review" feature where editing quits the review without setting the session to nil, so it can be resumed at the same point.

With that, org-fc-review could be changed to check if there is an active review session, and if so, offer to resume that session or start a new one.

It would be hard to resume at the exact point in the review (e.g. rate-mode on a text input card where the text has already been entered) but I wouldn't mind flipping the updated card again.

l3kn commented 4 years ago

A related problem is whether the buffer should be set to read-only during review, to avoid accidental changes by hitting the wrong keys.

I can only think of one "feature" I'm using that would break with this change, editing tags during review with C-c C-q, which can be solved with a new command that disables read-only mode before calling org-set-tags-command.

natask commented 4 years ago

If I understand it correctly, this doesn't show any of the hidden parts of the buffer. Is that the behavior you'd prefer?

I didn't think about that. I suppose there is no harm in showing the hidden parts as well, more specially the hidden parts of a card beyond more visual clutter. There maybe instances where showing the hidden parts is desirable and useful although I can't think of one. The reason I didn't think about them is because my use case stops at modifying cards that I think should be updated and that means the things that I have chosen to show about the card.

I was thinking of a kind of "resume review" feature where editing quits the review without setting the session to nil, so it can be resumed at the same point.

Are you also thinking that when a review is resumed, along side with loading up the last card viewed, It also updates the set of cards to quiz on as some may have become due? If so saving the last card's id (nil if finished session) then bringing it up on the pile of cards is one way to go about it. I was not sure what the output of org-fc-awk-index-paths was but now that I know that it includes the id in a queriable format, we can use that. Something like the following would work. I haven't run it so it may have bugs.

(defvar org-fc-last-id nil)

;; in quit or pause session
(setq org-fc-last-id (org-id-get))  ;;may not work if point is not on current id. maybe can get the latest card's id using (plist-get card :id)

;; in session completed
(setq org-fc-last-id nil)

  ;; in org-fc-review
    (let* ((index (org-fc-index context))
           (cards
            (org-fc-bring-last-reviewed-card-to-head ;; added line
            (org-fc-index-shuffled-positions
             (org-fc-index-filter-due index)))))

(defun org-fc-is-last-reviewed-card (card) 
  "check if id matches with last reviewed card."
   (let ((id (plist-get card :id)))
     (string= id org-fc-last-id)
   )
)
(defun org-fc-bring-last-reviewed-card-to-head (cards)
 "get the last_card out of cards and place it at head."
 (let ((last_card (cl-remove-if 'org-fc-is-last-reviewed-card cards)))
     (setq last_card (car last_card)) ;; handles issues when there are duplicate ids in a clumsy way
     (cons last_card (remove last_card cards))
  )
)

I can only think of one "feature" I'm using that would break with this change, editing tags during review with C-c C-q, which can be solved with a new command that disables read-only mode before calling org-set-tags-command.

I don't have much opinion on enabling read-only buffer. It is also possible to leave such commands to be executed after pausing org-fc. For me, a more annoying problem steaming from mistakes during org-fc-review-session is that sometimes I incorrectly press a rate key, setting something that I wanted to set "hard", "good" or "again", "suspend" for example. It would be nice to have an "undo" button that returns to previous state.

l3kn commented 4 years ago

Considering how short most editing "pauses" are going to be, do you think scanning for due cards again is worth the additional complexity? After importing thousands of cards new cards, even with the awk indexer scanning for due cards takes ~4s on my machine so that would break the review/edit flow for me.

I'm working on a "org-fc-cache" mode that keeps card data in memory and only processes files that have changed since the last review. That alleviates the problem but it's not production ready yet.

Regarding the "undo" button, I've opened a new issue for it: https://github.com/l3kn/org-fc/issues/35

l3kn commented 4 years ago

I think a good first solution would be to

  1. Add an edit / pause button that resets the buffer (exiting the org-fc modes & showing hidden parts) without resetting the review session
  2. Set a flag in the session to mark it as paused
  3. Add a org-fc-resume that resumes the review
  4. In org-fc-review, check if there is a paused session and offer to resume it
natask commented 4 years ago

Considering how short most editing "pauses" are going to be, do you think scanning for due cards again is worth the additional complexity? After importing thousands of cards new cards, even with the awk indexer scanning for due cards takes ~4s on my machine so that would break the review/edit flow for me.

I agree with you. Most use cases are going to fall under editing. I had imagined you were also considering handling more longer pauses where pausing a session and coming back to it after lunch or something.

It is still possible to handle that case with the same pausing logic but the down side is that new due cards won't be added. This may be good sometimes because cards that were reviewed in the incomplete session won't come up and that may be desirable.

For me, I am perfectly fine with quitting and restarting for these types of cases.

I think a good first solution would be to

  1. Add an edit / pause button that resets the buffer (exiting the org-fc modes & showing hidden parts) without resetting the review session

one additional thing is that when the buffer is reset, buffer scroll location should still be at the last card. The primary use for this is to modify cards on hand and keeping the most relevant card in visual sphere would be useful.

I worry that showing hidden parts of the buffer may be counter productive as it would increase visual clutter and may hint on the contents of other cards that are due to be reviewed. To me, showing other stuff may detract from the primary goal of this feature which is to modify incorrect stuff or add new content to the card at hand. What do you think about this?

l3kn commented 4 years ago

I'll try to implement this tomorrow, maybe we can make the showing of hidden parts configurable. I think it might be useful for cloze deletions, so both the hint and the content can be edited.

Also there can be card types where some/all of the contents of the card as shown during review are generated dynamically, so there would be no use in editing the review version.

For example with the multiple-choice card type I'm working on:

2020-06-27_multiple_choice

natask commented 4 years ago

I think it might be useful for cloze deletions, so both the hint and the content can be edited.

I see. That makes sense. I agree with your perspective on this.

l3kn commented 4 years ago

I've pushed an inital implementation. Notable changes from the code you've posted:

I still think card overlays should be removed during editing, but it would be nice to keep the headlines narrowed. That requires some changes to the way org-fc handles overlays and will be added later.

Can you try it out and let me know what you think / how it works for you? I'm open for discussion about all of the design decicions mentioned above.

l3kn commented 4 years ago

I've used this feature for a few weeks and it's working as expected, I'll close this issue for now.