bug#56538: 29.0.50; [PATCH] Colored highlight in Lucid backend

[Po Lu]

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

> ++++
> +** New X resources: "highlightForeground" and "highlightBackground"
> +This controls colors used for highlighted menu item.

This should say which widget it applies to, and that it only applies
under the Lucid build.

> +  {XmNtopHighlightShadowColor, XmCTopHighlightShadowColor, XtRPixel,
> +   sizeof (Pixel), offset (menu.top_highlight_shadow_color),
> +   XtRImmediate, (XtPointer)-1},
> +  {XmNbottomHighlightShadowColor, XmCBottomHighlightShadowColor, XtRPixel,
> +   sizeof (Pixel), offset (menu.bottom_highlight_shadow_color),
> +   XtRImmediate, (XtPointer)-1},
> +  {XmNtopHighlightShadowPixmap, XmCTopHighlightShadowPixmap, XtRPixmap,
> +   sizeof (Pixmap), offset (menu.top_highlight_shadow_pixmap),XtRImmediate,
> +   (XtPointer)None},
> +  {XmNbottomHighlightShadowPixmap, XmCBottomHighlightShadowPixmap, XtRPixmap,
> +   sizeof (Pixmap), offset 
> (menu.bottom_highlight_shadow_pixmap),XtRImmediate,
> +   (XtPointer)None},

Please fix the coding style here so that there is at least a space
between casts and what is being casted.

I know the rest of xlwmenu.c doesn't follow that coding style strictly,
but there is no excuse to add even more incorrectly formatted code.

> +      /* XXX the following permutation is on purpose */

This comment seems redundant.

> +/*
> + * Generic draw shadow rectangle function.  It is used to draw menus, menu 
> items
> + * and also toggle buttons.  When ERASE_P is true, it clears shadows.  
> DOWN_P is
> + * true when a menu item is pushed or a button toggled.  TOP_GC and BOTTOM_GC
> + * are the graphic contexts used to draw the top and bottom shadow 
> respectively.
> + */

Please fix and re-fill the comment.  We write comments like this:

  /* Take BAR and BAZ, and call foo_1 if appropriate.
     Otherwise, return false.  */

> +  xgcv.foreground = mw->menu.highlight_foreground;
> +  xgcv.background = (mw->menu.highlight_background == -1) ?
> +    mw->core.background_pixel : mw->menu.highlight_background;
> +  mw->menu.highlight_foreground_gc = XtGetGC ((Widget)mw, mask, &xgcv);
> +
> +  xgcv.foreground = (mw->menu.highlight_background == -1) ?
> +    mw->core.background_pixel : mw->menu.highlight_background;
> +  xgcv.background = mw->menu.foreground;
> +  mw->menu.highlight_background_gc = XtGetGC ((Widget)mw, mask, &xgcv);

What I said about coding style also applies here.  Also, don't write:

  abc = foo_1_2 () ?
    mw->core.background_pixel : foo_1 ();

but write:

  abc = (foo_1_2 ()
         ? mw->core.background_pixel
         : foo_1 ());

>  static void
> @@ -1724,12 +1847,16 @@ release_drawing_gcs (XlwMenuWidget mw)
>    XtReleaseGC ((Widget) mw, mw->menu.disabled_gc);
>    XtReleaseGC ((Widget) mw, mw->menu.inactive_button_gc);
>    XtReleaseGC ((Widget) mw, mw->menu.background_gc);
> +  XtReleaseGC ((Widget) mw, mw->menu.highlight_foreground_gc);
> +  XtReleaseGC ((Widget) mw, mw->menu.highlight_background_gc);
>    /* let's get some segvs if we try to use these... */
>    mw->menu.foreground_gc = (GC) -1;
>    mw->menu.button_gc = (GC) -1;
>    mw->menu.disabled_gc = (GC) -1;
>    mw->menu.inactive_button_gc = (GC) -1;
>    mw->menu.background_gc = (GC) -1;
> +  mw->menu.highlight_foreground_gc = (GC) -1;
> +  mw->menu.highlight_background_gc = (GC) -1;
>  }
>  
>  #ifndef emacs
> @@ -1738,29 +1865,34 @@ #define MINL(x,y) ((((unsigned long) (x)) < 
> ((unsigned long) (y))) \
>  #endif
>  
>  static void
> -make_shadow_gcs (XlwMenuWidget mw)
> +compute_shadow_colors(XlwMenuWidget mw,
> +                   Pixel *top_color,
> +                   Pixel *bottom_color,
> +                   Boolean *free_top_p,
> +                   Boolean *free_bottom_p,
> +                   Pixmap *top_pixmap,
> +                   Pixmap *bottom_pixmap,
> +                   Pixel fore_color,
> +                   Pixel back_color)

There should be a space after "compute_shadow_colors".

> +static void
> +make_shadow_gcs (XlwMenuWidget mw)
> +{
> +  XGCValues xgcv;
> +  unsigned long pm = 0;
> +
> +  /* Normal shadows */
> +  compute_shadow_colors(mw,
> +                     &(mw->menu.top_shadow_color),
> +                     &(mw->menu.bottom_shadow_color),
> +                     &(mw->menu.free_top_shadow_color_p),
> +                     &(mw->menu.free_bottom_shadow_color_p),
> +                     &(mw->menu.top_shadow_pixmap),
> +                     &(mw->menu.bottom_shadow_pixmap),
> +                     mw->menu.foreground,
> +                     mw->core.background_pixel);
> +
> +  /* Highlight shadows */
> +  compute_shadow_colors(mw,
> +                     &(mw->menu.top_highlight_shadow_color),
> +                     &(mw->menu.bottom_highlight_shadow_color),
> +                     &(mw->menu.free_top_highlight_shadow_color_p),
> +                     &(mw->menu.free_bottom_highlight_shadow_color_p),
> +                     &(mw->menu.top_highlight_shadow_pixmap),
> +                     &(mw->menu.bottom_highlight_shadow_pixmap),
> +                     mw->menu.highlight_foreground,
> +                     mw->menu.highlight_background);
>  
>    xgcv.fill_style = FillStippled;
>    xgcv.foreground = mw->menu.top_shadow_color;
> @@ -1862,6 +2017,16 @@ make_shadow_gcs (XlwMenuWidget mw)
>    xgcv.stipple = mw->menu.bottom_shadow_pixmap;
>    pm = (xgcv.stipple ? GCStipple|GCFillStyle : 0);

Please put spaces between "GCStipple", "|", and "GCFillStyle".  Also,
place a space after "compute_shadow_colors".

>    mw->menu.shadow_bottom_gc = XtGetGC ((Widget)mw, GCForeground | pm, &xgcv);
> +
> +  xgcv.foreground = mw->menu.top_highlight_shadow_color;
> +  xgcv.stipple = mw->menu.top_highlight_shadow_pixmap;
> +  pm = (xgcv.stipple ? GCStipple|GCFillStyle : 0);
> +  mw->menu.highlight_shadow_top_gc = XtGetGC ((Widget)mw, GCForeground | pm, 
> &xgcv);
> +
> +  xgcv.foreground = mw->menu.bottom_highlight_shadow_color;
> +  xgcv.stipple = mw->menu.bottom_highlight_shadow_pixmap;
> +  pm = (xgcv.stipple ? GCStipple|GCFillStyle : 0);
> +  mw->menu.highlight_shadow_bottom_gc = XtGetGC ((Widget)mw, GCForeground | 
> pm, &xgcv);
>  }

What was said about casts also applies here.

> @@ -2038,12 +2203,14 @@ XlwMenuRealize (Widget w, Mask *valueMask, 
> XSetWindowAttributes *attributes)
>  #if defined USE_CAIRO || defined HAVE_XFT
>    if (mw->menu.xft_font)
>      {
> -      XColor colors[3];
> +      XColor colors[4];
>        colors[0].pixel = mw->menu.xft_fg.pixel = mw->menu.foreground;
>        colors[1].pixel = mw->menu.xft_bg.pixel = mw->core.background_pixel;
>        colors[2].pixel = mw->menu.xft_disabled_fg.pixel
>          = mw->menu.disabled_foreground;
> -      XQueryColors (XtDisplay (mw), mw->core.colormap, colors, 3);
> +      colors[3].pixel = mw->menu.xft_highlight_fg.pixel
> +     = mw->menu.highlight_foreground;
> +      XQueryColors (XtDisplay (mw), mw->core.colormap, colors, 4);
>        mw->menu.xft_fg.color.alpha = 0xFFFF;
>        mw->menu.xft_fg.color.red = colors[0].red;
>        mw->menu.xft_fg.color.green = colors[0].green;
> @@ -2056,6 +2223,10 @@ XlwMenuRealize (Widget w, Mask *valueMask, 
> XSetWindowAttributes *attributes)
>        mw->menu.xft_disabled_fg.color.red = colors[2].red;
>        mw->menu.xft_disabled_fg.color.green = colors[2].green;
>        mw->menu.xft_disabled_fg.color.blue = colors[2].blue;
> +      mw->menu.xft_highlight_fg.color.alpha = 0xFFFF;
> +      mw->menu.xft_highlight_fg.color.red = colors[3].red;
> +      mw->menu.xft_highlight_fg.color.green = colors[3].green;
> +      mw->menu.xft_highlight_fg.color.blue = colors[3].blue;
>      }
>  #endif
>  }
> diff --git a/lwlib/xlwmenu.h b/lwlib/xlwmenu.h
> index 7f4bf35939..f68d913b5a 100644
> --- a/lwlib/xlwmenu.h
> +++ b/lwlib/xlwmenu.h
> @@ -58,6 +58,10 @@ #define XtNallowResize "allowResize"
>  #define XtCAllowResize "AllowResize"
>  #define XtNborderThickness "borderThickness"
>  #define XtCBorderThickness "BorderThickness"
> +#define XtNhighlightForeground "highlightForeground"
> +#define XtCHighlightForeground "HighlightForeground"
> +#define XtNhighlightBackground "highlightBackground"
> +#define XtCHighlightBackground "HighlightBackground"
>  
>  /* Motif-compatible resource names */
>  #define XmNshadowThickness   "shadowThickness"
> @@ -70,6 +74,16 @@ #define XmNtopShadowPixmap "topShadowPixmap"
>  #define XmCTopShadowPixmap   "TopShadowPixmap"
>  #define XmNbottomShadowPixmap        "bottomShadowPixmap"
>  #define XmCBottomShadowPixmap        "BottomShadowPixmap"

> +#define XmNtopHighlightShadowColor   "topHighlightShadowColor"
> +#define XmCTopHighlightShadowColor   "TopHighlightShadowColor"
> +#define XmNbottomHighlightShadowColor        "bottomHighlightShadowColor"
> +#define XmCBottomHighlightShadowColor        "BottomHighlightShadowColor"
> +#define XmNtopHighlightShadowPixmap  "topHighlightShadowPixmap"
> +#define XmCTopHighlightShadowPixmap  "TopHighlightShadowPixmap"
> +#define XmNbottomHighlightShadowPixmap       "bottomHighlightShadowPixmap"
> +#define XmCBottomHighlightShadowPixmap       "BottomHighlightShadowPixmap"

Motif doesn't have widget resources named topHighlightShadowColor,
bottomHighlightShadowColor, topHighlightShadowPixmap or
bottomHighlightShadowPixmap.  So please delete these.

>  #define XmRHorizontalDimension       "HorizontalDimension"
>  
>  typedef struct _XlwMenuRec *XlwMenuWidget;
> diff --git a/lwlib/xlwmenuP.h b/lwlib/xlwmenuP.h
> index 455ecdbce0..c314eb3e91 100644
> --- a/lwlib/xlwmenuP.h
> +++ b/lwlib/xlwmenuP.h
> @@ -63,13 +63,15 @@ #define _XlwMenuP_h
>  #if defined USE_CAIRO || defined HAVE_XFT
>    int           default_face;
>    XftFont*      xft_font;
> -  XftColor      xft_fg, xft_bg, xft_disabled_fg;
> +  XftColor      xft_fg, xft_bg, xft_disabled_fg, xft_highlight_fg;
>  #endif
>    String     fontName;
>    XFontStruct*       font;
>    Pixel              foreground;
>    Pixel              disabled_foreground;
>    Pixel              button_foreground;
> +  Pixel              highlight_foreground;
> +  Pixel              highlight_background;
>    Dimension  margin;
>    Dimension  horizontal_spacing;
>    Dimension  vertical_spacing;
> @@ -80,6 +82,10 @@ #define _XlwMenuP_h
>    Pixel      bottom_shadow_color;
>    Pixmap     top_shadow_pixmap;
>    Pixmap     bottom_shadow_pixmap;
> +  Pixel      top_highlight_shadow_color;
> +  Pixel      bottom_highlight_shadow_color;
> +  Pixmap     top_highlight_shadow_pixmap;
> +  Pixmap     bottom_highlight_shadow_pixmap;
>    Cursor     cursor_shape;
>    XtCallbackList     open;
>    XtCallbackList     select, highlight;
> @@ -88,8 +94,10 @@ #define _XlwMenuP_h
>    int                horizontal;
>  
>    /* True means top_shadow_color and/or bottom_shadow_color must be freed.  
> */
> -  bool_bf free_top_shadow_color_p : 1;
> -  bool_bf free_bottom_shadow_color_p : 1;
> +  Boolean free_top_shadow_color_p;
> +  Boolean free_bottom_shadow_color_p;
> +  Boolean free_top_highlight_shadow_color_p;
> +  Boolean free_bottom_highlight_shadow_color_p;
>  
>    /* State of the XlwMenu */
>    int                   top_depth;
> @@ -112,9 +120,13 @@ #define _XlwMenuP_h
>    GC                 button_gc;
>    GC                 background_gc;
>    GC                 disabled_gc;
> +  GC                 highlight_foreground_gc;
> +  GC                 highlight_background_gc;
>    GC                 inactive_button_gc;
>    GC                 shadow_top_gc;
>    GC                 shadow_bottom_gc;
> +  GC                 highlight_shadow_top_gc;
> +  GC                 highlight_shadow_bottom_gc;
>    Cursor             cursor;
>    Boolean            popped_up;
>    Pixmap             gray_pixmap;

I haven't looked at the code here in much detail yet, but please verify
that it builds and works correctly under Xft, Cairo, and without either
of the two, and under a pseudo-color visual.  (To get an X server with a
pseudo color visual, run "Xephyr -screen 800x800x8".)

Thanks.