Subject: Re: bug#70820: [PATCH] Editable grep buffers

From: Visuwesh <[email-masked]>
Date: Wed, 14 Aug 2024 08:13:42 +0530

[செவ்வாய் ஆகஸ்ட் 13, 2024] Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" wrote:

>> +(defun compilation--update-markers (loc marker screen-columns first-column)
>> +  "Update markers in LOC, and set MARKER to location pointed by LOC.
>> +SCREEN-COLUMNS and FIRST-COLUMN are the value of
>> +`compilation-error-screen-columns' and `compilation-first-column' to use
>> +if they are not set buffer-locally in the target buffer."
>> +  (with-current-buffer
>> +      (if (bufferp (caar (compilation--loc->file-struct loc)))
>> +          (caar (compilation--loc->file-struct loc))
>> +        (apply #'compilation-find-file
>> +               marker
>> +               (caar (compilation--loc->file-struct loc))
>> +               (cadr (car (compilation--loc->file-struct loc)))
>> +               (compilation--file-struct->formats
>> +                (compilation--loc->file-struct loc))))
>> +    (let ((screen-columns
>> +           ;; Obey the compilation-error-screen-columns of the target
>> +           ;; buffer if its major mode set it buffer-locally.
>> +           (if (local-variable-p 'compilation-error-screen-columns)
>> +               compilation-error-screen-columns screen-columns))
>> +          (compilation-first-column
>> +           (if (local-variable-p 'compilation-first-column)
>> +               compilation-first-column first-column))
>> +          (last 1))
>> +      (save-restriction
>> +        (widen)
>> +        (goto-char (point-min))
>> +        ;; Treat file's found lines in forward order, 1 by 1.
>> +        (dolist (line (reverse (cddr (compilation--loc->file-struct loc))))
>> +          (when (car line)     ; else this is a filename without a line#
>> +            (compilation-beginning-of-line (- (car line) last -1))
>> +            (setq last (car line)))
>> +          ;; Treat line's found columns and store/update a marker for each.
>> +          (dolist (col (cdr line))
>> +            (if (compilation--loc->col col)
>> +                (if (eq (compilation--loc->col col) -1)
>> +                    ;; Special case for range end.
>> +                    (end-of-line)
>> +                  (compilation-move-to-column (compilation--loc->col col)
>> +                                              screen-columns))
>> +              (beginning-of-line)
>> +              (skip-chars-forward " \t"))
>> +            (if (compilation--loc->marker col)
>> +                (set-marker (compilation--loc->marker col) (point))
>> +              (setf (compilation--loc->marker col) (point-marker)))
>> +            ;; (setf (compilation--loc->timestamp col) timestamp)
>> +            ))))))
>
> Are there any changes in this code, or is it "verbatim" the code
> extracted from `compilation-next-error-function`?

It is extracted verbatim from compilation-next-error-function.

>> +(defvar grep-edit-mode-hook nil
>> +  "Hooks run when changing to Grep-Edit mode.")
>
> It's just "Hook" because `grep-edit-mode-hook` is a hook.  Hooks contain
> functions, so when you run a hook, the corresponding functions are called.

Ah, I was under the impression that the functions were also referred to
as hooks.  I will correct them in a future patch.

>> +(defun grep-edit-mode ()
>> +  "Major mode for editing *grep* buffers.
>> +In this mode, changes to the *grep* buffer are applied to the
>> +originating files.
>> +\\<grep-edit-mode-map>
>> +Type \\[grep-edit-save-changes] to exit Grep-Edit mode, return to Grep
>> +mode.
>> +
>> +The only editable texts in a Grep-Edit buffer are the match results."
>> +  (interactive)
>> +  (error "This mode can be enabled only by `grep-change-to-grep-edit-mode'"))
>> +(put 'grep-edit-mode 'mode-class 'special)
>> +
>> +(defun grep-change-to-grep-edit-mode ()
>> +  "Switch to `grep-edit-mode' to edit *grep* buffer."
>> +  (interactive)
>> +  (unless (derived-mode-p 'grep-mode)
>> +    (error "Not a Grep buffer"))
>> +  (when (get-buffer-process (current-buffer))
>> +    (error "Cannot switch when grep is running"))
>> +  (use-local-map grep-edit-mode-map)
>> +  (grep-edit--prepare-buffer)
>> +  (setq buffer-read-only nil)
>> +  (setq major-mode 'grep-edit-mode)
>> +  (setq mode-name "Grep-Edit")
>> +  (buffer-enable-undo)
>> +  (set-buffer-modified-p nil)
>> +  (setq buffer-undo-list nil)
>> +  (add-hook 'after-change-functions #'occur-after-change-function nil t)
>> +  (run-mode-hooks 'grep-edit-mode-hook)
>> +  (message "Editing: \\[grep-edit-save-changes] to return to Grep mode"))
>
> I'm tempted to say you should use `major-mode-suspend/resume` (which
> would avoid the duplication of parts of `grep-mode` in
> `grep-edit-save-changes`), but I guess this might re-introduce the
> problem with buffer-local variables.

Yes, unfortunately it will re-introduce the problem with buffer-local
variables.  

If everyone is okay with the current patch, I can get to writing the
NEWS entry and updating the manual.  Thanks.