GNU bug report logs - #56538
29.0.50; [PATCH] Colored highlight in Lucid backend

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 56538 in the body.
You can then email your comments to 56538 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: bug-gnu-emacs <at> gnu.org
Subject: 29.0.50; [PATCH] Colored highlight in Lucid backend
Date: Wed, 13 Jul 2022 15:38:35 +0200

[Message part 1 (text/plain, inline)]

Hi,

Here is a patch that add a X resource to color the highlighted menu
entry in the Lucid backend.  There is much going on here because I
needed to understand this code and end up needed to refactor some part
to implement this feature.  This was about ten clunky patches that I
finally squashed into this one.

I have tested that it works with the 2 following configuration
     - 	--with-x-toolkit=athena
     - 	--with-x-toolkit=athena --without-xft --without-cairo

Here is how it looks like:

[sshot.png (image/png, inline)]

[0001-Colored-menu-highlight-in-Lucid-backend.patch (text/x-patch, attachment)]

[Message part 4 (text/plain, inline)]

In GNU Emacs 29.0.50 (build 1, x86_64-unknown-openbsd7.1, X toolkit, cairo version 1.17.6, Xaw scroll bars)
 of 2022-07-13 built on elite.giraud
Repository revision: f51e1dbd3174d727a12e673e0b820b4cf35e7046
Repository branch: mgi/ui3
Windowing system distributor 'The X.Org Foundation', version 11.0.12101003
System Description: OpenBSD elite.giraud 7.1 GENERIC.MP#579 amd64

Configured using:
 'configure --prefix=/home/manuel/emacs --bindir=/home/manuel/bin
 --with-x-toolkit=athena --without-sound --without-compress-install
 CPPFLAGS=-I/usr/local/include LDFLAGS=-L/usr/local/lib'

Configured features:
CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG JSON
LCMS2 LIBOTF LIBXML2 MODULES NOTIFY KQUEUE PDUMPER PNG RSVG SQLITE3
THREADS TIFF TOOLKIT_SCROLL_BARS WEBP X11 XDBE XIM XINPUT2 XPM LUCID
ZLIB

Important settings:
  value of $LC_ALL: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  global-git-commit-mode: t
  magit-auto-revert-mode: t
  paredit-mode: t
  icomplete-mode: t
  display-time-mode: t
  shell-dirtrack-mode: t
  global-so-long-mode: t
  repeat-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
/home/manuel/.emacs.d/elpa/transient-20220527.2213/transient hides /home/manuel/emacs/share/emacs/29.0.50/lisp/transient

Features:
(shadow sort mail-extr emacsbug vc-git whitespace magit-patch
magit-extras face-remap magit-bookmark magit-submodule magit-obsolete
magit-blame magit-stash magit-reflog magit-bisect magit-push magit-pull
magit-fetch magit-clone magit-remote magit-commit magit-sequence
magit-notes magit-worktree magit-tag magit-merge magit-branch
magit-reset magit-files magit-refs magit-status magit magit-repos
magit-apply magit-wip magit-log which-func imenu magit-diff smerge-mode
diff git-commit log-edit pcvs-util add-log magit-core magit-autorevert
autorevert magit-margin magit-transient magit-process with-editor
magit-mode transient magit-git magit-base magit-section dash compat-27
compat-26 compat pulse vc-hg diff-mode vc-dispatcher vc-svn conf-mode
facemenu paredit edmacro icomplete time battery exwm-randr xcb-randr
exwm-config exwm exwm-input xcb-keysyms xcb-xkb exwm-manage
exwm-floating xcb-cursor xcb-render exwm-layout exwm-workspace exwm-core
xcb-ewmh xcb-icccm xcb xcb-xproto xcb-types xcb-debug kmacro server
stimmung-themes modus-operandi-theme modus-themes osm bookmark mingus
libmpdee transmission diary-lib diary-loaddefs color calc-bin calc-ext
calc calc-loaddefs rect calc-macs w3m-load mu4e mu4e-org mu4e-main
mu4e-view mu4e-view-gnus mu4e-view-common mu4e-headers mu4e-compose
mu4e-context mu4e-draft mu4e-actions ido rfc2368 smtpmail mu4e-mark
mu4e-proc mu4e-utils doc-view filenotify jka-compr image-mode exif
mu4e-lists mu4e-message flow-fill mule-util hl-line mu4e-vars mu4e-meta
supercite regi ebdb-message ebdb-gnus gnus-msg gnus-art mm-uu mml2015
mm-view mml-smime smime gnutls dig gnus-sum shr pixel-fill kinsoku
url-file url-dired svg dom gnus-group gnus-undo gnus-start gnus-dbus
gnus-cloud nnimap nnmail mail-source utf7 netrc nnoo gnus-spec gnus-int
gnus-range message sendmail yank-media puny rfc822 mml mml-sec epa
derived epg rfc6068 epg-config mm-decode mm-bodies mm-encode mail-parse
rfc2231 rfc2047 rfc2045 ietf-drums gmm-utils mailheader gnus-win gnus
nnheader gnus-util mail-utils range mm-util mail-prsvr ebdb-mua ebdb-com
crm ebdb-format ebdb mailabbrev eieio-opt cl-extra help-mode speedbar
ezimage dframe eieio-base pcase timezone org ob ob-tangle ob-ref ob-lob
ob-table ob-exp org-macro org-footnote org-src ob-comint org-pcomplete
org-list org-faces org-entities org-version ob-emacs-lisp ob-core
ob-eval org-table oc-basic bibtex ol rx org-keys oc org-compat org-macs
org-loaddefs find-func cal-menu calendar cal-loaddefs visual-basic-mode
cl web-mode disp-table erlang-start smart-tabs-mode skeleton cc-mode
cc-fonts cc-guess cc-menus cc-cmds cc-styles cc-align cc-engine cc-vars
cc-defs slime-asdf grep slime-tramp tramp tramp-loaddefs trampver
tramp-integration cus-edit cus-load wid-edit files-x tramp-compat shell
pcomplete parse-time iso8601 time-date ls-lisp format-spec slime-fancy
slime-indentation slime-cl-indent cl-indent slime-trace-dialog
slime-fontifying-fu slime-package-fu slime-references
slime-compiler-notes-tree advice slime-scratch slime-presentations
bridge slime-macrostep macrostep slime-mdot-fu slime-enclosing-context
slime-fuzzy slime-fancy-trace slime-fancy-inspector slime-c-p-c
slime-editing-commands slime-autodoc slime-repl slime-parse slime
compile text-property-search etags fileloop generator xref project
arc-mode archive-mode noutline outline pp comint ansi-color ring
hyperspec thingatpt slime-autoloads dired-aux dired-x dired
dired-loaddefs so-long notifications dbus xml repeat easy-mmode tex-site
hyperbole-autoloads magit-autoloads git-commit-autoloads
magit-section-autoloads dash-autoloads rust-mode-autoloads
stimmung-themes-autoloads with-editor-autoloads info compat-autoloads
package browse-url url url-proxy url-privacy url-expand url-methods
url-history url-cookie generate-lisp-file url-domsuf url-util mailcap
url-handlers url-parse auth-source cl-seq eieio eieio-core cl-macs
password-cache json subr-x map byte-opt gv bytecomp byte-compile cconv
url-vars cl-loaddefs cl-lib rmc iso-transl tooltip eldoc paren electric
uniquify ediff-hook vc-hooks lisp-float-type elisp-mode mwheel
term/x-win x-win term/common-win x-dnd tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode lisp-mode
prog-mode register page tab-bar menu-bar rfn-eshadow isearch easymenu
timer select scroll-bar mouse jit-lock font-lock syntax font-core
term/tty-colors frame minibuffer nadvice seq simple cl-generic
indonesian philippine cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
composite emoji-zwj charscript charprop case-table epa-hook
jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button loaddefs
faces cus-face macroexp files window text-properties overlay sha1 md5
base64 format env code-pages mule custom widget keymap
hashtable-print-readable backquote threads dbusbind kqueue lcms2
dynamic-setting system-font-setting font-render-setting cairo x-toolkit
xinput2 x multi-tty make-network-process emacs)

Memory information:
((conses 16 563139 59476)
 (symbols 48 54905 4)
 (strings 32 171200 8786)
 (string-bytes 1 5703800)
 (vectors 16 99692)
 (vector-slots 8 1289985 58168)
 (floats 8 522 566)
 (intervals 56 2981 2094)
 (buffers 992 16))

-- 
Manuel Giraud

Message #8 received at 56538 <at> debbugs.gnu.org (full text, mbox):

From: Po Lu <luangruo <at> yahoo.com>
To: Manuel Giraud <manuel <at> ledu-giraud.fr>
Cc: 56538 <at> debbugs.gnu.org
Subject: Re: 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.

Message #11 received at 56538 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Po Lu <luangruo <at> yahoo.com>
Cc: 56538 <at> debbugs.gnu.org
Subject: Re: bug#56538: 29.0.50; [PATCH] Colored highlight in Lucid backend
Date: Thu, 14 Jul 2022 11:42:06 +0200

[Message part 1 (text/plain, inline)]

Po Lu <luangruo <at> yahoo.com> writes:

Hi Po and thanks for your comments,

[...]

>> +      /* XXX the following permutation is on purpose */
>
> This comment seems redundant.

I have attached a new version of my patch taking your remarks into
account but I did not remove this comment.  I think it could be useful
to understand that "top_gc = something_bottom_gc" is not a typo here.

I have also done the following testing with Xft/cairo/PseudoColor
combinations:

|-------------+-----------------+-----------------+-----------------+---------------------|
|             | Xft + cairo     | Xft only        | cairo only      | None                |
|-------------+-----------------+-----------------+-----------------+---------------------|
| X Truecolor | ok              | ok              | ok              | ok but with a       |
|             |                 |                 |                 | default tiny face   |
|-------------+-----------------+-----------------+-----------------+---------------------|
| Xephyr      | ok but color    | ok but color    | ok but color    | ok, colors ok, but  |
| Pseudocolor | not quite right | not quite right | not quite right | with a default tiny |
|             | around each     | around menu     | around each     | face [fn:2]         |
|             | glyph [fn:1]    | labels [fn:2]   | glyph [fn:1]    |                     |
|-------------+-----------------+-----------------+-----------------+---------------------|

* Footnotes
[fn:1] Aside from the Lucid widget, the emacs buffer is completly
blank.

[fn:2] This time the emacs buffer is here. tool-bar icons are ugly.

[0001-Colored-menu-highlight-in-Lucid-backend.patch (text/x-patch, attachment)]

[Message part 3 (text/plain, inline)]

-- 
Manuel Giraud

Message #14 received at 56538 <at> debbugs.gnu.org (full text, mbox):

From: Po Lu <luangruo <at> yahoo.com>
To: Manuel Giraud <manuel <at> ledu-giraud.fr>
Cc: 56538 <at> debbugs.gnu.org
Subject: Re: bug#56538: 29.0.50; [PATCH] Colored highlight in Lucid backend
Date: Thu, 14 Jul 2022 18:34:43 +0800

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

> I have attached a new version of my patch taking your remarks into
> account but I did not remove this comment.  I think it could be useful
> to understand that "top_gc = something_bottom_gc" is not a typo here.

We have the same general pattern (random_gc = random_opposite_gc) in
most of the X code, so it is redundant IMHO.  But I won't insist.

> I have also done the following testing with Xft/cairo/PseudoColor
> combinations:
>
> |-------------+-----------------+-----------------+-----------------+---------------------|
> |             | Xft + cairo     | Xft only        | cairo only      | None                |
> |-------------+-----------------+-----------------+-----------------+---------------------|
> | X Truecolor | ok              | ok              | ok              | ok but with a       |
> |             |                 |                 |                 | default tiny face   |
> |-------------+-----------------+-----------------+-----------------+---------------------|
> | Xephyr      | ok but color    | ok but color    | ok but color    | ok, colors ok, but  |
> | Pseudocolor | not quite right | not quite right | not quite right | with a default tiny |
> |             | around each     | around menu     | around each     | face [fn:2]         |
> |             | glyph [fn:1]    | labels [fn:2]   | glyph [fn:1]    |                     |
> |-------------+-----------------+-----------------+-----------------+---------------------|
>
> * Footnotes
> [fn:1] Aside from the Lucid widget, the emacs buffer is completly
> blank.

Yes, that isn't expected to work, because Cairo is broken.  Also, you
can't build Emacs with both Xft and Cairo, since they're mutually
exclusive.

> [fn:2] This time the emacs buffer is here. tool-bar icons are ugly.

Could you please describe how the colors are "not quite right"?

One last nit:

> +static void
> +draw_highlight (XlwMenuWidget mw,
> +		Window window,
> +		int x,
> +		int y,
> +		int width,
> +		int height)

[...]

>  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)

Here and elsewhere new functions are added, the argument list should be
reformatted to not place each argument on its own line.

Thanks.

Message #19 received at 56538 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Po Lu <luangruo <at> yahoo.com>
Cc: 56538 <at> debbugs.gnu.org
Subject: Re: bug#56538: 29.0.50; [PATCH] Colored highlight in Lucid backend
Date: Thu, 14 Jul 2022 15:17:54 +0200

[Message part 1 (text/plain, inline)]

Po Lu <luangruo <at> yahoo.com> writes:

[...]

> We have the same general pattern (random_gc = random_opposite_gc) in
> most of the X code, so it is redundant IMHO.  But I won't insist.

Ok.  I can remove this comment then.

[...]

> Yes, that isn't expected to work, because Cairo is broken.  Also, you
> can't build Emacs with both Xft and Cairo, since they're mutually
> exclusive.

😅 sorry. So then "Xft + cairo" is just "cairo".

> Could you please describe how the colors are "not quite right"?

Here is how it look here™ on a PseudoColor Xephyr.  This is Cairo with a
bitmap font:

[sshot-pseudo-cairo.png (image/png, inline)]

[Message part 3 (text/plain, inline)]

This Xft with the same font:

[sshot-pseudo-xft.png (image/png, inline)]

[Message part 5 (text/plain, inline)]

And for the record, here are the same with a vector font:

[sshot-pseudo-cairo-vector.png (image/png, inline)]

[sshot-pseudo-xft-vector.png (image/png, inline)]

[Message part 8 (text/plain, inline)]

[...]

> Here and elsewhere new functions are added, the argument list should be
> reformatted to not place each argument on its own line.

FWIW, draw_shadow_rectangle, draw_shadow_rhombus… are already like that.

I'll do it but what are your recommandations?  Split at fill-colum +
indent?  Leave the old ones untouched?
-- 
Manuel Giraud

Message #22 received at 56538 <at> debbugs.gnu.org (full text, mbox):

From: Po Lu <luangruo <at> yahoo.com>
To: Manuel Giraud <manuel <at> ledu-giraud.fr>
Cc: 56538 <at> debbugs.gnu.org
Subject: Re: bug#56538: 29.0.50; [PATCH] Colored highlight in Lucid backend
Date: Fri, 15 Jul 2022 10:06:26 +0800

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

> Here is how it look here™ on a PseudoColor Xephyr.  This is Cairo with a
> bitmap font:
>
>
>
>
> This Xft with the same font:
>
>
>
>
> And for the record, here are the same with a vector font:
>
>
>
>
>

Right, that's acceptable.  I think the problem will go away if you
disable the rendering extension on the X server.

> FWIW, draw_shadow_rectangle, draw_shadow_rhombus… are already like that.
>
> I'll do it but what are your recommandations?  Split at fill-colum +
> indent?  Leave the old ones untouched?

Yes.  Looking at functions like `x_dnd_do_unsupported_drop' in xterm.c
should help as well.

Otherwise, LGTM.

Message #25 received at 56538 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Po Lu <luangruo <at> yahoo.com>
Cc: 56538 <at> debbugs.gnu.org
Subject: Re: bug#56538: 29.0.50; [PATCH] Colored highlight in Lucid backend
Date: Fri, 15 Jul 2022 13:50:26 +0200

[Message part 1 (text/plain, inline)]

Po Lu <luangruo <at> yahoo.com> writes:

[...]

> Right, that's acceptable.  I think the problem will go away if you
> disable the rendering extension on the X server.

Ok and I guess that not many people are using a PseudoColor display
nowadays.

> Yes.  Looking at functions like `x_dnd_do_unsupported_drop' in xterm.c
> should help as well.
>
> Otherwise, LGTM.

Here is a new version of this patch.  What's new:

     - fix some comments
     
     - format arguments list on both new functions and the one I have
       modified their arguments
       
     - add some forgot freeing in `release_shadow_gcs'

[0001-Colored-menu-highlight-in-Lucid-backend.patch (text/x-patch, attachment)]

[Message part 3 (text/plain, inline)]

-- 
Manuel Giraud

Message #28 received at 56538 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Po Lu <luangruo <at> yahoo.com>
Cc: 56538 <at> debbugs.gnu.org
Subject: Re: bug#56538: 29.0.50; [PATCH] Colored highlight in Lucid backend
Date: Fri, 15 Jul 2022 13:56:17 +0200

[Message part 1 (text/plain, inline)]

Po Lu <luangruo <at> yahoo.com> writes:

[...]

> Right, that's acceptable.  I think the problem will go away if you
> disable the rendering extension on the X server.

Ok and I guess that not many people are using a PseudoColor display
nowadays.

> Yes.  Looking at functions like `x_dnd_do_unsupported_drop' in xterm.c
> should help as well.
>
> Otherwise, LGTM.

Here is a new version of this patch.  What's new:

     - fix some comments
     
     - format arguments list on both new functions and the one I have
       modified their arguments
       
     - add some forgot freeing in `release_shadow_gcs'

[0001-Colored-menu-highlight-in-Lucid-backend.patch (text/x-patch, attachment)]

[Message part 3 (text/plain, inline)]

-- 
Manuel Giraud

Message #31 received at 56538 <at> debbugs.gnu.org (full text, mbox):

From: Po Lu <luangruo <at> yahoo.com>
To: Manuel Giraud <manuel <at> ledu-giraud.fr>
Cc: 56538 <at> debbugs.gnu.org
Subject: Re: bug#56538: 29.0.50; [PATCH] Colored highlight in Lucid backend
Date: Fri, 15 Jul 2022 20:58:46 +0800

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

> Here is a new version of this patch.  What's new:
>
>      - fix some comments
>      
>      - format arguments list on both new functions and the one I have
>        modified their arguments
>        
>      - add some forgot freeing in `release_shadow_gcs'

Thanks.  One (late) last comment on the documentation:

> +@item highlightForeground
> +Foreground color for a highlighted menu item.
> +@item highlightBackground
> +Background color for a highlighted menu item.

Shouldn't this say "the menu item highlighted by the mouse"?  Otherwise,
what "highlighted" means is not exactly clear.

> +** New X resources: "highlightForeground" and "highlightBackground"
> +Only in the Lucid build, this controls colors used for highlighted
> +menu item widget.

And here, shouldn't "widget" be in plural?

Message #34 received at 56538 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Po Lu <luangruo <at> yahoo.com>
Cc: 56538 <at> debbugs.gnu.org
Subject: Re: bug#56538: 29.0.50; [PATCH] Colored highlight in Lucid backend
Date: Fri, 15 Jul 2022 15:27:23 +0200

Po Lu <luangruo <at> yahoo.com> writes:

[...]

>> +@item highlightForeground
>> +Foreground color for a highlighted menu item.
>> +@item highlightBackground
>> +Background color for a highlighted menu item.
>
> Shouldn't this say "the menu item highlighted by the mouse"?  Otherwise,
> what "highlighted" means is not exactly clear.

I did not add "by the mouse" because menus could also be navigated with
keyboard arrows.  Do you have any other suggestions to make it clearer?

>> +** New X resources: "highlightForeground" and "highlightBackground"
>> +Only in the Lucid build, this controls colors used for highlighted
>> +menu item widget.
>
> And here, shouldn't "widget" be in plural?

Ok.  I'll add it to the next patch when the previous issue is resolved.
-- 
Manuel Giraud

Message #37 received at 56538 <at> debbugs.gnu.org (full text, mbox):

From: Po Lu <luangruo <at> yahoo.com>
To: Manuel Giraud <manuel <at> ledu-giraud.fr>
Cc: 56538 <at> debbugs.gnu.org
Subject: Re: bug#56538: 29.0.50; [PATCH] Colored highlight in Lucid backend
Date: Sat, 16 Jul 2022 11:04:34 +0800

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

>> Shouldn't this say "the menu item highlighted by the mouse"?  Otherwise,
>> what "highlighted" means is not exactly clear.
>
> I did not add "by the mouse" because menus could also be navigated with
> keyboard arrows.  Do you have any other suggestions to make it clearer?

How about "by the mouse or key navigation"?

>>> +** New X resources: "highlightForeground" and "highlightBackground"
>>> +Only in the Lucid build, this controls colors used for highlighted
>>> +menu item widget.
>>
>> And here, shouldn't "widget" be in plural?
>
> Ok.  I'll add it to the next patch when the previous issue is resolved.

Thanks.

Message #40 received at 56538 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Po Lu <luangruo <at> yahoo.com>
Cc: 56538 <at> debbugs.gnu.org
Subject: Re: bug#56538: 29.0.50; [PATCH] Colored highlight in Lucid backend
Date: Sat, 16 Jul 2022 12:56:29 +0200

[Message part 1 (text/plain, inline)]

Po Lu <luangruo <at> yahoo.com> writes:

> How about "by the mouse or key navigation"?

👍 Here's the updated version.

[0001-Colored-menu-highlight-in-Lucid-backend.patch (text/x-patch, attachment)]

[Message part 3 (text/plain, inline)]

-- 
Manuel Giraud

Message #45 received at 56538-done <at> debbugs.gnu.org (full text, mbox):

From: Po Lu <luangruo <at> yahoo.com>
To: Manuel Giraud <manuel <at> ledu-giraud.fr>
Cc: 56538-done <at> debbugs.gnu.org
Subject: Re: bug#56538: 29.0.50; [PATCH] Colored highlight in Lucid backend
Date: Sat, 16 Jul 2022 18:59:35 +0800

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

> Po Lu <luangruo <at> yahoo.com> writes:
>
>> How about "by the mouse or key navigation"?
>
> 👍 Here's the updated version.
>
> From f0a2392c04b2d3f514716df9757f0b8ed96efa22 Mon Sep 17 00:00:00 2001
> From: Manuel Giraud <manuel <at> ledu-giraud.fr>
> Date: Mon, 11 Jul 2022 11:14:08 +0200
> Subject: [PATCH] Colored menu highlight in Lucid backend

Installed, so I'm closing this bug.

Thanks for working on Emacs.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.