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

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