[mp26@os.inf.tu-dresden.de: Patch for sgml-mode.el]

[mp26@os.inf.tu-dresden.de: Patch for sgml-mode.el]

[Richard Stallman]

Would someone please DTRT then ack?

------- Start of forwarded message -------
X-Spam-Status: No, score=0.0 required=5.0 tests=UNPARSEABLE_RELAY 
	autolearn=failed version=3.1.0
Message-ID: <47457D57.7010008@os.inf.tu-dresden.de>
Date: Thu, 22 Nov 2007 14:00:07 +0100
From: Martin Pohlack <mp26@os.inf.tu-dresden.de>
MIME-Version: 1.0
To: bug-gnu-emacs@gnu.org
Content-Type: multipart/mixed; boundary="------------040006040108080505080703"
Subject: Patch for sgml-mode.el

This is a multi-part message in MIME format.
- --------------040006040108080505080703
Content-Type: text/plain; charset=ISO-8859-15
Content-Transfer-Encoding: 7bit

Hi,

sgml-mode.el currently contains such lines:

(define-derived-mode sgml-mode text-mode '(sgml-xml-mode "XML" "SGML")

and

(define-derived-mode html-mode sgml-mode '(sgml-xml-mode "XHTML" "HTML")
.

This violates that the third argument of define-derived-mode shall be a
string.  Furthermore, it results in "mode-name" not being a string for
html buffers.

The attached small patch against current CVS fixes these problems.

Cheers,
Martin Pohlack


- --------------040006040108080505080703
Content-Type: text/plain;
 name="sgml-mode_mode-name-fix.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="sgml-mode_mode-name-fix.diff"

- --- sgml-mode.el	2007-11-22 13:52:32.000000000 +0100
+++ /usr/share/emacs/23.0.60/lisp/textmodes/sgml-mode.el	2007-11-22 13:50:10.000000000 +0100
@@ -408,7 +406,7 @@
 	 (eq (char-before) ?<))))
 
 ;;;###autoload
- -(define-derived-mode sgml-mode text-mode '(sgml-xml-mode "XML" "SGML")
+(define-derived-mode sgml-mode text-mode (if sgml-xml-mode '"XML" '"SGML")
   "Major mode for editing SGML documents.
 Makes > match <.
 Keys <, &, SPC within <>, \", / and ' can be electric depending on
@@ -1890,7 +1888,7 @@
 
 \f
 ;;;###autoload
- -(define-derived-mode html-mode sgml-mode '(sgml-xml-mode "XHTML" "HTML")
+(define-derived-mode html-mode sgml-mode (if sgml-xml-mode '"XHTML" '"HTML")
   "Major mode based on SGML mode for editing HTML documents.
 This allows inserting skeleton constructs used in hypertext documents with
 completion.  See below for an introduction to HTML.  Use

- --------------040006040108080505080703--
------- End of forwarded message -------

Re: Fwd: Patch for sgml-mode.el

[Stefan Monnier]

> sgml-mode.el currently contains such lines:

> (define-derived-mode sgml-mode text-mode '(sgml-xml-mode "XML" "SGML")

> and

> (define-derived-mode html-mode sgml-mode '(sgml-xml-mode "XHTML" "HTML")
> .

> This violates that the third argument of define-derived-mode shall be a
> string.  Furthermore, it results in "mode-name" not being a string for
> html buffers.

I don't see a problem with that.  It's obviously been done on purpose.
Please describe where it causes a problem so we can fix that spot.


        Stefan

Re: Fwd: Patch for sgml-mode.el

[Martin Pohlack]

Stefan Monnier wrote:
>> sgml-mode.el currently contains such lines:
> 
>> (define-derived-mode sgml-mode text-mode '(sgml-xml-mode "XML" "SGML")
> 
>> and
> 
>> (define-derived-mode html-mode sgml-mode '(sgml-xml-mode "XHTML" "HTML")
>> .
> 
>> This violates that the third argument of define-derived-mode shall be a
>> string.  Furthermore, it results in "mode-name" not being a string for
>> html buffers.
> 
> I don't see a problem with that.  It's obviously been done on purpose.

Well, this was not obvious to me ...

> Please describe where it causes a problem so we can fix that spot.

Most obviously: the documentation for "define-derived-mode" states:

  (define-derived-mode child parent name &optional docstring &rest body)

  ...

  name: a string which will appear in the status line (e.g. "Hypertext")

So name should be a string.

Second, I could not find the documentation for the varibale mode-name in
emacs, but in xemacs it is documented as:

  Pretty name of current buffer's major mode (a string).

Third, I have a buffer-switching module here locally, which relies on
mode-name being a string.

  http://os.inf.tu-dresden.de/~mp26/download/cycbuf.el

Could you advice another way to get the mode-name as a string for a
buffer apart from accessing mode-name?  Maybe this could be documented?

Cheers,
Martin

Re: Fwd: Patch for sgml-mode.el

[Stefan Monnier]

> Third, I have a buffer-switching module here locally, which relies on
> mode-name being a string.

>   http://os.inf.tu-dresden.de/~mp26/download/cycbuf.el

> Could you advice another way to get the mode-name as a string for a
> buffer apart from accessing mode-name?  Maybe this could be documented?

Try (format-mode-line mode-name)


        Stefan

Re: Fwd: Patch for sgml-mode.el

[Glenn Morris]

Stefan Monnier wrote:

>> Furthermore, it results in "mode-name" not being a string
>> for html buffers.
>
> I don't see a problem with that.  It's obviously been done on purpose.
> Please describe where it causes a problem so we can fix that spot.

I have being meaning to post about this.

See for example

http://lists.gnu.org/archive/html/emacs-devel/2007-11/msg00891.html

  From:   h1t
  Subject:    Bug in ibuffer-mark-by-mode-regexp function
  Date:       Wed, 14 Nov 2007 10:27:04 +0200

A quick grep for mode-name in lisp/ appears to show several places
that assume it is a string. It's documented as being a string, so I
would think at the very least should be a NEWS entry saying it might
not be any more.

Re: Fwd: Patch for sgml-mode.el

[Stefan Monnier]

>>>>> "GM" == Glenn Morris <rgm@gnu.org> writes:

> Stefan Monnier wrote:
>>> Furthermore, it results in "mode-name" not being a string
>>> for html buffers.
>> 
>> I don't see a problem with that.  It's obviously been done on purpose.
>> Please describe where it causes a problem so we can fix that spot.

> I have being meaning to post about this.

> See for example

> http://lists.gnu.org/archive/html/emacs-devel/2007-11/msg00891.html

>   From:   h1t
>   Subject:    Bug in ibuffer-mark-by-mode-regexp function
>   Date:       Wed, 14 Nov 2007 10:27:04 +0200

> A quick grep for mode-name in lisp/ appears to show several places
> that assume it is a string. It's documented as being a string, so I
> would think at the very least should be a NEWS entry saying it might
> not be any more.

Does the patch below fix those problems?


        Stefan


Index: lisp/ibuffer.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/ibuffer.el,v
retrieving revision 1.88
diff -u -r1.88 ibuffer.el
--- lisp/ibuffer.el	19 Nov 2007 08:50:03 -0000	1.88
+++ lisp/ibuffer.el	30 Nov 2007 04:46:25 -0000
@@ -1722,7 +1722,7 @@
    ('mouse-face 'highlight
 		'keymap ibuffer-mode-name-map
 		'help-echo "mouse-2: filter by this mode"))
-  (format "%s" mode-name))
+  (format-mode-line mode-name))
 
 (define-ibuffer-column process
   (:summarizer
Index: lisp/ibuf-ext.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/ibuf-ext.el,v
retrieving revision 1.59
diff -u -r1.59 ibuf-ext.el
--- lisp/ibuf-ext.el	23 Oct 2007 15:54:06 -0000	1.59
+++ lisp/ibuf-ext.el	30 Nov 2007 04:46:25 -0000
@@ -1134,11 +1134,11 @@
   (string-lessp (downcase
 		  (with-current-buffer
 		      (car a)
-		    mode-name))
+		    (format-mode-line mode-name)))
 		(downcase
 		 (with-current-buffer
 		     (car b)
-		   mode-name))))
+		   (format-mode-line mode-name)))))
 
 ;;;###autoload (autoload 'ibuffer-do-sort-by-alphabetic "ibuf-ext")
 (define-ibuffer-sorter alphabetic
@@ -1386,7 +1386,7 @@
   (ibuffer-mark-on-buffer
    #'(lambda (buf)
        (with-current-buffer buf
-	 (string-match regexp mode-name)))))
+	 (string-match regexp (format-mode-line mode-name))))))
 
 ;;;###autoload
 (defun ibuffer-mark-by-file-name-regexp (regexp)

Re: Fwd: Patch for sgml-mode.el

[Glenn Morris]

Stefan Monnier wrote:

> Does the patch below fix those problems?

Well, it probably does, but does the incompatible change of allowing
mode-name to be a non-string bring any benefits?

Re: Fwd: Patch for sgml-mode.el

[Stefan Monnier]

>> Does the patch below fix those problems?
> Well, it probably does, but does the incompatible change of allowing
> mode-name to be a non-string bring any benefits?

The obvious one: no need to manually recompute the string when one of
its constituents changes.  I.e. sgml-xml-mode does not need to know that
enabling/disabling it can change the mode-name from/to SGML/XML or
HTML/XHTML.  Cleaner code, more obviously correct.

If you look at what Ibuffer does with its mode-name, you'll see that it
might also benefit from using this feature (tho I guess using
mode-line-process could be better so that the annotations don't show up
in the buffer's mode name in buffer lists).


        Stefan

Re: Fwd: Patch for sgml-mode.el

[Stefan Monnier]

>> Does the patch below fix those problems?
> Well, it probably does,

That I can tell, but I'd like for someone to confirm it.


        Stefan

Re: Fwd: Patch for sgml-mode.el

[Glenn Morris]

Stefan Monnier wrote:

> The obvious one: no need to manually recompute the string when one of
> its constituents changes.  I.e. sgml-xml-mode does not need to know that
> enabling/disabling it can change the mode-name from/to SGML/XML or
> HTML/XHTML.  Cleaner code, more obviously correct.


I don't see that it's any cleaner than the following, but I'm not
going to argue about it.


*** sgml-mode.el.~1.129.~	2007-08-14 00:36:28.000000000 -0700
--- sgml-mode.el	2007-12-02 13:55:42.000000000 -0800
***************
*** 408,414 ****
  	 (eq (char-before) ?<))))
  
  ;;;###autoload
! (define-derived-mode sgml-mode text-mode '(sgml-xml-mode "XML" "SGML")
    "Major mode for editing SGML documents.
  Makes > match <.
  Keys <, &, SPC within <>, \", / and ' can be electric depending on
--- 408,414 ----
  	 (eq (char-before) ?<))))
  
  ;;;###autoload
! (define-derived-mode sgml-mode text-mode "SGML"
    "Major mode for editing SGML documents.
  Makes > match <.
  Keys <, &, SPC within <>, \", / and ' can be electric depending on
***************
*** 462,468 ****
         'sgml-mode-facemenu-add-face-function)
    (set (make-local-variable 'sgml-xml-mode) (sgml-xml-guess))
    (if sgml-xml-mode
!       ()
      (set (make-local-variable 'skeleton-transformation-function)
           sgml-transformation-function))
    ;; This will allow existing comments within declarations to be
--- 462,468 ----
         'sgml-mode-facemenu-add-face-function)
    (set (make-local-variable 'sgml-xml-mode) (sgml-xml-guess))
    (if sgml-xml-mode
!       (setq mode-name "XML")
      (set (make-local-variable 'skeleton-transformation-function)
           sgml-transformation-function))
    ;; This will allow existing comments within declarations to be