bastibe / org-journal

A simple org-mode based journaling mode
BSD 3-Clause "New" or "Revised" License
1.23k stars 122 forks source link

Improvements to org-journal-new-scheduled-entry and org-journal-reschedule-scheduled-entry #420

Closed TPKato closed 6 months ago

TPKato commented 8 months ago

This commit improves the behavior of org-journal-new-scheduled-entry and org-journal-reschedule-scheduled-entry. More specifically,

Related issues/pull requests:

Description:

Note that org-journal-reschedule-scheduled-entry now only changes a timestamp with "SCHEDULED: " (and the time in a heading). Bare timestamp <...> without "SCHEDULED: " remains untouched.

bastibe commented 8 months ago

From a quick glance, this looks reasonable. But I'm not using this feature (any more), so I can't comment on whether this breaks existing workflows or not.

jmay commented 8 months ago

@TPKato Could you possibly write a test case for this change? Maybe duplicate org-journal-scheduled-weekly-test and add something that verifies the new behavior.

TPKato commented 7 months ago

@jmay I tried to write the tests, but I'm not sure if I wrote them correctly because this is my first time writing tests with ERT. Feel free to modify or completely rewrite them ;-) (And the test for org-journal-reschedule-scheduled-entry is very minimal.)

TPKato commented 7 months ago

If we want to, we could apply the following additional patch to my pull request and introduce a new variable to make it less breaking (though I personally don't think this is necessary). Even with the following patch, it is still breaking, but for example * 20:00 TODO* TODO 20:00, or <2023-11-03 Fr 00:00><2023-11-03 Fr> (removing the unnecessary 00:00) would be considered bug fixes.

With this patch, the string "SCHEDULED:" will be inserted only if the user has set the variable (*1). If we consider it as an enhancement, this patch might be reasonable.

(*1) To make my PR less breaking, I've set the default to "", but I strongly believe it's better to set it to org-scheduled-string for the compatibility with org-mode. In this case, we can write something like this in CHANGELOG or README:

If you want to avoid inserting "SCHEDULED:" before the timestamp (like the old version of org-journal), you can set the value of org-journal-schdeuled-string to "" in your setup: (setq org-journal-schdeuled-string "")

(Sorry, the patch was incomplete and has been edited.)

--- a/org-journal.el
+++ b/org-journal.el
@@ -394,6 +394,10 @@ This prefix key is used for:
 - `org-journal-search' (key \"s\")"
   :type 'string)

+(defcustom org-journal-scheduled-string ""      ; or org-scheduled-string
+  "String added before a time stamp for schedules."
+  :type 'string)
+
 (defvar org-journal-after-entry-create-hook nil
   "Hook called after journal entry creation.")

@@ -1125,7 +1129,8 @@ With non-nil prefix argument create a regular entry instead of a TODO entry."
         (insert (format-time-string org-journal-time-format time)))
     (save-excursion
       (insert "\n"
-         org-scheduled-string " ") ; "SCHEDULED: "
+              org-journal-scheduled-string
+              (if (length> org-journal-scheduled-string 0) " " ""))
       (org-insert-time-stamp
        time org-time-was-given nil nil nil (list org-end-time-was-given)))))

@@ -1149,7 +1154,7 @@ With non-nil prefix argument create a regular entry instead of a TODO entry."
                 "\\(TODO \\)?"
                 "\\("
                 (replace-regexp-in-string "[[:digit:]]" "[[:digit:]]"
-                     (format-time-string org-journal-time-format '(0 0) t))
+                                          (format-time-string org-journal-time-format '(0 0) t))
                 "\\)")))
           (if (re-search-forward regexp (line-end-position) t)
               (progn
@@ -1158,11 +1163,14 @@ With non-nil prefix argument create a regular entry instead of a TODO entry."
                     (insert (format-time-string org-journal-time-format time))))))
         ;; update time of timestamp (<...>)
         (org-back-to-heading)
-        (if (re-search-forward (concat org-scheduled-string "[[:blank:]]*" org-ts-regexp) nil t)
+        (if (re-search-forward (concat (regexp-quote org-journal-scheduled-string)
+                                       "[[:blank:]]*" org-ts-regexp)
+                               nil t)
             (replace-match "")
           (org-end-of-subtree)
           (insert "\n"))
-        (insert org-scheduled-string " ")
+        (insert org-journal-scheduled-string
+                (if (length> org-journal-scheduled-string 0) " " ""))
         (org-insert-time-stamp time org-time-was-given nil nil nil (list org-end-time-was-given))
         (org-cut-subtree))
       (let (org-journal-carryover-items)
bastibe commented 7 months ago

Compatibility with different date formats is an issue I run into frequently. Any improvement on that front would be highly appreciated.

jmay commented 7 months ago

Combined this PR with @TPKato's patch and PR #422, submitting a separate PR with the results