Try to avoid sorting already-sorted buffers

This should reduce the disruption to things like the mark
ring. (Related to #57 and #55, and might address #35 as well?)

* subed/subed-common.el (sanitize): Separate the sanitize and
  sanitize-format functions.
  (sanitize-format): New generic function.
  (validate): Separate the validate and validate-format functions.
  (validate-format): New generic function.
  (regenerate-ids): Have generic implementation.
  (subed-sanitize-functions): Move to subed-config.el.
  (subed-validate-functions): Move to subed-config.el.
  (subed-prepare-to-save): Call subed-sanitize and subed-validate.
  (subed--sorted-p): New function to test if the subtitles are sorted.
  (sort): Make interactive. Sort only if unsorted, which should
  minimize interference with flycheck and the mark ring.
* subed/subed-config.el (subed-sanitize-functions): New customizable
  variable.
  (subed-validate-functions): New customizable variable.
* subed/subed-srt.el (subed--sanitize-format): Define this instead of
  subed--sanitize.
  (subed--validate-format): Define this instead of subed--validate.
* subed/subed-vtt.el (subed--sanitize-format): Define this instead of
  subed--sanitize.
  (subed--validate-format): Define this instead of subed--validate.
  (subed--sort): Remove format-specific implementation.
* tests/test-subed-common.el ("Trimming subtitles when configured to
  check on save reports overlaps."): Specify options.
  ("Sorting"): Add test cases.
* tests/test-subed-vtt.el ("Sorting preserves point in the current
  subtitle when subtitle text is empty."): Tweak position check.

subed/subed-common.el

@@ -339,13 +339,26 @@ If BEG and END are not specified, use the whole buffer."
     (nreverse result)))
 
 (subed-define-generic-function sanitize ()
-  "Remove surplus newlines and whitespace.")
+  "Sanitize this file."
+  (interactive)
+  (run-hooks 'subed-sanitize-functions))
+
+(subed-define-generic-function sanitize-format ()
+  "Remove surplus newlines and whitespace."
+  nil)
 
 (subed-define-generic-function validate ()
-  "Move point to the first invalid subtitle and report an error.")
+  "Move point to the first invalid subtitle and report an error."
+  (interactive)
+  (run-hooks 'subed-validate-functions))
+
+(subed-define-generic-function validate-format ()
+  "Validate format-specific rules."
+  nil)
 
 (subed-define-generic-function regenerate-ids ()
-  "Ensure consecutive, unduplicated subtitle IDs in formats that use them.")
+  "Ensure consecutive, unduplicated subtitle IDs in formats that use them."
+  nil)
 
 (defvar-local subed--regenerate-ids-soon-timer nil)
 (subed-define-generic-function regenerate-ids-soon ()
@@ -1745,42 +1758,51 @@ the stop time isn't smaller than the start time."
 
 ;;; Sorting and sanitizing
 
-(defvar-local subed-sanitize-functions
-  '(subed-sort subed-trim-overlap-maybe-sanitize)
-  "Functions to sanitize this buffer.
-Functions can clean up whitespace, rearrange subtitles, etc.")
-
-(defvar-local subed-validate-functions
-  '(subed-validate subed-trim-overlap-maybe-check)
-  "Functions to validate this buffer.
-Validation functions should throw an error or prompt the user for
-action.")
-
 (defun subed-prepare-to-save ()
   "Sanitize and validate this buffer."
   (interactive)
   (atomic-change-group
-    (run-hooks 'subed-sanitize-functions)
-    (run-hooks 'subed-validate-functions)))
+    (subed-sanitize)
+    (subed-validate)))
+
+(defun subed--sorted-p (&optional list)
+  "Return non-nil if LIST is sorted by start time.
+If LIST is nil, use the subtitles in the current buffer."
+  (let ((subtitles (or list (subed-subtitle-list)))
+        (sorted t))
+    (while (cdr subtitles)
+      (if (and
+           (numberp (elt (car subtitles) 1))
+           (numberp (elt (cadr subtitles) 1))
+           (> (elt (car subtitles) 1)
+              (elt (cadr subtitles) 1))) ; starts later than the next one
+          (setq sorted nil
+                subtitles nil)
+        (setq subtitles (cdr subtitles))))
+    sorted))
 
 (subed-define-generic-function sort ()
   "Sort subtitles."
-  (atomic-change-group
-    (subed-sanitize)
-    (subed-validate)
-    (subed-save-excursion
-     (goto-char (point-min))
-     (unless (subed-subtitle-id)
-       (subed-forward-subtitle-id))
-     (sort-subr nil
-                ;; nextrecfun (move to next record/subtitle or to end-of-buffer
-                ;; if there are no more records)
-                (lambda () (unless (subed-forward-subtitle-id)
-                             (goto-char (point-max))))
-                ;; endrecfun (move to end of current record/subtitle)
-                #'subed-jump-to-subtitle-end
-                ;; startkeyfun (return sort value of current record/subtitle)
-                #'subed-subtitle-msecs-start))))
+  (interactive)
+  (subed-sanitize-format)
+  (subed-validate-format)
+  (unless (subed--sorted-p)
+    (subed-batch-edit
+      (atomic-change-group
+        (subed-save-excursion
+         (goto-char (point-min))
+         (unless (subed-jump-to-subtitle-id)
+           (subed-forward-subtitle-id))
+         (sort-subr nil
+                    ;; nextrecfun (move to next record/subtitle or to end-of-buffer
+                    ;; if there are no more records)
+                    (lambda () (unless (subed-forward-subtitle-id)
+                                 (goto-char (point-max))))
+                    ;; endrecfun (move to end of current record/subtitle)
+                    #'subed-jump-to-subtitle-end
+                    ;; startkeyfun (return sort value of current record/subtitle)
+                    #'subed-subtitle-msecs-start))
+        (subed-regenerate-ids)))))
 
 ;;; Initialization
 

subed/subed-config.el

@@ -164,6 +164,25 @@ subtitles or negative duration."
   :type 'boolean
   :group 'subed)
 
+(defcustom subed-sanitize-functions
+  '(subed-sanitize-format
+    subed-sort
+    subed-trim-overlap-maybe-sanitize)
+  "Functions to call when sanitizing subtitles."
+  :type '(repeat function)
+  :local t
+  :group 'subed)
+
+(defcustom subed-validate-functions
+  '(subed-validate-format
+    subed-trim-overlap-maybe-check)
+  "Functions to validate this buffer.
+Validation functions should throw an error or prompt the user for
+action."
+  :type '(repeat function)
+  :local t
+  :group 'subed)
+
 (defcustom subed-loop-seconds-before 1
   "Prelude in seconds when looping over subtitle(s)."
   :type 'float

subed/subed-srt.el

@@ -314,7 +314,7 @@ Use the format-specific function for MAJOR-MODE."
                 (insert id-str)))
             (setq id (1+ id))))))))
 
-(cl-defmethod subed--sanitize (&context (major-mode subed-srt-mode))
+(cl-defmethod subed--sanitize-format (&context (major-mode subed-srt-mode))
   "Remove surplus newlines and whitespace.
 Use the format-specific function for MAJOR-MODE."
   (atomic-change-group
@@ -361,7 +361,7 @@ Use the format-specific function for MAJOR-MODE."
            (unless (= (length (match-string 0)) 5)
              (replace-match " --> "))))))))
 
-(cl-defmethod subed--validate (&context (major-mode subed-srt-mode))
+(cl-defmethod subed--validate-format (&context (major-mode subed-srt-mode))
   "Move point to the first invalid subtitle and report an error.
 Use the format-specific function for MAJOR-MODE."
   (when (> (buffer-size) 0)
@@ -390,10 +390,6 @@ Use the format-specific function for MAJOR-MODE."
               (error "Found invalid stop time: %S" (substring (or (thing-at-point 'line :no-properties) "\n") 0 -1))))
           (goto-char orig-point))))))
 
-(cl-defmethod subed--sort :after (&context (major-mode subed-srt-mode))
-  "Renumber after sorting. Format-specific for MAJOR-MODE."
-  (subed-regenerate-ids))
-
 (cl-defmethod subed--insert-subtitle :after (&context (major-mode subed-srt-mode) &optional arg)
   "Renumber afterwards. Format-specific for MAJOR-MODE."
   (subed-regenerate-ids-soon)

subed/subed-vtt.el

@@ -285,7 +285,7 @@ Use the format-specific function for MAJOR-MODE."
 ;;; Maintenance
 
 
-(cl-defmethod subed--sanitize (&context (major-mode subed-vtt-mode))
+(cl-defmethod subed--sanitize-format (&context (major-mode subed-vtt-mode))
   "Remove surplus newlines and whitespace.
 Use the format-specific function for MAJOR-MODE."
   (atomic-change-group
@@ -332,7 +332,7 @@ Use the format-specific function for MAJOR-MODE."
            (unless (= (length (match-string 0)) 5)
              (replace-match " --> "))))))))
 
-(cl-defmethod subed--validate (&context (major-mode subed-vtt-mode))
+(cl-defmethod subed--validate-format (&context (major-mode subed-vtt-mode))
   "Move point to the first invalid subtitle and report an error.
 Use the format-specific function for MAJOR-MODE."
   (when (> (buffer-size) 0)
@@ -357,25 +357,6 @@ Use the format-specific function for MAJOR-MODE."
               (error "Found invalid stop time: %S" (substring (or (thing-at-point 'line :no-properties) "\n") 0 -1))))
           (goto-char orig-point))))))
 
-(cl-defmethod subed--sort (&context (major-mode subed-vtt-mode))
-  "Sanitize, then sort subtitles by start time.
-Use the format-specific function for MAJOR-MODE."
-  (atomic-change-group
-    (subed-sanitize)
-    (subed-validate)
-    (subed-save-excursion
-     (goto-char (point-min))
-     (subed-forward-subtitle-id)
-     (sort-subr nil
-                ;; nextrecfun (move to next record/subtitle or to end-of-buffer
-                ;; if there are no more records)
-                (lambda () (unless (subed-forward-subtitle-id)
-                             (goto-char (point-max))))
-                ;; endrecfun (move to end of current record/subtitle)
-                #'subed-jump-to-subtitle-end
-                ;; startkeyfun (return sort value of current record/subtitle)
-                #'subed-subtitle-msecs-start))))
-
 ;;;###autoload
 (define-derived-mode subed-vtt-mode subed-mode "Subed-VTT"
   "Major mode for editing WebVTT subtitle files."

tests/test-subed-common.el

@@ -3081,16 +3081,20 @@ This is another.
            (expect (subed-subtitle-msecs-stop 2) :to-equal 3800)
            (expect (subed-subtitle-msecs-stop 3) :to-equal 4800)))))
     (describe "when configured to check on save,"
-      (it "reports overlaps after sorting."
+      (it "reports overlaps."
         (with-temp-srt-buffer
          (insert "1\n00:00:01,000 --> 00:00:02,000\nA\n\n"
                  "2\n00:00:04,000 --> 00:00:06,000\nA\n\n"
                  "3\n00:00:03,000 --> 00:00:04,500\nA\n\n"
                  "4\n00:00:05,000 --> 00:00:06,000\nA\n\n")
          (let ((subed-trim-overlap-check-on-save t)
+               (subed-trim-overlap-on-save nil)
                (subed-subtitle-spacing 200))
+           (spy-on 'subed-trim-overlap-check :and-call-through)
+           (spy-on 'subed-trim-overlaps :and-call-through)
            (spy-on 'yes-or-no-p :and-return-value t)
            (subed-prepare-to-save)
+           (expect 'subed-trim-overlap-check :to-have-been-called)
            (expect 'yes-or-no-p :to-have-been-called)
            (expect (subed-subtitle-msecs-stop 1) :to-equal 2000)
            (expect (subed-subtitle-msecs-stop 2) :to-equal 3800)
@@ -3130,7 +3134,49 @@ This is another.
                '((1 61000 65123 "Foo.")
                  (2 122234 130345 "Bar.")))))
     )
-  )
+  (describe "Sorting"
+    (it "detects sorted lists."
+      (expect (subed--sorted-p '((1 1000 2000 "Test")
+                                 (2 2000 3000 "Test")
+                                 (3 3000 4000 "Test")))))
+    (it "detects unsorted lists."
+      (expect (subed--sorted-p '((1 3000 2000 "Test")
+                                 (2 4000 3000 "Test")
+                                 (3 1000 4000 "Test")))
+              :to-be nil))
+    (it "doesn't happen in an empty buffer."
+      (with-temp-srt-buffer
+        (spy-on 'sort-subr :and-call-through)
+        (subed-sort)
+        (expect 'sort-subr :not :to-have-been-called)))
+    (describe "already-sorted subtitles"
+      (it "doesn't rearrange subtitles."
+        (with-temp-srt-buffer
+          (insert mock-srt-data)
+          (spy-on 'sort-subr :and-call-through)
+          (subed-sort)
+          (expect 'sort-subr :not :to-have-been-called)))
+      (it "maintains the mark ring."
+        (with-temp-srt-buffer
+          (insert mock-srt-data)
+          (let ((mark-ring))
+            (push-mark 10 t nil)
+            (push-mark 20 t nil)
+            (push-mark 3 t nil)
+            (expect (marker-position (car mark-ring)) :to-be 20)
+            (expect (marker-position (cadr mark-ring)) :to-be 10)
+            (subed-sort)
+            (expect (marker-position (car mark-ring)) :to-be 20)
+            (expect (marker-position (cadr mark-ring)) :to-be 10)))))
+    (it "sorts subtitles by start time."
+      (with-temp-srt-buffer
+        (insert mock-srt-data "\n4\n00:02:01,000 --> 00:03:01,000\nNot sorted.\n")
+        (expect (subed--sorted-p) :to-be nil)
+        (goto-char (point-min))
+        (subed-sort)
+        (expect (subed-subtitle-text 2) :to-equal "Not sorted.")
+        (expect (subed-subtitle-text 3) :to-equal "Bar.")
+        (expect (subed-subtitle-text 4) :to-equal "Baz.")))))
 
 (describe "An old generic function"
   :var ((function-list

tests/test-subed-srt.el

@@ -1410,17 +1410,19 @@ Baz.
        (expect (buffer-string) :to-equal "")))
     (it "runs before saving."
       (with-temp-srt-buffer
-       (insert mock-srt-data)
-       (goto-char (point-min))
-       (re-search-forward " --> ")
-       (replace-match "  --> ")
-       (re-search-forward " --> ")
-       (replace-match " -->  ")
-       (re-search-forward " --> ")
-       (replace-match "-->")
-       (expect (buffer-string) :not :to-equal mock-srt-data)
-       (subed-prepare-to-save)
-       (expect (buffer-string) :to-equal mock-srt-data))))
+        (insert mock-srt-data)
+        (goto-char (point-min))
+        (re-search-forward " --> ")
+        (replace-match "  --> ")
+        (re-search-forward " --> ")
+        (replace-match " -->  ")
+        (re-search-forward " --> ")
+        (replace-match "-->")
+        (spy-on 'subed-sanitize :and-call-through)
+        (expect (buffer-string) :not :to-equal mock-srt-data)
+        (subed-prepare-to-save)
+        (expect 'subed-sanitize :to-have-been-called)
+        (expect (buffer-string) :to-equal mock-srt-data))))
 
   (describe "Renumbering"
     (it "ensures consecutive subtitle IDs."

tests/test-subed-vtt.el

@@ -1236,9 +1236,10 @@ Baz.
       (it "when subtitle text is empty."
         (with-temp-vtt-buffer
          (insert "WEBVTT\n\n00:12:01.000 --> 00:01:05.123\n")
-         (goto-char (point-max))
-         (subed-sort)
-         (expect (point) :to-equal (1- (point-max)))))
+         (let ((pos (point)))
+           (subed-sort)
+           (expect (buffer-string) :to-equal "WEBVTT\n\n00:12:01.000 --> 00:01:05.123\n\n")
+           (expect (point) :to-equal pos))))
       )
     )
   (describe "Converting msecs to timestamp"