Menu bar items structure

Menu bar items structure

[Manuel Giraud]

Hi,

I'm still trying to improve the X11 no toolkit menus and for this I end
up having to add an entry for each menu bar items.  It currently has 4:
a key, a name, an item_property_def(?) and a starting char position.

AFAIU, those items are stored as a flat array.  So there are many usage
of this magical "4" number in keyboard.c and *term.c to walk this array.

So I guess my question is: could (should?) it not be done with a proper
struct?  Maybe it is historical or maybe I'm missing something?

Best regards,
-- 
Manuel Giraud

Re: Menu bar items structure

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Date: Wed, 16 Nov 2022 18:30:58 +0100
> 
> AFAIU, those items are stored as a flat array.  So there are many usage
> of this magical "4" number in keyboard.c and *term.c to walk this array.
> 
> So I guess my question is: could (should?) it not be done with a proper
> struct?  Maybe it is historical or maybe I'm missing something?

I'm not sure how you intended to use a C 'struct' in this case.
Menu-bar items is a Lisp vector, so how do you replace it with a C
struct and still allow Lisp to populate a menu?

Re: Menu bar items structure

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Manuel Giraud <manuel@ledu-giraud.fr>
>> Date: Wed, 16 Nov 2022 18:30:58 +0100
>> 
>> AFAIU, those items are stored as a flat array.  So there are many usage
>> of this magical "4" number in keyboard.c and *term.c to walk this array.
>> 
>> So I guess my question is: could (should?) it not be done with a proper
>> struct?  Maybe it is historical or maybe I'm missing something?
>
> I'm not sure how you intended to use a C 'struct' in this case.
> Menu-bar items is a Lisp vector, so how do you replace it with a C
> struct and still allow Lisp to populate a menu?

I suggest not trying to change the internal menu representation.

As it is, it allows easily creating very deeply nested menus, and
garbage collection of menu items "just works".

When working on the Haiku port, I tried to represent the menu bar
contents in some more intuitive format, which led to garbage collection
issues and C stack overflows with somewhat deeply nested menus.

Re: Menu bar items structure

[Manuel Giraud]

Po Lu <luangruo@yahoo.com> writes:

[...]

> I suggest not trying to change the internal menu representation.
>
> As it is, it allows easily creating very deeply nested menus, and
> garbage collection of menu items "just works".
>
> When working on the Haiku port, I tried to represent the menu bar
> contents in some more intuitive format, which led to garbage collection
> issues and C stack overflows with somewhat deeply nested menus.

That is unfortunate.  I was not trying to change the internal menu
representation for the sake of change but I have a problem with the no
toolkit menu bar.

The 4th element of a menu item is a char position in the menu bar for
this menu entry (filled with xdisp.c/display_menu_bar).  This char
position is then used to identify if mouse click is on this menu entry
(around line 6000 of keyboard.c)… but that does not work well if you
have changed your menu face font (eg. with (set-face-font 'menu
"AnotherFont-20")).  This seems to be because pixel_to_glyph_coords will
then be off.

To overcome this issue my idea was two fold:

      1- change the semantics of this index from a char position to a
         pixel position
         
      2- add a fifth element to a menu item that records the end
         position of a menu entry (also in pixel and filled in
         display_menu_bar)

I have a patch that does just that and it seems to work for me™… but I
see that it will also impact every other *term (on system I cannot even
test).  This led me to this kind of "refactoring" question.  Any ideas?
-- 
Manuel Giraud

Re: Menu bar items structure

[Po Lu]

Manuel Giraud <manuel@ledu-giraud.fr> writes:

> That is unfortunate.  I was not trying to change the internal menu
> representation for the sake of change but I have a problem with the no
> toolkit menu bar.
>
> The 4th element of a menu item is a char position in the menu bar for
> this menu entry (filled with xdisp.c/display_menu_bar).  This char
> position is then used to identify if mouse click is on this menu entry
> (around line 6000 of keyboard.c)… but that does not work well if you
> have changed your menu face font (eg. with (set-face-font 'menu
> "AnotherFont-20")).  This seems to be because pixel_to_glyph_coords will
> then be off.
>
> To overcome this issue my idea was two fold:
>
>       1- change the semantics of this index from a char position to a
>          pixel position
>          
>       2- add a fifth element to a menu item that records the end
>          position of a menu entry (also in pixel and filled in
>          display_menu_bar)
>
> I have a patch that does just that and it seems to work for me™… but I
> see that it will also impact every other *term (on system I cannot even
> test).  This led me to this kind of "refactoring" question.  Any ideas?

Why use pixel_to_glyph_coords?  Why not use x_y_to_hpos_vpos on the menu
bar pseudo-window?

Re: Menu bar items structure

[Manuel Giraud]

Po Lu <luangruo@yahoo.com> writes:

[...]

> Why use pixel_to_glyph_coords?  Why not use x_y_to_hpos_vpos on the menu
> bar pseudo-window?

It's not me that is using pixel_to_glyph_coords.  It is line 5977 in
keyboard.c.  Do you suggest I use x_y_to_hpos_vpos here instead?
-- 
Manuel Giraud

Re: Menu bar items structure

[Po Lu]

Manuel Giraud <manuel@ledu-giraud.fr> writes:

> It's not me that is using pixel_to_glyph_coords.  It is line 5977 in
> keyboard.c.  Do you suggest I use x_y_to_hpos_vpos here instead?

Yes.  But at the same time, I think we might as well fix the code to
stop assuming that each character in the menu bar item string
corresponds to a single glyph, so the changes that have to be done are
actually much more subtle.

Re: Menu bar items structure

[Po Lu]

Po Lu <luangruo@yahoo.com> writes:

> Yes.  But at the same time, I think we might as well fix the code to
> stop assuming that each character in the menu bar item string
> corresponds to a single glyph, so the changes that have to be done are
> actually much more subtle.

Scratch the last part.  I think that problem is already solved by this
piece of display_menu_bar:

      /* Remember where item was displayed.  */
      ASET (items, i + 3, make_fixnum (it.hpos));

      /* Display the item, pad with one space.  */
      if (it.current_x < it.last_visible_x)
	display_string (NULL, string, Qnil, 0, 0, &it,
			SCHARS (string) + 1, 0, 0, STRING_MULTIBYTE (string));

Re: Menu bar items structure

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Thu, 17 Nov 2022 10:57:31 +0100
> 
> The 4th element of a menu item is a char position in the menu bar for
> this menu entry (filled with xdisp.c/display_menu_bar).  This char
> position is then used to identify if mouse click is on this menu entry
> (around line 6000 of keyboard.c)… but that does not work well if you
> have changed your menu face font (eg. with (set-face-font 'menu
> "AnotherFont-20")).  This seems to be because pixel_to_glyph_coords will
> then be off.
> 
> To overcome this issue my idea was two fold:
> 
>       1- change the semantics of this index from a char position to a
>          pixel position
>          
>       2- add a fifth element to a menu item that records the end
>          position of a menu entry (also in pixel and filled in
>          display_menu_bar)

The first alternative is better, IMO: on TTY frames each character is
1 pixel, so the change doesn't affect TTY menus.

Re: Menu bar items structure

[Manuel Giraud]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

>> To overcome this issue my idea was two fold:
>> 
>>       1- change the semantics of this index from a char position to a
>>          pixel position
>>          
>>       2- add a fifth element to a menu item that records the end
>>          position of a menu entry (also in pixel and filled in
>>          display_menu_bar)
>
> The first alternative is better, IMO: on TTY frames each character is
> 1 pixel, so the change doesn't affect TTY menus.

I was not clear.  My patch does the two points.
-- 
Manuel Giraud

Re: Menu bar items structure

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: Manuel Giraud <manuel@ledu-giraud.fr>,  luangruo@yahoo.com,
>   emacs-devel@gnu.org
> Date: Thu, 17 Nov 2022 16:06:22 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> [...]
> 
> >> To overcome this issue my idea was two fold:
> >> 
> >>       1- change the semantics of this index from a char position to a
> >>          pixel position
> >>          
> >>       2- add a fifth element to a menu item that records the end
> >>          position of a menu entry (also in pixel and filled in
> >>          display_menu_bar)
> >
> > The first alternative is better, IMO: on TTY frames each character is
> > 1 pixel, so the change doesn't affect TTY menus.
> 
> I was not clear.  My patch does the two points.

Why do you need the second one?

Re: Menu bar items structure

[Manuel Giraud]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

>> >>       2- add a fifth element to a menu item that records the end
>> >>          position of a menu entry (also in pixel and filled in
>> >>          display_menu_bar)
>> >
>> > The first alternative is better, IMO: on TTY frames each character is
>> > 1 pixel, so the change doesn't affect TTY menus.
>> 
>> I was not clear.  My patch does the two points.
>
> Why do you need the second one?

It seems more natural because in display_menu_bar I have all the
information I needed handy (as we have just filled the menu bar).  But
anyway, I now going to ignore this second index and use x_y_to_hpos_vpos
as Po Lu suggested.  Thanks.
-- 
Manuel Giraud

Re: Menu bar items structure

[Manuel Giraud]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

> I'm not sure how you intended to use a C 'struct' in this case.
> Menu-bar items is a Lisp vector, so how do you replace it with a C
> struct and still allow Lisp to populate a menu?

Ok and this is what I was missing 😅
-- 
Manuel Giraud