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

Previous Next

Full log

View this message in rfc822 format

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

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

I figured this was a bug in Emacs so I came here first, but I'll go
ahead and patch org-mode if this turns out to be intended behavior.

>> 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.

It's possible that I'm reading the code wrong, but it seems to me that
it's being called for every change.  If not, I could just make it so.
I'm not familiar with `modify_text' so I could be misunderstanding, but
shouldn't it only be called on the text that is actively being modified
(in this case replaced)?

> 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?

Sure, I'll do that.  For a start, just to demonstrate the problem,
here's a test which fails on a July build (I don't think anything
changed since?) and passes with my patch.  Specifically, it's the
`should-not-error' below that fails.  (Also, is there a canonical way to
write `should-not-error' that I don't know about?)

(ert-deftest test-subst-char-in-region-before-change-functions ()
  (let ((hook (lambda (_beg _end) (error "Poor man's read-only"))))
    (with-temp-buffer
      (insert (propertize "foo" 'modification-hooks (list hook)) "," "bar")
      (should-error (subst-char-in-region (point-min) (point-max) ?o ?e)))

    (with-temp-buffer
      (insert "foo" (propertize "," 'modification-hooks (list hook)) "bar")
      ;; My best approximation of `should-not-error'.
      (should (ignore-errors
                (subst-char-in-region (point-min) (point-max) ?o ?e)
                t))
      (should (equal "fee,bar" (buffer-string))))))

Previous Next

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