Package: emacs;
Reported by: Manuel Giraud <manuel <at> ledu-giraud.fr>
Date: Wed, 13 Jul 2022 13:39:02 UTC
Severity: wishlist
Tags: patch
Found in version 29.0.50
Done: Po Lu <luangruo <at> yahoo.com>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: Po Lu <luangruo <at> yahoo.com> To: Manuel Giraud <manuel <at> ledu-giraud.fr> Cc: 56538 <at> debbugs.gnu.org Subject: bug#56538: 29.0.50; [PATCH] Colored highlight in Lucid backend Date: Thu, 14 Jul 2022 08:53:39 +0800
Manuel Giraud <manuel <at> 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.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.