Re: [ELPA] New package: tchanges

[Philip Kaludercic <philipk@posteo.net>]

[-- Attachment #1: Type: text/plain, Size: 351 bytes --]

James Thomas <jimjoe@gmx.net> writes:

> Hi,
>
> I propose to include this package that I wrote, to GNU ELPA. It's for
> collaborating with users of word-processing software, such as
> Libreoffice Writer, using the latter's 'track changes' feature.
>
>   https://codeberg.org/quotuva/tchanges

I haven't tested it, but here are a few quick comments:


[-- Attachment #2: Type: text/plain, Size: 7386 bytes --]

diff --git a/tchanges.el b/tchanges.el
index a3e6829..4bf6395 100644
--- a/tchanges.el
+++ b/tchanges.el
@@ -5,6 +5,7 @@
 ;; Author: James Thomas <jimjoe@gmx.net>
 ;; Maintainer: James Thomas <jimjoe@gmx.net>
 ;; Keywords: wp, convenience, files
+;; Package-Requires: ((emacs "30"))
 ;; URL: https://codeberg.org/quotuva/tchanges
 ;; Version: 0.1
 
@@ -26,19 +27,24 @@
 (require 'bookmark)
 
 (defgroup tchanges nil
-  "Collaborate with Office (docx) users using 'track changes'.")
+  "Collaborate with Office (docx) users using 'track changes'."
+  :group '???)                           ;this is missing!
 
 (defcustom tchanges-output-format
   "org"
-  "Default output format. See man page `pandoc(1)' for available options."
-  :type '(string)
-  :group 'tchanges)
+  "Default output format.
+See man page `pandoc(1)' for available options."
+  ;; By default, a user option is added to the last declared group, so
+  ;; you can omit it here!
+  :type 'string)                        ;Is this actually a string, or
+                                        ;a symbol?  Should you give a
+                                        ;list of suggestions?
 
 (defcustom tchanges-pandoc-extra-args
   nil
-  "Extra args passed to pandoc. Value is a list of strings, which may be nil."
-  :type '(repeat (string :tag "Argument"))
-  :group 'tchanges)
+  "Extra args passed to pandoc.
+Value is a list of strings, which may be nil."
+  :type '(repeat :tag "Arguments" string))
 
 (defvar tchanges-script
   (expand-file-name
@@ -51,12 +57,13 @@
 
 ;; Global value used during import, the local in the comment editor.
 (defvar tchanges-buffer nil)
+
 ;; Separate uses for the org buffer and the comment editor.
 (defvar-local tchanges-comments nil)
 
 (defun tchanges--sentinel (process event)
-  "Sentinel for pandoc process completion."
-  (if (not (string-match "finished" event))
+  "Sentinel for pandoc process completion." ;checkdoc would like some more documentation!
+  (if (not (string-match "finished" event)) ;(elisp) Sentinels says it should be "finished\n".  Or use `process-status'.
       (error "%s %s" process event)
     (message "tchanges: Running pandoc...done")
     (tchanges--postproc)))
@@ -71,17 +78,18 @@
   (setq tchanges-buffer nil))
 
 (defun tchanges--postproc-2 (tag)
+  ;; Documentation missing here!  Flymake + checkdoc should warn you about this.
   (let ((insdel-regexp "\x1e\\([id]\\)\x1e[^\x1e]*?\x1e[^\x1e]*?\x1e\\([^\x1e]*?\\)\x1f")
         (comments-regexp "\x1c[se][^\x1d]*?\x1d")
         (bookmark-make-record-function #'tchanges-bookmark-make-record)
         (bookmark-use-annotations nil)
         bookmark-alist
         (count 0))
-    (cl-letf (((symbol-function #'bookmark--set-fringe-mark)
-               (symbol-function #'ignore)))
+    (cl-letf (((symbol-function #'bookmark--set-fringe-mark) #'ignore))
       (goto-char (point-min))
       (while
           (re-search-forward
+           ;; I think that using `rx' would make this more readable and maintainable.
            "\x1cs\\([^\x1c]*?\\)\x1c\\([^\x1c]*?\\)\x1c\\([^\x1c]*?\\)\x1c\\([^\x1c\x1d]*?\\)\x1d"
            nil t)
         (let* ((beg (match-beginning 0))
@@ -142,13 +150,14 @@
 
 ;;;###autoload
 (defun tchanges-find-file (file &optional format)
+  ;; checkdoc warns you that the first sentence should fit on a line!
   "Convert a word processor document FILE with \\='tracked changes\\='
 to pre- and post- buffers. These may be `diff'ed or, more conveniently,
 `ediff3'd with the original version of the file before it was shared (as
 \\='ancestor\\='), inorder to hide (or nullify) common changes (the ones
 related to conversion).
 
-(For even more accuracy, you may \\='regenerate\\=' the above original
+\(For even more accuracy, you may \\='regenerate\\=' the above original
 version by a reverse conversion using the same Pandoc arguments
 immediately after the forward conversion)
 
@@ -180,10 +189,10 @@ Comment regions are highlighted with tooltips, but also see
 (defun tchanges-inline-comments ()
   "Prepare the buffer by inlining the comments, for export or saving."
   (interactive)
-  (if (or (not (bound-and-true-p tchanges-comments)))
+  (if (or (not (bound-and-true-p tchanges-comments))) ;isn't it always bound?
       (user-error "No comments in buffer.")
     (let ((bookmark-alist
-           (sort tchanges-comments
+           (sort tchanges-comments      ;the :lessp requires Emacs 30, but you could also call `sort' without keyword parameters
                  :lessp
                  (lambda (x y)
                    (> (bookmark-get-position x)
@@ -239,7 +248,7 @@ Comment regions are highlighted with tooltips, but also see
 Copied from the default function."
   (let* ((posn (or (and (use-region-p) (region-beginning))
                    (point)))
-         (len (when (use-region-p) (- (region-end) posn))))
+         (len (and (use-region-p) (- (region-end) posn)))) ;is it ok if this is nil?
     `((buffer . ,(current-buffer))
       (front-context-string
        . ,(if (>= (- (point-max) (point))
@@ -263,7 +272,7 @@ Copied from the default function."
       (handler . ,#'tchanges--bookmark-handler))))
 
 (defun tchanges-edit-annotation-confirm ()
-  "TODO"
+  "TODO"                                ;TODO
   (interactive)
   (let* ((buf tchanges-buffer)
          (newp (string-prefix-p "g" bookmark-annotation-name))
@@ -318,16 +327,15 @@ Copied from the default function."
         (bookmark-use-annotations t)
         bookmark-alist
         (buf (current-buffer)))
-    (cl-letf (((symbol-function #'bookmark--set-fringe-mark)
-               (symbol-function #'ignore)))
+    (cl-letf (((symbol-function #'bookmark--set-fringe-mark) #'ignore))
       (bookmark-set (symbol-name (gensym)) t))
     ;; Single bookmark
     (setq-local tchanges-buffer buf)
     (setq tchanges-comments bookmark-alist)
     (tchanges--remap '(bookmark-edit-annotation-confirm))))
 
-(defun tchanges-bmenu--revert ()
-  "Re-populate `tabulated-list-entries'. (Copied from the bookmark-...)"
+(defun tchanges-bmenu--revert ()        ;Copied from the bookmark-...
+  "Re-populate `tabulated-list-entries'."
   (setq tabulated-list-entries nil)
   (dolist (full-record bookmark-alist)
     (let* ((name       (bookmark-name-from-full-record full-record))
@@ -354,8 +362,9 @@ Copied from the default function."
   (tabulated-list-print t))
 
 (defun tchanges-list-comments ()
-  "View buffer with all the comments. May be run in the buffer, after using
-`tchanges-comment'. If point is at a comment, jump to it in the listing."
+  "View buffer with all the comments.
+May be run in the buffer, after using `tchanges-comment'.  If point is at
+a comment, jump to it in the listing."
   (interactive)
   (let ((id (get-char-property (point) 'tchanges-comment))
         (buf (current-buffer))
@@ -402,9 +411,9 @@ Copied from the default function."
                               tchanges-buffer))
          bookmark-watch-bookmark-file)
     (cl-letf (((symbol-function #'bookmark--remove-fringe-mark)
-               (symbol-function #'tchanges--remove-highlight))
+               #'tchanges--remove-highlight)
               ((symbol-function #'bookmark-bmenu-list)
-               (symbol-function #'tchanges-bmenu--revert)))
+               #'tchanges-bmenu--revert))
       (bookmark-bmenu-execute-deletions))
     (with-current-buffer tchanges-buffer
       (setq tchanges-comments bookmark-alist))))

[-- Attachment #3: Type: text/plain, Size: 709 bytes --]



>
> Here's a short screencast (.gif) demonstrating most of the features:
>
>   https://codeberg.org/attachments/5096c13b-ff1f-47f6-8741-f3d955e34211

I am afraid this demonstration is not that useful if you don't already
understand what is going on.  If you believe a visual introduction to be
useful, I would recommend preparing a recorded version with narration
and uploading it to a Peertube server.

> I'd previously mentioned this in the other mailing lists as well[1].
>
> Regards,
> James
>
> 1:
> https://lists.gnu.org/archive/html/emacs-orgmode/2024-07/msg00009.html
> https://lists.gnu.org/archive/html/help-gnu-emacs/2024-05/msg00167.html
> (older repo URL)
>
>

-- 
	Philip Kaludercic on siskin