Requesting review for change to lisp/textmodes/sgml-mode.el

Requesting review for change to lisp/textmodes/sgml-mode.el

[Jackson Hamilton]

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

Hello comrades,

I made an adjustment (fix?) to the way SGML attributes are indented.

Previously, if one wrote a form like the following:

<element attribute="value">

He could break the attribute onto a new line and it would be indented like
so:

<element
   attribute="value">

But sgml-basic-offset defaults to 2, not 3, so it doesn't make much sense
that
the attribute is indented by 3 spaces.

And if I (setq sgml-basic-offset 4), now my attributes are indented by 5
spaces. Personally I do not expect this behavior, I expect the indentation
to
match.

Perhaps it could be argued that the extra space helps to improve
readability;
maybe so, but it still seems to contradict the offset value. In teams where
many
people use editors that insert multiples of N spaces or tabs, this +1
indentation strategy feels rather alienating. I think it would be better to
stick to a multiple of the specified offset when an attribute is sitting on
its
own line.

Hence the attached patch to remove the +1 indentation behavior.

Thanks for reviewing,
Jackson

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

[-- Attachment #2: 0001-lisp-textmodes-sgml-mode.el-sgml-calculate-indent-Fi.patch --]
[-- Type: text/x-patch, Size: 1560 bytes --]

From 4e4f4519641946ee0b36836646fb339cbafad182 Mon Sep 17 00:00:00 2001
From: Jackson Ray Hamilton <jackson@jacksonrayhamilton.com>
Date: Wed, 25 Feb 2015 01:56:49 -0800
Subject: [PATCH] * lisp/textmodes/sgml-mode.el (sgml-calculate-indent): Fix
 indent.

Previously, SGML attributes were always indented one additional space
past `sgml-basic-offset'. This fixes that.
---
 lisp/ChangeLog              | 5 +++++
 lisp/textmodes/sgml-mode.el | 4 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/lisp/ChangeLog b/lisp/ChangeLog
index fc2893e..17717e0 100644
--- a/lisp/ChangeLog
+++ b/lisp/ChangeLog
@@ -1,3 +1,8 @@
+2015-02-25  Jackson Ray Hamilton  <jackson@jacksonrayhamilton.com>
+
+	* lisp/textmodes/sgml-mode.el (sgml-calculate-indent): Fix
+	attribute indentation.
+
 2015-02-25  Stefan Monnier  <monnier@iro.umontreal.ca>
 
 	* emacs-lisp/edebug.el (edebug--display): Save-excursion (bug#19611).
diff --git a/lisp/textmodes/sgml-mode.el b/lisp/textmodes/sgml-mode.el
index 12d98c8..887c70d 100644
--- a/lisp/textmodes/sgml-mode.el
+++ b/lisp/textmodes/sgml-mode.el
@@ -1510,13 +1510,13 @@ LCON is the lexical context, if any."
     (`pi nil)
 
     (`tag
-     (goto-char (1+ (cdr lcon)))
+     (goto-char (cdr lcon))
      (skip-chars-forward "^ \t\n")	;Skip tag name.
      (skip-chars-forward " \t")
      (if (not (eolp))
 	 (current-column)
        ;; This is the first attribute: indent.
-       (goto-char (1+ (cdr lcon)))
+       (goto-char (cdr lcon))
        (+ (current-column) sgml-basic-offset)))
 
     (`text
-- 
1.9.1

Fwd: Requesting review for change to lisp/textmodes/sgml-mode.el

[Jackson Hamilton]

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

Hey guys, still hoping to get this reviewed. I wouldn't want to merge
something in that wasn't given the "A-OK."

---------- Forwarded message ----------
From: Jackson Hamilton <jackson@jacksonrayhamilton.com>
Date: Wed, Feb 25, 2015 at 2:35 AM
Subject: Requesting review for change to lisp/textmodes/sgml-mode.el
To: emacs-devel <emacs-devel@gnu.org>


Hello comrades,

I made an adjustment (fix?) to the way SGML attributes are indented.

Previously, if one wrote a form like the following:

<element attribute="value">

He could break the attribute onto a new line and it would be indented like
so:

<element
   attribute="value">

But sgml-basic-offset defaults to 2, not 3, so it doesn't make much sense
that
the attribute is indented by 3 spaces.

And if I (setq sgml-basic-offset 4), now my attributes are indented by 5
spaces. Personally I do not expect this behavior, I expect the indentation
to
match.

Perhaps it could be argued that the extra space helps to improve
readability;
maybe so, but it still seems to contradict the offset value. In teams where
many
people use editors that insert multiples of N spaces or tabs, this +1
indentation strategy feels rather alienating. I think it would be better to
stick to a multiple of the specified offset when an attribute is sitting on
its
own line.

Hence the attached patch to remove the +1 indentation behavior.

Thanks for reviewing,
Jackson

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

[-- Attachment #2: 0001-lisp-textmodes-sgml-mode.el-sgml-calculate-indent-Fi.patch --]
[-- Type: text/x-patch, Size: 1560 bytes --]

From 4e4f4519641946ee0b36836646fb339cbafad182 Mon Sep 17 00:00:00 2001
From: Jackson Ray Hamilton <jackson@jacksonrayhamilton.com>
Date: Wed, 25 Feb 2015 01:56:49 -0800
Subject: [PATCH] * lisp/textmodes/sgml-mode.el (sgml-calculate-indent): Fix
 indent.

Previously, SGML attributes were always indented one additional space
past `sgml-basic-offset'. This fixes that.
---
 lisp/ChangeLog              | 5 +++++
 lisp/textmodes/sgml-mode.el | 4 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/lisp/ChangeLog b/lisp/ChangeLog
index fc2893e..17717e0 100644
--- a/lisp/ChangeLog
+++ b/lisp/ChangeLog
@@ -1,3 +1,8 @@
+2015-02-25  Jackson Ray Hamilton  <jackson@jacksonrayhamilton.com>
+
+	* lisp/textmodes/sgml-mode.el (sgml-calculate-indent): Fix
+	attribute indentation.
+
 2015-02-25  Stefan Monnier  <monnier@iro.umontreal.ca>
 
 	* emacs-lisp/edebug.el (edebug--display): Save-excursion (bug#19611).
diff --git a/lisp/textmodes/sgml-mode.el b/lisp/textmodes/sgml-mode.el
index 12d98c8..887c70d 100644
--- a/lisp/textmodes/sgml-mode.el
+++ b/lisp/textmodes/sgml-mode.el
@@ -1510,13 +1510,13 @@ LCON is the lexical context, if any."
     (`pi nil)
 
     (`tag
-     (goto-char (1+ (cdr lcon)))
+     (goto-char (cdr lcon))
      (skip-chars-forward "^ \t\n")	;Skip tag name.
      (skip-chars-forward " \t")
      (if (not (eolp))
 	 (current-column)
        ;; This is the first attribute: indent.
-       (goto-char (1+ (cdr lcon)))
+       (goto-char (cdr lcon))
        (+ (current-column) sgml-basic-offset)))
 
     (`text
-- 
1.9.1

Re: Fwd: Requesting review for change to lisp/textmodes/sgml-mode.el

[Thien-Thi Nguyen]

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

() Jackson Hamilton <jackson@jacksonrayhamilton.com>
() Sat, 7 Mar 2015 16:49:59 -0800

   But sgml-basic-offset defaults to 2, not 3, so it doesn't
   make much sense that the attribute is indented by 3 spaces.

Another way to look at it is that the attribute name is indented
2 spaces with respect to the element name, so things are working
as designed.

Maybe a better approach would be to introduce a variable that
controls the ‘1+’ such that indentation-wrt-element-name (status
quo) remains as is doesn't ruffle the feathers of long-time
users, yet indentation-wrt-element-angle-brace is available for
those (including yourself) who prefer it.

In this way, we can avoid another post in N years saying that
lack of ‘1+’ "doesn't make much sense", its accompanying patch,
and some of the circular discussion that would ensue.

-- 
Thien-Thi Nguyen
   GPG key: 4C807502
   (if you're human and you know it)
      read my lisp: (responsep (questions 'technical)
                               (not (via 'mailing-list)))
                     => nil

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: Fwd: Requesting review for change to lisp/textmodes/sgml-mode.el

[Eli Zaretskii]

> From: Thien-Thi Nguyen <ttn@gnu.org>
> Date: Sun, 08 Mar 2015 09:10:41 +0100
> 
> () Jackson Hamilton <jackson@jacksonrayhamilton.com>
> () Sat, 7 Mar 2015 16:49:59 -0800
> 
>    But sgml-basic-offset defaults to 2, not 3, so it doesn't
>    make much sense that the attribute is indented by 3 spaces.
> 
> Another way to look at it is that the attribute name is indented
> 2 spaces with respect to the element name, so things are working
> as designed.

I completely agree: the indentation of the attribute seems to have
nothing to do with sgml-basic-offset.  It's a separate feature.

> Maybe a better approach would be to introduce a variable that
> controls the ‘1+’ such that indentation-wrt-element-name (status
> quo) remains as is doesn't ruffle the feathers of long-time
> users, yet indentation-wrt-element-angle-brace is available for
> those (including yourself) who prefer it.

Indeed, that would be a better change.

Thanks.

Re: Fwd: Requesting review for change to lisp/textmodes/sgml-mode.el

[Robin Templeton]

Thien-Thi Nguyen <ttn@gnu.org> writes:

> () Jackson Hamilton <jackson@jacksonrayhamilton.com>
> () Sat, 7 Mar 2015 16:49:59 -0800
>
>    But sgml-basic-offset defaults to 2, not 3, so it doesn't
>    make much sense that the attribute is indented by 3 spaces.
>
> Another way to look at it is that the attribute name is indented
> 2 spaces with respect to the element name, so things are working
> as designed.

That makes sense, but it seems inconsistent with the behavior of other
major modes in similar contexts. For example, Lisp indentation is
relative to the enclosing delimiter rather than the operator name.

> Maybe a better approach would be to introduce a variable that
> controls the ‘1+’ such that indentation-wrt-element-name (status
> quo) remains as is doesn't ruffle the feathers of long-time
> users, yet indentation-wrt-element-angle-brace is available for
> those (including yourself) who prefer it.

Another option would be to adopt nxml's solution and add a new variable
for the attribute indentation relative to the tag delimiter, which has
the additional advantage of allowing attribute and element indentation
to be customized independently. It could default to `(1+
sgml-basic-offset)' to avoid changing the existing behavior.

-- 
Inteligenta persono lernas la lingvon Esperanton rapide kaj facile.
Esperanto estas moderna, kultura lingvo por la mondo. Simpla, fleksebla,
belsona, Esperanto estas la praktika solvo de la problemo de universala
interkompreno. Lernu la interlingvon Esperanton!

Re: Fwd: Requesting review for change to lisp/textmodes/sgml-mode.el

[Stefan Monnier]

>> Another way to look at it is that the attribute name is indented
>> 2 spaces with respect to the element name, so things are working
>> as designed.

That's indeed the way I look at it.

> That makes sense, but it seems inconsistent with the behavior of other
> major modes in similar contexts. For example, Lisp indentation is
> relative to the enclosing delimiter rather than the operator name.

It's a philosophical question.  I tend to look at XML's <...> as
a parenthesis-like thingy that encloses a sub-language.

> Another option would be to adopt nxml's solution and add a new variable
> for the attribute indentation relative to the tag delimiter, which has
> the additional advantage of allowing attribute and element indentation
> to be customized independently. It could default to `(1+
> sgml-basic-offset)' to avoid changing the existing behavior.

Yes, that's the main issue: the two are distinct cases that should
have separate indentation rules.
I think we all violently agree on this.


        Stefan

bug#20161: Fwd: Requesting review for change to lisp/textmodes/sgml-mode.el

[Jackson Hamilton]

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

No one on emacs-devel seemed to notice, so I'm pushing this on to the bug
mailing list. If someone could please review this I'd appreciate it.

---------- Forwarded message ----------
From: Jackson Hamilton <jackson@jacksonrayhamilton.com>
Date: Sat, Mar 7, 2015 at 4:49 PM
Subject: Fwd: Requesting review for change to lisp/textmodes/sgml-mode.el
To: emacs-devel <emacs-devel@gnu.org>


Hey guys, still hoping to get this reviewed. I wouldn't want to merge
something in that wasn't given the "A-OK."

---------- Forwarded message ----------
From: Jackson Hamilton <jackson@jacksonrayhamilton.com>
Date: Wed, Feb 25, 2015 at 2:35 AM
Subject: Requesting review for change to lisp/textmodes/sgml-mode.el
To: emacs-devel <emacs-devel@gnu.org>


Hello comrades,

I made an adjustment (fix?) to the way SGML attributes are indented.

Previously, if one wrote a form like the following:

<element attribute="value">

He could break the attribute onto a new line and it would be indented like
so:

<element
   attribute="value">

But sgml-basic-offset defaults to 2, not 3, so it doesn't make much sense
that
the attribute is indented by 3 spaces.

And if I (setq sgml-basic-offset 4), now my attributes are indented by 5
spaces. Personally I do not expect this behavior, I expect the indentation
to
match.

Perhaps it could be argued that the extra space helps to improve
readability;
maybe so, but it still seems to contradict the offset value. In teams where
many
people use editors that insert multiples of N spaces or tabs, this +1
indentation strategy feels rather alienating. I think it would be better to
stick to a multiple of the specified offset when an attribute is sitting on
its
own line.

Hence the attached patch to remove the +1 indentation behavior.

Thanks for reviewing,
Jackson

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

[-- Attachment #2: 0001-lisp-textmodes-sgml-mode.el-sgml-calculate-indent-Fi.patch --]
[-- Type: text/x-patch, Size: 1560 bytes --]

From 4e4f4519641946ee0b36836646fb339cbafad182 Mon Sep 17 00:00:00 2001
From: Jackson Ray Hamilton <jackson@jacksonrayhamilton.com>
Date: Wed, 25 Feb 2015 01:56:49 -0800
Subject: [PATCH] * lisp/textmodes/sgml-mode.el (sgml-calculate-indent): Fix
 indent.

Previously, SGML attributes were always indented one additional space
past `sgml-basic-offset'. This fixes that.
---
 lisp/ChangeLog              | 5 +++++
 lisp/textmodes/sgml-mode.el | 4 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/lisp/ChangeLog b/lisp/ChangeLog
index fc2893e..17717e0 100644
--- a/lisp/ChangeLog
+++ b/lisp/ChangeLog
@@ -1,3 +1,8 @@
+2015-02-25  Jackson Ray Hamilton  <jackson@jacksonrayhamilton.com>
+
+	* lisp/textmodes/sgml-mode.el (sgml-calculate-indent): Fix
+	attribute indentation.
+
 2015-02-25  Stefan Monnier  <monnier@iro.umontreal.ca>
 
 	* emacs-lisp/edebug.el (edebug--display): Save-excursion (bug#19611).
diff --git a/lisp/textmodes/sgml-mode.el b/lisp/textmodes/sgml-mode.el
index 12d98c8..887c70d 100644
--- a/lisp/textmodes/sgml-mode.el
+++ b/lisp/textmodes/sgml-mode.el
@@ -1510,13 +1510,13 @@ LCON is the lexical context, if any."
     (`pi nil)
 
     (`tag
-     (goto-char (1+ (cdr lcon)))
+     (goto-char (cdr lcon))
      (skip-chars-forward "^ \t\n")	;Skip tag name.
      (skip-chars-forward " \t")
      (if (not (eolp))
 	 (current-column)
        ;; This is the first attribute: indent.
-       (goto-char (1+ (cdr lcon)))
+       (goto-char (cdr lcon))
        (+ (current-column) sgml-basic-offset)))
 
     (`text
-- 
1.9.1

bug#20161: Fwd: Requesting review for change to lisp/textmodes/sgml-mode.el

[Stefan Monnier]

> He could break the attribute onto a new line and it would be indented like
> so:

> <element
>    attribute="value">

> But sgml-basic-offset defaults to 2, not 3, so it doesn't make much sense
> that
> the attribute is indented by 3 spaces.

As is clear from the code you're changing the "1+" is quite deliberate,
which shows some people prefer it that way.

So as discussed, this indentation step should have its own custom var.
You can make it default to the behavior you prefer if you want.

Other than that, the patch looks OK.


        Stefan

bug#20161: Fwd: Requesting review for change to lisp/textmodes/sgml-mode.el

[Jackson Hamilton]

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

Okay, created a defcustom for it. Patch is attached.

On Sun, Mar 22, 2015 at 7:03 AM, Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> > He could break the attribute onto a new line and it would be indented
> like
> > so:
>
> > <element
> >    attribute="value">
>
> > But sgml-basic-offset defaults to 2, not 3, so it doesn't make much sense
> > that
> > the attribute is indented by 3 spaces.
>
> As is clear from the code you're changing the "1+" is quite deliberate,
> which shows some people prefer it that way.
>
> So as discussed, this indentation step should have its own custom var.
> You can make it default to the behavior you prefer if you want.
>
> Other than that, the patch looks OK.
>
>
>         Stefan
>

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

[-- Attachment #2: 0001-Have-sgml-attribute-offset-control-SGML-attribute-in.patch --]
[-- Type: text/x-patch, Size: 2883 bytes --]

From c4ab20eb5a0454438d96635b1c12f7b299e10579 Mon Sep 17 00:00:00 2001
From: Jackson Ray Hamilton <jackson@jacksonrayhamilton.com>
Date: Sun, 22 Mar 2015 08:22:29 -0700
Subject: [PATCH] Have `sgml-attribute-offset' control SGML attribute
 indentation

Fixes: debbugs:20161

* textmodes/sgml-mode.el (sgml-attribute-offset): New defcustom.
(sgml-calculate-indent): Use `sgml-attribute-offset' for attribute
indentation.
---
 lisp/ChangeLog                       |  6 ++++++
 lisp/textmodes/sgml-mode.el          | 23 +++++++++++++++++++++--
 test/indent/sgml-mode-attribute.html | 14 ++++++++++++++
 3 files changed, 41 insertions(+), 2 deletions(-)
 create mode 100644 test/indent/sgml-mode-attribute.html

diff --git a/lisp/ChangeLog b/lisp/ChangeLog
index 2f9c430..ba95401 100644
--- a/lisp/ChangeLog
+++ b/lisp/ChangeLog
@@ -1,3 +1,9 @@
+2015-03-22  Jackson Ray Hamilton  <jackson@jacksonrayhamilton.com>
+
+	* textmodes/sgml-mode.el (sgml-attribute-offset): New defcustom.
+	(sgml-calculate-indent): Use `sgml-attribute-offset' for attribute
+	indentation.
+
 2015-03-21  Titus von der Malsburg  <malsburg@posteo.de>
 
 	* window.el (window-font-width, window-font-height)
diff --git a/lisp/textmodes/sgml-mode.el b/lisp/textmodes/sgml-mode.el
index 12d98c8..8266647 100644
--- a/lisp/textmodes/sgml-mode.el
+++ b/lisp/textmodes/sgml-mode.el
@@ -46,6 +46,25 @@
   :type 'integer
   :group 'sgml)
 
+(defcustom sgml-attribute-offset 0
+  "Specifies a delta for attribute indentation in `sgml-indent-line'.
+
+When 0, attribute indentation looks like this:
+
+  <element
+    attribute=\"value\">
+  </element>
+
+When 2, attribute indentation looks like this:
+
+  <element
+      attribute=\"value\">
+  </element>"
+  :version "25.1"
+  :type 'integer
+  :safe 'integerp
+  :group 'sgml)
+
 (defcustom sgml-xml-mode nil
   "When non-nil, tag insertion functions will be XML-compliant.
 It is set to be buffer-local when the file has
@@ -1510,13 +1529,13 @@ LCON is the lexical context, if any."
     (`pi nil)
 
     (`tag
-     (goto-char (1+ (cdr lcon)))
+     (goto-char (+ (cdr lcon) sgml-attribute-offset))
      (skip-chars-forward "^ \t\n")	;Skip tag name.
      (skip-chars-forward " \t")
      (if (not (eolp))
 	 (current-column)
        ;; This is the first attribute: indent.
-       (goto-char (1+ (cdr lcon)))
+       (goto-char (+ (cdr lcon) sgml-attribute-offset))
        (+ (current-column) sgml-basic-offset)))
 
     (`text
diff --git a/test/indent/sgml-mode-attribute.html b/test/indent/sgml-mode-attribute.html
new file mode 100644
index 0000000..4cbec0a
--- /dev/null
+++ b/test/indent/sgml-mode-attribute.html
@@ -0,0 +1,14 @@
+<element attribute="value"></element>
+
+<element
+    attribute="value">
+  <element
+      attribute="value">
+  </element>
+</element>
+
+<!--
+    Local Variables:
+    sgml-attribute-offset: 2
+    End:
+  -->
-- 
1.9.1

bug#20161: Fwd: Requesting review for change to lisp/textmodes/sgml-mode.el

[Stefan Monnier]

> Okay, created a defcustom for it. Patch is attached.

Fine by me, thank you,


        Stefan