Philip Kaludercic <philipk@posteo.net> writes:

[...]

>
> gnu-indent:
>
> diff --git a/gnu-indent.el b/gnu-indent.el
> index 1a37851e96..8aea4161dc 100644
> --- a/gnu-indent.el
> +++ b/gnu-indent.el
> @@ -51,18 +51,17 @@
>  ;; Autoload so that users can set it as file local variable without
>  ;; warning.
>  ;;;###autoload
> -(progn
> -  (defcustom gnu-indent-options nil
> -    "Arguments to pass to GNU Indent."
> -    :type '(repeat string)
> -    :safe (lambda (val)
> -            (let ((valid t))
> -              (while (and valid val)
> -                (unless (stringp (car val))
> -                  (setq valid nil))
> -                (setq val (cdr val)))
> -              valid))
> -    :group 'gnu-indent))
> +(defcustom gnu-indent-options nil
> +  "Arguments to pass to GNU Indent."
> +  :type '(repeat string)
> +  :safe (lambda (val)
> +          (let ((valid t))
> +            (while (and valid val)
> +              (unless (stringp (car val))
> +                (setq valid nil))
> +              (setq val (cdr val)))
> +            valid))
> +  :group 'gnu-indent)

Done.

>
>  ;;;###autoload
>  (defun gnu-indent-region (beg end)
> @@ -88,14 +87,15 @@ When called non-interactively, indent text between BEG and END."
>              (send-region process beg end)
>              (process-send-eof process)
>              (redisplay)
> -            (while (process-live-p process)
> -              (sleep-for 0.01))
> +            (while (accept-process-output process nil 10))

MILLISEC argument is obsolete, I used SECONDS instead.

>              (unless (eq (process-exit-status process) 0)
> -              (display-buffer (process-buffer process))
> +              (pop-to-buffer (process-buffer process))

Done.

>                (error "GNU Indent exited with non-zero status"))
>              (save-restriction
>                (let ((inhibit-read-only t))
>                  (narrow-to-region beg end)
> +		;; Perhaps something should be done to try an preserve
> +		;; the point after indentation?
>                  (insert-file-contents temp-file nil nil nil
>                                        t))))

On my computer, the point doesn't move relative to text, because the
fifth argument to insert-file-contents is t.

>          (delete-file temp-file)))
> @@ -108,11 +108,24 @@ When called non-interactively, indent text between BEG and END."
>    (interactive)
>    (gnu-indent-region (point-min) (point-max)))
>
> +;; A little suggestion
> +;;;###autoload
> +(defun gnu-indent-defun-or-fill (arg)
> +  "Indent current function with GNU Indent.
> +If point is in a comment, call `fill-paragraph' instead.  A
> +prefix argument ARG is passed to `fill-paragraph'."
> +  (interactive "P")
> +  (if (nth 8 (syntax-ppss))           ;if in a comment
> +      (fill-paragraph arg)
> +    (let ((bounds (bounds-of-thing-at-point 'defun)))
> +      (if (consp bounds)
> +	  (gnu-indent-region (car bounds) (cdr bounds))
> +	(user-error "No defun at point")))))
> +

Great idea.  But would it cause problem to assign copyright if I take
your change?  (AFAIK you've completed paperwork, but in this case you're
not contributing to FSF-copyrighted code, so is this change covered by
your paperwork?  Right now, no, you're the copyright holder.  (According
to Section 2 of copyright assignment agreement sent to me.))

>  ;;;###autoload
>  (define-minor-mode gnu-indent-mode
>    "Indent buffer automatically with GNU Indent."
>    :lighter " GNU-Indent"
> -  :keymap nil
>    (if gnu-indent-mode
>        (add-hook 'before-save-hook #'gnu-indent-buffer nil t)
>      (remove-hook 'before-save-hook #'gnu-indent-buffer t)))

Done.

>
>
> devhelp:
>
> diff --git a/devhelp.el b/devhelp.el
> index 6b3d9a1ce9..05aeb1e18e 100644
> --- a/devhelp.el
> +++ b/devhelp.el
> @@ -48,6 +48,10 @@
>  ;;             "~/.guix-profile/share/doc/"
>  ;;             "~/.guix-profile/share/gtk-doc/html/"))
>
> +;; Do you think it makes sense to automatically detect this (if the
> +;; user has a ~/.guix-profile directory) and make the changes to the
> +;; default value?
> +

Yes, it makes sense.  But I didn't find any way to detected the
directories, except heuristics.

>  ;; You can also bookmark pages, with the standard `bookmark-set' function.
>
>  ;;; Code:
> @@ -89,7 +93,7 @@ Integer means use that many columns.  Nil means use full window width."
>
>  Note that on GNU Guix, Nix or other FHS (Filesystem Hierarchy Standard)
>  non-compliant distributions, the default value won't work.  For GNU Guix,
> -set it to '(\"/run/current-system/profile/share/doc/\"
> +set it to \\='(\"/run/current-system/profile/share/doc/\"

Done.

>  \"/run/current-system/profile/share/gtk-doc/html/\"
>  \"~/.guix-profile/share/doc/\" \"~/.guix-profile/share/gtk-doc/html/\")."
>    :type '(repeat directory))
> @@ -147,6 +151,7 @@ absolute path to it."
>                     ,(expand-file-name (dom-attr sec 'link) base)
>                     ,(mapcar #'process-section (children-by-tag
>                                                 sec 'sub)))))
> +      ;; `List' would be better here, right?
>        `(,(or (dom-attr dom 'title) "Untitled")
>          ,(or (dom-attr dom 'name) (file-name-base file))
>          ,(or (dom-attr dom 'language) "any")

Yes.  Change done.

> @@ -219,6 +224,7 @@ If a single file was opened, only show that book's table of contents."
>  See `devhelp-toc' for more details."
>    (let ((inhibit-read-only t))
>      (erase-buffer)
> +    ;; Why not prepare the document in SXML and then use `dom-print'?
>      (insert
>       "<html><head><title>Table of contents</title></head><body><ul>"
>       (let ((book-tocs

Hmm, that would be a cleaner approach.  Now I need to research the
format of SXML.

> @@ -447,6 +453,8 @@ When BASE is given, use it to make relative URLs absolute."
>      (recenter)))
>
>  (defun devhelp-history-back (&optional n)
> +  ;; Is the variable actually optional, or won't the + raise a wrong
> +  ;; type signal if invoked without an argument?
>    "Go to the previous page.
>
>  When prefix argument N is given, go to Nth previous page."

Oh, yes, it would; now fixed.

>
>
> why-this:
>
> diff --git a/why-this.el b/why-this.el
> index 287ee99921..19acb90f9f 100644
> --- a/why-this.el
> +++ b/why-this.el
> @@ -36,6 +36,10 @@
>  (require 'timezone)
>  (require 'color)
>
> +;; A very high-level request that would probably take too much work to
> +;; actualise, would be to figure out some way to integrate this into
> +;; VC.  That way you can make use of this even if you don't use Git or Hg.
> +

Exactly, using VC would be a lot better idea.  I just use Git, and the
Hg backend is just a proof-of-concept that Why-This can support other
VCs.

>  (defgroup why-this nil
>    "Show why the current line contains this."
>    :group 'tools
> @@ -64,6 +68,7 @@ the first argument is the command (which is a symbol):
>      `:author'   Name of the author.
>      `:time'     Time of change (local).
>      `:desc'     Single line description of change."
> +  ;; Would it make sense to use a `cl-defstruct'?
>    :type 'hook
>    :options (list #'why-this-git
>                   #'why-this-hg)

Maybe.  But I think the plist approach is simpler and good enough.

> @@ -326,6 +331,7 @@ TIME-FORMAT is used to format data."
>               (?t . (why-this-format-time
>                      time-format (plist-get data :time)))
>               (?i . (plist-get data :desc)))))
> +      ;; `format-spec' should make this simpler
>        (replace-regexp-in-string
>         "%."
>         (lambda (str)

Thanks, I just learned something new.  format-spec made it a lot easier.

> @@ -342,7 +348,7 @@ TIME-FORMAT is used to format data."
>  (defun why-this--overlay-bg-type (pos)
>    "Return the background type for overlay at POS."
>    (cond
> -   ((and (use-region-p)
> +   ((and (use-region-p)                         ;perhaps `region-active-p'?
>           (>= pos (region-beginning))
>           (< pos (region-end)))
>      'region)

Hmm, it doesn't matter, both works.  I changed to use region-active-p.

> @@ -364,6 +370,7 @@ TIME-FORMAT is used to format data."
>                     'solaire-region-face
>                   'region))
>                ('line
> +               ;; Looks like a `cond' to me
>                 (if (bound-and-true-p hl-line-mode)
>                     (if (bound-and-true-p solaire-mode)
>                         'solaire-hl-line-face

Yes, but I think its more efficient.  (IIUC cond would test hl-line-mode
before returning why-this-face.)

> @@ -382,12 +389,10 @@ TIME-FORMAT is used to format data."
>    (while why-this--overlays
>      (delete-overlay (pop why-this--overlays)))
>    (when why-this-mode
> -    (let* ((begin (line-number-at-pos (if (use-region-p)
> -                                          (region-beginning)
> -                                        (point))))
> -           (end (1+ (line-number-at-pos (if (use-region-p)
> -                                            (region-end)
> -                                          (point)))))
> +    (let* ((line (line-number-at-pos (if (use-region-p)
> +                                         (region-beginning)
> +                                       (point))))
> +           (begin line) (end (1+ line))
>             (backend why-this--backend)
>             (data (funcall backend 'line-data begin end)))
>        (dolist (i (number-sequence 0 (- end begin 1)))

Thanks.

> @@ -446,6 +451,8 @@ TIME-FORMAT is used to format data."
>                                           (region-end)
>                                         (point))))))
>      (setq
> +     ;; Perhaps this could be made more readble by using
> +     ;; `thread-last' or the above let binding
>       why-this--overlays
>       (delq
>        nil

Thanks, just learned another thing.  Changed to use thread-last.

> @@ -458,10 +465,10 @@ TIME-FORMAT is used to format data."
>                           (< line end)
>                           (eq line (overlay-get ov 'why-this-line)))))
>               (progn
> -               (let* ((ov-start (overlay-start ov))
> -                      line-begin
> -                      line-end
> -                      column)
> +               (let ((ov-start (overlay-start ov))
> +                     line-begin
> +                     line-end
> +                     column)
>                   (save-excursion
>                     (goto-char ov-start)
>                     (setq line-begin (line-beginning-position))

Done.

> @@ -537,6 +544,11 @@ Actually the supported backend is returned."
>                     (* (- (nth i b-color)
>                           (nth i a-color))
>                        ratio)))))
> +    ;; Note that RGB interpolation doesn't always behave the way you
> +    ;; think it does.  You'd have to convert it into some other color
> +    ;; space like CIELAB to get perceptual mixing right (but even that
> +    ;; is trying because it requires some kind of a white-reference
> +    ;; point).
>      (color-rgb-to-hex (funcall mix 0)
>                        (funcall mix 1)
>                        (funcall mix 2)

Any edge case of my code?

>
>
> A general comment might be that adding a .dir-locals.el file that
> regulates Emcas' whitespace behaviour would be nice to have.
>

Thanks.  I'll add.

> Also, I don't think you need to distribute the texinfo.tex files with
> your packages, or at the least you could mark them as ignorable in the
> .elpaignore file.
>

That's there for someone to build the PDF manuals.  I'll add them to
.elpaignore, since they aren't needed for Info manual.

--
Akib Azmain Turja, GPG key: 70018CE5819F17A3BBA666AFE74F0EFA922AE7F5
Fediverse: akib@hostux.social
Codeberg: akib
emailselfdefense.fsf.org | "Nothing can be secure without encryption."