bug#67794: 30.0.50; mouse-face is not respected on SVG images

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi,

The mouse-face text property is not applied on SVG images while other
faces works "correctly".  My recipe:

      0. emacs -Q
      1. Load the following code:
--8<---------------cut here---------------start------------->8---
(defun +insert-stuff ()
  (interactive)
  (let ((str "foobar"))
    (add-text-properties 0 1
			 (list 'display
			       (find-image
				'((:type xpm :file "close.xpm" :ascent center))
				t))
			 str)
    (add-text-properties 3 4
			 (list 'display
			       (find-image
				'((:type svg :file "radio-checked.svg" :ascent center))
				t))
			 str)
    (add-text-properties 0 6
			 (list 'mouse-face '(:background "red" :foreground "blue")) str)
    (insert str)))
--8<---------------cut here---------------end--------------->8---
      2. Into a writable buffer, do 'M-x +insert-stuff'

Now a 'C-s foobar' shows the matched string respecting the isearch face
even on the SVG image.  But when, you move your mouse over the "foobar"
string, the SVG image keeps the foreground and background of the default
face.

(note: I have tried to track this down but debugging Emacs display is
*hard*.  I have read and tried the "Debugging Emacs redisplay problems"
section of etc/DEBUG with no luck so far.)

Thanks,


In GNU Emacs 30.0.50 (build 3, x86_64-unknown-openbsd7.4) of 2023-12-11
 built on computer
Repository revision: 6abea4d98d1d964c68a78cb9b5321071da851654
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101009
System Description: OpenBSD computer 7.4 GENERIC.MP#1473 amd64

Configured using:
 'configure CPPFLAGS=-I/usr/local/include LDFLAGS=-L/usr/local/lib
 MAKEINFO=gmakeinfo --prefix=/home/manuel/emacs
 --bindir=/home/manuel/bin --with-x-toolkit=no --without-cairo
 --without-dbus --without-gconf --without-gsettings --without-sound
 --without-compress-install'

Configured features:
FREETYPE GIF GLIB GMP GNUTLS HARFBUZZ JPEG JSON LCMS2 LIBOTF LIBXML2
MODULES NOTIFY KQUEUE OLDXMENU PDUMPER PNG RSVG SQLITE3 THREADS TIFF
TREE_SITTER WEBP X11 XDBE XFT XIM XINPUT2 XPM ZLIB

Important settings:
  value of $LC_CTYPE: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Text

Minor modes in effect:
  display-time-mode: t
  display-battery-mode: t
  desktop-save-mode: t
  server-mode: t
  override-global-mode: t
  repeat-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  minibuffer-regexp-mode: t
  line-number-mode: t
  auto-fill-function: do-auto-fill
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
/home/manuel/.emacs.d/elpa/ef-themes-1.4.0/theme-loaddefs hides /home/manuel/emacs/share/emacs/30.0.50/lisp/theme-loaddefs

Features:
(shadow sort mail-extr emacsbug org-agenda vc-cvs vc-rcs log-view
pcvs-util org-indent oc-basic org-element org-persist org-id avl-tree
ol-eww eww url-queue mm-url ol-rmail ol-mhe ol-irc ol-info ol-gnus
nnselect ol-docview doc-view image-mode exif ol-bibtex bibtex ol-bbdb
ol-w3m ol-doi org-link-doi gnus-icalendar org-capture org-refile org ob
ob-tangle ob-ref ob-lob ob-table ob-exp org-macro org-src ob-comint
org-pcomplete org-list org-footnote org-faces org-entities ob-emacs-lisp
ob-core ob-eval org-cycle org-table ol org-fold org-fold-core org-keys
oc org-loaddefs org-version org-compat org-macs sh-script smie treesit
executable vc-hg conf-mode pascal paredit vc-dir ewoc mule-util
jka-compr on-screen autorevert filenotify vc-git diff-mode vc
vc-dispatcher bug-reference gnus-dired time battery cus-load desktop
frameset exwm-randr xcb-randr exwm-config ido exwm exwm-input
xcb-keysyms xcb-xkb exwm-manage exwm-floating xcb-cursor xcb-render
exwm-layout exwm-workspace exwm-core xcb-ewmh xcb-icccm xcb xcb-xproto
xcb-types xcb-debug server modus-operandi-theme modus-themes zone
speed-type url-http url-auth url-gw nsm compat ytdious mingus libmpdee
reporter edebug debug backtrace transmission color calc-bin calc-ext
calc calc-loaddefs rect calc-macs supercite regi ebdb-message ebdb-gnus
gnus-msg gnus-art mm-uu mml2015 mm-view mml-smime smime gnutls dig
gnus-sum shr pixel-fill kinsoku url-file svg dom gnus-group gnus-undo
gnus-start gnus-dbus gnus-cloud nnimap nnmail mail-source utf7 nnoo
gnus-spec gnus-int gnus-range message sendmail yank-media puny rfc822
mml mml-sec epa epg rfc6068 epg-config mm-decode mm-bodies mm-encode
mail-parse rfc2231 rfc2047 rfc2045 ietf-drums gmm-utils mailheader
gnus-win ebdb-mua ebdb-com crm ebdb-format ebdb mailabbrev eieio-opt
speedbar ezimage dframe find-func eieio-base timezone icalendar gnus
nnheader gnus-util mail-utils range mm-util mail-prsvr wid-edit web-mode
derived disp-table erlang-start skeleton cc-mode cc-fonts cc-guess
cc-menus cc-cmds cc-styles cc-align cc-engine cc-vars cc-defs slime-asdf
grep slime-tramp tramp rx trampver tramp-integration files-x
tramp-message tramp-compat xdg shell pcomplete parse-time iso8601
time-date format-spec tramp-loaddefs slime-fancy slime-indentation
slime-cl-indent cl-indent slime-trace-dialog slime-fontifying-fu
slime-package-fu slime-references slime-compiler-notes-tree advice
slime-scratch slime-presentations bridge slime-macrostep macrostep
slime-mdot-fu slime-enclosing-context slime-fuzzy slime-fancy-trace
slime-fancy-inspector slime-c-p-c slime-editing-commands slime-autodoc
slime-repl slime-parse slime apropos compile text-property-search etags
fileloop generator xref project arc-mode archive-mode noutline outline
icons pp comint ansi-osc ansi-color ring hyperspec thingatpt
slime-autoloads edmacro kmacro use-package-bind-key bind-key appt
diary-lib diary-loaddefs cal-menu calendar cal-loaddefs pcase dired-x
dired-aux dired dired-loaddefs notifications dbus xml cl-extra help-mode
use-package-core repeat easy-mmode debbugs-autoloads ebdb-autoloads
ef-themes-autoloads exwm-autoloads hyperbole-autoloads magit-autoloads
git-commit-autoloads magit-section-autoloads dash-autoloads
on-screen-autoloads osm-autoloads paredit-autoloads rust-mode-autoloads
speed-type-autoloads transmission-autoloads with-editor-autoloads info
compat-autoloads ytdious-autoloads package browse-url url url-proxy
url-privacy url-expand url-methods url-history url-cookie
generate-lisp-file url-domsuf url-util mailcap url-handlers url-parse
auth-source cl-seq eieio eieio-core cl-macs password-cache json subr-x
map byte-opt gv bytecomp byte-compile url-vars cl-loaddefs cl-lib rmc
iso-transl tooltip cconv eldoc paren electric uniquify ediff-hook
vc-hooks lisp-float-type elisp-mode mwheel term/x-win x-win
term/common-win x-dnd touch-screen tool-bar dnd fontset image regexp-opt
fringe tabulated-list replace newcomment text-mode lisp-mode prog-mode
register page tab-bar menu-bar rfn-eshadow isearch easymenu timer select
scroll-bar mouse jit-lock font-lock syntax font-core term/tty-colors
frame minibuffer nadvice seq simple cl-generic indonesian philippine
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite emoji-zwj charscript
charprop case-table epa-hook jka-cmpr-hook help abbrev obarray oclosure
cl-preloaded button loaddefs theme-loaddefs faces cus-face macroexp
files window text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget keymap hashtable-print-readable backquote
threads kqueue lcms2 dynamic-setting font-render-setting xinput2 x
multi-tty move-toolbar make-network-process emacs)

Memory information:
((conses 16 692952 74044) (symbols 48 52068 2)
 (strings 32 165492 7209) (string-bytes 1 5372823) (vectors 16 103327)
 (vector-slots 8 2122739 223406) (floats 8 515 178)
 (intervals 56 25843 106) (buffers 992 46))

-- 
Manuel Giraud

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Eli Zaretskii]

> Date: Tue, 12 Dec 2023 14:12:51 +0100
> From:  Manuel Giraud via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> 
> The mouse-face text property is not applied on SVG images while other
> faces works "correctly".  My recipe:
> 
>       0. emacs -Q
>       1. Load the following code:
> --8<---------------cut here---------------start------------->8---
> (defun +insert-stuff ()
>   (interactive)
>   (let ((str "foobar"))
>     (add-text-properties 0 1
> 			 (list 'display
> 			       (find-image
> 				'((:type xpm :file "close.xpm" :ascent center))
> 				t))
> 			 str)
>     (add-text-properties 3 4
> 			 (list 'display
> 			       (find-image
> 				'((:type svg :file "radio-checked.svg" :ascent center))
> 				t))
> 			 str)
>     (add-text-properties 0 6
> 			 (list 'mouse-face '(:background "red" :foreground "blue")) str)
>     (insert str)))
> --8<---------------cut here---------------end--------------->8---
>       2. Into a writable buffer, do 'M-x +insert-stuff'
> 
> Now a 'C-s foobar' shows the matched string respecting the isearch face
> even on the SVG image.  But when, you move your mouse over the "foobar"
> string, the SVG image keeps the foreground and background of the default
> face.

I think we only support colors from 'face' properties on SVG images,
not from 'mouse-face'.  Alan, am I right?

Basically, SVG images specify their own background color, and the
Emacs display cannot override that, since the image is generated by
librsvg.  So to change the background color, we wrap the SVG in
another SVG, see svg_load_image.  This way, the SVG spec submitted to
librsvg specifies different colors according to what Emacs wants.  And
we only do that for colors that come from 'face' properties.

> (note: I have tried to track this down but debugging Emacs display is
> *hard*.  I have read and tried the "Debugging Emacs redisplay problems"
> section of etc/DEBUG with no luck so far.)

The mouse highlight is implemented in note_mouse_highlight and its
subroutines.  If you already discovered that, then what were the
difficulties you faced in understanding what happens?

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

>> Now a 'C-s foobar' shows the matched string respecting the isearch face
>> even on the SVG image.  But when, you move your mouse over the "foobar"
>> string, the SVG image keeps the foreground and background of the default
>> face.
>
> I think we only support colors from 'face' properties on SVG images,
> not from 'mouse-face'.  Alan, am I right?
>
> Basically, SVG images specify their own background color, and the
> Emacs display cannot override that, since the image is generated by
> librsvg.  So to change the background color, we wrap the SVG in
> another SVG, see svg_load_image.  This way, the SVG spec submitted to
> librsvg specifies different colors according to what Emacs wants.  And
> we only do that for colors that come from 'face' properties.

This means that when you do a 'C-s foobar' and that the SVG is correctly
displayed with the isearch face (foreground and background), such an
embedded SVG was created on the fly?  If so, I think we should do the
same for mouse-face too because this SVG generation is very fast!

>> (note: I have tried to track this down but debugging Emacs display is
>> *hard*.  I have read and tried the "Debugging Emacs redisplay problems"
>> section of etc/DEBUG with no luck so far.)
>
> The mouse highlight is implemented in note_mouse_highlight and its
> subroutines.  If you already discovered that, then what were the
> difficulties you faced in understanding what happens?

Yes, I used to put a break point on show_mouse_face but then diving down
to x_draw_glyph_string it is hard to "see" what is going on here.  As it
was working with face such as isearch, my new strategy was to find out
how it worked here but now I don't know where/how to break to the
correct place.
-- 
Manuel Giraud

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Alan Third]

On Tue, Dec 12, 2023 at 03:07:13PM +0100, Manuel Giraud wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> [...]
> 
> >> Now a 'C-s foobar' shows the matched string respecting the isearch face
> >> even on the SVG image.  But when, you move your mouse over the "foobar"
> >> string, the SVG image keeps the foreground and background of the default
> >> face.
> >
> > I think we only support colors from 'face' properties on SVG images,
> > not from 'mouse-face'.  Alan, am I right?

Yes, that's right.

> > Basically, SVG images specify their own background color, and the
> > Emacs display cannot override that, since the image is generated by
> > librsvg.  So to change the background color, we wrap the SVG in
> > another SVG, see svg_load_image.  This way, the SVG spec submitted to
> > librsvg specifies different colors according to what Emacs wants.  And
> > we only do that for colors that come from 'face' properties.
> 
> This means that when you do a 'C-s foobar' and that the SVG is correctly
> displayed with the isearch face (foreground and background), such an
> embedded SVG was created on the fly?  If so, I think we should do the
> same for mouse-face too because this SVG generation is very fast!

I don't see any reason why we shouldn't do that.

lookup_image in image.c is where most of the work is done. It takes a
face ID number. There are three calls to it from xdisp.c. I don't know
if it would be best to change the callers to pass in the mouse face
instead of the local face, or to pass in the mouse face as well as the
local face and then allow lookup_image to work it out.

This assumes the mouse face details have been worked out before the
calls to lookup_image, I don't know much about how it works.

-- 
Alan Third

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: Alan Third <alan@idiocy.org>,  67794@debbugs.gnu.org
> Date: Tue, 12 Dec 2023 15:07:13 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Basically, SVG images specify their own background color, and the
> > Emacs display cannot override that, since the image is generated by
> > librsvg.  So to change the background color, we wrap the SVG in
> > another SVG, see svg_load_image.  This way, the SVG spec submitted to
> > librsvg specifies different colors according to what Emacs wants.  And
> > we only do that for colors that come from 'face' properties.
> 
> This means that when you do a 'C-s foobar' and that the SVG is correctly
> displayed with the isearch face (foreground and background), such an
> embedded SVG was created on the fly?  If so, I think we should do the
> same for mouse-face too because this SVG generation is very fast!

Not sure "fast" is fast enough, since unlike face properties,
mouse-highlight is highly dynamic, and creating a new image each time
(as I think this would mean) is something we want.  Let's wait for
Alan to chime in with his insights.

> >> (note: I have tried to track this down but debugging Emacs display is
> >> *hard*.  I have read and tried the "Debugging Emacs redisplay problems"
> >> section of etc/DEBUG with no luck so far.)
> >
> > The mouse highlight is implemented in note_mouse_highlight and its
> > subroutines.  If you already discovered that, then what were the
> > difficulties you faced in understanding what happens?
> 
> Yes, I used to put a break point on show_mouse_face but then diving down
> to x_draw_glyph_string it is hard to "see" what is going on here.

We are drawing a series of glyphs of the same type which share the
same face/mouse-face.  For SVG, it would probably mean we are drawing
that single SVG (which is a single glyph), since series of images are
usually rare.

> As it was working with face such as isearch, my new strategy was to
> find out how it worked here but now I don't know where/how to break
> to the correct place.

show_mouse_face calls draw_row_with_mouse_face, which calls
draw_glyphs, which builds a "glyph string" and then calls the frame's
draw_glyph_string method, which on X is x_draw_glyph_string.  The
latter eventually calls x_draw_image_glyph_string.  HTH.

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Alan Third]

On Tue, Dec 12, 2023 at 04:47:13PM +0200, Eli Zaretskii wrote:
> > From: Manuel Giraud <manuel@ledu-giraud.fr>
> > Cc: Alan Third <alan@idiocy.org>,  67794@debbugs.gnu.org
> > Date: Tue, 12 Dec 2023 15:07:13 +0100
> > 
> > Eli Zaretskii <eliz@gnu.org> writes:
> > 
> > > Basically, SVG images specify their own background color, and the
> > > Emacs display cannot override that, since the image is generated by
> > > librsvg.  So to change the background color, we wrap the SVG in
> > > another SVG, see svg_load_image.  This way, the SVG spec submitted to
> > > librsvg specifies different colors according to what Emacs wants.  And
> > > we only do that for colors that come from 'face' properties.
> > 
> > This means that when you do a 'C-s foobar' and that the SVG is correctly
> > displayed with the isearch face (foreground and background), such an
> > embedded SVG was created on the fly?  If so, I think we should do the
> > same for mouse-face too because this SVG generation is very fast!
> 
> Not sure "fast" is fast enough, since unlike face properties,
> mouse-highlight is highly dynamic, and creating a new image each time
> (as I think this would mean) is something we want.  Let's wait for
> Alan to chime in with his insights.
> 
<snip>
> > As it was working with face such as isearch, my new strategy was to
> > find out how it worked here but now I don't know where/how to break
> > to the correct place.
> 
> show_mouse_face calls draw_row_with_mouse_face, which calls
> draw_glyphs, which builds a "glyph string" and then calls the frame's
> draw_glyph_string method, which on X is x_draw_glyph_string.  The
> latter eventually calls x_draw_image_glyph_string.  HTH.

Hmm, yes, that makes it harder. Images are loaded, and in the case of
SVGs rasterised, earlier in the process. We would presumably need to
create a whole new image and substitute it at the point where we draw
to the screen.

A quick look at nsterm.m's image code makes me think it should be
relatively straight forward to extract the image spec from
s->img->spec and generate a new image with the mouse face, however
this will happen for every image type, even very large images that
don't use face colours, and it will have to be implemented separately
in each term.

It could also result in some undesirable effects if the image file was
changed between redisplay calculating the original image size and the
substitute being created.
-- 
Alan Third

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Alan Third <alan@idiocy.org> writes:

[...]

>> This means that when you do a 'C-s foobar' and that the SVG is correctly
>> displayed with the isearch face (foreground and background), such an
>> embedded SVG was created on the fly?  If so, I think we should do the
>> same for mouse-face too because this SVG generation is very fast!
>
> I don't see any reason why we shouldn't do that.
>
> lookup_image in image.c is where most of the work is done. It takes a
> face ID number. There are three calls to it from xdisp.c. I don't know
> if it would be best to change the callers to pass in the mouse face
> instead of the local face, or to pass in the mouse face as well as the
> local face and then allow lookup_image to work it out.
>
> This assumes the mouse face details have been worked out before the
> calls to lookup_image, I don't know much about how it works.

Thanks, I think your explanation would greatly help.  BTW, mouse-face is
a standard Emacs face so I don't why it seems not to be handled by one
of those calls.
-- 
Manuel Giraud

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: Eli Zaretskii <eliz@gnu.org>,  67794@debbugs.gnu.org
> Date: Tue, 12 Dec 2023 17:35:18 +0100
> 
> BTW, mouse-face is a standard Emacs face so I don't why it seems not
> to be handled by one of those calls.

No, mouse-face is not a face, it's a text property.

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Manuel Giraud <manuel@ledu-giraud.fr>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  67794@debbugs.gnu.org
>> Date: Tue, 12 Dec 2023 17:35:18 +0100
>> 
>> BTW, mouse-face is a standard Emacs face so I don't why it seems not
>> to be handled by one of those calls.
>
> No, mouse-face is not a face, it's a text property.

Ok.  I thought it was a face because it is declared as MOUSE_FACE_ID in
the face_id enum.  And as Alan talked about the 3 calls to lookup_image,
I thought that it could be called MOUSE_FACE_ID as argument.
-- 
Manuel Giraud

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: alan@idiocy.org,  67794@debbugs.gnu.org
> Date: Wed, 13 Dec 2023 09:18:56 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Manuel Giraud <manuel@ledu-giraud.fr>
> >> Cc: Eli Zaretskii <eliz@gnu.org>,  67794@debbugs.gnu.org
> >> Date: Tue, 12 Dec 2023 17:35:18 +0100
> >> 
> >> BTW, mouse-face is a standard Emacs face so I don't why it seems not
> >> to be handled by one of those calls.
> >
> > No, mouse-face is not a face, it's a text property.
> 
> Ok.  I thought it was a face because it is declared as MOUSE_FACE_ID in
> the face_id enum.  And as Alan talked about the 3 calls to lookup_image,
> I thought that it could be called MOUSE_FACE_ID as argument.

It behaves exactly like a face, and has the same attributes, thus the
MOUSE_FACE_ID enumeration.  But it is handled specially by the display
engine, because it changes the appearance depending on mouse pointer
position, without any changes to the mouse-face definition itself.

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

>> Ok.  I thought it was a face because it is declared as MOUSE_FACE_ID in
>> the face_id enum.  And as Alan talked about the 3 calls to lookup_image,
>> I thought that it could be called MOUSE_FACE_ID as argument.
>
> It behaves exactly like a face, and has the same attributes, thus the
> MOUSE_FACE_ID enumeration.  But it is handled specially by the display
> engine, because it changes the appearance depending on mouse pointer
> position, without any changes to the mouse-face definition itself.

Ok and so my next question is: Is mouse highlight handled completely
outside the « regular » display engine?  If so, I will never ever see a
lookup_image call with a face_id == MOUSE_FACE_ID, right?
-- 
Manuel Giraud

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: alan@idiocy.org,  67794@debbugs.gnu.org
> Date: Wed, 13 Dec 2023 15:13:08 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > It behaves exactly like a face, and has the same attributes, thus the
> > MOUSE_FACE_ID enumeration.  But it is handled specially by the display
> > engine, because it changes the appearance depending on mouse pointer
> > position, without any changes to the mouse-face definition itself.
> 
> Ok and so my next question is: Is mouse highlight handled completely
> outside the « regular » display engine?  If so, I will never ever see a
> lookup_image call with a face_id == MOUSE_FACE_ID, right?

Right.

mouse-highlight works differently than the "regular" display: it
redraws already drawn portions of the screen by (re-)drawing the same
glyphs with different colors.  By contrast, lookup_image is called
when a 'display' property is found during _generation_ of those
glyphs, before showing them for the first time.

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

>> Ok and so my next question is: Is mouse highlight handled completely
>> outside the « regular » display engine?  If so, I will never ever see a
>> lookup_image call with a face_id == MOUSE_FACE_ID, right?
>
> Right.
>
> mouse-highlight works differently than the "regular" display: it
> redraws already drawn portions of the screen by (re-)drawing the same
> glyphs with different colors.

Ok.  And it works with a pixmap because it uses its mask.  But for SVG,
we have a fully rasterized image without mask.  Thank you for clarifying
it.

> By contrast, lookup_image is called when a 'display' property is found
> during _generation_ of those glyphs, before showing them for the first
> time.

Yes but the glyph generation is not done for every face under the sun,
right?  So this could be done "dynamically"?

If so, what would be a right place to have a call to lookup_image into
mouse-highlight?  Is it "too late" for glyphs generation?
-- 
Manuel Giraud

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: alan@idiocy.org,  67794@debbugs.gnu.org
> Date: Wed, 13 Dec 2023 17:04:39 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> [...]
> 
> > By contrast, lookup_image is called when a 'display' property is found
> > during _generation_ of those glyphs, before showing them for the first
> > time.
> 
> Yes but the glyph generation is not done for every face under the sun,
> right?  So this could be done "dynamically"?

Glyph generation uses the single face that's applicable to the buffer
or string whose glyphs are being generated.  That face is obtained by
merging all the faces in effect there (see "Displaying Faces" in the
ELisp manual for the details), but eventually this merging produces a
single face that is used for the glyph(s).

> If so, what would be a right place to have a call to lookup_image into
> mouse-highlight?  Is it "too late" for glyphs generation?

It's too late, yes.  mouse-highlight, as it currently works, doesn't
regenerate glyphs, it uses glyphs already present in the glyph
matrices.

Which is not to say we couldn't add calls to lookup_image there, but
that would need rather serious changes in show_mouse_face: it
currently just identifies the glyphs that need to be redrawn, but will
instead need to replace the image glyphs it finds with new ones (and
then maybe replace them back in clear_mouse_face?).

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

> It's too late, yes.  mouse-highlight, as it currently works, doesn't
> regenerate glyphs, it uses glyphs already present in the glyph
> matrices.

Thanks for your all explanations and links.  BTW, do you know why
mouse-highlight is this way?  Is it because it was an afterthought of
the display engine or is it because of performance issues or…?
-- 
Manuel Giraud

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: alan@idiocy.org,  67794@debbugs.gnu.org
> Date: Wed, 13 Dec 2023 17:50:08 +0100
> 
> BTW, do you know why mouse-highlight is this way?  Is it because it
> was an afterthought of the display engine or is it because of
> performance issues or…?

How would you design it instead?

I'm not saying that what we have is the only possible design, but I'm
curious what alternative design ideas could be used.

Btw, AFAIU the person who at the time decided how this will work is
here: it's Gerd (CC'ed).  Maybe he can share his memories about why he
ended up with this design.

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Manuel Giraud <manuel@ledu-giraud.fr>
>> Cc: alan@idiocy.org,  67794@debbugs.gnu.org
>> Date: Wed, 13 Dec 2023 17:50:08 +0100
>> 
>> BTW, do you know why mouse-highlight is this way?  Is it because it
>> was an afterthought of the display engine or is it because of
>> performance issues or…?
>
> How would you design it instead?
>
> I'm not saying that what we have is the only possible design, but I'm
> curious what alternative design ideas could be used.
>
> Btw, AFAIU the person who at the time decided how this will work is
> here: it's Gerd (CC'ed).  Maybe he can share his memories about why he
> ended up with this design.

IIRC, this way of mouse-highlighting stems from Emacs 19, at least in
principle. At that time, X event handling was done from a SIGIO signal
handler. Being called from a signal handler made it impossible to use
anything from the event handling code which wasn't reentrant. And there
was not much reentrant code in Emacs, in particular not Lisp, but also
most other stuff.

Maybe things could be done differently today, since event handling is no
longer done in SIGIO handlers.

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: Manuel Giraud <manuel@ledu-giraud.fr>,  alan@idiocy.org,
>   67794@debbugs.gnu.org
> Date: Wed, 13 Dec 2023 18:57:13 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Btw, AFAIU the person who at the time decided how this will work is
> > here: it's Gerd (CC'ed).  Maybe he can share his memories about why he
> > ended up with this design.
> 
> IIRC, this way of mouse-highlighting stems from Emacs 19, at least in
> principle. At that time, X event handling was done from a SIGIO signal
> handler. Being called from a signal handler made it impossible to use
> anything from the event handling code which wasn't reentrant. And there
> was not much reentrant code in Emacs, in particular not Lisp, but also
> most other stuff.

That's what I thought.

Still, given that the response to mouse movement should be very fast,
and the glyphs are already there, just with the wrong colors, I think
a design that just reuses the glyphs for redrawing a portion of the
screen sounds kinda...natural.

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Manuel Giraud <manuel@ledu-giraud.fr>
>> Cc: alan@idiocy.org,  67794@debbugs.gnu.org
>> Date: Wed, 13 Dec 2023 17:50:08 +0100
>> 
>> BTW, do you know why mouse-highlight is this way?  Is it because it
>> was an afterthought of the display engine or is it because of
>> performance issues or…?
>
> How would you design it instead?
>
> I'm not saying that what we have is the only possible design, but I'm
> curious what alternative design ideas could be used.

Sorry I do not have an alternative design idea 😅.  It's just that from
my, obviously limited, point of view the Emacs display engine is a work
horse that could handled many cases regarding how to display glyphs and
I was wondering why it was not used for mouse highlight also.

> Btw, AFAIU the person who at the time decided how this will work is
> here: it's Gerd (CC'ed).  Maybe he can share his memories about why he
> ended up with this design.

Thanks for summoning Gerd.  I have an answer now even though I do not
have the knowledge to understand it fully.
-- 
Manuel Giraud

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: Gerd Möllmann <gerd.moellmann@gmail.com>,
>   alan@idiocy.org,
>   67794@debbugs.gnu.org
> Date: Wed, 13 Dec 2023 19:24:48 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Manuel Giraud <manuel@ledu-giraud.fr>
> >> Cc: alan@idiocy.org,  67794@debbugs.gnu.org
> >> Date: Wed, 13 Dec 2023 17:50:08 +0100
> >> 
> >> BTW, do you know why mouse-highlight is this way?  Is it because it
> >> was an afterthought of the display engine or is it because of
> >> performance issues or…?
> >
> > How would you design it instead?
> >
> > I'm not saying that what we have is the only possible design, but I'm
> > curious what alternative design ideas could be used.
> 
> Sorry I do not have an alternative design idea 😅.  It's just that from
> my, obviously limited, point of view the Emacs display engine is a work
> horse that could handled many cases regarding how to display glyphs and
> I was wondering why it was not used for mouse highlight also.

We do use the display engine, just the last part of its processing:
the drawing of glyphs on the glass.

> > Btw, AFAIU the person who at the time decided how this will work is
> > here: it's Gerd (CC'ed).  Maybe he can share his memories about why he
> > ended up with this design.
> 
> Thanks for summoning Gerd.  I have an answer now even though I do not
> have the knowledge to understand it fully.

What Gerd says, in a nutshell, is that since this code was running
from a signal handler, there was a basic requirement to do it as
simple as possible, and in particular not to do anything that would
mean memory allocation, since memory allocation functions (and many
other functions) in a typical libc were non-reentrant back then.  So
the chosen design was one that reused existing data structures as much
as possible.

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi,

Would it be possible to lookup_image with the current mouse face at
glyphs generation time for a string with the mouse-face property and
then, using this image glyph when showing the mouse face?
-- 
Manuel Giraud

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: gerd.moellmann@gmail.com,  alan@idiocy.org,  67794@debbugs.gnu.org
> Date: Thu, 14 Dec 2023 10:07:41 +0100
> 
> Would it be possible to lookup_image with the current mouse face at
> glyphs generation time for a string with the mouse-face property and
> then, using this image glyph when showing the mouse face?

I guess it's possible, but it requires changes in how the display code
works:

  . when we generate glyphs, we currently pay no attention to
    mouse-face properties, only to face properties
  . image glyphs will need to store 2 image IDs, not 1, and use the
    appropriate one in each case

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Manuel Giraud <manuel@ledu-giraud.fr>
>> Cc: gerd.moellmann@gmail.com,  alan@idiocy.org,  67794@debbugs.gnu.org
>> Date: Thu, 14 Dec 2023 10:07:41 +0100
>> 
>> Would it be possible to lookup_image with the current mouse face at
>> glyphs generation time for a string with the mouse-face property and
>> then, using this image glyph when showing the mouse face?
>
> I guess it's possible, but it requires changes in how the display code
> works:
>
>   . when we generate glyphs, we currently pay no attention to
>     mouse-face properties, only to face properties
>   . image glyphs will need to store 2 image IDs, not 1, and use the
>     appropriate one in each case

Ok and do you think this is a good enough design?  Or should we do it
differently?
-- 
Manuel Giraud

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: gerd.moellmann@gmail.com,  alan@idiocy.org,  67794@debbugs.gnu.org
> Date: Thu, 14 Dec 2023 10:48:23 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I guess it's possible, but it requires changes in how the display code
> > works:
> >
> >   . when we generate glyphs, we currently pay no attention to
> >     mouse-face properties, only to face properties
> >   . image glyphs will need to store 2 image IDs, not 1, and use the
> >     appropriate one in each case
> 
> Ok and do you think this is a good enough design?  Or should we do it
> differently?

Sounds okay to me to try that, but there could always be surprises in
that area.

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi,

Regarding this feature, I came up with the following patch:

diff --git a/src/xdisp.c b/src/xdisp.c
index 75d769600c4..735200fdf65 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -30985,6 +30985,24 @@ draw_glyphs (struct window *w, int x, struct glyph_row *row,
 	  }
     }
 
+  /* Update image glyphs with mouse face features.  */
+  if (hl == DRAW_MOUSE_FACE)
+    {
+      Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (f);
+      for (s = head; s; s = s->next)
+	if (s->first_glyph->type == IMAGE_GLYPH)
+	  if (s->img)
+	    {
+	      /* Alter the spec a bit to have a different hash for the
+		 image cache.  */
+	      ptrdiff_t id;
+	      Lisp_Object spec = Fcopy_sequence (s->img->spec);
+	      spec = nconc2 (spec, list2 (intern (":highlight"), Qt));
+	      id = lookup_image (f, spec, hlinfo->mouse_face_face_id);
+	      s->img = IMAGE_FROM_ID (f, id);
+	    }
+    }
+
   /* Draw all strings.  */
   for (s = head; s; s = s->next)
     FRAME_RIF (f)->draw_glyph_string (s);

It does something on the an image glyph when it is hover but the image
is completely messed up.  I have stepped this code down to
svg_load_image but I do not understand what went wrong.  Any ideas?
-- 
Manuel Giraud

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Alan Third]

On Mon, Dec 18, 2023 at 12:02:22PM +0100, Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors wrote:
> +	      /* Alter the spec a bit to have a different hash for the
> +		 image cache.  */
> +	      ptrdiff_t id;
> +	      Lisp_Object spec = Fcopy_sequence (s->img->spec);
> +	      spec = nconc2 (spec, list2 (intern (":highlight"), Qt));
> +	      id = lookup_image (f, spec, hlinfo->mouse_face_face_id);
> +	      s->img = IMAGE_FROM_ID (f, id);

You shouldn't have to change the image spec as the image cache
differentiates by colour too. So if the mouse face has the same
colours as the default face it will use the same image, but if they're
different it will create a new image.

If that's not happening then there must be a bug somewhere.

> It does something on the an image glyph when it is hover but the image
> is completely messed up.  I have stepped this code down to
> svg_load_image but I do not understand what went wrong.  Any ideas?

The code looks OK to me. In what way is the image messed up? I'm not
sure how to test mouse face on my machine, is there a built-in theme
that uses it?
-- 
Alan Third

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Alan Third <alan@idiocy.org> writes:

> On Mon, Dec 18, 2023 at 12:02:22PM +0100, Manuel Giraud via Bug
> reports for GNU Emacs, the Swiss army knife of text editors wrote:
>> +	      /* Alter the spec a bit to have a different hash for the
>> +		 image cache.  */
>> +	      ptrdiff_t id;
>> +	      Lisp_Object spec = Fcopy_sequence (s->img->spec);
>> +	      spec = nconc2 (spec, list2 (intern (":highlight"), Qt));
>> +	      id = lookup_image (f, spec, hlinfo->mouse_face_face_id);
>> +	      s->img = IMAGE_FROM_ID (f, id);
>
> You shouldn't have to change the image spec as the image cache
> differentiates by colour too. So if the mouse face has the same
> colours as the default face it will use the same image, but if they're
> different it will create a new image.

Thanks, I'll try that.

> If that's not happening then there must be a bug somewhere.
>
>> It does something on the an image glyph when it is hover but the image
>> is completely messed up.  I have stepped this code down to
>> svg_load_image but I do not understand what went wrong.  Any ideas?
>
> The code looks OK to me. In what way is the image messed up? I'm not
> sure how to test mouse face on my machine, is there a built-in theme
> that uses it?

The image contains some random pixels that look like part of previously
loaded picture.  You could try it with the previous patch and test it
with the following command:

--8<---------------cut here---------------start------------->8---
(defun +insert-stuff ()
  (interactive)
  (let ((str "foobar"))
    (add-text-properties 0 1
			 (list 'display
			       (find-image
				'((:type xpm :file "close.xpm" :ascent center))
				t))
			 str)
    (add-text-properties 3 4
			 (list 'display
			       (find-image
				'((:type svg :file "radio-checked.svg" :ascent center))
				t))
			 str)
    (add-text-properties 0 6
			 (list 'mouse-face '(:background "red" :foreground "blue")) str)
    (insert str)))
--8<---------------cut here---------------end--------------->8---
-- 
Manuel Giraud

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Alan Third]

On Mon, Dec 18, 2023 at 07:36:49PM +0100, Manuel Giraud wrote:
> Alan Third <alan@idiocy.org> writes:
> 
> > If that's not happening then there must be a bug somewhere.
> >
> >> It does something on the an image glyph when it is hover but the image
> >> is completely messed up.  I have stepped this code down to
> >> svg_load_image but I do not understand what went wrong.  Any ideas?
> >
> > The code looks OK to me. In what way is the image messed up? I'm not
> > sure how to test mouse face on my machine, is there a built-in theme
> > that uses it?
> 
> The image contains some random pixels that look like part of previously
> loaded picture.  You could try it with the previous patch and test it
> with the following command:

Thanks, that worked. It was fine in NS (under GNUstep no less! It's
unusual for GNUstep to work when other things don't) but failed under
GTK/Cairo.

I believe that we need to do some final set-up for the image under
X/GTK/whatever and it looks as though this is all that's needed:

     id = lookup_image (f, s->img->spec, hlinfo->mouse_face_face_id);
     s->img = IMAGE_FROM_ID (f, id);
     prepare_image_for_display (f, s->img); <------

Under X we need to send the image to the X server, and under Cairo it
does some other stuff. That's what prepare_image_for_display does. It
does nothing under NS, so the image worked fine under GNUstep.
-- 
Alan Third

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Alan Third <alan@idiocy.org> writes:

[...]

> Thanks, that worked. It was fine in NS (under GNUstep no less! It's
> unusual for GNUstep to work when other things don't) but failed under
> GTK/Cairo.
>
> I believe that we need to do some final set-up for the image under
> X/GTK/whatever and it looks as though this is all that's needed:
>
>      id = lookup_image (f, s->img->spec, hlinfo->mouse_face_face_id);
>      s->img = IMAGE_FROM_ID (f, id);
>      prepare_image_for_display (f, s->img); <------
>
> Under X we need to send the image to the X server, and under Cairo it
> does some other stuff. That's what prepare_image_for_display does. It
> does nothing under NS, so the image worked fine under GNUstep.

Thank you Alan!  That does the trick.  So here is an updated version of
the patch that works with XRender (i'll test with cairo).
-- 
Manuel Giraud

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Respect-mouse-face-on-image-glyphs-bug-67794.patch --]
[-- Type: text/x-patch, Size: 1163 bytes --]

From 10c123988308cea24384205d118af75fdc09635c Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Tue, 19 Dec 2023 12:25:24 +0100
Subject: [PATCH] Respect mouse-face on image glyphs (bug#67794)

* src/xdisp.c (draw_glyphs): Maybe update image glyphs with mouse face
features before drawing.
---
 src/xdisp.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/src/xdisp.c b/src/xdisp.c
index 75d769600c4..cd9ce57c0da 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -30985,6 +30985,21 @@ draw_glyphs (struct window *w, int x, struct glyph_row *row,
 	  }
     }
 
+  /* Update image glyphs with mouse face features.  */
+  if (hl == DRAW_MOUSE_FACE)
+    {
+      Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (f);
+      for (s = head; s; s = s->next)
+	if (s->first_glyph->type == IMAGE_GLYPH)
+	  if (s->img)
+	    {
+	      ptrdiff_t id;
+	      id = lookup_image (f, s->img->spec, hlinfo->mouse_face_face_id);
+	      s->img = IMAGE_FROM_ID (f, id);
+	      prepare_image_for_display(f, s->img);
+	    }
+    }
+
   /* Draw all strings.  */
   for (s = head; s; s = s->next)
     FRAME_RIF (f)->draw_glyph_string (s);
-- 
2.43.0

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: Eli Zaretskii <eliz@gnu.org>,  gerd.moellmann@gmail.com,
>   67794@debbugs.gnu.org
> Date: Tue, 19 Dec 2023 13:45:03 +0100
> 
> > I believe that we need to do some final set-up for the image under
> > X/GTK/whatever and it looks as though this is all that's needed:
> >
> >      id = lookup_image (f, s->img->spec, hlinfo->mouse_face_face_id);
> >      s->img = IMAGE_FROM_ID (f, id);
> >      prepare_image_for_display (f, s->img); <------
> >
> > Under X we need to send the image to the X server, and under Cairo it
> > does some other stuff. That's what prepare_image_for_display does. It
> > does nothing under NS, so the image worked fine under GNUstep.
> 
> Thank you Alan!  That does the trick.  So here is an updated version of
> the patch that works with XRender (i'll test with cairo).

With this change, if you move the mouse pointer on and off the image
several times, what happens to the Emacs memory footprint?
Specifically, how many times is memory allocated for the SVG image,
and if it's allocated each time, does it ever get deallocated?

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Manuel Giraud <manuel@ledu-giraud.fr>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  gerd.moellmann@gmail.com,
>>   67794@debbugs.gnu.org
>> Date: Tue, 19 Dec 2023 13:45:03 +0100
>> 
>> > I believe that we need to do some final set-up for the image under
>> > X/GTK/whatever and it looks as though this is all that's needed:
>> >
>> >      id = lookup_image (f, s->img->spec, hlinfo->mouse_face_face_id);
>> >      s->img = IMAGE_FROM_ID (f, id);
>> >      prepare_image_for_display (f, s->img); <------
>> >
>> > Under X we need to send the image to the X server, and under Cairo it
>> > does some other stuff. That's what prepare_image_for_display does. It
>> > does nothing under NS, so the image worked fine under GNUstep.
>> 
>> Thank you Alan!  That does the trick.  So here is an updated version of
>> the patch that works with XRender (i'll test with cairo).
>
> With this change, if you move the mouse pointer on and off the image
> several times, what happens to the Emacs memory footprint?
> Specifically, how many times is memory allocated for the SVG image,
> and if it's allocated each time, does it ever get deallocated?

I didn't measure this memory footprint but AFAIU lookup_image uses the
image cache so an image is allocated once (for one foreground/background
combination).  And I think that there is an eviction timeout in the
image cache, no?
-- 
Manuel Giraud

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: alan@idiocy.org,  gerd.moellmann@gmail.com,  67794@debbugs.gnu.org
> Date: Tue, 19 Dec 2023 14:23:27 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Manuel Giraud <manuel@ledu-giraud.fr>
> >> Cc: Eli Zaretskii <eliz@gnu.org>,  gerd.moellmann@gmail.com,
> >>   67794@debbugs.gnu.org
> >> Date: Tue, 19 Dec 2023 13:45:03 +0100
> >> 
> >> > I believe that we need to do some final set-up for the image under
> >> > X/GTK/whatever and it looks as though this is all that's needed:
> >> >
> >> >      id = lookup_image (f, s->img->spec, hlinfo->mouse_face_face_id);
> >> >      s->img = IMAGE_FROM_ID (f, id);
> >> >      prepare_image_for_display (f, s->img); <------
> >> >
> >> > Under X we need to send the image to the X server, and under Cairo it
> >> > does some other stuff. That's what prepare_image_for_display does. It
> >> > does nothing under NS, so the image worked fine under GNUstep.
> >> 
> >> Thank you Alan!  That does the trick.  So here is an updated version of
> >> the patch that works with XRender (i'll test with cairo).
> >
> > With this change, if you move the mouse pointer on and off the image
> > several times, what happens to the Emacs memory footprint?
> > Specifically, how many times is memory allocated for the SVG image,
> > and if it's allocated each time, does it ever get deallocated?
> 
> I didn't measure this memory footprint but AFAIU lookup_image uses the
> image cache so an image is allocated once (for one foreground/background
> combination).  And I think that there is an eviction timeout in the
> image cache, no?

I know the theory, thank you, but I wanted to make sure there isn't
something that doesn't meet the eye here.  Before we install this, we
had better be sure we understand the consequences, since we have never
before done anything like that in mouse-highlight implementation.

OK?

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

>> I didn't measure this memory footprint but AFAIU lookup_image uses the
>> image cache so an image is allocated once (for one foreground/background
>> combination).  And I think that there is an eviction timeout in the
>> image cache, no?
>
> I know the theory, thank you, but I wanted to make sure there isn't
> something that doesn't meet the eye here.

Of course, I did not want to lecture you on this matter, I was just
stating what I understood here.

> Before we install this, we had better be sure we understand the
> consequences, since we have never before done anything like that in
> mouse-highlight implementation.
>
> OK?

Ok.  Maybe I could look at the evolution of the memory footprint.  How
do you think I could do this?
-- 
Manuel Giraud

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: alan@idiocy.org,  gerd.moellmann@gmail.com,  67794@debbugs.gnu.org
> Date: Tue, 19 Dec 2023 14:48:37 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Before we install this, we had better be sure we understand the
> > consequences, since we have never before done anything like that in
> > mouse-highlight implementation.
> >
> > OK?
> 
> Ok.  Maybe I could look at the evolution of the memory footprint.  How
> do you think I could do this?

Depending on your system, there surely is some tools which show the
memory map of a given process in some graphical manner.

If worse comes to worst, you could use 'top' of "M-x proced" in
another Emacs, although that will only show a single figure or two.

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Manuel Giraud <manuel@ledu-giraud.fr>
>> Cc: alan@idiocy.org,  gerd.moellmann@gmail.com,  67794@debbugs.gnu.org
>> Date: Tue, 19 Dec 2023 14:48:37 +0100
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > Before we install this, we had better be sure we understand the
>> > consequences, since we have never before done anything like that in
>> > mouse-highlight implementation.
>> >
>> > OK?
>> 
>> Ok.  Maybe I could look at the evolution of the memory footprint.  How
>> do you think I could do this?
>
> Depending on your system, there surely is some tools which show the
> memory map of a given process in some graphical manner.
>
> If worse comes to worst, you could use 'top' of "M-x proced" in
> another Emacs, although that will only show a single figure or two.

Ok, I thought you were thinking of something into Emacs.  I'll try to do
something like this.
-- 
Manuel Giraud

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: alan@idiocy.org,  gerd.moellmann@gmail.com,  67794@debbugs.gnu.org
> Date: Tue, 19 Dec 2023 15:39:28 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> > Before we install this, we had better be sure we understand the
> >> > consequences, since we have never before done anything like that in
> >> > mouse-highlight implementation.
> >> >
> >> > OK?
> >> 
> >> Ok.  Maybe I could look at the evolution of the memory footprint.  How
> >> do you think I could do this?
> >
> > Depending on your system, there surely is some tools which show the
> > memory map of a given process in some graphical manner.
> >
> > If worse comes to worst, you could use 'top' of "M-x proced" in
> > another Emacs, although that will only show a single figure or two.
> 
> Ok, I thought you were thinking of something into Emacs.  I'll try to do
> something like this.

Thanks.

Btw, does this only work for SVG, and if librsvg is later than some
recent version?  Or does this work with any image?  If the former,
this code should be executed only conditionally, under those
conditions under which it really changes the background color of an
image.

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

>> > Depending on your system, there surely is some tools which show the
>> > memory map of a given process in some graphical manner.
>> >
>> > If worse comes to worst, you could use 'top' of "M-x proced" in
>> > another Emacs, although that will only show a single figure or two.
>> 
>> Ok, I thought you were thinking of something into Emacs.  I'll try to do
>> something like this.
>
> Thanks.

So with "ps", I measured the RSS (resident set, here in KB).  I start
from "emacs -Q" with this patch.  I used the following code to test the
feature (the SVG used here is 580x580 pixels so produces a quite large
image on display):
--8<---------------cut here---------------start------------->8---
(defun +insert-stuff ()
  (interactive)
  (let ((str "foobar"))
    (add-text-properties 0 1
			 (list 'display
			       (find-image
				'((:type xpm :file "close.xpm" :ascent center))
				t))
			 str)
    (add-text-properties 3 4
			 (list 'display
			       (find-image
				'((:type svg :file "/usr/local/share/qgis/svg/tourist/tourist_fountain.svg"))
				t))
			 str)
    (add-text-properties 0 6
			 (list 'mouse-face '(:background "red" :foreground "blue")) str)
    (insert str)))
--8<---------------cut here---------------end--------------->8---

Here are the results:

Right after "emacs -Q": rss = 44688
After 'M-x +insert-stuff': rss = 54292
After first hover on text: rss = 54464
After repeated hover on/off text: rss = 54496

> Btw, does this only work for SVG, and if librsvg is later than some
> recent version?  Or does this work with any image?  If the former,
> this code should be executed only conditionally, under those
> conditions under which it really changes the background color of an
> image.

You're right: it does only work with SVG.  XPM are already handled with
mask.  I have tested and it does not work with PNG with transparent
background: don't know why.  Maybe instead of guard it with a "only for
SVG" test, we should resolve the issue for transparent PNG at a later
time.

I'm using librsvg 2.57 and I could not easily test it with an older
version.
-- 
Manuel Giraud

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: alan@idiocy.org,  gerd.moellmann@gmail.com,  67794@debbugs.gnu.org
> Date: Wed, 20 Dec 2023 09:47:48 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> So with "ps", I measured the RSS (resident set, here in KB).  I start
> from "emacs -Q" with this patch.  I used the following code to test the
> feature (the SVG used here is 580x580 pixels so produces a quite large
> image on display):
> --8<---------------cut here---------------start------------->8---
> (defun +insert-stuff ()
>   (interactive)
>   (let ((str "foobar"))
>     (add-text-properties 0 1
> 			 (list 'display
> 			       (find-image
> 				'((:type xpm :file "close.xpm" :ascent center))
> 				t))
> 			 str)
>     (add-text-properties 3 4
> 			 (list 'display
> 			       (find-image
> 				'((:type svg :file "/usr/local/share/qgis/svg/tourist/tourist_fountain.svg"))
> 				t))
> 			 str)
>     (add-text-properties 0 6
> 			 (list 'mouse-face '(:background "red" :foreground "blue")) str)
>     (insert str)))
> --8<---------------cut here---------------end--------------->8---
> 
> Here are the results:
> 
> Right after "emacs -Q": rss = 44688
> After 'M-x +insert-stuff': rss = 54292
> After first hover on text: rss = 54464
> After repeated hover on/off text: rss = 54496

Does it get bumped each hover-over, or does it stabilize after a few?

> > Btw, does this only work for SVG, and if librsvg is later than some
> > recent version?  Or does this work with any image?  If the former,
> > this code should be executed only conditionally, under those
> > conditions under which it really changes the background color of an
> > image.
> 
> You're right: it does only work with SVG.  XPM are already handled with
> mask.  I have tested and it does not work with PNG with transparent
> background: don't know why.  Maybe instead of guard it with a "only for
> SVG" test, we should resolve the issue for transparent PNG at a later
> time.

It would be a shame to waste cycles on PNG images as long as we don't
solve this problem, so I'd prefer to enable this only for SVG and
leave a FIXME comment about PNG.

> I'm using librsvg 2.57 and I could not easily test it with an older
> version.

Alan, is the trick with wrapping SVG with another one supposed to work
with all versions of librsvg?  I see some version conditions in
svg_load_image, but I'm not sure I understand the consequences.
Specifically, what does this code do:

  #if LIBRSVG_CHECK_VERSION (2, 48, 0)
    rsvg_handle_set_stylesheet (rsvg_handle, (guint8 *)css, strlen (css), NULL);
  #endif

It is only used for librsvg 2.48 and later.

And for librsvg older than 2.32, it looks like we don't use the
wrapped SVG at all?

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Alan Third]

On Wed, Dec 20, 2023 at 03:19:29PM +0200, Eli Zaretskii wrote:
> > I'm using librsvg 2.57 and I could not easily test it with an older
> > version.
> 
> Alan, is the trick with wrapping SVG with another one supposed to work
> with all versions of librsvg?

Yes.

> I see some version conditions in
> svg_load_image, but I'm not sure I understand the consequences.

It's quite complex because the librsvg API has changed a lot over the
last few years. Generally we try to use as many features of librsvg as
possible for each version. So, for example 2.52 and above allow us to
ask directly the size of the image (the most accurate way), 2.48
requires us to do more calculations and below 2.48 we need to actually
generate the rasterized image and get it's size from that.

> Specifically, what does this code do:
> 
>   #if LIBRSVG_CHECK_VERSION (2, 48, 0)
>     rsvg_handle_set_stylesheet (rsvg_handle, (guint8 *)css, strlen (css), NULL);
>   #endif
> 
> It is only used for librsvg 2.48 and later.

This sets a CSS stylesheet which contains the font and font size. It's
only used on 2.48+ because that's when it was introduced. There is a
previous call to the same function that has more explanation. There
are two calls because we need to set up the SVG, parse it, get it's
size, then wrap it, set up the wrapped version and parse again before
finally rasterizing.

> And for librsvg older than 2.32, it looks like we don't use the
> wrapped SVG at all?

I think we do. The #if/#endif's are hard to follow, but I'm sure the
wrapper is used in all versions.

IIRC the main difference between 2.32 and older versions is the
initial setup of the SVG handle.

I'll see if I can come up with some better comments.

-- 
Alan Third

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

>> Here are the results:
>> 
>> Right after "emacs -Q": rss = 44688
>> After 'M-x +insert-stuff': rss = 54292
>> After first hover on text: rss = 54464
>> After repeated hover on/off text: rss = 54496
>
> Does it get bumped each hover-over, or does it stabilize after a few?

This I don't know.  When I write "repeated hover", it is because I hover
on and off maybe ten times just to see if there was a growing trend.

[...]

>> You're right: it does only work with SVG.  XPM are already handled with
>> mask.  I have tested and it does not work with PNG with transparent
>> background: don't know why.  Maybe instead of guard it with a "only for
>> SVG" test, we should resolve the issue for transparent PNG at a later
>> time.
>
> It would be a shame to waste cycles on PNG images as long as we don't
> solve this problem, so I'd prefer to enable this only for SVG and
> leave a FIXME comment about PNG.

Yes I'm ok with it.  I'll modify the patch.
-- 
Manuel Giraud

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Eli Zaretskii <eliz@gnu.org> writes:

[...]

> It would be a shame to waste cycles on PNG images as long as we don't
> solve this problem, so I'd prefer to enable this only for SVG and
> leave a FIXME comment about PNG.

Here is an updated version of this patch:

     - I had to export 'image_spec_value' from image.c to use it in
       xdisp.c
     - I also guard this feature behind a #ifdef HAVE_RSVG


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Respect-mouse-face-on-SVG-image-glyphs-bug-67794.patch --]
[-- Type: text/x-patch, Size: 2506 bytes --]

From afc1097a0fe0ee483bcdb2cac2045201a3f00a72 Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Tue, 19 Dec 2023 12:25:24 +0100
Subject: [PATCH] Respect mouse-face on SVG image glyphs (bug#67794)

* src/dispextern.h:
* src/image.c (image_spec_value): Export 'image_spec_value'.
* src/xdisp.c (draw_glyphs): Maybe update SVG image glyphs with mouse
face features before drawing.
---
 src/dispextern.h |  1 +
 src/image.c      |  2 +-
 src/xdisp.c      | 20 ++++++++++++++++++++
 3 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/src/dispextern.h b/src/dispextern.h
index 3a4d6095f73..020c33a2628 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -3618,6 +3618,7 @@ #define TRY_WINDOW_IGNORE_FONTS_CHANGE	(1 << 1)
 bool valid_image_p (Lisp_Object);
 void prepare_image_for_display (struct frame *, struct image *);
 ptrdiff_t lookup_image (struct frame *, Lisp_Object, int);
+Lisp_Object image_spec_value (Lisp_Object, Lisp_Object, bool *);
 
 #if defined HAVE_X_WINDOWS || defined USE_CAIRO || defined HAVE_NS \
   || defined HAVE_HAIKU || defined HAVE_ANDROID
diff --git a/src/image.c b/src/image.c
index 38744fc1cce..651ec0b34e5 100644
--- a/src/image.c
+++ b/src/image.c
@@ -1543,7 +1543,7 @@ parse_image_spec (Lisp_Object spec, struct image_keyword *keywords,
    if KEY is not present in SPEC.  Set *FOUND depending on whether KEY
    was found in SPEC.  */
 
-static Lisp_Object
+Lisp_Object
 image_spec_value (Lisp_Object spec, Lisp_Object key, bool *found)
 {
   Lisp_Object tail;
diff --git a/src/xdisp.c b/src/xdisp.c
index 75d769600c4..731ad231058 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -30985,6 +30985,26 @@ draw_glyphs (struct window *w, int x, struct glyph_row *row,
 	  }
     }
 
+#ifdef HAVE_RSVG
+  /* Update SVG image glyphs with mouse face features.  FIXME: it
+     should be possible to have this behaviour with transparent
+     background PNG.  */
+  if (hl == DRAW_MOUSE_FACE)
+    {
+      Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (f);
+      for (s = head; s; s = s->next)
+	if (s->first_glyph->type == IMAGE_GLYPH)
+	  if (s->img
+	      && (EQ (image_spec_value (s->img->spec, QCtype, NULL), Qsvg)))
+	    {
+	      ptrdiff_t id;
+	      id = lookup_image (f, s->img->spec, hlinfo->mouse_face_face_id);
+	      s->img = IMAGE_FROM_ID (f, id);
+	      prepare_image_for_display(f, s->img);
+	    }
+    }
+#endif
+
   /* Draw all strings.  */
   for (s = head; s; s = s->next)
     FRAME_RIF (f)->draw_glyph_string (s);
-- 
2.43.0


[-- Attachment #3: Type: text/plain, Size: 18 bytes --]

-- 
Manuel Giraud

bug#67794: 30.0.50; mouse-face is not respected on SVG images

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: alan@idiocy.org,  gerd.moellmann@gmail.com,  67794@debbugs.gnu.org
> Date: Wed, 20 Dec 2023 18:53:41 +0100
> 
> > It would be a shame to waste cycles on PNG images as long as we don't
> > solve this problem, so I'd prefer to enable this only for SVG and
> > leave a FIXME comment about PNG.
> 
> Here is an updated version of this patch:
> 
>      - I had to export 'image_spec_value' from image.c to use it in
>        xdisp.c
>      - I also guard this feature behind a #ifdef HAVE_RSVG

Thanks, installed on master, and closing the bug.