bug#62675: 29.0.60; emoji-zoom-increase interacts badly with faces

bug#62675: 29.0.60; emoji-zoom-increase interacts badly with faces

[Robert Pluim]

--begin--
emacs -Q
C-p C-p
C-x 8 e + + + +
C-u C-x =
--end--

Visually, this does the right thing: the ';' has its height increased,
but the text properties look like this:

  face                 ((:height 1.1) font-lock-comment-delimiter-face :height 1.3310000000000004)
  fontified            t
  rear-nonsticky       t

Thatʼs not a valid 'face property, unless Iʼm misreading (info
"(elisp) Special Properties"), plus the ':height 1.331' bit is wrong,
thatʼs (expt 1.1 3), not (expt 1.1 4). Yet it works, the height has
been increased.

Is this a bug in our interpretation of 'face properties?

I can fix the code in `emoji-zoom-increase' to produce something like

((:height 1.4641000000000004) font-lock-comment-delimiter-face)

instead, but first Iʼd like to understand whatʼs going on.

In GNU Emacs 29.0.60 (build 15, x86_64-pc-linux-gnu, GTK+ Version
 3.24.24, cairo version 1.16.0) of 2023-03-14 built on rltb
Repository revision: 4b6f2a7028b91128934a19f83572f24106782225
Repository branch: emacs-29
Windowing system distributor 'The X.Org Foundation', version 11.0.12009000
System Description: Debian GNU/Linux 11 (bullseye)

Robert
-- 

bug#62675: 29.0.60; emoji-zoom-increase interacts badly with faces

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Date: Wed, 05 Apr 2023 09:38:32 +0200
> 
> 
> --begin--
> emacs -Q
> C-p C-p
> C-x 8 e + + + +
> C-u C-x =
> --end--
> 
> Visually, this does the right thing: the ';' has its height increased,
> but the text properties look like this:
> 
>   face                 ((:height 1.1) font-lock-comment-delimiter-face :height 1.3310000000000004)
>   fontified            t
>   rear-nonsticky       t
> 
> Thatʼs not a valid 'face property, unless Iʼm misreading (info
> "(elisp) Special Properties"), plus the ':height 1.331' bit is wrong,
> thatʼs (expt 1.1 3), not (expt 1.1 4). Yet it works, the height has
> been increased.

The first "+" doesn't increase the height, though.

> Is this a bug in our interpretation of 'face properties?

Face properties can be specified in many strange ways, and the fact
that some invalid specifications seem to "work" doesn't mean we have a
bug, it just means some invalid faces fall through the cracks.

> I can fix the code in `emoji-zoom-increase' to produce something like
> 
> ((:height 1.4641000000000004) font-lock-comment-delimiter-face)
> 
> instead

Please do, and thanks.  We don't need to understand why some bug seems
to work, it is enough to fix the bug.

bug#62675: 29.0.60; emoji-zoom-increase interacts badly with faces

[Robert Pluim]

>>>>> On Wed, 05 Apr 2023 11:45:35 +0300, Eli Zaretskii <eliz@gnu.org> said:

    >> Thatʼs not a valid 'face property, unless Iʼm misreading (info
    >> "(elisp) Special Properties"), plus the ':height 1.331' bit is wrong,
    >> thatʼs (expt 1.1 3), not (expt 1.1 4). Yet it works, the height has
    >> been increased.

    Eli> The first "+" doesn't increase the height, though.

Hmm, youʼre right. Although if I then move to a different place in the
buffer, and do 'C-x 8 e +' it does increase the height first time. Is
this an interaction with font-lock?

Oh, and it doesnʼt work at eob. And increase followed by decrease
doesnʼt get you back to :height 1.0. And thereʼs no reset. I think
Iʼll only fix the first one on emacs-29 :-)

    >> Is this a bug in our interpretation of 'face properties?

    Eli> Face properties can be specified in many strange ways, and the fact
    Eli> that some invalid specifications seem to "work" doesn't mean we have a
    Eli> bug, it just means some invalid faces fall through the cracks.

OK. Iʼll make the code match the documented behaviour, and not worry
about it too much 😀

    >> I can fix the code in `emoji-zoom-increase' to produce something like
    >> 
    >> ((:height 1.4641000000000004) font-lock-comment-delimiter-face)
    >> 
    >> instead

    Eli> Please do, and thanks.  We don't need to understand why some bug seems
    Eli> to work, it is enough to fix the bug.

This seems to work:

diff --git a/lisp/international/emoji.el b/lisp/international/emoji.el
index bcd4aac4f29..a9ed6e79cc9 100644
--- a/lisp/international/emoji.el
+++ b/lisp/international/emoji.el
@@ -708,22 +708,24 @@ emoji-zoom-increase
 FACTOR is the multiplication factor for the size."
   (interactive)
   (set-transient-map emoji-zoom-map t nil "Zoom with %k")
-  (let* ((factor (or factor 1.1))
-         (old (get-text-property (point) 'face))
-         (height (or (and (consp old)
-                          (plist-get old :height))
-                     1.0))
-         (inhibit-read-only t))
-    (with-silent-modifications
-      (if (consp old)
-          (add-text-properties
-           (point) (1+ (point))
-           (list 'face (plist-put (copy-sequence old) :height (* height factor))
-                 'rear-nonsticky t))
-        (add-face-text-property (point) (1+ (point))
-                                (list :height (* height factor)))
-        (put-text-property (point) (1+ (point))
-                           'rear-nonsticky t)))))
+  (unless (eobp)
+    (let* ((factor (or factor 1.1))
+           (old (get-text-property (point) 'face))
+           (height (or (and (consp old)
+                            (plist-get (car old) :height))
+                       1.0))
+           (inhibit-read-only t))
+      (with-silent-modifications
+        (if (consp old)
+            (add-text-properties
+             (point) (1+ (point))
+             (list 'face (cons (plist-put (car old) :height (* height factor))
+                               (cdr old))
+                   'rear-nonsticky t))
+          (add-face-text-property (point) (1+ (point))
+                                  (list :height (* height factor)))
+          (put-text-property (point) (1+ (point))
+                             'rear-nonsticky t))))))
 
 ;;;###autoload
 (defun emoji-zoom-decrease ()

Robert
-- 

bug#62675: 29.0.60; emoji-zoom-increase interacts badly with faces

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: 62675@debbugs.gnu.org
> Date: Wed, 05 Apr 2023 11:12:31 +0200
> 
>     Eli> Please do, and thanks.  We don't need to understand why some bug seems
>     Eli> to work, it is enough to fix the bug.
> 
> This seems to work:

Looks reasonable, thanks.  Please try both with font-lock-mode ON and
OFF, and in the latter case both with some non-default face at point
and without.

bug#62675: 29.0.60; emoji-zoom-increase interacts badly with faces

[Robert Pluim]

>>>>> On Wed, 05 Apr 2023 12:54:27 +0300, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Robert Pluim <rpluim@gmail.com>
    >> Cc: 62675@debbugs.gnu.org
    >> Date: Wed, 05 Apr 2023 11:12:31 +0200
    >> 
    Eli> Please do, and thanks.  We don't need to understand why some bug seems
    Eli> to work, it is enough to fix the bug.
    >> 
    >> This seems to work:

    Eli> Looks reasonable, thanks.  Please try both with font-lock-mode ON and
    Eli> OFF, and in the latter case both with some non-default face at point
    Eli> and without.

Ah yes, font-lock off makes it all more complex. Tested in those 3
cases. Is this too much for emacs-29?

diff --git a/lisp/international/emoji.el b/lisp/international/emoji.el
index bcd4aac4f29..ff55170875e 100644
--- a/lisp/international/emoji.el
+++ b/lisp/international/emoji.el
@@ -708,22 +708,32 @@ emoji-zoom-increase
 FACTOR is the multiplication factor for the size."
   (interactive)
   (set-transient-map emoji-zoom-map t nil "Zoom with %k")
-  (let* ((factor (or factor 1.1))
-         (old (get-text-property (point) 'face))
-         (height (or (and (consp old)
-                          (plist-get old :height))
-                     1.0))
-         (inhibit-read-only t))
-    (with-silent-modifications
-      (if (consp old)
-          (add-text-properties
-           (point) (1+ (point))
-           (list 'face (plist-put (copy-sequence old) :height (* height factor))
-                 'rear-nonsticky t))
-        (add-face-text-property (point) (1+ (point))
-                                (list :height (* height factor)))
-        (put-text-property (point) (1+ (point))
-                           'rear-nonsticky t)))))
+  (unless (eobp)
+    (let* ((factor (or factor 1.1))
+           (old (get-text-property (point) 'face))
+           ;; The text property is either a named face, or a plist
+           ;; with :height, or a list starting with such a plist,
+           ;; followed by one or more faces.
+           (newheight (* (or (and (consp old)
+                                  (or (plist-get (car old) :height)
+                                      (plist-get old :height)))
+                             1.0)
+                         factor))
+           (inhibit-read-only t))
+      (with-silent-modifications
+        (if (consp old)
+            (add-text-properties
+             (point) (1+ (point))
+             (list 'face
+                   (if (eq (car old) :height)
+                       (plist-put old :height newheight)
+                     (cons (plist-put (car old) :height newheight)
+                           (cdr old)))
+                   'rear-nonsticky t))
+          (add-face-text-property (point) (1+ (point))
+                                  (list :height newheight))
+          (put-text-property (point) (1+ (point))
+                             'rear-nonsticky t))))))
 
 ;;;###autoload
 (defun emoji-zoom-decrease ()

Robert
-- 

bug#62675: 29.0.60; emoji-zoom-increase interacts badly with faces

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: 62675@debbugs.gnu.org
> Date: Wed, 05 Apr 2023 13:55:22 +0200
> 
>     Eli> Looks reasonable, thanks.  Please try both with font-lock-mode ON and
>     Eli> OFF, and in the latter case both with some non-default face at point
>     Eli> and without.
> 
> Ah yes, font-lock off makes it all more complex. Tested in those 3
> cases. Is this too much for emacs-29?

Not too much, because this code is new in Emacs 29.

Thanks.

bug#62675: 29.0.60; emoji-zoom-increase interacts badly with faces

[Robert Pluim]

>>>>> On Wed, 05 Apr 2023 15:08:49 +0300, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Robert Pluim <rpluim@gmail.com>
    >> Cc: 62675@debbugs.gnu.org
    >> Date: Wed, 05 Apr 2023 13:55:22 +0200
    >> 
    Eli> Looks reasonable, thanks.  Please try both with font-lock-mode ON and
    Eli> OFF, and in the latter case both with some non-default face at point
    Eli> and without.
    >> 
    >> Ah yes, font-lock off makes it all more complex. Tested in those 3
    >> cases. Is this too much for emacs-29?

    Eli> Not too much, because this code is new in Emacs 29.

OK. I guess that means I can add `emoji-zoom-reset' and fix the
increase/decrease assymetry there as well.

Robert
-- 

bug#62675: 29.0.60; emoji-zoom-increase interacts badly with faces

[Robert Pluim]

tags 62675 fixed
close 62675 29.1
quit

>>>>> On Wed, 05 Apr 2023 15:53:00 +0200, Robert Pluim <rpluim@gmail.com> said:

>>>>> On Wed, 05 Apr 2023 15:08:49 +0300, Eli Zaretskii <eliz@gnu.org> said:
    >>> From: Robert Pluim <rpluim@gmail.com>
    >>> Cc: 62675@debbugs.gnu.org
    >>> Date: Wed, 05 Apr 2023 13:55:22 +0200
    >>> 
    Eli> Looks reasonable, thanks.  Please try both with font-lock-mode ON and
    Eli> OFF, and in the latter case both with some non-default face at point
    Eli> and without.
    >>> 
    >>> Ah yes, font-lock off makes it all more complex. Tested in those 3
    >>> cases. Is this too much for emacs-29?

    Eli> Not too much, because this code is new in Emacs 29.

    Robert> OK. I guess that means I can add `emoji-zoom-reset' and fix the
    Robert> increase/decrease assymetry there as well.

Iʼve added `emoji-zoom-reset' as well as fixing this bug.

Closing.
Committed as 470d269ec1f

Robert
--