GNU bug report logs - #79838
[PATCH] indent.el: better indent-for-tab-command function that call list of functions as a steps

Previous Next

To reply to this bug, email your comments to 79838 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Vitaliy Chepelev <vitalij <at> gmx.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] indent.el: better indent-for-tab-command function that call
 list of functions as a steps
Date: Sat, 15 Nov 2025 09:35:41 -0000

[Message part 1 (text/plain, inline)]

Tags: patch

This is patch proposed for convinience.

Now, to manage TAB key in Emacs user should set several variables or
 reimplement full function indent-for-tab-command.

In this patch seggested reimplementation of `indent-for-tab-command'
 function by splitting logic to a list of six functions, called in
 order. User may easily change this list of function by changing order
 and adding own functions.

This code should be tested at master Emacs version.

Some changed to `indent-for-tab-command' was made:
- `indent-relative-bol-only' used instead of `indent-relative', because
 second block execution of other functions of list of six
 functions.
 - fill-prefix logic was altered.

Org mode was tested to  be compatible with new `indent-for-tab-command'
 impelemntation, but this should be rechecked.
 
Why does this matter?
1) The TAB key is the main interface used with AI (along with the chat
 window).
2) Org mode reimplements the TAB key from scratch. If all modes are
 too separated, they will use boilerplate code that is prone to errors
 during updates.
3) it is frequently just needed to set different completions and
 suggestions depending on point position. User should no be forced to
 reimplement full `indent-for-tab-command'.


For GNU Emacs master https://github.com/emacs-mirror/emacs

[0001-indent.el-better-indent-for-tab-command-function-tha.patch (text/patch, attachment)]

[Message part 3 (text/plain, inline)]

-- 
Best regards,
Vitaliy
PGP pub key: 7850B0B5E3F536601D2E6A9DE1C43E074A047699

Message #8 received at 79838 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Vitaliy Chepelev <vitalij <at> gmx.com>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 79838 <at> debbugs.gnu.org
Subject: Re: bug#79838: [PATCH] indent.el: better indent-for-tab-command
 function that call list of functions as a steps
Date: Sat, 15 Nov 2025 13:33:42 +0200

> Date: Sat, 15 Nov 2025 09:35:41 -0000
> From:  Vitaliy Chepelev via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> This is patch proposed for convinience.

Thanks.

> Now, to manage TAB key in Emacs user should set several variables or
>  reimplement full function indent-for-tab-command.

Could you please name those variables?

> Why does this matter?
> 1) The TAB key is the main interface used with AI (along with the chat
>  window).

Could you elaborate on this part? why is indent-for-tab-command
relevant to interfaces with AI, and what do users need to do about
that?

> 2) Org mode reimplements the TAB key from scratch. If all modes are
>  too separated, they will use boilerplate code that is prone to errors
>  during updates.

Which other modes reimplement indent-for-tab-command?

> 3) it is frequently just needed to set different completions and
>  suggestions depending on point position. User should no be forced to
>  reimplement full `indent-for-tab-command'.

Please elaborate on this one, and please show a couple of examples of
these use cases, so that we could know what is at stake.

Last, but not least: to accept a contribution of this size, we will
need you to assign the copyright to the FSF.  If you agree, I will
send you the form to fill and the instructions, to start the
assignment process.

Message #11 received at 79838 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Vitaliy Chepelev <vitalij <at> gmx.com>
Cc: 79838 <at> debbugs.gnu.org
Subject: Re: bug#79838: [PATCH] indent.el: better indent-for-tab-command
 function that call list of functions as a steps
Date: Sat, 15 Nov 2025 10:06:37 -0500

> Now, to manage TAB key in Emacs user should set several variables or
> reimplement full function indent-for-tab-command.
>
> In this patch seggested reimplementation of `indent-for-tab-command'
> function by splitting logic to a list of six functions, called in
> order. User may easily change this list of function by changing order
> and adding own functions.

I've had it on my TODO for quite a few years, so I very much like
the idea.  See comments below.

The main question for me, tho, is whether it provides the flexibility
we need.  I.e. will it indeed make it unnecessary for other packages to
rebind TAB to their own command?

> Org mode was tested to  be compatible with new `indent-for-tab-command'
>  impelemntation, but this should be rechecked.

By "compatible" do you mean that you did make sure that Org could be
changed to use the new variable *instead* of rebinding TAB to `org-cycle`?

What about other packages that rebind the TAB key like `outline.el`,
`cperl.el`, `inferior-python-mode`, `vhdl-mode.el`, `wid-edit.el`, ...


        Stefan


> -(defvar indent-line-function 'indent-relative
> +(defvar indent-line-function 'indent-relative-bol-only
>    "Function to indent the current line.
>  This function will be called with no arguments.
>  If it is called somewhere where it cannot auto-indent, the function
>  should return `noindent' to signal that it didn't.
>  Setting this function is all you need to make TAB indent appropriately.
> +It is set to `indent-relative-bol-only'  by default to allow other steps
> +in `indent-for-tab-steps'.
>  Don't rebind TAB unless you really need to.")

Is it really necessary?  Why/where exactly?
This is a major change in behavior in some modes, so I think we need to
find a solution that works without it.

> +(defcustom indent-for-tab-steps
> +  (list
> +   'indent-for-tab-step-1-region-to-column
> +   'indent-for-tab-step-2-region-fill-prefix
> +   'indent-for-tab-step-3-region-indent-lines
> +   'indent-for-tab-step-4-insert-tab
> +   'indent-for-tab-step-5-indent-line
> +   'indent-for-tab-step-6-completion)

Since users may want to change the order, I don't think the function
names should include the position number.
[ As a general rule, function names (and docstrings) should
  describe/document what the function does rather than where&how
  it's used.  ]

[ Tho, I must admit that the numbers were handy while reviewing
  the code 🙂.  ]

> +  "List of steps to perform in the `indent-for-tab-command' function.
> +Stops at the first function that returns non nil."
> +  :type '(repeat function)
> +  :group 'indent)

I think it should be a hook rather than a list of functions.
I.e. call it `indent-for-tab-functions`.
Also the docstring should say how the functions are called.

> +(defun indent-region-column (start end &optional column)
> +  "Indent to COLUMN region defined by START and END."
> +  (interactive "r\nP")
> +  (setq column (prefix-numeric-value column))
> +    (save-excursion
> +      (goto-char end)
> +      (setq end (point-marker))
> +      (goto-char start)
> +      (or (bolp) (forward-line 1))
> +      (while (< (point) end)
> +        (delete-region (point) (progn (skip-chars-forward " \t") (point)))
> +        (or (eolp)
> +            (indent-to column 0))
> +        (forward-line 1))
> +      (move-marker end nil)))

AFAICT this is a new behavior/feature, right?
I so, please move it to a separate patch, so as not to distract from the
core refactoring.

> +(defun indent-region-fill-prefix (start end)
> +  "Insert fill-prefix for region defined by START and END."
> +  (interactive "r")
> +  (save-excursion
> +      (goto-char end)
> +      (setq end (point-marker))
> +      (goto-char start)
> +      (let ((regexp (regexp-quote fill-prefix)))
> +        (while (< (point) end)
> +          (or (looking-at regexp)
> +              (and (bolp) (eolp))
> +              (insert fill-prefix))
> +          (forward-line 1)))))
> +
> +(defun indent-for-tab-step-1-region-to-column (&optional arg)
> +  "Indent the region if it is activated.
> +If a numeric prefix is given, indent to that column."
> +  (when (and arg (use-region-p))
> +    (indent-region-column (region-beginning) (region-end) arg)
> +    (setq deactivate-mark t)))
> +
> +(defun indent-for-tab-step-2-region-fill-prefix (&optional arg)
> +  (when (and fill-prefix (use-region-p))
> +    (indent-region-fill-prefix (region-beginning) (region-end))
> +    (setq deactivate-mark t)))

Same here.

> +(defun indent-for-tab-step-3-region-indent-lines (&optional arg)
> +  "Indent the region if it is activated.
> +If a numeric prefix is given, indent to that column."
> +  (when (use-region-p)
> +    (if indent-region-function
> +        (save-restriction
> +          (widen)
> +          (funcall indent-region-function (region-beginning) (region-end)))
> +      ;; Else, use a default implementation that calls indent-line-function on
> +      ;; each line.
> +      (save-restriction
> +        (widen)
> +        (indent-region-line-by-line (region-beginning) (region-end))))
> +    (setq deactivate-mark t)))

The existing code uses just

    ((use-region-p)
     (indent-region (region-beginning) (region-end)))

Any reason for this change?

> +(defun indent-for-tab-step-4-insert-tab (&optional arg)
> +  "Insert a tab character if necessary.
> +Numeric prefix specify number of time to insert."
> +  (when (or (eq indent-line-function 'indent-to-left-margin)
> +             (and (not tab-always-indent)
> +                  (or (> (current-column) (current-indentation))
> +                      (eq this-command last-command))))
> +    (insert-tab arg)))

The existing code is:

    ((or ;; indent-to-left-margin is only meant for indenting,
         ;; so we force it to always insert a tab here.
         (eq indent-line-function 'indent-to-left-margin)
         (and (not tab-always-indent)
              (or (> (current-column) (current-indentation))
                  (eq this-command last-command))))
     (insert-tab arg))

Please try and preserve the comments during such refactorings.

> +(defun indent-for-tab-step-5-indent-line (&optional arg)
> +  "Indent the current line.
> +Halt execution if `indent-line-function' returns `noindent'.
> +If universal argument was given indent following sexp.
> +Return non-nil if indentation occured or was forcely halted."
> +  (let ((old-tick (buffer-chars-modified-tick))
> +        (old-indent (current-indentation))
> +        ;; - first indent attempt
> +        (halted (eq 'noindent (indent--funcall-widened indent-line-function))))
> +    (when (and (not halted)
> +               (eql old-indent (current-indentation)))
> +      (or (indent--default-inside-comment) ; should return True
> +          (when (or (<= (current-column) (current-indentation))
> +                    (not (eq tab-always-indent 'complete)))
> +            ;; - second indent attempt
> +            (indent--funcall-widened (default-value 'indent-line-function)))))
> +    (let ((moved-or-modified (or (not (eql old-indent (current-indentation)))
> +                                 (not (eql old-tick (buffer-chars-modified-tick))))))
> +      (when (and arg moved-or-modified)
> +        (indent-following-sexp old-indent))
> +      ;; - return
> +      (or moved-or-modified
> +          halted)))) ; halted with 'noindent

[ Please make sure your code does not go beyond 80 columns.  ]

Based on the existing code I would have expected something like:

    (let ((old-tick (buffer-chars-modified-tick))
          (old-point (point))
	  (old-indent (current-indentation)))

      ;; Indent the line.
      (or (not (eq (indent--funcall-widened indent-line-function) 'noindent))
          (indent--default-inside-comment)
          (when (or (<= (current-column) (current-indentation))
                    (not (eq tab-always-indent 'complete)))
            (indent--funcall-widened (default-value 'indent-line-function))))

      (cond
       ((and (eql old-point (point))
             (eql old-tick (buffer-chars-modified-tick)))
        ;; If the text was already indented right, we've done nothing.
        nil)

       ;; If a prefix argument was given, rigidly indent the following
       ;; sexp to match the change in the current line's indentation.
       (arg
        (indent-following-sexp old-indent)))))))

What is the intention behind this change?

> +(defun indent-for-tab-step-6-completion (&optional arg)
> +  "Perform completion if necessary based on nearby characters."
> +  (when (and (eq tab-always-indent 'complete)
> +             (or (eq last-command this-command)
> +                 (let ((next-syn (syntax-class (syntax-after (point)))) ; at cur position
> +                       (prev-syn (and (> (point) (point-min))
> +                                      (syntax-class (syntax-after (1- (point)))))) ; before cur pos: may be nil, 12 at begining
> +                       (bmt-old (buffer-modified-tick)))
> +                   (when (and (memq prev-syn '(2 3)) ; Prev is word or symbol constituent
> +                              (memq next-syn '(0 12 6 7 4 5 nil))) ; Next is whitespace or new line, or 6 ('), 7 ("), 4 (() 5 ())
> +                     (completion-at-point)
> +                     (equal bmt-old (buffer-modified-tick)))))))) ; check that completion-at-point was success

Similarly here, the old code is

       ((and (eq tab-always-indent 'complete)
             [...]
             (or (eq last-command this-command)
                 (let ((syn (syntax-class (syntax-after (point)))))
                   (pcase tab-first-completion
                     ('nil t)
                     ('eol (eolp))
                     ('word (not (eql 2 syn)))
                     ('word-or-paren (not (memq syn '(2 4 5))))
                     ('word-or-paren-or-punct (not (memq syn '(2 4 5 1))))))))
        (completion-at-point))

I understand you add the modified-tick check, but the others?
Regarding the modified-tick check, I'm not sure this is right:
`completion-at-point` may have made no change to the buffer but have
popped the *Completions* buffer, in which case I don't think returning
nil would be right because we wouldn't want to keep running other
functions in that case.

> +  (seq-find (lambda(step)
> +                (message (symbol-name step)) ; debug
> +                (funcall step arg))
> +            indent-for-tab-steps))

This would be `(run-hook-with-args-until-success
'indent-for-tab-functions arg)`.

BTW, if you place your new functions *after* `indent-for-tab-command,
`diff` (especially if you ask to ignore whitespace changes) should be
able to match large parts of the original code with the new code
(especially if you make refrain from making cosmetic changes) and
thus highlight the actual changes, making the code review (and the
argument that the new code behaves just like the old code) easier.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.