GNU bug report logs - #76108
Call `modify_text' only on the text being replaced in `subst-char-in-region'

Previous Next

Full log

Message #8 received at 76108 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Thuna <thuna.cing <at> gmail.com>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 76108 <at> debbugs.gnu.org
Subject: Re: bug#76108: Call `modify_text' only on the text being replaced in
 `subst-char-in-region'
Date: Fri, 07 Feb 2025 10:36:21 +0200

> From: Thuna <thuna.cing <at> gmail.com>
> Date: Fri, 07 Feb 2025 00:41:12 +0100
> 
> Currently `subst-char-in-region' calls `modify_text' on the entire text,
> starting from the first match to the end of the text, which fires off
> `before-change-functions' for (almost) every character in the region
> it's called in.

That's right.  But if the character in question is substituted more
than once, we call modify_text only once, for the entire region.

> The specific place where this caused a problem for me is while filling
> paragraphs with latex fragments in org-mode.  Latex fragments are
> overlays with the `modification-hooks' property set to delete the
> overlay, so when a paragraph is filled, that calls
>   (subst-char-in-region ?\n ?\s ...)
> on the entire paragraph which then calls `modify_text' on the entire
> paragraph (sans the first line), which causes all latex fragments to
> disappear.

Why do you think this is a bug in Emacs core and not in how org-mode
handles latex fragments?

> I'm assuming that there isn't any particular reason why `modify_text'
> should be called on the whole paragraph, but if there is, then that's ok
> too, I suppose.  I don't see any reason why this should be breaking
> stuff, but I haven't actually checked.

AFAIU, your change will cause modify_text to be called only once, like
we do now, but only for the first replacement of the character in
question.  So if the loop in subst-char-in-region replaces the
character more than once, the fact that there were changes in other
places in the region will not be reported.

Or maybe I'm missing something.  But to establish whether this change
introduces a regression, we need some test, and the test should
include both single and multiple substitutions, and also both ASCII
and non-ASCII characters.  Then we will have a good base for assessing
this patch, and also our test suite will become better -- a nice
bonus.

So could you please write such a test, and then see whether your
change still passes it?

TIA

P.S. Adding Stefan to the discussion, because I think we once
discussed this function in this respect, and decided that the way it
calls modify_text is correct -- but I cannot find that discussion in
my archives, so maybe I was dreaming.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.