bug#58406: 29.0.50; Bars refactoring?

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Date: Mon, 10 Oct 2022 09:37:43 +0200
> 
> I'm trying to have some kind of highlight feature on the no-toolkit menu
> bar.  As this feature is already present in tool/tab bar, I'd like to
> mimic those.
> 
> While trying this, I think that I found that some "bar" functions could
> be factorized (up to a certain point I guess).  Here is a patch that
> shows my direction.
> 
> Do you think that this approach could make it into master or would it be
> frown upon?  And if so, what would be a better way?

The approach is OK, and welcome.  But let's please wait with this
refactoring until after the emacs-29 branch is cut.  OK?

Some minor comments below:

> +  /* Get the text property `menu-item' at pos. The value of that
> +     property is the start index of this item's properties in
> +     F->tool_bar_items.  */

The "F->tool_bar_items" part of the comment is outdated.

Also, our style is to leave two spaces between sentences in comments
and documentation.

> +  /* Get the start of this tool-bar item's properties in
> +     f->tool_bar_items.  */

Likewise here: outdated reference to tool_bar_items.

> +  /* Is mouse on the highlighted item?  */
> +  if (bar == TAB_BAR)
> +    return *prop_idx == f->last_tab_bar_item ? 0 : 1;
> +  else if (EQ (window, hlinfo->mouse_face_window)
> +      && *vpos >= hlinfo->mouse_face_beg_row
> +      && *vpos <= hlinfo->mouse_face_end_row
> +      && (*vpos > hlinfo->mouse_face_beg_row
> +       || *hpos >= hlinfo->mouse_face_beg_col)
> +      && (*vpos < hlinfo->mouse_face_end_row
> +       || *hpos < hlinfo->mouse_face_end_col
> +       || hlinfo->mouse_face_past_end))
> +    return 0;

The "else if" clause should only be used for the tool bar, not for the
menu bar, AFAIU.

> @@ -15485,6 +15451,7 @@ handle_tool_bar_click_with_device (struct frame *f, 
> int x, int y, bool down_p,
>    Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (f);
>    struct window *w = XWINDOW (f->tool_bar_window);
>    int hpos, vpos, prop_idx;
> +  bool close_p;

The value of this is ignored here, so it is better to call this
variable 'ignored' or 'dummy' or something to that effect.