bug#64440: 30.0.50; [PATCH] Highlight on non toolkit menu bar items

[Manuel Giraud]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Manuel Giraud <manuel@ledu-giraud.fr>
>> Cc: Po Lu <luangruo@yahoo.com>,  stefankangas@gmail.com,  
>> 64440@debbugs.gnu.org
>> Date: Tue, 05 Sep 2023 11:53:38 +0200
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > I'd appreciate a walkthrough of the patches with explanations for the
>> > significant hunks.  It's a non-trivial change, so I think the
>> > rationale and the main ideas of the implementation should be described
>> > and discussed.
>> 
>> Hi,
>> 
>> For this walkthrough, I have inserted the 4 patches, made some elisions
>> and comment inline.
>
> Thanks.
>
>> +  /* Convert to pixels bounds.  */
>> +  row = MATRIX_ROW (w->current_matrix, *vpos);
>> +  *x_start = 0;
>> +  for (i = 0; i < *h_start; ++i)
>> +    *x_start += row->glyphs[TEXT_AREA][i].pixel_width;
>> +
>> +  *x_end = *x_start;
>> +  for (i = *h_start; i < *h_end; ++i)
>> +    *x_end += row->glyphs[TEXT_AREA][i].pixel_width;
>> 
>> Here, I convert those limits from chars to pixels.
>
> What does this glyph_row look like?  Does it include several strings
> one after the other or something?
>
> In general, we always test the index of a glyph in a glyph row against
> glyphs[TEXT_AREA].used, and I'm worried that these tests are absent
> from the code above.  Which is why I'm asking for more details about
> the arrangement of the menu text in these glyph rows.

Hi,

Here's what a "p *row" returns right after the "row = MATRIX_ROW
(w->current_matrix, *vpos);" line:

(gdb) p *row
$9 = {
  glyphs = {0xef4a19dd000, 0xef4a19dd000, 0xef4a19e0510, 0xef4a19e0510},
  used = {0, 120, 0, 0},
  hash = 221095522,
  x = 0,
  y = 0,
  pixel_width = 840,
  ascent = 11,
  height = 13,
  phys_ascent = 10,
  phys_height = 12,
  visible_height = 13,
  extra_line_spacing = 0,
  start = {
    pos = {
      charpos = 0,
      bytepos = 0
    },
    overlay_string_index = 0,
    string_pos = {
      charpos = 0,
      bytepos = 0
    },
    dpvec_index = 0
  },
  end = {
    pos = {
      charpos = 0,
      bytepos = 0
    },
    overlay_string_index = 0,
    string_pos = {
      charpos = 0,
      bytepos = 0
    },
    dpvec_index = 0
  },
  minpos = {
    charpos = 0,
    bytepos = 0
  },
  maxpos = {
    charpos = 0,
    bytepos = 0
  },
  overlay_arrow_bitmap = 0,
  left_user_fringe_bitmap = 0,
  right_user_fringe_bitmap = 0,
  left_fringe_bitmap = 0,
  right_fringe_bitmap = 0,
  left_user_fringe_face_id = 0,
  right_user_fringe_face_id = 0,
  left_fringe_face_id = 0,
  right_fringe_face_id = 0,
  left_fringe_offset = 0,
  right_fringe_offset = 0,
  fringe_bitmap_periodic_p = false,
  redraw_fringe_bitmaps_p = false,
  enabled_p = true,
  truncated_on_left_p = false,
  truncated_on_right_p = false,
  continued_p = false,
  displays_text_p = false,
  ends_at_zv_p = false,
  fill_line_p = false,
  indicate_empty_line_p = false,
  contains_overlapping_glyphs_p = false,
  full_width_p = true,
  mode_line_p = false,
  tab_line_p = false,
  overlapped_p = false,
  ends_in_middle_of_char_p = false,
  starts_in_middle_of_char_p = false,
  overlapping_p = false,
  mouse_face_p = false,
  ends_in_newline_from_string_p = false,
  exact_window_width_line_p = false,
  cursor_in_fringe_p = false,
  ends_in_ellipsis_p = false,
  indicate_bob_p = false,
  indicate_top_line_p = false,
  indicate_eob_p = false,
  indicate_bottom_line_p = false,
  reversed_p = false,
  stipple_p = false,
  continuation_lines_width = 0,
  clip = 0x0
}

Is that what you are looking for?

>> +/* Possibly highlight a menu-bar item on frame F when mouse moves to
>> +   menu-bar window-relative coordinates X/Y.  Called from
>> +   note_mouse_highlight.  */
>> +
>> +static void
>> +note_menu_bar_highlight (struct frame *f, int x, int y)
>> +{
>
> Is this function under a suitable #if condition?  AFAIU, it is only
> appropriate in a build with X but without any toolkit, so it shouldn't
> be compiled in other configurations.

Yes, you are right.  I'll had the #ifdef proposed by Po.

>> --- a/src/xdisp.c
>> +++ b/src/xdisp.c
>> @@ -33878,6 +33878,9 @@ show_mouse_face (Mouse_HLInfo *hlinfo, enum 
>> draw_glyphs_face draw)
>>    if (FRAME_WINDOW_P (f) && NILP (track_mouse))
>>      {
>>        if (draw == DRAW_NORMAL_TEXT
>> +#ifndef HAVE_EXT_MENU_BAR
>> +      && !EQ (hlinfo->mouse_face_window, f->menu_bar_window)
>> +#endif
>
> Won't this cpp conditional be true in a build --without-x?  Should it?

Same.
-- 
Manuel Giraud