bug#20895: [patch] lexical binding broke part of sgml-mode.el

bug#20895: [patch] lexical binding broke part of sgml-mode.el

[Stephen Berman]

The following change broke some functionality in sgml-mode.el:

   commit 84e0b7dad6f1a8e53261f9b96f5a9080fea681a4
   Author: Stefan Monnier <monnier@iro.umontreal.ca>
   Date:   Mon Apr 13 15:51:15 2015 -0400
   [...]
       * lisp/textmodes/sgml-mode.el: Use lexical-binding.

To see the bug, visit an (X)HTML file and insert a tag (`C-c C-t') that
takes an attribute, such `p' or `a'.

The following patch fixes the problems I have encountered, but I'm not
sure if further adjustments are needed, so I'll defer to whoever knows
the code better about whether to commit it as is.


2015-06-25  Stephen Berman  <stephen.berman@gmx.net>

	* sgml-mode.el (sgml-attributes, sgml-value): Make them work with
	lexical binding.


diff --git a/lisp/textmodes/sgml-mode.el b/lisp/textmodes/sgml-mode.el
index 2ffe9c1..dbb9662 100644
--- a/lisp/textmodes/sgml-mode.el
+++ b/lisp/textmodes/sgml-mode.el
@@ -759,9 +759,9 @@ If QUIET, do not print a message when there are no attributes for TAG."
 	    (insert ?\s)
 	    (insert (funcall skeleton-transformation-function
 			     (setq attribute
-				   (skeleton-read '(completing-read
+				   (skeleton-read `(completing-read
 						    "Attribute: "
-						    alist)))))
+						    ',alist)))))
 	    (if (string= "" attribute)
 		(setq i 0)
 	      (sgml-value (assoc (downcase attribute) alist))
@@ -1177,13 +1177,13 @@ See `sgml-tag-alist' for info about attribute rules."
     (if (and (eq (car alist) t) (not sgml-xml-mode))
 	(when (cdr alist)
 	  (insert "=\"")
-	  (setq alist (skeleton-read '(completing-read "Value: " (cdr alist))))
+	  (setq alist (skeleton-read `(completing-read "Value: " (cdr ',alist))))
 	  (if (string< "" alist)
 	      (insert alist ?\")
 	    (delete-char -2)))
       (insert "=\"")
       (if (cdr alist)
-          (insert (skeleton-read '(completing-read "Value: " alist)))
+          (insert (skeleton-read `(completing-read "Value: " ',alist)))
         (when (null alist)
           (insert (skeleton-read '(read-string "Value: ")))))
       (insert ?\"))))

bug#20895: [patch] lexical binding broke part of sgml-mode.el

[Dmitry Gutov]

Hi!

On 06/25/2015 01:42 PM, Stephen Berman wrote:

> The following patch fixes the problems I have encountered, but I'm not
> sure if further adjustments are needed, so I'll defer to whoever knows
> the code better about whether to commit it as is.

Thanks for the patch, but please check out the discussion at 
http://debbugs.gnu.org/cgi/bugreport.cgi?bug=20386.

bug#20895: [patch] lexical binding broke part of sgml-mode.el

[Stephen Berman]

On Thu, 25 Jun 2015 16:03:04 +0300 Dmitry Gutov <dgutov@yandex.ru> wrote:

> Hi!
>
> On 06/25/2015 01:42 PM, Stephen Berman wrote:
>
>> The following patch fixes the problems I have encountered, but I'm not
>> sure if further adjustments are needed, so I'll defer to whoever knows
>> the code better about whether to commit it as is.
>
> Thanks for the patch, but please check out the discussion at
> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=20386.

Oh, I missed (or forgot about) that (I've been a bit out of the Emacs
development loop lately, and only recently updated my build and hit this
bug).  Do you disagree with Stefan's suggestion or just haven't gotten
around to implementing it?  I can understand if the latter is the
reason, as I have little time myself to come up with a more involved
fix; still, it would be nice to have it fixed soon.

Steve Berman

bug#20895: [patch] lexical binding broke part of sgml-mode.el

[Dmitry Gutov]

On 06/25/2015 04:27 PM, Stephen Berman wrote:
> or just haven't gotten around to implementing it?

That, more or less. And sgml-mode is not a priority for me personally, 
so if someone else takes that up, it would be welcome.

bug#20895: [patch] lexical binding broke part of sgml-mode.el

[Stephen Berman]

On Thu, 25 Jun 2015 17:47:01 +0300 Dmitry Gutov <dgutov@yandex.ru> wrote:

> On 06/25/2015 04:27 PM, Stephen Berman wrote:
>> or just haven't gotten around to implementing it?
>
> That, more or less. And sgml-mode is not a priority for me personally, so if
> someone else takes that up, it would be welcome.

A usable sgml-mode is a priority for me, that's why I tried to fix it,
and for the time being it's good enough for me.  I probably won't be
able to try and implement the better fix in the near future, but will
take a look when I can, INOBMTI (if no one beats me to it).

On Thu, 25 Jun 2015 17:34:31 +0300 Eli Zaretskii <eliz@gnu.org> wrote:

> Whatever comes out of this discussion, would you please consider
> adding a suitable test to sgml-mode-tests.el?  TIA.

I'll try to do this too INOBMTI, though it may take even longer (it will
mean having to learn how to use ERT first).

Steve Berman

bug#20895: [patch] lexical binding broke part of sgml-mode.el

[Eli Zaretskii]

> From: Stephen Berman <stephen.berman@gmx.net>
> Cc: Eli Zaretskii <eliz@gnu.org>, 20895@debbugs.gnu.org
> Date: Thu, 25 Jun 2015 21:28:12 +0200
> 
> > Whatever comes out of this discussion, would you please consider
> > adding a suitable test to sgml-mode-tests.el?  TIA.
> 
> I'll try to do this too INOBMTI, though it may take even longer (it will
> mean having to learn how to use ERT first).

Thanks.  The tests already in sgml-mode-tests are pretty good
examples, and I'm sure you will find a way of mimicking them in no
time.

bug#20895: [patch] lexical binding broke part of sgml-mode.el

[Eli Zaretskii]

> From: Stephen Berman <stephen.berman@gmx.net>
> Date: Thu, 25 Jun 2015 12:42:35 +0200
> 
> The following change broke some functionality in sgml-mode.el:
> 
>    commit 84e0b7dad6f1a8e53261f9b96f5a9080fea681a4
>    Author: Stefan Monnier <monnier@iro.umontreal.ca>
>    Date:   Mon Apr 13 15:51:15 2015 -0400
>    [...]
>        * lisp/textmodes/sgml-mode.el: Use lexical-binding.
> 
> To see the bug, visit an (X)HTML file and insert a tag (`C-c C-t') that
> takes an attribute, such `p' or `a'.
> 
> The following patch fixes the problems I have encountered, but I'm not
> sure if further adjustments are needed, so I'll defer to whoever knows
> the code better about whether to commit it as is.

Whatever comes out of this discussion, would you please consider
adding a suitable test to sgml-mode-tests.el?  TIA.