Re: [ELPA] New package: tchanges

[James Thomas <jimjoe@gmx.net>]

Hi,

I've made all the changes except for the few ones noted below:

Philip Kaludercic wrote:

> 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:
>
> 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"))

Passed over in favour of removing the 'lessp' (below) as you suggested.

>  ;; 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?

Logically, no: because Pandoc's format code-names need not correspond to
any Emacs major mode symbols, if any, for those formats. The only
benefit to symbols I see is validation, but Pandoc could add new ones.

>
>  (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 ".\\([id]\\).[^.]*?.[^.]*?.\\([^.]*?\\)\x1f")
>          (comments-regexp ".[se][^.]*?.")
>          (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.
>             ".s\\([^.]*?\\).\\([^.]*?\\).\\([^.]*?\\).\\([^.\x1d]*?\\)."

Me too; but it could wait, right? I was being conservative.

>             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?

Yes, but it also needed some other changes to handle zero-width
comments, which I've now made.

>      `((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))))
>
>
>
>>
>> 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.

Is this better? I've improved it by referring to the README as a guide:

  https://codeberg.org/attachments/d9ce4f60-0acc-4316-ae45-dab6d59adc1e

Regards,
James