bug#43397: 28.0.50; Adding tool bar items: update tool bar

bug#43397: 28.0.50; Adding tool bar items: update tool bar

[Caio Henrique]

Hi all,

I don't know if this is a bug or the expected behavior of the function
tool-bar-add-item-from-menu: 

1. emacs -Q
2. paste and eval this:
(progn
  (tool-bar-add-item-from-menu 'undo-redo
			       "redo" nil :vert-only t)
  (redraw-display)
  (force-mode-line-update))

I'm using both redraw-display and force-mode-line-update to try to force
the tool-bar to draw the icon (I know that I should'nt do this, I'm just
trying to figure if this is a bug); but the icon only appears when I
click one or two times with the mouse anywhere in the buffer. 

If there is a bug, this is possibly related to it:
https://lists.gnu.org/archive/html/emacs-devel/2020-09/msg00603.html
https://lists.gnu.org/archive/html/emacs-devel/2020-09/msg00726.html
https://lists.gnu.org/archive/html/emacs-devel/2020-09/msg00780.html

And also this:
https://lists.gnu.org/archive/html/emacs-devel/2020-09/msg01089.html
https://lists.gnu.org/archive/html/emacs-devel/2020-09/msg01101.html
> Eli: Here, the above displays nothing at all on the tool bar.
(If I click with the mouse anywhere in the buffer, that icon also gets
displayed for me) 

Thanks!

____
In GNU Emacs 28.0.50 (build 19, x86_64-pc-linux-gnu, GTK+ Version
3.24.13, cairo version 1.16.0)

Configured features:
XPM JPEG TIFF GIF PNG CAIRO SOUND DBUS GSETTINGS GLIB NOTIFY INOTIFY
LIBSELINUX GNUTLS LIBXML2 FREETYPE HARFBUZZ ZLIB TOOLKIT_SCROLL_BARS
GTK3 X11 XDBE XIM MODULES THREADS JSON PDUMPER

bug#43397: 28.0.50; Adding tool bar items: update tool bar

[Caio Henrique]

Gnus has a function called gnus-tool-bar-update. I tried using it but
got the same result, i.e. the icon is only displayed when I click with
the mouse on the buffer. 

1. emacs -Q
2. eval this:
(progn
  (defvar tool-bar-mode)

  (defun gnus-tool-bar-update (&rest ignore)
    "Update the tool bar."
    (when (and (boundp 'tool-bar-mode)
	       tool-bar-mode)
      (let* ((args nil)
	     (func (cond ((fboundp 'tool-bar-update)
			  'tool-bar-update)
			 ((fboundp 'force-window-update)
			  'force-window-update)
			 ((fboundp 'redraw-frame)
			  (setq args (list (selected-frame)))
			  'redraw-frame)
			 (t 'ignore))))
	(apply func args))))

  (tool-bar-add-item-from-menu 'undo-redo
			       "redo" nil :vert-only t)

  (gnus-tool-bar-update))

bug#43397: 28.0.50; Adding tool bar items: update tool bar

[Eli Zaretskii]

> From: Caio Henrique <caiohcs0@gmail.com>
> Date: Mon, 14 Sep 2020 11:30:22 -0300
> 
> 1. emacs -Q
> 2. paste and eval this:
> (progn
>   (tool-bar-add-item-from-menu 'undo-redo
> 			       "redo" nil :vert-only t)
>   (redraw-display)
>   (force-mode-line-update))
> 
> I'm using both redraw-display and force-mode-line-update to try to force
> the tool-bar to draw the icon (I know that I should'nt do this, I'm just
> trying to figure if this is a bug)

(Calling force-mode-line-update won't help, because
tool-bar-add-item-from-menu does it internally.)

> but the icon only appears when I
> click one or two times with the mouse anywhere in the buffer. 

I've now looked into this, and I'm quite sure it is not a redisplay
bug.  The display engine faithfully inspects the tool-bar items each
time it is invoked after the above code runs, and each time it finds
that the tool-bar items haven't changed -- until they do.

Based on what I see, and on the modified recipe below, it looks like
we stick to the old value of the tool-bar items, like if we cached
them somewhere.  Since I don't understand where is that "cache", I
don't really have a clear idea of what triggers the flushing of that
"cache", but one trigger I found is -- surprise! -- GC.  To see this,
perform the following greatly simplified recipe:

  emacs -Q
  M-x blink-cursor-mode RET
  M-x global-eldoc-mode RET

(The last two commands are to make sure there are no redisplay cycles
except due to changes in buffers or strings.)

Then evaluate:

  (defun myfun ()
    (interactive)
    (tool-bar-add-item "redo" 'undo-redo 'undo-redo)
    (garbage-collect))

  (global-set-key [f5] 'myfun)

Finally, press F5: you should see the "redo" icon appear immediately.

Now repeat the same, in a fresh Emacs session, but this time remove
the call to garbage-collect from myfun, and instead do this before
evaluating the function and the global-set-key form:

  M-x set-variable RET garbage-collection-messages RET t RET

Then evaluate the forms and press F5.  The tool bar won't change.  Now
do some random clicks, watching the echo area: you will see that the
tool bar is updated with the "redo" icon precisely when the "Garbage
collecting..." message appears in the echo area.

Maybe Stefan (CC'ed) can help us understand why this happens and how
GC is involved in this...

bug#43397: 28.0.50; Adding tool bar items: update tool bar

[Stefan Monnier]

> Maybe Stefan (CC'ed) can help us understand why this happens and how
> GC is involved in this...

I'm afraid I don't know much more than you do here.
`compact_font_caches` maybe?


        Stefan

bug#43397: 28.0.50; Adding tool bar items: update tool bar

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Caio Henrique <caiohcs0@gmail.com>,  43397@debbugs.gnu.org
> Date: Tue, 15 Sep 2020 16:05:53 -0400
> 
> > Maybe Stefan (CC'ed) can help us understand why this happens and how
> > GC is involved in this...
> 
> I'm afraid I don't know much more than you do here.

We are basically talking about modifying a keymap, since this is what
tool-bar-add-item and friends do.  The question is: why isn't the
modified keymap immediately visible? why does the old keymap continue
to be visible until the next GC?

> `compact_font_caches` maybe?

I will look there, thanks.

bug#43397: 28.0.50; Adding tool bar items: update tool bar

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Then evaluate the forms and press F5.  The tool bar won't change.

This was a year ago, but I can still reproduce this on the current
trunk.  Here's an even easier variation of the recipe:

(progn
  (blink-cursor-mode -1)
  (global-eldoc-mode -1)

  (defun myfun ()
    (interactive)
    (tool-bar-add-item "redo" 'undo-redo 'undo-redo))

  (global-set-key [f5] 'myfun))

And then M-: (garbage-collect) makes the tool bar update.

However!  The garbage-collect in itself isn't sufficient to trigger it.
With this variation:

(progn
  (blink-cursor-mode -1)
  (global-eldoc-mode -1)
  (tool-bar-add-item "redo" 'undo-redo 'undo-redo)
  (global-set-key [f5] (lambda () (interactive) (garbage-collect))))

pressing <f5> does not make the icon appear.

> Now do some random clicks, watching the echo area: you will see that
> the tool bar is updated with the "redo" icon precisely when the
> "Garbage collecting..." message appears in the echo area.

Very mysterious.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43397: 28.0.50; Adding tool bar items: update tool bar

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

Lars Ingebrigtsen [2021-08-27 19:40:13] wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
>> Then evaluate the forms and press F5.  The tool bar won't change.
>
> This was a year ago, but I can still reproduce this on the current
> trunk.  Here's an even easier variation of the recipe:
>
> (progn
>   (blink-cursor-mode -1)
>   (global-eldoc-mode -1)
>
>   (defun myfun ()
>     (interactive)
>     (tool-bar-add-item "redo" 'undo-redo 'undo-redo))
>
>   (global-set-key [f5] 'myfun))
>
> And then M-: (garbage-collect) makes the tool bar update.
> However!  The garbage-collect in itself isn't sufficient to trigger it.

Of course, it's the use of the minibuffer (as part of `M-:`) which
triggered it.


        Stefan

bug#43397: 28.0.50; Adding tool bar items: update tool bar

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Of course, it's the use of the minibuffer (as part of `M-:`) which
> triggered it.

No, I'm able to trigger the appearance of the icon by just `C-x C-e'-ing
after:

(garbage-collect)

`C-x C-e' after 

(progn
  (garbage-collect)
  nil)

does not trigger it.

This triggers it:

(progn
  (garbage-collect)
  (message "Foo\nfoo\nbar")
  nil)

This does not:

(progn
  (message "Foo\nfoo\nbar")
  nil)

So: We have to have a garbage-collect, and then we have to have
something that changes the size of the minibuffer area (which probably
triggers a more complete repaint)...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43397: 28.0.50; Adding tool bar items: update tool bar

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> This triggers it:
>
> (progn
>   (garbage-collect)
>   (message "Foo\nfoo\nbar")
>   nil)
>
> This does not:
>
> (progn
>   (message "Foo\nfoo\nbar")
>   nil)
>
> So: We have to have a garbage-collect, and then we have to have
> something that changes the size of the minibuffer area (which probably
> triggers a more complete repaint)...

This is still the case in Emacs 29, and I still don't understand why.

Calling flush_frame doesn't help either, or calling

	      fset_redisplay (f);
	      redisplay_internal ();

etc.  So something in tool bar seems to be hanging on to the old shapes
until...  something...  triggers a special kind of redraw.  Anybody got
any ideas?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43397: 28.0.50; Adding tool bar items: update tool bar

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

>> This triggers it:
>>
>> (progn
>>   (garbage-collect)
>>   (message "Foo\nfoo\nbar")
>>   nil)
>>
>> This does not:
>>
>> (progn
>>   (message "Foo\nfoo\nbar")
>>   nil)

My crystal ball suggests there's a weak hash-table in here somewhere.


        Stefan

bug#43397: 28.0.50; Adding tool bar items: update tool bar

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> My crystal ball suggests there's a weak hash-table in here somewhere.

That's a really good crystal ball:

(defconst tool-bar-keymap-cache (make-hash-table :weakness t :test 'equal))

If I remove the :weakness t there, then the toolbar is never updated.

So it's a caching problem, one of the two difficult problems in
programming.

Hm...  poking around in that code, I don't quite follow how this is all
supposed to be hooked up, or which caches should be flushed.  Does the
crystal ball have more insights?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43397: 28.0.50; Adding tool bar items: update tool bar

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Hm...  poking around in that code, I don't quite follow how this is all
> supposed to be hooked up, or which caches should be flushed.  Does the
> crystal ball have more insights?

Well, actually, that's not really material.  We just have to flush the
cache after adding elements, so I've now done that in Emacs 29, and that
makes the tool bar item show up immediately.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43397: 28.0.50; Adding tool bar items: update tool bar

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

> Hm...  poking around in that code, I don't quite follow how this is all
> supposed to be hooked up, or which caches should be flushed.  Does the
> crystal ball have more insights?

This cache is broken:

    (defun tool-bar-make-keymap (&optional _ignore)
      "Generate an actual keymap from `tool-bar-map'.
    Its main job is to figure out which images to use based on the display's
    color capability and based on the available image libraries."
      (let ((key (cons (frame-terminal) tool-bar-map)))
        (or (gethash key tool-bar-keymap-cache)
            (puthash key (tool-bar-make-keymap-1) tool-bar-keymap-cache))))

`tool-bar-map` is a normal keymap, which we modify in the usual way,
i.e. via side-effect.  So the key we place in this `equal` hash table
will be routinely modified via side-effect, thus changing its sxhash.

Maybe we'd be better off using an `eq` hash table and manually flushing
the corresponding entry whenever `tool-bar-map` is modified by
side-effect.

I also see that we use a `:weakness t` but the values stored there will
usually not be stored anywhere else, so the hash table will be
completely flushed at every GC (and partly refilled soon after as part
of redisplay).  It should have `:weakness 'key` instead (but without
fixing the current bug report, this will cause the keymap to never be
refreshed until we manually flush the hash table ;-).


        Stefan

bug#43397: 28.0.50; Adding tool bar items: update tool bar

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> `tool-bar-map` is a normal keymap, which we modify in the usual way,
> i.e. via side-effect.  So the key we place in this `equal` hash table
> will be routinely modified via side-effect, thus changing its sxhash.

Hm...  I assumed that that was the point, really, but that it didn't
work for...  reasons...  I.e., whenever somebody modifies the map, the
cache is supposed to be refreshed.  I don't understand why that didn't
work, but it doesn't.

> Maybe we'd be better off using an `eq` hash table and manually flushing
> the corresponding entry whenever `tool-bar-map` is modified by
> side-effect.

Yup.  Should be faster, too.

> I also see that we use a `:weakness t` but the values stored there will
> usually not be stored anywhere else, so the hash table will be
> completely flushed at every GC (and partly refilled soon after as part
> of redisplay).

Yeah, that's true, too.  But if we have flushing, then we can probably
make it non-weak, because we'll get rid up the values that way.

> It should have `:weakness 'key` instead (but without
> fixing the current bug report, this will cause the keymap to never be
> refreshed until we manually flush the hash table ;-).

I added flushing in one of the interface functions -- I didn't know
whether any needed it, but they probably do.  

I'll poke around a bit.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43397: 28.0.50; Adding tool bar items: update tool bar

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I added flushing in one of the interface functions -- I didn't know
> whether any needed it, but they probably do.  

Now done -- seems to work fine.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43397: 28.0.50; Adding tool bar items: update tool bar

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 43397@debbugs.gnu.org,  Eli Zaretskii <eliz@gnu.org>,  Caio Henrique
>  <caiohcs0@gmail.com>
> Date: Tue, 03 May 2022 18:19:57 +0200
> 
> Lars Ingebrigtsen <larsi@gnus.org> writes:
> 
> > Hm...  poking around in that code, I don't quite follow how this is all
> > supposed to be hooked up, or which caches should be flushed.  Does the
> > crystal ball have more insights?
> 
> Well, actually, that's not really material.  We just have to flush the
> cache after adding elements, so I've now done that in Emacs 29, and that
> makes the tool bar item show up immediately.

With what recipe?  If I evaluate the below by "C-x C-e" after the
right parenthesis:

  (tool-bar-add-item-from-menu 'undo-redo "redo" nil :vert-only t)

it doesn't show up immediately here, I need at least to type "M-x",
and sometimes also C-g or something else.

bug#43397: 28.0.50; Adding tool bar items: update tool bar

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> With what recipe?  If I evaluate the below by "C-x C-e" after the
> right parenthesis:
>
>   (tool-bar-add-item-from-menu 'undo-redo "redo" nil :vert-only t)
>
> it doesn't show up immediately here, I need at least to type "M-x",
> and sometimes also C-g or something else.

Is this before or after the latest tool-bar.el changes?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43397: 28.0.50; Adding tool bar items: update tool bar

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 43397@debbugs.gnu.org,  Eli Zaretskii <eliz@gnu.org>,  Caio Henrique
>  <caiohcs0@gmail.com>
> Date: Tue, 03 May 2022 18:46:41 +0200
> 
> Lars Ingebrigtsen <larsi@gnus.org> writes:
> 
> > I added flushing in one of the interface functions -- I didn't know
> > whether any needed it, but they probably do.  
> 
> Now done -- seems to work fine.

Works here too, thanks.

I just hope we didn't make redisplay slower by flushing the tool bar
too frequently...

bug#43397: 28.0.50; Adding tool bar items: update tool bar

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I just hope we didn't make redisplay slower by flushing the tool bar
> too frequently...

I think it should actually be faster -- the hash table is an eql one now
instead of equal, so we don't have to hash the entire contents.  And gc
no longer clears out the data, which would then have to be recreated.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43397: 28.0.50; Adding tool bar items: update tool bar

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

Lars Ingebrigtsen [2022-05-03 18:39:51] wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> `tool-bar-map` is a normal keymap, which we modify in the usual way,
>> i.e. via side-effect.  So the key we place in this `equal` hash table
>> will be routinely modified via side-effect, thus changing its sxhash.
> Hm...  I assumed that that was the point, really, but that it didn't
> work for...  reasons...  I.e., whenever somebody modifies the map, the
> cache is supposed to be refreshed.  I don't understand why that didn't
> work, but it doesn't.

Since the key is stored as-is in the hash table, modifying the key and
looking it up again should find it its hash value is unchanged, and the
`equal` test will return t since both the in-hashtable key and the
lookup-key are one and the same object.

In any case side effects on a key's stored in an `equal` hash table lead
in general to the hash table having an inconsistent internal state, so
they should be avoided as much as possible.

[ Just like using `aset` on the `symbol-name` of an interned symbol,
  for the same reason.  ]


        Stefan

bug#43397: 28.0.50; Adding tool bar items: update tool bar

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Since the key is stored as-is in the hash table, modifying the key and
> looking it up again should find it its hash value is unchanged, and the
> `equal` test will return t since both the in-hashtable key and the
> lookup-key are one and the same object.

D'oh.  I completely blanked about that.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no