bug#2034: 23.0.60; c-subword-mode incompatible with xml-mode

bug#2034: 23.0.60; c-subword-mode incompatible with xml-mode

[me]

Running "M-x c-subword-mode" in a buffer using xml-mode will raise the
following error:

c-update-modeline: Wrong type argument: stringp, (sgml-xml-mode "XML" "SGML")

Here is the output from the debugger:

Debugger entered--Lisp error: (wrong-type-argument stringp (sgml-xml-mode "XML" "SGML"))
  string-match("\\(^[^/]*\\)/" (sgml-xml-mode "XML" "SGML"))
  c-update-modeline()
  c-subword-mode()

Apparently, the c-update-modeline counts on mode-name being a string
when in an xml-mode buffer mode-name is a list.


In GNU Emacs 23.0.60.1 (i486-pc-linux-gnu, GTK+ Version 2.14.4)
 of 2008-12-05 on iridium, modified by Debian
 (emacs-snapshot package, version 1:20081205-1)
Windowing system distributor `The X.Org Foundation', version 11.0.10502000
configured using `configure  '--build' 'i486-linux-gnu' '--host' 'i486-linux-gnu' '--prefix=/usr' '--sharedstatedir=/var/lib' '--libexecdir=/usr/lib' '--localstatedir=/var' '--infodir=/usr/share/info' '--mandir=/usr/share/man' '--with-pop=yes' '--enable-locallisppath=/etc/emacs-snapshot:/etc/emacs:/usr/local/share/emacs/23.0.60/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/23.0.60/site-lisp:/usr/share/emacs/site-lisp' '--with-x=yes' '--with-x-toolkit=gtk' 'build_alias=i486-linux-gnu' 'host_alias=i486-linux-gnu' 'CFLAGS=-DDEBIAN -DSITELOAD_PURESIZE_EXTRA=5000 -g -O2' 'LDFLAGS=-g -Wl,--as-needed' 'CPPFLAGS=''

Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: nil
  locale-coding-system: utf-8-unix
  default-enable-multibyte-characters: t

Major mode: Debugger

Minor modes in effect:
  shell-dirtrack-mode: t
  show-paren-mode: t
  server-mode: t
  diff-auto-refine-mode: t
  tooltip-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  global-auto-composition-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent input:
C-y C-x C-s C-x b <return> <down> <down> <up> <down> 
<down> <return> <return> <down> <down> <down> <down> 
<down> <down> <down> <down> <down> <down> <down> <down> 
<down> <down> C-y C-x C-s <up> C-SPC <down> C-w <up> 
C-y C-x C-s C-x b <return> <down> <down> <return> <return> 
<down> <down> <down> <down> <down> <down> <down> <down> 
<down> <down> <down> <down> <down> <down> <down> <down> 
<down> C-y C-x C-s C-x b <return> <down> m m m m m 
<down> <down> <down> <up> <up> <up> m <down> <down> 
m m m <down> m m <up> <up> <up> <up> <up> <up> <up> 
C-x o C-x b <return> <up> <down> C-SPC <down> M-w <up> 
<up> C-SPC <down> <down> M-w <up> C-SPC <down> M-w 
C-x o M-< Q C-y M-y <return> C-y M-y C-g C-g Q <up> 
<down> C-y <return> <up> <up> <return> Q <return> SPC 
C-x C-s C-x b <return> M-x t o g g <tab> d e b <tab> 
r <backspace> e r r <tab> <return> Q <return> <backspace> 
<backspace> C-h f s g m l - x m l <tab> m o <tab> <return> 
<tab> <backspace> <backspace> <tab> C-g C-g C-g <down> 
<down> <right> <right> S-SPC C-g <up> C-SPC M-f M-f 
C-c p g <return> C-g C-x b m e s s <return> M-v C-x 
b <return> <up> <return> <up> <down> C-s s t r i n 
g p C-g C-g C-g <down> <down> <down> <down> <down> 
<down> <down> <down> <down> <down> <down> <down> <down> 
<down> <up> <up> <down> <down> <down> <down> <down> 
<down> <down> <down> <down> <down> <down> <down> <down> 
<down> <up> <up> <up> <up> <up> <up> <up> <up> <up> 
<up> <up> <up> <up> <up> <up> <up> <up> <up> <up> <up> 
<up> <up> <up> <up> <up> <up> <down> <down> <down> 
C-x o M-x r e p o <tab> r <tab> <tab> b <tab> <ret
urn>

Recent messages:
uncompressing cc-cmds.el.gz...done
Note: file is write protected
nnimap: Updating info for devel...done
nnimap: Updating info for devel...done
nnimap: Updating info for devel...done
Quit [2 times]
nnimap: Updating info for buy.lists...done
nnimap: Updating info for buy.lists...done
nnimap: Updating info for buy.lists...done
Making completion list... [2 times]

bug#2034: 23.0.60; c-subword-mode incompatible with xml-mode

[Stefan Monnier]

> Running "M-x c-subword-mode" in a buffer using xml-mode will raise the
> following error:

> c-update-modeline: Wrong type argument: stringp, (sgml-xml-mode "XML" "SGML")

c-subword-mode is obviously written specifically for modes provided by
the cc-*.el files.  It may accidentally work in some other modes, but at
least c-update-modeline makes several assumptions about the format used
for `mode-name'.


        Stefan

bug#2034: 23.0.60; c-subword-mode incompatible with xml-mode

[Ross Patterson]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Running "M-x c-subword-mode" in a buffer using xml-mode will raise the
>> following error:
>
>> c-update-modeline: Wrong type argument: stringp, (sgml-xml-mode "XML" "SGML")
>
> c-subword-mode is obviously written specifically for modes provided by
> the cc-*.el files.  It may accidentally work in some other modes, but at
> least c-update-modeline makes several assumptions about the format used
> for `mode-name'.

Well I find it very useful to use c-subword-mode in pretty much all my
buffers.  Here's the patched version of this function I'm using to work
around this issue:

(defun c-update-modeline ()
  (let* ((fmt (format "/%s%s%s%s"
                      (if c-electric-flag "l" "")
                      (if (and c-electric-flag c-auto-newline)
                          "a" "")
                      (if c-hungry-delete-key "h" "")
                      (if (and
                           ;; cc-subword might not be loaded.
                           (boundp 'c-subword-mode)
                           (symbol-value 'c-subword-mode))
                          "w"
                        "")))
         (str-mode-name (if (listp mode-name)
                            (nth 1 mode-name)
                          mode-name))
         (bare-mode-name (if (string-match "\\(^[^/]*\\)/" str-mode-name)
                             (substring str-mode-name (match-beginning 1) (match-end 1))
                           str-mode-name)))
    ;;     (setq c-submode-indicators
    ;; 	  (if (> (length fmt) 1)
    ;; 	      fmt))
    (setq mode-name
          (if (> (length fmt) 1)
              (concat bare-mode-name fmt)
            bare-mode-name))
    (force-mode-line-update)))

Ross

bug#2034: marked as done (23.0.60; c-subword-mode incompatible with xml-mode)

[Emacs bug Tracking System]

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

Your message dated Sat, 23 Jan 2010 17:34:58 -0500
with message-id <8cbpgkqwkt.fsf@fencepost.gnu.org>
and subject line Re: Bug#2034: c-subword-mode incompatible with xml-mode
has caused the Emacs bug report #2034,
regarding 23.0.60; c-subword-mode incompatible with xml-mode
to be marked as done.

This means that you claim that the problem has been dealt with.
If this is not the case it is now your responsibility to reopen the
bug report if necessary, and/or fix the problem forthwith.

(NB: If you are a system administrator and have no idea what this
message is talking about, this may indicate a serious mail system
misconfiguration somewhere. Please contact bug-gnu-emacs@gnu.org
immediately.)


-- 
2034: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=2034
Emacs Bug Tracking System
Contact bug-gnu-emacs@gnu.org with problems

[-- Attachment #2: Type: message/rfc822, Size: 6011 bytes --]

From: me@rpatterson.net
To: emacs-pretest-bug@gnu.org
Subject: 23.0.60; c-subword-mode incompatible with xml-mode
Date: Sat, 24 Jan 2009 18:10:37 -0800
Message-ID: <87skn8xhia.fsf@transitory.lefae.org>


Running "M-x c-subword-mode" in a buffer using xml-mode will raise the
following error:

c-update-modeline: Wrong type argument: stringp, (sgml-xml-mode "XML" "SGML")

Here is the output from the debugger:

Debugger entered--Lisp error: (wrong-type-argument stringp (sgml-xml-mode "XML" "SGML"))
  string-match("\\(^[^/]*\\)/" (sgml-xml-mode "XML" "SGML"))
  c-update-modeline()
  c-subword-mode()

Apparently, the c-update-modeline counts on mode-name being a string
when in an xml-mode buffer mode-name is a list.


In GNU Emacs 23.0.60.1 (i486-pc-linux-gnu, GTK+ Version 2.14.4)
 of 2008-12-05 on iridium, modified by Debian
 (emacs-snapshot package, version 1:20081205-1)
Windowing system distributor `The X.Org Foundation', version 11.0.10502000
configured using `configure  '--build' 'i486-linux-gnu' '--host' 'i486-linux-gnu' '--prefix=/usr' '--sharedstatedir=/var/lib' '--libexecdir=/usr/lib' '--localstatedir=/var' '--infodir=/usr/share/info' '--mandir=/usr/share/man' '--with-pop=yes' '--enable-locallisppath=/etc/emacs-snapshot:/etc/emacs:/usr/local/share/emacs/23.0.60/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/23.0.60/site-lisp:/usr/share/emacs/site-lisp' '--with-x=yes' '--with-x-toolkit=gtk' 'build_alias=i486-linux-gnu' 'host_alias=i486-linux-gnu' 'CFLAGS=-DDEBIAN -DSITELOAD_PURESIZE_EXTRA=5000 -g -O2' 'LDFLAGS=-g -Wl,--as-needed' 'CPPFLAGS=''

Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: nil
  locale-coding-system: utf-8-unix
  default-enable-multibyte-characters: t

Major mode: Debugger

Minor modes in effect:
  shell-dirtrack-mode: t
  show-paren-mode: t
  server-mode: t
  diff-auto-refine-mode: t
  tooltip-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  global-auto-composition-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent input:
C-y C-x C-s C-x b <return> <down> <down> <up> <down> 
<down> <return> <return> <down> <down> <down> <down> 
<down> <down> <down> <down> <down> <down> <down> <down> 
<down> <down> C-y C-x C-s <up> C-SPC <down> C-w <up> 
C-y C-x C-s C-x b <return> <down> <down> <return> <return> 
<down> <down> <down> <down> <down> <down> <down> <down> 
<down> <down> <down> <down> <down> <down> <down> <down> 
<down> C-y C-x C-s C-x b <return> <down> m m m m m 
<down> <down> <down> <up> <up> <up> m <down> <down> 
m m m <down> m m <up> <up> <up> <up> <up> <up> <up> 
C-x o C-x b <return> <up> <down> C-SPC <down> M-w <up> 
<up> C-SPC <down> <down> M-w <up> C-SPC <down> M-w 
C-x o M-< Q C-y M-y <return> C-y M-y C-g C-g Q <up> 
<down> C-y <return> <up> <up> <return> Q <return> SPC 
C-x C-s C-x b <return> M-x t o g g <tab> d e b <tab> 
r <backspace> e r r <tab> <return> Q <return> <backspace> 
<backspace> C-h f s g m l - x m l <tab> m o <tab> <return> 
<tab> <backspace> <backspace> <tab> C-g C-g C-g <down> 
<down> <right> <right> S-SPC C-g <up> C-SPC M-f M-f 
C-c p g <return> C-g C-x b m e s s <return> M-v C-x 
b <return> <up> <return> <up> <down> C-s s t r i n 
g p C-g C-g C-g <down> <down> <down> <down> <down> 
<down> <down> <down> <down> <down> <down> <down> <down> 
<down> <up> <up> <down> <down> <down> <down> <down> 
<down> <down> <down> <down> <down> <down> <down> <down> 
<down> <up> <up> <up> <up> <up> <up> <up> <up> <up> 
<up> <up> <up> <up> <up> <up> <up> <up> <up> <up> <up> 
<up> <up> <up> <up> <up> <up> <down> <down> <down> 
C-x o M-x r e p o <tab> r <tab> <tab> b <tab> <ret
urn>

Recent messages:
uncompressing cc-cmds.el.gz...done
Note: file is write protected
nnimap: Updating info for devel...done
nnimap: Updating info for devel...done
nnimap: Updating info for devel...done
Quit [2 times]
nnimap: Updating info for buy.lists...done
nnimap: Updating info for buy.lists...done
nnimap: Updating info for buy.lists...done
Making completion list... [2 times]



[-- Attachment #3: Type: message/rfc822, Size: 2136 bytes --]

From: Glenn Morris <rgm@gnu.org>
To: 2034-done@debbugs.gnu.org
Subject: Re: Bug#2034: c-subword-mode incompatible with xml-mode
Date: Sat, 23 Jan 2010 17:34:58 -0500
Message-ID: <8cbpgkqwkt.fsf@fencepost.gnu.org>


In the current trunk, c-subword-mode has been generalized into `subword-mode'.
This seems to work fine with xml-mode.

bug#2034: [PATCH] 27.0.50; Support mode line constructs for `mode-name' in c-mode

[Phil Sainty]

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

`c-update-modeline' in cc-cmds.el has a long-standing bug (and FIXME
comment) whereby a `mode-name' which is not a string will trigger
errors, on account of the function using string manipulations to add
the minor mode flags to the original `mode-name'.

This is what bug #2034 was originally about. i.e.:

> c-update-modeline: Wrong type argument: stringp,
> (sgml-xml-mode "XML" "SGML")

This new patch instead uses mode-line constructs to concatenate the
flags, thus eliminating the string manipulation code and this bug.

In the process I've added a user option to allow the user to choose
whether or not these flags should be displayed at all, as this was not
previously configurable.

e.g.: `c-update-modeline' will now convert the mode-name "C++" into
'("C++" (c-modeline-display-flags c-modeline-flags)); and it only
updates the buffer-local value of `c-modeline-flags' which is then
conditionally displayed based on option `c-modeline-display-flags'.

This bug fix also resolves an incompatibility with the `delight'
package in GNU ELPA, which wraps any targeted `mode-name' values
in additional mode line constructs, and consequently triggered the
`c-update-modeline' bug with its assumption of a string, when used
with any of the major modes in question.

In my testing thus far this new approach seems to work nicely.


-Phil

[-- Attachment #2: 0001-Support-mode-line-constructs-for-mode-name-in-c-mode.patch --]
[-- Type: text/x-patch, Size: 3262 bytes --]

From 7454e8aca12b47ae36fdb46b869ebf1e2ea7afa4 Mon Sep 17 00:00:00 2001
From: Phil Sainty <psainty@orcon.net.nz>
Date: Mon, 2 Jul 2018 23:58:34 +1200
Subject: [PATCH] Support mode line constructs for `mode-name' in c-mode
 (bug#2034)

Also make the inclusion of minor mode flags in `mode-name' optional.

* cc-cmds.el (c-modeline-flags): New variable.
(c-modeline-display-flags): New user option.
(c-update-modeline): Use them.  Use mode line constructs, rather than
string concatenation, to optionally include minor mode flags in
`mode-name'.
---
 lisp/progmodes/cc-cmds.el | 60 ++++++++++++++++++++++++++++-------------------
 1 file changed, 36 insertions(+), 24 deletions(-)

diff --git a/lisp/progmodes/cc-cmds.el b/lisp/progmodes/cc-cmds.el
index 31cf0b1..131d7d1 100644
--- a/lisp/progmodes/cc-cmds.el
+++ b/lisp/progmodes/cc-cmds.el
@@ -258,31 +258,43 @@ c-syntactic-information-on-region
 (defvar c-block-comment-flag nil)
 (make-variable-buffer-local 'c-block-comment-flag)
 
+(defcustom c-modeline-display-flags t
+  "If non-nil, `mode-name' includes indicators for certain minor modes.
+
+These flags are set by `c-update-modeline'.
+
+See Info node `(ccmode) Minor Modes'."
+  :type 'boolean
+  :group 'c)
+
+(defvar-local c-modeline-flags ""
+  "Minor mode indicators to be appended to `mode-name'.
+
+Hidden when `c-modeline-display-flags' is nil.
+
+See Info node `(ccmode) Minor Modes'.")
+
 (defun c-update-modeline ()
-  (let ((fmt (format "/%s%s%s%s%s"
-		     (if c-block-comment-flag "*" "/")
-		     (if c-electric-flag "l" "")
-		     (if (and c-electric-flag c-auto-newline)
-			 "a" "")
-		     (if c-hungry-delete-key "h" "")
-		     (if (and
-			  ;; (cc-)subword might not be loaded.
-			  (boundp 'c-subword-mode)
-			  (symbol-value 'c-subword-mode))
-                         ;; FIXME: subword-mode already comes with its
-                         ;; own lighter!
-			 "w"
-		       "")))
-        ;; FIXME: Derived modes might want to use something else
-        ;; than a string for `mode-name'.
-	(bare-mode-name (if (string-match "\\(^[^/]*\\)/" mode-name)
-			    (match-string 1 mode-name)
-			  mode-name)))
-    (setq mode-name
-	  (if (> (length fmt) 1)
-	      (concat bare-mode-name fmt)
-	bare-mode-name))
-    (force-mode-line-update)))
+  "Update `c-modeline-flags' and append to `mode-name'."
+  (when (stringp mode-name)
+    (setq mode-name (list mode-name (list 'c-modeline-display-flags
+                                          'c-modeline-flags))))
+  (setq c-modeline-flags
+        (format "/%s%s%s%s%s"
+                (if c-block-comment-flag "*" "/")
+                (if c-electric-flag "l" "")
+                (if (and c-electric-flag c-auto-newline)
+                    "a" "")
+                (if c-hungry-delete-key "h" "")
+                (if (and
+                     ;; (cc-)subword might not be loaded.
+                     (boundp 'c-subword-mode)
+                     (symbol-value 'c-subword-mode))
+                    ;; FIXME: subword-mode already comes with its
+                    ;; own lighter!
+                    "w"
+                  "")))
+  (force-mode-line-update))
 
 (defun c-toggle-syntactic-indentation (&optional arg)
   "Toggle syntactic indentation.
-- 
2.8.3

bug#2034: [PATCH] 27.0.50; Support mode line constructs for `mode-name' in c-mode

[Eli Zaretskii]

> From: Phil Sainty <psainty@orcon.net.nz>
> Date: Tue, 3 Jul 2018 00:40:49 +1200
> 
> `c-update-modeline' in cc-cmds.el has a long-standing bug (and FIXME
> comment) whereby a `mode-name' which is not a string will trigger
> errors, on account of the function using string manipulations to add
> the minor mode flags to the original `mode-name'.
> 
> This is what bug #2034 was originally about. i.e.:
> 
> > c-update-modeline: Wrong type argument: stringp,
> > (sgml-xml-mode "XML" "SGML")
> 
> This new patch instead uses mode-line constructs to concatenate the
> flags, thus eliminating the string manipulation code and this bug.

I've just skimmed the patch, so apologies in advance if what I'm
saying makes no sense.  That said, did you try to compare the old and
the new code when the flag strings have text properties, like faces or
colors?  The mode-line formatting code is tricky when text properties
are involved.

> +(defcustom c-modeline-display-flags t
> +  "If non-nil, `mode-name' includes indicators for certain minor modes.
> +
> +These flags are set by `c-update-modeline'.
> +
> +See Info node `(ccmode) Minor Modes'."
> +  :type 'boolean
> +  :group 'c)

Please always provide a :version tag for new/modified defcustoms.

Finally, I think this needs a NEWS entry, if not a suitable change to
the manual.

Thanks.

bug#2034: [PATCH] 27.0.50; Support mode line constructs for `mode-name' in c-mode

[Phil Sainty]

On 2018-07-03 03:29, Eli Zaretskii wrote:
> I've just skimmed the patch, so apologies in advance if what I'm
> saying makes no sense.  That said, did you try to compare the old and
> the new code when the flag strings have text properties, like faces or
> colors?  The mode-line formatting code is tricky when text properties
> are involved.

I haven't, but I don't *think* that's going to be a concern here.

The code which formats the string of flags hasn't changed, and the
substrings (individual flags) which are passed to `format' are string
literals with no properties, so AFAICS the formatted string of flags
will not have any text properties when it is generated (which happens
afresh each time `c-update-modeline' is called).

If I'm missing something here, I think I'll need some guidance on
how to test for potential problems.


> Please always provide a :version tag for new/modified defcustoms.

Right, I knew I'd missed something there.


> Finally, I think this needs a NEWS entry, if not a suitable change to
> the manual.

Agreed.


My current approach may need more work.  At present it still assumes 
that
`mode-name' will *start out* as a string, and it tests `stringp' to 
decide
whether to wrap the additional constructs around it.

That is still an improvement on the pre-existing situation which throws
errors, but does mean that if a mode was itself defined to set a 
non-string
`mode-name' then the new code would not add the minor mode flags at all;
so it might be desirable to support that case as well.

To do this I think I'd need an additional buffer-local variable to 
indicate
(in place of that `stringp' test) whether or not `mode-name' had been
processed by `c-update-modeline', as otherwise it seems like it would be
awkward to decide whether or not to add the new constructs.

Although as I have added the buffer-local `c-modeline-flags', I guess I
can make that nil by default, and use that state as the indicator I 
need.

I'll write a revised patch to address these points.


-Phil

bug#2034: [PATCH] 27.0.50; Support mode line constructs for `mode-name' in c-mode

[Phil Sainty]

On 03/07/18 10:53, Phil Sainty wrote:
> I'll write a revised patch to address these points.

An updated version is on branch origin/fix/bug-2034 with the
following change log.


Support mode line constructs for 'mode-name' in c-mode (bug#2034)

Also make the inclusion of minor mode flags in 'mode-name' optional.

* lisp/progmodes/cc-cmds.el (c-modeline-flags): New variable.
(c-modeline-flags-major-modes-processed): New variable.
(c-modeline-display-flags): New user option.
(c-update-modeline): Use them.  Use mode line constructs, rather than
string concatenation, to optionally include minor mode flags in
'mode-name'.

* lisp/progmodes/cc-mode.el (c-submit-bug-report): Format 'mode-name'.

* lisp/progmodes/cc-styles.el (c-set-style): Format 'mode-name'.

* etc/NEWS: Mention new user option and behaviors.

* doc/misc/cc-mode.texi: Document 'c-modeline-display-flags'.

bug#2034: [PATCH] 27.0.50; Support mode line constructs for `mode-name' in c-mode

[Eli Zaretskii]

> Date: Tue, 03 Jul 2018 10:53:30 +1200
> From: Phil Sainty <psainty@orcon.net.nz>
> Cc: 2034@debbugs.gnu.org, bug-gnu-emacs
>  <bug-gnu-emacs-bounces+psainty=orcon.net.nz@gnu.org>
> 
> On 2018-07-03 03:29, Eli Zaretskii wrote:
> > I've just skimmed the patch, so apologies in advance if what I'm
> > saying makes no sense.  That said, did you try to compare the old and
> > the new code when the flag strings have text properties, like faces or
> > colors?  The mode-line formatting code is tricky when text properties
> > are involved.
> 
> I haven't, but I don't *think* that's going to be a concern here.
> 
> The code which formats the string of flags hasn't changed, and the
> substrings (individual flags) which are passed to `format' are string
> literals with no properties, so AFAICS the formatted string of flags
> will not have any text properties when it is generated (which happens
> afresh each time `c-update-modeline' is called).
> 
> If I'm missing something here, I think I'll need some guidance on
> how to test for potential problems.

Give each of the substrings you concatenate a different face, and see
what happens after concatenation when the result is shown on the mode
line.

bug#2034: [PATCH] 27.0.50; Support mode line constructs for `mode-name' in c-mode

[Alan Mackenzie]

Hello, Phil.

In article <mailman.3006.1530625089.1292.bug-gnu-emacs@gnu.org> you wrote:
> On 03/07/18 10:53, Phil Sainty wrote:
>> I'll write a revised patch to address these points.

> An updated version is on branch origin/fix/bug-2034 with the
> following change log.


> Support mode line constructs for 'mode-name' in c-mode (bug#2034)

> Also make the inclusion of minor mode flags in 'mode-name' optional.

> * lisp/progmodes/cc-cmds.el (c-modeline-flags): New variable.
> (c-modeline-flags-major-modes-processed): New variable.
> (c-modeline-display-flags): New user option.
> (c-update-modeline): Use them.  Use mode line constructs, rather than
> string concatenation, to optionally include minor mode flags in
> 'mode-name'.

> * lisp/progmodes/cc-mode.el (c-submit-bug-report): Format 'mode-name'.

> * lisp/progmodes/cc-styles.el (c-set-style): Format 'mode-name'.

> * etc/NEWS: Mention new user option and behaviors.

> * doc/misc/cc-mode.texi: Document 'c-modeline-display-flags'.

For fitting in better with CC Mode, please:
(i) Put c-modeline-display-flags (and any other configuration variables)
in cc-vars.el rather than cc-cmds.el.
(ii) In cc-mode.info, make a second @vindex entry for your new variable,
like all the other variables have two @vindexes.

Also, in chapter "Minor Modes", I'd be happier if the paragraph
beginning "@ccmode{} displays the current state" was amended to
something like "@ccmode{}, by default, displays the current state".

But I must confess, I'm not filled with enthusiasm by this change.  What
is the problem it is fixing?  The original problem (use of a CC Mode
command by a non CC Mode mode) went away when cc-subword.el became just
subword-mode.

This change introduces complexity, even if, perhaps, not very much.  Do
we really need a buffer local variable for the display of these flags?
(That's a real question, not a rhetorical one.)  It seems to be inviting
misuse, given that it prevails for ever, but is really only valid for
the short time between it being calculated and the mode line being
displayed.

    +(defvar-local c-modeline-flags-major-modes-processed nil
    +  "Major modes for which `c-update-modeline' has processed `mode-name'.

.... seems confused.  It's a poor quality doc string, since it doesn't
say what the format is: is it a list of major modes, or a list of major
mode names, or what?  Why is this collection of major modes relevant to
a single buffer?  What's the purpose of the variable?

I'm rather sceptical about

    (setq mode-name (list mode-name .....

, which is just screaming out for an unbounded appending of other
things, many times over, if anything goes wrong with the enclosing
`unless' form.  What happens to it when the major mode is changed?
Would it not be possible, somehow, either to leave mode-name unmolested,
or calculate it unrecursively when needed?

As a final point, how is the backward compatibility of this change?  How
many former Emacsen will it work in?

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#2034: [PATCH] 27.0.50; Support mode line constructs for `mode-name' in c-mode

[Phil Sainty]

Hi Alan,

On 2018-07-05 08:11, Alan Mackenzie wrote:
> But I must confess, I'm not filled with enthusiasm by this change.  
> What
> is the problem it is fixing?  The original problem (use of a CC Mode
> command by a non CC Mode mode) went away when cc-subword.el became just
> subword-mode.

I believe the original problem was the same as the problem I'm aiming to 
fix,
which is that `c-update-modeline' imposes a non-standard restriction 
upon
`mode-name' that it be a simple string (as opposed to containing 
arbitrary
mode line constructs, as it should be able to do).

It seems that the original symptom of the problem in this bug report 
went
away as a side-effect of the change to subword-mode, but the bug itself
remained.

`c-update-modeline' even contains a "FIXME" comment about it:

         ;; FIXME: Derived modes might want to use something else
         ;; than a string for `mode-name'.

> This change introduces complexity, even if, perhaps, not very much.  Do
> we really need a buffer local variable for the display of these flags?
> (That's a real question, not a rhetorical one.)  It seems to be 
> inviting
> misuse, given that it prevails for ever, but is really only valid for
> the short time between it being calculated and the mode line being
> displayed.

In the current version of the patch, the buffer-local variable is used 
every
time the mode line is rendered, as the variable *symbol* is incorporated 
into
`mode-name'.  However Stefan made the suggestion that the flags 
themselves
could become a list of mode line constructs, which would then mean it 
could
be a global value rather than a buffer-local one, as each buffer would 
render
that single construct into the appropriate flags for its own buffer.


>     +(defvar-local c-modeline-flags-major-modes-processed nil
>     +  "Major modes for which `c-update-modeline' has processed 
> `mode-name'.
> 
> .... seems confused.

That was a mistake on my part, and I was intending to change it from a 
list
to a single value record of the most recent `major-mode' to be 
processed.

The reason for having a record in the first place is that a major mode 
which
is *derived* from, say, `c-mode' can trigger `c-update-modeline' 
repeatedly
with different values for `major-mode', and if we see a *new* 
`major-mode'
value then `mode-name' will also have been reset (to a string, in the 
existing
cases), and needs to be processed again, as each major mode body resets
`mode-name'.

i.e. We need to process `mode-name' exactly once for whatever the final
major mode is.

Something like: (unless (eq major-mode c-modeline-major-mode)...), with
buffer-local `c-modeline-major-mode' then set to the value of 
`major-mode'.


> I'm rather sceptical about
> 
>     (setq mode-name (list mode-name .....
> 
> , which is just screaming out for an unbounded appending of other
> things, many times over, if anything goes wrong with the enclosing
> `unless' form. What happens to it when the major mode is changed?

Hopefully you find the aforementioned proposed change less concerning.


> Would it not be possible, somehow, either to leave mode-name 
> unmolested,
> or calculate it unrecursively when needed?

Not that I can think of.  AFAIK using mode line constructs in 
`mode-name'
is exactly how these kinds of things are supposed to be done.


> As a final point, how is the backward compatibility of this change?  
> How
> many former Emacsen will it work in?

I've not checked.  I think these mode line constructs have been stable 
for
a long time?  If a new third-party derived mode was written to have a 
fancy
`mode-name' then obviously that would only work with an Emacs version 
which
contained these changes.  I'm not sure what you're getting at here, 
though...
Are you saying that people will use newer cc-mode code with older 
emacsen?
I can do some testing if I know what the use-case is.


-Phil

bug#2034: [PATCH] 27.0.50; Support mode line constructs for `mode-name' in c-mode

[Phil Sainty]

I've updated branch origin/fix/bug-2034 based on recent discussions.

> * lisp/progmodes/cc-vars.el (c-modeline-display-flags): New user
> option.
> * lisp/progmodes/cc-cmds.el (c-modeline-flags): New variable.
> (c--modeline-major-mode): New internal buffer-local variable.
> (c-update-modeline): Use mode line constructs, rather than string
> concatenation, to optionally include minor mode flags in 'mode-name'.

As per Stefan's suggestion, `c-modeline-flags' is now a global
variable containing a mode line construct which will render
appropriately in any buffer.  Its value is:

(c-modeline-display-flags
 ("/"
  (c-block-comment-flag "*" "/")
  (c-electric-flag
   ("l"
    (c-auto-newline "a")))
  (c-hungry-delete-key "h")
  (c-subword-mode "w")))


As a consequence of the change above, we no longer need to call
`c-update-modeline' when toggling the state of one of those minor
modes in a buffer, hence:

> * lisp/progmodes/cc-cmds.el (c-toggle-auto-newline)
> (c-toggle-hungry-state, c-toggle-auto-hungry-state)
> (c-toggle-electric-state, c-toggle-comment-style):
> * lisp/progmodes/cc-mode.el (c-electric-indent-mode-hook)
> (c-electric-indent-local-mode-hook): Remove redundant calls to
> 'c-update-modeline'.  It is no longer necessary to call this function
> every time one of the minor mode states changes.  The remaining calls
> are in 'c-basic-common-init' (which is called via 'c-common-init' by
> all the major modes defined in cc-mode.el), and in the :after-hook of
> those modes (which ensures that 'mode-name' is still processed for a
> derived mode that doesn't call 'c-common-init' itself).


On 05/07/18 08:11, Alan Mackenzie wrote:
> For fitting in better with CC Mode, please:
> (i) Put c-modeline-display-flags (and any other configuration variables)
> in cc-vars.el rather than cc-cmds.el.
> (ii) In cc-mode.info, make a second @vindex entry for your new variable,
> like all the other variables have two @vindexes.
>
> Also, in chapter "Minor Modes", I'd be happier if the paragraph
> beginning "@ccmode{} displays the current state" was amended to
> something like "@ccmode{}, by default, displays the current state".

All done.

Do you still have concerns about these changes, Alan?  I've revised
some of the code that you were unhappy with, so please have another
look at the branch.

> As a final point, how is the backward compatibility of this change?
> How many former Emacsen will it work in?

Let me know if there's anything in particular I should be testing.


On 04/07/18 14:41, Eli Zaretskii wrote:
> Give each of the substrings you concatenate a different face, and
> see what happens after concatenation when the result is shown on the
> mode line.

This works; although, as per the `mode-line-format' documentation, if
a variable is used to store the construct then that variable needs to
have a non-nil `risky-local-variable' property, otherwise the text
properties are ignored.

So both of the following work, provided we have:
(put 'c-modeline-flags 'risky-local-variable t)

(defvar c-modeline-flags
  `(c-modeline-display-flags
    (,(propertize "/")
     (c-block-comment-flag
      ,(propertize "*" 'help-echo "Comment style" 'face 'warning)
      ,(propertize "/" 'help-echo "Comment style" 'face 'hi-yellow))
     (c-electric-flag
      (,(propertize "l" 'help-echo "Electric mode" 'face 'hi-green)
       (c-auto-newline
        ,(propertize "a" 'help-echo "Auto-newline mode" 'face 'hi-pink))))
     (c-hungry-delete-key
      ,(propertize "h" 'help-echo "Hungry-delete mode" 'face 'hi-red-b))
     (c-subword-mode
      ,(propertize "w" 'help-echo "Subword mode" 'face 'error)))))

(defvar c-modeline-flags
  '(c-modeline-display-flags
    ((:propertize "/")
     (c-block-comment-flag
      (:propertize "*" help-echo "Comment style" face warning)
      (:propertize "/" help-echo "Comment style" face hi-yellow))
     (c-electric-flag
      ((:propertize "l" help-echo "Electric mode" face hi-green)
       (c-auto-newline
	(:propertize "a" help-echo "Auto-newline mode" face hi-pink))))
     (c-hungry-delete-key
      (:propertize "h" help-echo "Hungry-delete mode" face hi-red-b))
     (c-subword-mode
      (:propertize "w" help-echo "Subword mode" face error)))))

As there's nothing dynamic going on in these particular text
properties, I'd be inclined to use the former approach over the
latter, if properties were to be added.

Obviously the faces used are only for testing.  I'm not sure that
these actually need a face, but the `help-echo' text could be pretty
handy for clarifying what these potentially-mysterious characters in
the mode line actually mean, for users who don't know.

I was also pondering making a mouse click visit "(ccmode) Minor Modes",
as that provides the clearest documentation.  I've referenced this
node a couple of docstrings, but having direct access to it from the
flags in question in the mode line could be even more helpful.


On 04/07/18 11:57, Phil Sainty wrote:
> Grepping for uses of c-update-modeline I found this related comment in
> lisp/progmodes/antlr-mode.el:
>
> (define-derived-mode antlr-mode prog-mode
>   ;; FIXME: Since it uses cc-mode, it bumps into c-update-modeline's
>   ;; limitation to mode-name being a string.
>   ;; '("Antlr." (:eval (cadr (assq antlr-language antlr-language-alist))))
>   "Antlr"

I've added a second commit to the branch which restores this intended
construct for `antlr-mode', which makes that more dynamic than the
`mode-name' that it was using.


-Phil

bug#2034: [PATCH] 27.0.50; Support mode line constructs for `mode-name' in c-mode

[Alan Mackenzie]

Hello, Phil.

Just as an aside, something in the email chain between you and me has
irritatingly formatted your (and my) text like this:

XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
XXX
XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
XXX

, with every second line containing just one (or two) words.  Would you
please consider investigating this a little.

On Thu, Jul 05, 2018 at 09:13:38 +1200, Phil Sainty wrote:
> Hi Alan,

> On 2018-07-05 08:11, Alan Mackenzie wrote:
> > But I must confess, I'm not filled with enthusiasm by this change.
> > What is the problem it is fixing?  The original problem (use of a CC
> > Mode command by a non CC Mode mode) went away when cc-subword.el
> > became just subword-mode.

> I believe the original problem was the same as the problem I'm aiming
> to fix, which is that `c-update-modeline' imposes a non-standard
> restriction upon `mode-name' that it be a simple string (as opposed to
> containing arbitrary mode line constructs, as it should be able to do).

That's unnecessarily harsh.  The problem is that the definition of
mode-name was changed incompatibly some while back.  This seems a foolish
change - the name of a mode is essentially a string, and changing this
has led to problems.  The current code in CC Mode conforms to the
original definition of mode-name.

> It seems that the original symptom of the problem in this bug report
> went away as a side-effect of the change to subword-mode, but the bug
> itself remained.

> `c-update-modeline' even contains a "FIXME" comment about it:

>          ;; FIXME: Derived modes might want to use something else
>          ;; than a string for `mode-name'.

Why should anybody want to do that?  (Not a rhetorical question).

> > This change introduces complexity, even if, perhaps, not very much.
> > Do we really need a buffer local variable for the display of these
> > flags?  (That's a real question, not a rhetorical one.)  It seems to
> > be inviting misuse, given that it prevails for ever, but is really
> > only valid for the short time between it being calculated and the
> > mode line being displayed.

> In the current version of the patch, the buffer-local variable is used
> every time the mode line is rendered, as the variable *symbol* is
> incorporated into `mode-name'.  However Stefan made the suggestion that
> the flags themselves could become a list of mode line constructs, which
> would then mean it could be a global value rather than a buffer-local
> one, as each buffer would render that single construct into the
> appropriate flags for its own buffer.


> >     +(defvar-local c-modeline-flags-major-modes-processed nil
> >     +  "Major modes for which `c-update-modeline' has processed `mode-name'.

> > .... seems confused.

> That was a mistake on my part, and I was intending to change it from a
> list to a single value record of the most recent `major-mode' to be
> processed.

> The reason for having a record in the first place is that a major mode
> which is *derived* from, say, `c-mode' can trigger `c-update-modeline'
> repeatedly with different values for `major-mode', and if we see a
> *new* `major-mode' value then `mode-name' will also have been reset (to
> a string, in the existing cases), and needs to be processed again, as
> each major mode body resets `mode-name'.

This emphasises my earlier comment about the foolishness of the change to
the definition of mode-name.

> i.e. We need to process `mode-name' exactly once for whatever the final
> major mode is.

mode-name belongs to the major mode, and shouldn't be tampered with by
other SW.  It is part of the mode, as defined on Elisp page "Major Mode
Conventions", which states that the major mode set this variable to THE
pretty name of the mode.  It doesn't state that mode-name is merely a
template for manipulation by random code.  This, I believe, is the root
cause of the bug you are wanting to fix.

[ .... ]

> > As a final point, how is the backward compatibility of this change?
> > How many former Emacsen will it work in?

> I've not checked.  I think these mode line constructs have been stable
> for a long time?  If a new third-party derived mode was written to have
> a fancy `mode-name' then obviously that would only work with an Emacs
> version which contained these changes.  I'm not sure what you're
> getting at here, though...  Are you saying that people will use newer
> cc-mode code with older emacsen?  I can do some testing if I know what
> the use-case is.

Changes like this made in Emacs without backward compatibility code
exacerbate the growing difference between the Emacs version and the
upstream version.  The incompatible change in mode-name happened in Emacs
23.1.  Upstream CC Mode is still compatible with XEmacs, and (probably)
with Emacs 22, and it is one of the project's aims to preserve this
compatibility as much as possible.  Your proposed change destroys it.

That means either (i) Half of us have to write compatibility code for
both the Emacs version of and upstream CC Mode; or (ii) The compatibility
code is confined to upstream CC Mode (which is a royal pain when merging
upstream changes into Emacs); or (iii) the new code doesn't get merged
into the upstream (likewise a pain).

> -Phil

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#2034: [PATCH] 27.0.50; Support mode line constructs for `mode-name' in c-mode

[Phil Sainty]

Hi Alan,

On 09/07/18 08:08, Alan Mackenzie wrote:
> Just as an aside, something in the email chain between you and
> me has irritatingly formatted your (and my) text ... with every
> second line containing just one (or two) words.

I've not observed that myself, but I'll refill the text to 65
cols and see if that helps.

> On Thu, Jul 05, 2018 at 09:13:38 +1200, Phil Sainty wrote:
>> I believe the original problem was the same as the problem I'm
>> aiming to fix, which is that `c-update-modeline' imposes a
>> non-standard restriction upon `mode-name' that it be a simple
>> string (as opposed to containing arbitrary mode line
>> constructs, as it should be able to do).

> That's unnecessarily harsh.

Ignorance on my part, I'm afraid -- I'd neither realised that the
ability for mode-name to contain non-string values was a change
that post-dated the CC mode code in question, nor understood that
maintaining compatibility with such old emacsen was important for
these libraries.  No offence intended, regardless.


> The problem is that the definition of mode-name was changed
> incompatibly some while back.  This seems a foolish change -
> the name of a mode is essentially a string, and changing this
> has led to problems.  The current code in CC Mode conforms to
> the original definition of mode-name.

>>          ;; FIXME: Derived modes might want to use something
>>          ;; else than a string for `mode-name'.

> Why should anybody want to do that?  (Not a rhetorical
> question).

I believe it is a genuinely useful thing to be able to do,
particularly for end-user customisation purposes.  I can give a
few examples.

The first thing is that it's useful for *precisely* the same
reason that CC mode contains the code we're discussing -- because
including some kind of dynamic state information in the mode name
in the mode line is a really handy thing to do (and, backwards-
compatibility complications notwithstanding, I'd argue that the
implementation of the CC mode flags using that approach is quite
elegant).  End-users could even choose to implement their own
custom set of CC mode flags with relative ease, using this
technique.

More generally, mode line constructs make it easy for end-users
to implement similar custom displays for any mode at all.  For
example I use the following custom mode-name for
`emacs-lisp-mode':

'("Elisp" (lexical-binding ":Lex" ":Dyn"))

This means that all my elisp buffers show me the state of
`lexical-binding' with either "Elisp:Lex" or "Elisp:Dyn" as the
displayed mode name (which is very useful).

That isn't the *best* example, as one doesn't expect the value of
`lexical-binding' to vary dynamically within any given buffer,
but the point is that it is quite trivial to add dynamic state
information using this technique.

A more dynamic example from my own config is for `re-builder',
which allows the user to dynamically switch between four
different regexp syntaxes (read, string, rx, and sregexp), but
only has two different mode line states: "RE Builder" (for read
and string), and "RE Builder Lisp" (for rx and sregex), which is
on account of it using a separate major mode for each of those
two groups.  In my config I set the following mode-name for each
of those modes:

'("Regexp[" (:eval (symbol-name reb-re-syntax)) "]")

Which then provides me with the details which were lacking:

"Regexp[read]"
"Regexp[string]"
"Regexp[rx]"
"Regexp[sregex]"

Unlike my previous example, this one does vary dynamically, and
is particularly useful for checking whether 'read' or 'string'
syntax is active.

And of course we've already seen an example in `antlr-mode',
where the author had originally wanted to make mode-name react
dynamically to the value of the `antlr-language' variable.

You'll also observe that in my examples I've changed the main
text of the mode name to something shorter ("Elisp" instead of
"Emacs-Lisp", and "Regexp" instead of "RE Builder").  Reducing
the characters used by major and minor modes in the mode line is
a rather common desire (with split windows one quickly runs out
of room to see all the information available), and some people
take this to the extreme of using single characters for many of
their commonly-used modes.  An example might be using the HOT
BEVERAGE character "☕" as the mode-name for `java-mode'.

I mention this because I became aware of this bug in the first
place for related reasons, as my delight.el library was written
to make such customisations easy to configure (see
https://www.emacswiki.org/emacs/DelightedModes ), and I learned
recently that it was incompatible with CC Mode modes:
https://www.reddit.com/r/emacs/comments/8v8de4/change_modename_for_major_mode/

The reason for the incompatibility is that delight.el wants to
display the custom version of the mode in the mode line, but
still display the *original* name in other contexts (such as in
the `describe-mode' output when the user types C-h m), and it
does this by generating a mode line construct which displays one
or the other based on a conditional variable.  Hence my config
for elisp is actually more like:

(delight 'emacs-lisp-mode
         '("Elisp" (lexical-binding ":Lex" ":Dyn"))
         :major)

Resulting in this mode-name:

'(inhibit-mode-name-delight
  "Emacs-Lisp"
  ("Elisp" (lexical-binding ":Lex" ":Dyn")))

And one could use:

(delight 'java-mode
         (char-to-string (char-from-name "HOT BEVERAGE"))
         :major)

Resulting in, in my case:

'(inhibit-mode-name-delight "Java//l" "☕")

which of course then triggers the error in `c-update-modeline'
which I've been seeking to avoid.


Hopefully these various examples answer the question of "Why
should anybody want to do that?"


I believe I can update my patch to retain the previous behaviour
for old emacsen whilst implementing the improvement for newer
ones.  Offhand I think it would be a matter of making
`c-update-modeline' run either the new code or the old code
depending upon the Emacs version, and then I would restore the
calls to `c-update-modeline' that I removed from the various
minor mode toggle functions, etc.  Comments can indicate which
code is for compatibility with Emacs 22 and earlier, to make it
easy to remove if support for those versions is ever dropped in
future.  I am not sure which variant is needed for xemacs (or any
particular versions thereof), as I do not use it, so I would need
help with an appropriate test for that.

I'm happy to review and revise the patch along these lines, if
I've managed to convince you that it's a useful thing to support?


-Phil