Merge branch 'doc' into fix-partial-updates

CHANGELOG.md

@@ -11,6 +11,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Changed
 3. Display graph ranges in `chronometrist-spark` column
 4. `chronometrist-tags-add` and `chronometrist-key-values-unified-prompt` now also work interactively.
+### Fixed
+5. Partial update of `chronometrist-events` takes midnight-spanning intervals into account.
 
 ## [0.8.1] - 2021-06-01
 ### Changed

elisp/chronometrist.el

@@ -709,12 +709,12 @@ Return value is a ts struct (see `ts.el')."
     (ts-apply :hour h :minute m :second s
               (chronometrist-iso-timestamp-to-ts timestamp))))
 
-(defun chronometrist-events-maybe-split (event)
-  "Split EVENT if it spans midnight.
-Return a list of two events if EVENT was split, else nil."
-  (when (plist-get event :stop)
-    (let ((split-time (chronometrist-midnight-spanning-p (plist-get event :start)
-                                             (plist-get event :stop)
+(defun chronometrist-events-maybe-split (plist)
+  "Split PLIST if it spans midnight.
+Return a list of two events if PLIST was split, else nil."
+  (when (plist-get plist :stop)
+    (let ((split-time (chronometrist-midnight-spanning-p (plist-get plist :start)
+                                             (plist-get plist :stop)
                                              chronometrist-day-start-time)))
       (when split-time
         (let ((first-start  (plist-get (cl-first  split-time) :start))
@@ -723,8 +723,8 @@ Return a list of two events if EVENT was split, else nil."
               (second-stop  (plist-get (cl-second split-time) :stop))
               ;; plist-put modifies lists in-place. The resulting bugs
               ;; left me puzzled for a while.
-              (event-1      (cl-copy-list event))
-              (event-2      (cl-copy-list event)))
+              (event-1      (cl-copy-list plist))
+              (event-2      (cl-copy-list plist)))
           (list (-> event-1
                     (plist-put :start first-start)
                     (plist-put :stop  first-stop))
@@ -752,6 +752,30 @@ If REPLACE is non-nil, replace the last event with PLIST."
          (append it (list plist))
          (puthash date it chronometrist-events))))
 
+(defun chronometrist-events-add-helper (key plist table)
+  "Append PLIST to values for KEY in TABLE.
+TABLE must be a hash table where the value for KEY is a list."
+  (--> (append (gethash key table) (list plist))
+   (puthash key it table)))
+
+(cl-defun chronometrist-events-add (plist &optional (table chronometrist-events))
+  "Add PLIST to the end of TABLE, splitting PLIST if necessary."
+  (-let* ((start-ts          (chronometrist-iso-timestamp-to-ts
+                              (plist-get plist :start)))
+          (start-date        (ts-format "%F" start-ts))
+          (start-date-events (gethash start-date table))
+          (split             (chronometrist-events-maybe-split plist))
+          ((split-1 split-2) split)
+          (((&plist :start old-start :stop old-stop)
+            (&plist :start new-start :stop new-stop)) split)
+          (new-start-date    (when new-start
+                               (ts-format "%F"
+                                (chronometrist-iso-timestamp-to-ts new-start)))))
+    (if (null split)
+        (chronometrist-events-add-helper start-date plist table)
+      (chronometrist-events-add-helper start-date split-1 table)
+      (chronometrist-events-add-helper new-start-date split-2 table))))
+
 (defun chronometrist-events-last-date ()
   "Return an ISO-8601 date string for the latest date present in `chronometrist-events'."
   (--> (hash-table-keys chronometrist-events)
@@ -1225,8 +1249,9 @@ value of `revert-buffer-function'."
 
 (defun chronometrist-refresh-file (fs-event)
   "Procedure run when `chronometrist-file' changes.
-Re-read `chronometrist-file', update `chronometrist-events', and
-refresh the `chronometrist' buffer."
+Run `chronometrist-file-change-hook', re-read
+`chronometrist-file' to update `chronometrist-events', and refresh
+the `chronometrist' buffer."
   (run-hooks 'chronometrist-file-change-hook)
   ;; (message "chronometrist - file %s" fs-event)
   (-let* (((descriptor action _ _) fs-event)
@@ -1252,7 +1277,7 @@ refresh the `chronometrist' buffer."
                    ((&plist :name new-task)  (chronometrist-sexp-last)))
              (pcase change
                (:append ;; a new plist was added at the end of the file
-                (chronometrist-events-update (chronometrist-sexp-last))
+                (chronometrist-events-add (chronometrist-sexp-last))
                 (chronometrist-add-to-task-list new-task))
                (:modify ;; the last plist in the file was changed
                 (chronometrist-events-update (chronometrist-sexp-last) t)

elisp/chronometrist.org

@@ -87,18 +87,42 @@ There are still some operations which [[* refresh-file][=chronometrist-refresh-f
 [fn:2] As indicated by exploratory work in the =parsimonious-reading= branch, where I made a loop to only =read= and collect s-expressions from the file. It was near-instant...until I added event splitting to it.
 
 *** Determine type of change made to file
-Most changes, whether made through user-editing or by Chronometrist commands, happen at the end of the file. We try to detect the kind of change made - whether the last expression was modified, removed, or whether a new expression was added to the end - and make the corresponding change to =chronometrist-events=, instead of doing a full parse again (=chronometrist-events-populate=). The increase in responsiveness has been significant.
+:PROPERTIES:
+:CUSTOM_ID: explanation-optimization-type-of-change
+:END:
+Most changes, whether made through user-editing or by Chronometrist commands, happen at the end of the file. We try to detect the three most common kinds of changes -
+1. insertion of a plist at the end of the file
+2. modification of the last plist in the file
+3. removal of the last plist in the file
+Then we make the corresponding change to =chronometrist-events=, instead of doing a full parse again (=chronometrist-events-populate=). The increase in responsiveness has been significant.
 
-When =chronometrist-refresh-file= is run by the file system watcher, it uses =chronometrist-file-hash= to assign indices and a hash to =chronometrist--file-state=. The next time the file changes, =chronometrist-file-change-type= compares this state to the current state of the file to determine the type of change made.
+When =chronometrist-refresh-file= is run by the file system watcher, it assigns indices and a hash to =chronometrist--file-state= (using =chronometrist-file-hash=). The next time the file changes, =chronometrist-file-change-type= compares this state to the current state of the file to determine the type of change made.
 
 Challenges -
 1. Correctly detecting the type of change
 2. Updating =chronometrist-task-list= and the Chronometrist buffer, when a new task is added or the last interval for a task is removed (v0.6.4)
 3. Handling changes made to an active interval after midnight
    * use the date from the plist's =:start= timestamp instead of the date today
-   * =:append= - normally, add to table; for spanning intervals, invalid operation
-   * =:modify= - normally, replace in table; for spanning intervals, split and replace
-   * =:remove= - normally, remove from table; for spanning intervals, split and remove
+   * an added plist can be
+     |          |                   | action        |
+     | complete | non-spanning      | simple add    |
+     | complete | midnight spanning | split and add |
+     | active   | non-spanning      | simple add    |
+     | active   | midnight spanning | split and add |
+   * in case of modification, there is an old plist and a new plist
+     |          |                   | action for old | action for new |
+     | complete | non-spanning      | remove one     | add one        |
+     | complete | midnight spanning | remove two     | add two        |
+     | active   | non-spanning      | remove one     | add one        |
+     | active   | midnight spanning | remove two     | add two        |
+   * in case of removal -
+     |          |                   | action        |
+     | complete | non-spanning      | simple remove |
+     | complete | midnight spanning | remove two    |
+     | active   | non-spanning      | simple remove |
+     | active   | midnight spanning | remove two    |
+
+=chronometrist-events-maybe-split= returns nil for an active plist. Does this result in any undesired behavior?
 
 ** Midnight-spanning intervals
 :PROPERTIES:
@@ -149,7 +173,7 @@ After hacking, always test for and ensure the following -
 :END:
 A quick description, starting from the first time [[* chronometrist-report][=chronometrist-report=]] is run in an Emacs session -
 1. We get the current date as a ts struct, using =chronometrist-date=.
-2. The variable =chronometrist-report-week-start-day= stores the day we consider the week to start with. The default is "Sunday".
+2. The custom variable =chronometrist-report-week-start-day= stores the day we consider the week to start with. The default is "Sunday".
 
    We check if the date from #2 is on the week start day, else decrement it till we are, using =(chronometrist-report-previous-week-start)=.
 3. We store the date from #3 in the global variable =chronometrist-report--ui-date=.
@@ -1544,12 +1568,12 @@ Return value is a ts struct (see `ts.el')."
 
 *** events-maybe-split                                           :function:
 #+BEGIN_SRC emacs-lisp
-(defun chronometrist-events-maybe-split (event)
-  "Split EVENT if it spans midnight.
-Return a list of two events if EVENT was split, else nil."
-  (when (plist-get event :stop)
-    (let ((split-time (chronometrist-midnight-spanning-p (plist-get event :start)
-                                             (plist-get event :stop)
+(defun chronometrist-events-maybe-split (plist)
+  "Split PLIST if it spans midnight.
+Return a list of two events if PLIST was split, else nil."
+  (when (plist-get plist :stop)
+    (let ((split-time (chronometrist-midnight-spanning-p (plist-get plist :start)
+                                             (plist-get plist :stop)
                                              chronometrist-day-start-time)))
       (when split-time
         (let ((first-start  (plist-get (cl-first  split-time) :start))
@@ -1558,8 +1582,8 @@ Return a list of two events if EVENT was split, else nil."
               (second-stop  (plist-get (cl-second split-time) :stop))
               ;; plist-put modifies lists in-place. The resulting bugs
               ;; left me puzzled for a while.
-              (event-1      (cl-copy-list event))
-              (event-2      (cl-copy-list event)))
+              (event-1      (cl-copy-list plist))
+              (event-2      (cl-copy-list plist)))
           (list (-> event-1
                     (plist-put :start first-start)
                     (plist-put :stop  first-stop))
@@ -1604,6 +1628,8 @@ were none."
   (chronometrist-sexp-events-populate))
 #+END_SRC
 *** events-update                                                  :writer:
+Related reading - [[#explanation-optimization-type-of-change][Explanation/Optimization/Determine type of change made to file]]
+
 #+BEGIN_SRC emacs-lisp
 (defun chronometrist-events-update (plist &optional replace)
   "Add PLIST to the end of `chronometrist-events'.
@@ -1616,6 +1642,34 @@ If REPLACE is non-nil, replace the last event with PLIST."
          (append it (list plist))
          (puthash date it chronometrist-events))))
 #+END_SRC
+
+#+BEGIN_SRC emacs-lisp
+(defun chronometrist-events-add-helper (key plist table)
+  "Append PLIST to values for KEY in TABLE.
+TABLE must be a hash table where the value for KEY is a list."
+  (--> (append (gethash key table) (list plist))
+   (puthash key it table)))
+#+END_SRC
+
+#+BEGIN_SRC emacs-lisp
+(cl-defun chronometrist-events-add (plist &optional (table chronometrist-events))
+  "Add PLIST to the end of TABLE, splitting PLIST if necessary."
+  (-let* ((start-ts          (chronometrist-iso-timestamp-to-ts
+                              (plist-get plist :start)))
+          (start-date        (ts-format "%F" start-ts))
+          (start-date-events (gethash start-date table))
+          (split             (chronometrist-events-maybe-split plist))
+          ((split-1 split-2) split)
+          (((&plist :start old-start :stop old-stop)
+            (&plist :start new-start :stop new-stop)) split)
+          (new-start-date    (when new-start
+                               (ts-format "%F"
+                                (chronometrist-iso-timestamp-to-ts new-start)))))
+    (if (null split)
+        (chronometrist-events-add-helper start-date plist table)
+      (chronometrist-events-add-helper start-date split-1 table)
+      (chronometrist-events-add-helper new-start-date split-2 table))))
+#+END_SRC
 *** last-date                                                      :reader:
 #+BEGIN_SRC emacs-lisp
 (defun chronometrist-events-last-date ()
@@ -2268,13 +2322,16 @@ value of `revert-buffer-function'."
         (set-window-point window point)))))
 #+END_SRC
 **** refresh-file                                                 :writer:
+Related reading - [[#explanation-optimization-type-of-change][Explanation/Optimization/Determine type of change made to file]]
+
 =chronometrist-file-change-type= must be run /before/ we update =chronometrist--file-state= (the latter represents the old state of the file, which =chronometrist-file-change-type= compares with the newer current state).
 
 #+BEGIN_SRC emacs-lisp
 (defun chronometrist-refresh-file (fs-event)
   "Procedure run when `chronometrist-file' changes.
-Re-read `chronometrist-file', update `chronometrist-events', and
-refresh the `chronometrist' buffer."
+Run `chronometrist-file-change-hook', re-read
+`chronometrist-file' to update `chronometrist-events', and refresh
+the `chronometrist' buffer."
   (run-hooks 'chronometrist-file-change-hook)
   ;; (message "chronometrist - file %s" fs-event)
   (-let* (((descriptor action _ _) fs-event)
@@ -2300,7 +2357,7 @@ refresh the `chronometrist' buffer."
                    ((&plist :name new-task)  (chronometrist-sexp-last)))
              (pcase change
                (:append ;; a new plist was added at the end of the file
-                (chronometrist-events-update (chronometrist-sexp-last))
+                (chronometrist-events-add (chronometrist-sexp-last))
                 (chronometrist-add-to-task-list new-task))
                (:modify ;; the last plist in the file was changed
                 (chronometrist-events-update (chronometrist-sexp-last) t)
@@ -2333,7 +2390,7 @@ refresh the `chronometrist' buffer."
          (chronometrist-out))
     t))
 #+END_SRC
-**** chronometrist-in                                            :command:
+**** chronometrist-in                                                        :command:
 #+BEGIN_SRC emacs-lisp
 (defun chronometrist-in (task &optional _prefix)
   "Clock in to TASK; record current time in `chronometrist-file'.
@@ -2343,7 +2400,7 @@ TASK is the name of the task, a string. PREFIX is ignored."
     (chronometrist-sexp-new plist)
     (chronometrist-refresh)))
 #+END_SRC
-**** chronometrist-out                                           :command:
+**** chronometrist-out                                                       :command:
 #+BEGIN_SRC emacs-lisp
 (defun chronometrist-out (&optional _prefix)
   "Record current moment as stop time to last s-exp in `chronometrist-file'.
@@ -2368,7 +2425,7 @@ PREFIX is ignored."
     (chronometrist-out)
     (run-hook-with-args 'chronometrist-after-out-functions task)))
 #+END_SRC
-**** chronometrist-mode-map                                       :keymap:
+**** chronometrist-mode-map                                                   :keymap:
 #+BEGIN_SRC emacs-lisp
 (defvar chronometrist-mode-map
   (let ((map (make-sparse-keymap)))
@@ -2386,7 +2443,7 @@ PREFIX is ignored."
     map)
   "Keymap used by `chronometrist-mode'.")
 #+END_SRC
-**** chronometrist-mode                                       :major:mode:
+**** chronometrist-mode                                                   :major:mode:
 #+BEGIN_SRC emacs-lisp
 (define-derived-mode chronometrist-mode tabulated-list-mode "Chronometrist"
   "Major mode for `chronometrist'."