bug#73206: 28.2; xml comment with blank lines to do not indent correctly, nxml-mode.el

bug#73206: 28.2; xml comment with blank lines to do not indent correctly, nxml-mode.el

[John Ciolfi via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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


1. Create foo.xml containing:

<foo>
    <!--
        foo

bar
    -->
</foo>

Notice that we have a blank line in the xml comment.

2. Put the point on 'bar' and press TAB.

Notice that the 'bar' line is not indented correctly.

Solution
--------

The problem is that in nxml-compute-indent-in-delimited-token, there
is a (forward-line -1) which assumes that the prior line is indented. In
this case the prior line is a blank line and thus doesn't have
indentation. After the (forward-line -1), we need to keep going back
until we see a non-blank line:

 	   (forward-line -1)
+           (while (looking-at "^[:blank:]*$")
+	     (forward-line -1))

Attached is a patch for nxml-mode.el that fixes the issue. 

Thanks
John

In GNU Emacs 28.2 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.38, cairo version 1.16.0)
 of 2024-06-25, modified by Debian built on x86-conova-01
Windowing system distributor 'The X.Org Foundation', version 11.0.12101006
System Description: Debian GNU/Linux 12 (bookworm)

Configured using:
 'configure --build x86_64-linux-gnu --prefix=/usr
 --sharedstatedir=/var/lib --libexecdir=/usr/libexec
 --localstatedir=/var/lib --infodir=/usr/share/info
 --mandir=/usr/share/man --with-libsystemd --with-pop=yes
 --enable-locallisppath=/etc/emacs:/usr/local/share/emacs/28.2/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/28.2/site-lisp:/usr/share/emacs/site-lisp
 --with-sound=alsa --without-gconf --with-mailutils
 --with-native-compilation --build x86_64-linux-gnu --prefix=/usr
 --sharedstatedir=/var/lib --libexecdir=/usr/libexec
 --localstatedir=/var/lib --infodir=/usr/share/info
 --mandir=/usr/share/man --with-libsystemd --with-pop=yes
 --enable-locallisppath=/etc/emacs:/usr/local/share/emacs/28.2/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/28.2/site-lisp:/usr/share/emacs/site-lisp
 --with-sound=alsa --without-gconf --with-mailutils
 --with-native-compilation --with-cairo --with-x=yes
 --with-x-toolkit=gtk3 --with-toolkit-scroll-bars 'CFLAGS=-g -O2
 -ffile-prefix-map=/build/reproducible-path/emacs-28.2+1=. -fstack-protector-strong
 -Wformat -Werror=format-security -Wall' 'CPPFLAGS=-Wdate-time
 -D_FORTIFY_SOURCE=2' LDFLAGS=-Wl,-z,relro'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES
NATIVE_COMP NOTIFY INOTIFY PDUMPER PNG RSVG SECCOMP SOUND THREADS TIFF
TOOLKIT_SCROLL_BARS X11 XDBE XIM XPM GTK3 ZLIB

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny dired dired-loaddefs
rfc822 mml mml-sec epa derived epg rfc6068 epg-config gnus-util rmail
rmail-loaddefs auth-source cl-seq eieio eieio-core cl-macs
eieio-loaddefs password-cache json map text-property-search time-date
subr-x seq byte-opt gv bytecomp byte-compile cconv mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader cl-loaddefs
cl-lib sendmail rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils
iso-transl tooltip eldoc paren electric uniquify ediff-hook vc-hooks
lisp-float-type elisp-mode mwheel term/x-win x-win term/common-win x-dnd
tool-bar dnd fontset image regexp-opt fringe tabulated-list replace
newcomment text-mode lisp-mode prog-mode register page tab-bar menu-bar
rfn-eshadow isearch easymenu timer select scroll-bar mouse jit-lock
font-lock syntax font-core term/tty-colors frame minibuffer cl-generic
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite emoji-zwj charscript
charprop case-table epa-hook jka-cmpr-hook help simple abbrev obarray
cl-preloaded nadvice button loaddefs faces cus-face macroexp files
window text-properties overlay sha1 md5 base64 format env code-pages
mule custom widget hashtable-print-readable backquote threads dbusbind
inotify lcms2 dynamic-setting system-font-setting font-render-setting
cairo move-toolbar gtk x-toolkit x multi-tty make-network-process
native-compile emacs)

Memory information:
((conses 16 68059 5432)
 (symbols 48 6639 0)
 (strings 32 19490 1559)
 (string-bytes 1 664189)
 (vectors 16 13926)
 (vector-slots 8 297636 11873)
 (floats 8 22 31)
 (intervals 56 258 0)
 (buffers 992 10))


[-- Attachment #2: nxml-mode.el.patch --]
[-- Type: text/x-diff, Size: 399 bytes --]

--- ./orig/nxml-mode.el	2024-09-12 09:10:46.580451579 -0400
+++ nxml-mode.el	2024-09-12 09:04:48.473571128 -0400
@@ -1522,6 +1522,8 @@
 	((progn
 	   (goto-char pos)
 	   (forward-line -1)
+           (while (looking-at "^[:blank:]*$")
+	     (forward-line -1))
 	   (<= (point) xmltok-start))
 	 (goto-char (+ xmltok-start (length open-delim)))
 	 (when (and (string= open-delim "<!--")

bug#73206: 28.2; xml comment with blank lines to do not indent correctly, nxml-mode.el

[Robert Pluim]

>>>>> On Thu, 12 Sep 2024 13:53:26 -0400, John Ciolfi via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org> said:

    John> 1. Create foo.xml containing:

    John> <foo>
    John>     <!--
    John>         foo

    John> bar
    --> 
    John> </foo>

    John> Notice that we have a blank line in the xml comment.

    John> 2. Put the point on 'bar' and press TAB.

    John> Notice that the 'bar' line is not indented correctly.

    John> Solution
    John> --------

    John> The problem is that in nxml-compute-indent-in-delimited-token, there
    John> is a (forward-line -1) which assumes that the prior line is indented. In
    John> this case the prior line is a blank line and thus doesn't have
    John> indentation. After the (forward-line -1), we need to keep going back
    John> until we see a non-blank line:

    John>  	   (forward-line -1)
    John> +           (while (looking-at "^[:blank:]*$")
    John> +	     (forward-line -1))
 
    John> Attached is a patch for nxml-mode.el that fixes the issue. 

Your patch does the right thing, but for the wrong reasons. Regexp
character classes only have meaning with a bracket expression, so what
youʼre matching here is actually any lines containing zero or more of
':', 'b', 'l', 'a', 'n', or 'k'. IOW, you should use:

     (looking-at "^[[:blank:]]*$")

Robert
-- 

bug#73206: 28.2; xml comment with blank lines to do not indent correctly, nxml-mode.el

[John Ciolfi via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Thanks for catching my typo and for moving the fix along.
________________________________
From: Robert Pluim <rpluim@gmail.com>
Sent: Friday, September 13, 2024 3:43 AM
To: 73206@debbugs.gnu.org <73206@debbugs.gnu.org>
Cc: John Ciolfi <ciolfi@mathworks.com>
Subject: Re: bug#73206: 28.2; xml comment with blank lines to do not indent correctly, nxml-mode.el

>>>>> On Thu, 12 Sep 2024 13:53:26 -0400, John Ciolfi via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org> said:

    John> 1. Create foo.xml containing:

    John> <foo>
    John>     <!--
    John>         foo

    John> bar
    -->
    John> </foo>

    John> Notice that we have a blank line in the xml comment.

    John> 2. Put the point on 'bar' and press TAB.

    John> Notice that the 'bar' line is not indented correctly.

    John> Solution
    John> --------

    John> The problem is that in nxml-compute-indent-in-delimited-token, there
    John> is a (forward-line -1) which assumes that the prior line is indented. In
    John> this case the prior line is a blank line and thus doesn't have
    John> indentation. After the (forward-line -1), we need to keep going back
    John> until we see a non-blank line:

    John>           (forward-line -1)
    John> +           (while (looking-at "^[:blank:]*$")
    John> +          (forward-line -1))

    John> Attached is a patch for nxml-mode.el that fixes the issue.

Your patch does the right thing, but for the wrong reasons. Regexp
character classes only have meaning with a bracket expression, so what
you’re matching here is actually any lines containing zero or more of
':', 'b', 'l', 'a', 'n', or 'k'. IOW, you should use:

     (looking-at "^[[:blank:]]*$")

Robert
--


[-- Attachment #2: Type: text/html, Size: 3436 bytes --]

bug#73206: 28.2; xml comment with blank lines to do not indent correctly, nxml-mode.el

[Eli Zaretskii]

Stefan, does the patch with the regexp fix look correct to you?

> Date: Sun, 15 Sep 2024 12:08:17 +0000
> msip_labels: 
> From:  John Ciolfi via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Thanks for catching my typo and for moving the fix along.
> -----------------------------------------------------------------------------
> From: Robert Pluim <rpluim@gmail.com>
> Sent: Friday, September 13, 2024 3:43 AM
> To: 73206@debbugs.gnu.org <73206@debbugs.gnu.org>
> Cc: John Ciolfi <ciolfi@mathworks.com>
> Subject: Re: bug#73206: 28.2; xml comment with blank lines to do not indent correctly, nxml-mode.el 
>  
> >>>>> On Thu, 12 Sep 2024 13:53:26 -0400, John Ciolfi via "Bug reports for GNU Emacs, the Swiss army
> knife of text editors" <bug-gnu-emacs@gnu.org> said:
> 
>     John> 1. Create foo.xml containing:
> 
>     John> <foo>
>     John>     <!--
>     John>         foo
> 
>     John> bar
>     --> 
>     John> </foo>
> 
>     John> Notice that we have a blank line in the xml comment.
> 
>     John> 2. Put the point on 'bar' and press TAB.
> 
>     John> Notice that the 'bar' line is not indented correctly.
> 
>     John> Solution
>     John> --------
> 
>     John> The problem is that in nxml-compute-indent-in-delimited-token, there
>     John> is a (forward-line -1) which assumes that the prior line is indented. In
>     John> this case the prior line is a blank line and thus doesn't have
>     John> indentation. After the (forward-line -1), we need to keep going back
>     John> until we see a non-blank line:
> 
>     John>           (forward-line -1)
>     John> +           (while (looking-at "^[:blank:]*$")
>     John> +          (forward-line -1))
>  
>     John> Attached is a patch for nxml-mode.el that fixes the issue. 
> 
> Your patch does the right thing, but for the wrong reasons. Regexp
> character classes only have meaning with a bracket expression, so what
> you’re matching here is actually any lines containing zero or more of
> ':', 'b', 'l', 'a', 'n', or 'k'. IOW, you should use:
> 
>      (looking-at "^[[:blank:]]*$")
> 
> Robert
> -- 

bug#73206: 28.2; xml comment with blank lines to do not indent correctly, nxml-mode.el

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> Stefan, does the patch with the regexp fix look correct to you?

If we want to "keep going back until we see a non-blank line", surely
the fragment should read:

    (while (looking-at "^$")
      (forward-line -1))

Since

    (looking-at "^[[:blank:]]*$")

will match both blank lines, and lines containing only blank space.

Which of the two do we want here?

I think it would also be good to add one or more tests here.

bug#73206: 28.2; xml comment with blank lines to do not indent correctly, nxml-mode.el

[John Ciolfi via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1.1: Type: text/plain, Size: 2000 bytes --]

Hi

Using "^$" or "^[[:blank:]]*$" is fine. The difference is very minor. Consider foo.xml where the visibly "empty" line contains a single space and foo starts on the 1st column:

<foo>
  <!--
      bar

foo
  -->
</foo>

When using "^$" and you type tab on the foo line, you'll get the following where foo starts on the 2nd column:

<foo>
  <!--
      bar

 foo
  -->
</foo>

If you use "^[[:blank:]]*$, you'll get what I expected:

<foo>
  <!--
      bar

      foo
  -->
</foo>

However, if you select all and indent-region, C-M-\ on the original you'll get the expected result with either "^$" or "^[[:blank:]]*$ because nxml-mode will pad out the space line. If the "empty" line truly blank (no spaces or tabs), then the two regex's behave identical.

I suggest for test cases, two versions of foo.xml where one version of it has the empty line truly blank (no spaces or tabs) and the other version contains a space in the "empty" line.

You can use the attached nxml-mode-indent-fix.el which overrides the broken function to try things out on a stock Emacs, emacs -Q.

Thanks
John


________________________________
From: Stefan Kangas <stefankangas@gmail.com>
Sent: Sunday, September 29, 2024 4:47 PM
To: Eli Zaretskii <eliz@gnu.org>; John Ciolfi <ciolfi@mathworks.com>
Cc: rpluim@gmail.com <rpluim@gmail.com>; 73206@debbugs.gnu.org <73206@debbugs.gnu.org>
Subject: Re: bug#73206: 28.2; xml comment with blank lines to do not indent correctly, nxml-mode.el

Eli Zaretskii <eliz@gnu.org> writes:

> Stefan, does the patch with the regexp fix look correct to you?

If we want to "keep going back until we see a non-blank line", surely
the fragment should read:

    (while (looking-at "^$")
      (forward-line -1))

Since

    (looking-at "^[[:blank:]]*$")

will match both blank lines, and lines containing only blank space.

Which of the two do we want here?

I think it would also be good to add one or more tests here.


[-- Attachment #1.2: Type: text/html, Size: 9348 bytes --]

[-- Attachment #2: nxml-mode-indent-fix.el --]
[-- Type: text/x-emacs-lisp, Size: 1327 bytes --]

(defvar xmltok-start) ;; from xmltok.el
(defun nxml-compute-indent-in-delimited-token-override (pos open-delim close-delim)
  "Return the indent for a line that start inside a token with delimiters.
OPEN-DELIM and CLOSE-DELIM are strings giving the opening and closing
delimiters.  POS is the position of the first non-whitespace character
of the line.  This expects the xmltok-* variables to be set up as by
`xmltok-forward'.
Fix: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=73206"
  (cond ((let ((end (+ pos (length close-delim))))
	   (and (<= end (point-max))
		(string= (buffer-substring-no-properties pos end)
			 close-delim)))
	 (goto-char xmltok-start))
	((progn
	   (goto-char pos)
	   (forward-line -1)
           (while (looking-at "^[[:blank:]]*$")
	     (forward-line -1))
	   (<= (point) xmltok-start))
	 (goto-char (+ xmltok-start (length open-delim)))
	 (when (and (string= open-delim "<!--")
		    (looking-at " "))
	   (goto-char (1+ (point)))))
	(t (back-to-indentation)))
  (current-column))

(when (version<= emacs-version "29")
  ;; Assume https://debbugs.gnu.org/cgi/bugreport.cgi?bug=73206 will be fixed in Emacs 30
  (eval-after-load 'nxml-mode
    '(advice-add 'nxml-compute-indent-in-delimited-token
                 :override
                 #'nxml-compute-indent-in-delimited-token-override)))