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

From: Phil Sainty <[email-masked]>
Date: Tue, 03 Jul 2018 10:53:30 +1200

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