Hollow cursor under images

Hollow cursor under images

[Evgeny Zajcev]

[-- Attachment #1: Type: text/plain, Size: 338 bytes --]

I've got `cursor-type' set to `box', however when cursor is under image it
displays as if it was `hollow', i.e. image margins and non-mask pixels are
displayed with the background color of face under cursor, not the cursor
color.

Is this intended behaviour?  Hollow cursor is hardly noticeable on monitors
with dense dpi

Thanks

-- 
lg

[-- Attachment #2: Type: text/html, Size: 541 bytes --]

Re: Hollow cursor under images

[Eli Zaretskii]

> From: Evgeny Zajcev <lg.zevlg@gmail.com>
> Date: Mon, 4 Mar 2019 21:14:06 +0300
> 
> I've got `cursor-type' set to `box', however when cursor is under image it displays as if it was `hollow', i.e.
> image margins and non-mask pixels are displayed with the background color of face under cursor, not the
> cursor color.

What is "face under cursor" in this context?

> Is this intended behaviour?

If I understand what you described (and I'm not surer I did), then
no.  You should see the cursor color.

Re: Hollow cursor under images

[Evgeny Zajcev]

[-- Attachment #1: Type: text/plain, Size: 1907 bytes --]

пн, 4 мар. 2019 г. в 22:05, Eli Zaretskii <eliz@gnu.org>:

> > From: Evgeny Zajcev <lg.zevlg@gmail.com>
> > Date: Mon, 4 Mar 2019 21:14:06 +0300
> >
> > I've got `cursor-type' set to `box', however when cursor is under image
> it displays as if it was `hollow', i.e.
> > image margins and non-mask pixels are displayed with the background
> color of face under cursor, not the
> > cursor color.
>
> What is "face under cursor" in this context?
>

I have text property `face' attached to the region which image occupies,
consider:

(let ((svg (svg-create (frame-char-width) (* 2 (frame-char-height)))))
  (svg-circle svg 10 10 10 :fill-color "red")
  (insert (propertize "X" 'face 'region
                      'display (list '(slice 0.0 0.0 1.0 0.5)
                                     (svg-image svg :scale 1.0
                                                :ascent 'center
                                                :mask 'heuristic)))))

Background of the image gets color from `region' face, but when the cursor
is under the image it gets hollow type and still background of `region'
face is visible.

I just realized, that if I create image that fits into single char, such as:

(let ((svg (svg-create (frame-char-width) (frame-char-height))))
  (svg-circle svg 10 10 10 :fill-color "red")
  (insert (propertize "X" 'face 'region
                      'display (svg-image svg :scale 1.0
                                          :ascent 'center
                                          :mask 'heuristic))))

Then cursor behaves as expected initially!  It does not get hollow type and
background gets cursor color.  I expected the same for images that do not
fit into single char


> > Is this intended behaviour?
>
> If I understand what you described (and I'm not surer I did), then
> no.  You should see the cursor color.
>


-- 
lg

[-- Attachment #2: Type: text/html, Size: 3063 bytes --]

Re: Hollow cursor under images

[Eli Zaretskii]

> From: Evgeny Zajcev <lg.zevlg@gmail.com>
> Date: Mon, 4 Mar 2019 23:04:10 +0300
> Cc: emacs-devel <emacs-devel@gnu.org>
> 
> (let ((svg (svg-create (frame-char-width) (* 2 (frame-char-height)))))
>   (svg-circle svg 10 10 10 :fill-color "red")
>   (insert (propertize "X" 'face 'region
>                       'display (list '(slice 0.0 0.0 1.0 0.5)
>                                      (svg-image svg :scale 1.0
>                                                 :ascent 'center
>                                                 :mask 'heuristic)))))
> 
> Background of the image gets color from `region' face, but when the cursor is under the image it gets hollow
> type and still background of `region' face is visible.

Not on my system.

> I just realized, that if I create image that fits into single char, such as:
> 
> (let ((svg (svg-create (frame-char-width) (frame-char-height))))
>   (svg-circle svg 10 10 10 :fill-color "red")
>   (insert (propertize "X" 'face 'region
>                       'display (svg-image svg :scale 1.0
>                                           :ascent 'center
>                                           :mask 'heuristic))))
> 
> Then cursor behaves as expected initially!  It does not get hollow type and background gets cursor color.  I
> expected the same for images that do not fit into single char

I see no difference between these two cases.  The second image is
larger, but other than that, no difference regarding colors.

Re: Hollow cursor under images

[Evgeny Zajcev]

[-- Attachment #1: Type: text/plain, Size: 1788 bytes --]

пн, 4 мар. 2019 г. в 23:22, Eli Zaretskii <eliz@gnu.org>:

> > From: Evgeny Zajcev <lg.zevlg@gmail.com>
> > Date: Mon, 4 Mar 2019 23:04:10 +0300
> > Cc: emacs-devel <emacs-devel@gnu.org>
> >
> > (let ((svg (svg-create (frame-char-width) (* 2 (frame-char-height)))))
> >   (svg-circle svg 10 10 10 :fill-color "red")
> >   (insert (propertize "X" 'face 'region
> >                       'display (list '(slice 0.0 0.0 1.0 0.5)
> >                                      (svg-image svg :scale 1.0
> >                                                 :ascent 'center
> >                                                 :mask 'heuristic)))))
> >
> > Background of the image gets color from `region' face, but when the
> cursor is under the image it gets hollow
> > type and still background of `region' face is visible.
>
> Not on my system.
>
> > I just realized, that if I create image that fits into single char, such
> as:
> >
> > (let ((svg (svg-create (frame-char-width) (frame-char-height))))
> >   (svg-circle svg 10 10 10 :fill-color "red")
> >   (insert (propertize "X" 'face 'region
> >                       'display (svg-image svg :scale 1.0
> >                                           :ascent 'center
> >                                           :mask 'heuristic))))
> >
> > Then cursor behaves as expected initially!  It does not get hollow type
> and background gets cursor color.  I
> > expected the same for images that do not fit into single char
>
> I see no difference between these two cases.  The second image is
> larger, but other than that, no difference regarding colors.
>

Cool! What is your Emacs version?

I'm using GNU Emacs 27.0.50 (build 3, x86_64-pc-linux-gnu, GTK+ Version
3.22.30) of 2018-11-07

-- 
lg

[-- Attachment #2: Type: text/html, Size: 2735 bytes --]

Re: Hollow cursor under images

[Alan Third]

On Mon, Mar 04, 2019 at 11:04:10PM +0300, Evgeny Zajcev wrote:
> 
> I have text property `face' attached to the region which image occupies,
> consider:
> 
> (let ((svg (svg-create (frame-char-width) (* 2 (frame-char-height)))))
>   (svg-circle svg 10 10 10 :fill-color "red")
>   (insert (propertize "X" 'face 'region
>                       'display (list '(slice 0.0 0.0 1.0 0.5)
>                                      (svg-image svg :scale 1.0
>                                                 :ascent 'center
>                                                 :mask 'heuristic)))))
> 
> Background of the image gets color from `region' face, but when the cursor
> is under the image it gets hollow type and still background of `region'
> face is visible.
> 
> I just realized, that if I create image that fits into single char, such as:
> 
> (let ((svg (svg-create (frame-char-width) (frame-char-height))))
>   (svg-circle svg 10 10 10 :fill-color "red")
>   (insert (propertize "X" 'face 'region
>                       'display (svg-image svg :scale 1.0
>                                           :ascent 'center
>                                           :mask 'heuristic))))
> 
> Then cursor behaves as expected initially!  It does not get hollow type and
> background gets cursor color.  I expected the same for images that do not
> fit into single char

I can confirm this happens on X. I can see it in a build from 2017, so
it’s not been caused by my image resizing code, which was my initial
thought.

The key image dimension appears to be 32 pixels. At 32 and below the
cursor shows through the background colour. At 33 and above, the
background remains white and the cursor appears as a box round the
image.

So a 32x32 image shows the cursor, a 33x32 or 32x33 image doesn’t.

I can’t see what makes 32 special. 
-- 
Alan Third

Re: Hollow cursor under images

[Eli Zaretskii]

> From: Evgeny Zajcev <lg.zevlg@gmail.com>
> Date: Mon, 4 Mar 2019 23:27:25 +0300
> Cc: emacs-devel <emacs-devel@gnu.org>
> 
>  I see no difference between these two cases.  The second image is
>  larger, but other than that, no difference regarding colors.
> 
> Cool! What is your Emacs version?

I tested that in yesterday's master.

Re: Hollow cursor under images

[Evgeny Zajcev]

[-- Attachment #1: Type: text/plain, Size: 2143 bytes --]

вт, 5 мар. 2019 г. в 01:36, Alan Third <alan@idiocy.org>:

> On Mon, Mar 04, 2019 at 11:04:10PM +0300, Evgeny Zajcev wrote:
> >
> > I have text property `face' attached to the region which image occupies,
> > consider:
> >
> > (let ((svg (svg-create (frame-char-width) (* 2 (frame-char-height)))))
> >   (svg-circle svg 10 10 10 :fill-color "red")
> >   (insert (propertize "X" 'face 'region
> >                       'display (list '(slice 0.0 0.0 1.0 0.5)
> >                                      (svg-image svg :scale 1.0
> >                                                 :ascent 'center
> >                                                 :mask 'heuristic)))))
> >
> > Background of the image gets color from `region' face, but when the
> cursor
> > is under the image it gets hollow type and still background of `region'
> > face is visible.
> >
> > I just realized, that if I create image that fits into single char, such
> as:
> >
> > (let ((svg (svg-create (frame-char-width) (frame-char-height))))
> >   (svg-circle svg 10 10 10 :fill-color "red")
> >   (insert (propertize "X" 'face 'region
> >                       'display (svg-image svg :scale 1.0
> >                                           :ascent 'center
> >                                           :mask 'heuristic))))
> >
> > Then cursor behaves as expected initially!  It does not get hollow type
> and
> > background gets cursor color.  I expected the same for images that do not
> > fit into single char
>
> I can confirm this happens on X. I can see it in a build from 2017, so
> it’s not been caused by my image resizing code, which was my initial
> thought.
>
> The key image dimension appears to be 32 pixels. At 32 and below the
> cursor shows through the background colour. At 33 and above, the
> background remains white and the cursor appears as a box round the
> image.
>
> So a 32x32 image shows the cursor, a 33x32 or 32x33 image doesn’t.
>
>
In my case 32 for width is the trigger and `(frame-char-height) == 53' for
height triggers the hollow cursor behaviour.

Looks like a bug

-- 
lg

[-- Attachment #2: Type: text/html, Size: 2977 bytes --]

Re: Hollow cursor under images

[Evgeny Zajcev]

[-- Attachment #1: Type: text/plain, Size: 1123 bytes --]

вт, 5 мар. 2019 г. в 13:19, Evgeny Zajcev <lg.zevlg@gmail.com>:

> [...]
>
In my case 32 for width is the trigger and `(frame-char-height) == 53' for
> height triggers the hollow cursor behaviour.
>
> Looks like a bug
>

I've found this in xdisp.c:

....
  if (cursor_type == FILLED_BOX_CURSOR)
   {
     /* Using a block cursor on large images can be very annoying.
So use a hollow cursor for "large" images.
If image is not transparent (no mask), also use hollow cursor.  */
     struct image *img = IMAGE_OPT_FROM_ID (f, glyph->u.img_id);
     if (img != NULL && IMAGEP (img->spec))
{
 /* Arbitrarily, interpret "Large" as >32x32 and >NxN
    where N = size of default frame font size.
    This should cover most of the "tiny" icons people may use.  */
 if (!img->mask
     || img->width > max (32, WINDOW_FRAME_COLUMN_WIDTH (w))
     || img->height > max (32, WINDOW_FRAME_LINE_HEIGHT (w)))
   cursor_type = HOLLOW_BOX_CURSOR;
}
....

Maybe introduce some property in image spec to control cursor behaviour?
Or make customizable definition for "tiny" icons?

Thanks

-- 
lg

[-- Attachment #2: Type: text/html, Size: 2065 bytes --]

Re: Hollow cursor under images

[Evgeny Zajcev]

[-- Attachment #1.1: Type: text/plain, Size: 1431 bytes --]

сб, 14 сент. 2019 г. в 14:48, Evgeny Zajcev <lg.zevlg@gmail.com>:

>
>
> вт, 5 мар. 2019 г. в 13:19, Evgeny Zajcev <lg.zevlg@gmail.com>:
>
>> [...]
>>
> In my case 32 for width is the trigger and `(frame-char-height) == 53' for
>> height triggers the hollow cursor behaviour.
>>
>> Looks like a bug
>>
>
> I've found this in xdisp.c:
>
> ....
>   if (cursor_type == FILLED_BOX_CURSOR)
>    {
>      /* Using a block cursor on large images can be very annoying.
> So use a hollow cursor for "large" images.
> If image is not transparent (no mask), also use hollow cursor.  */
>      struct image *img = IMAGE_OPT_FROM_ID (f, glyph->u.img_id);
>      if (img != NULL && IMAGEP (img->spec))
> {
>  /* Arbitrarily, interpret "Large" as >32x32 and >NxN
>     where N = size of default frame font size.
>     This should cover most of the "tiny" icons people may use.  */
>  if (!img->mask
>      || img->width > max (32, WINDOW_FRAME_COLUMN_WIDTH (w))
>      || img->height > max (32, WINDOW_FRAME_LINE_HEIGHT (w)))
>    cursor_type = HOLLOW_BOX_CURSOR;
> }
> ....
>
> Maybe introduce some property in image spec to control cursor behaviour?
> Or make customizable definition for "tiny" icons?
>
> Thanks
>

This patch implements (box . WIDTH) cursor type, allowing user to define
his own notion for "Large" image

Could you please include it into master

Thanks

-- 
lg

[-- Attachment #1.2: Type: text/html, Size: 2682 bytes --]

[-- Attachment #2: 0001-Support-for-box-.-WIDTH-cursor-type.patch --]
[-- Type: text/x-patch, Size: 3489 bytes --]

From 6406d198e9f3f5b2d93987ed0fd4fe22bd590624 Mon Sep 17 00:00:00 2001
From: Zajcev Evgeny <zevlg@yandex.ru>
Date: Mon, 27 Jan 2020 15:49:46 +0300
Subject: [PATCH] * Support for (box . WIDTH) `cursor-type'

Before this commit, block cursor becomes hollow under "large" masked
images.  Notion for "large" was hardcoded to be image larger then
32x32 in any dimension.

This patch allows user to define his own notion for "large" image,
taking full control of block cursor look under masked images.

With cursor-type equal to `box' cursor will always be block under
masked images.  This differs from former behavior for box cursor.
To get former behavior, set `cursor-type' to (box . 32)
---
 src/buffer.c |  4 ++++
 src/xdisp.c  | 19 +++++++++++++++----
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/src/buffer.c b/src/buffer.c
index 80eaa97..4c47e9b 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -6247,6 +6247,9 @@ from (abs POSITION).  If POSITION is positive, point was at the front
   t               use the cursor specified for the frame
   nil             don't display a cursor
   box             display a filled box cursor
+  (box . WIDTH)   display a filled box cursor, but make it
+                  hollow if cursor is under masked image larger then
+                  WIDTH in either dimention.
   hollow          display a hollow box cursor
   bar             display a vertical bar cursor with default width
   (bar . WIDTH)   display a vertical bar cursor with width WIDTH
@@ -6255,6 +6258,7 @@ from (abs POSITION).  If POSITION is positive, point was at the front
   ANYTHING ELSE   display a hollow box cursor
 
 WIDTH and HEIGHT can't exceed the frame's canonical character size.
+Except for (box . WIDTH) case.
 
 When the buffer is displayed in a non-selected window, the
 cursor's appearance is instead controlled by the variable
diff --git a/src/xdisp.c b/src/xdisp.c
index 3080f89..371589c 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -30683,6 +30683,14 @@ get_specified_cursor_type (Lisp_Object arg, int *width)
   if (EQ (arg, Qbox))
     return FILLED_BOX_CURSOR;
 
+  if (CONSP (arg)
+      && EQ (XCAR (arg), Qbox)
+      && RANGED_FIXNUMP (0, XCDR (arg), INT_MAX))
+    {
+      *width = XFIXNUM (XCDR (arg));
+      return FILLED_BOX_CURSOR;
+    }
+
   if (EQ (arg, Qhollow))
     return HOLLOW_BOX_CURSOR;
 
@@ -30849,12 +30857,15 @@ get_window_cursor_type (struct window *w, struct glyph *glyph, int *width,
 	      struct image *img = IMAGE_OPT_FROM_ID (f, glyph->u.img_id);
 	      if (img != NULL && IMAGEP (img->spec))
 		{
-		  /* Arbitrarily, interpret "Large" as >32x32 and >NxN
+		  /* Interpret "large" as >WIDTHxWIDTH and >NxN
+                     where WIDTH is the value from cursor-type in form (box . WIDTH),
 		     where N = size of default frame font size.
-		     This should cover most of the "tiny" icons people may use.  */
+                     So, setting cursor-type to (box . 32) should cover most of
+                     the "tiny" icons people may use.  */
 		  if (!img->mask
-		      || img->width > max (32, WINDOW_FRAME_COLUMN_WIDTH (w))
-		      || img->height > max (32, WINDOW_FRAME_LINE_HEIGHT (w)))
+                      || (CONSP (BVAR (b, cursor_type))
+                          && img->width > max (*width, WINDOW_FRAME_COLUMN_WIDTH (w))
+                          && img->height > max (*width, WINDOW_FRAME_LINE_HEIGHT (w))))
 		    cursor_type = HOLLOW_BOX_CURSOR;
 		}
 	    }
-- 
2.7.4

Re: Hollow cursor under images

[Eli Zaretskii]

> From: Evgeny Zajcev <lg.zevlg@gmail.com>
> Date: Mon, 27 Jan 2020 15:54:56 +0300
> Cc: Eli Zaretskii <eliz@gnu.org>, emacs-devel <emacs-devel@gnu.org>
> 
> This patch implements (box . WIDTH) cursor type, allowing user to define his own notion for "Large" image

Thanks.  This needs a NEWS entry and also an update for the ELisp
manual.  See also a few comments below.

> From 6406d198e9f3f5b2d93987ed0fd4fe22bd590624 Mon Sep 17 00:00:00 2001
> From: Zajcev Evgeny <zevlg@yandex.ru>
> Date: Mon, 27 Jan 2020 15:49:46 +0300
> Subject: [PATCH] * Support for (box . WIDTH) `cursor-type'
> 
> Before this commit, block cursor becomes hollow under "large" masked
> images.  Notion for "large" was hardcoded to be image larger then
> 32x32 in any dimension.
> 
> This patch allows user to define his own notion for "large" image,
> taking full control of block cursor look under masked images.
> 
> With cursor-type equal to `box' cursor will always be block under
> masked images.  This differs from former behavior for box cursor.
> To get former behavior, set `cursor-type' to (box . 32)

Please include in the log message a ChangeLog-style list of files and
functions where you are making changes.

> +  (box . WIDTH)   display a filled box cursor, but make it
> +                  hollow if cursor is under masked image larger then
> +                  WIDTH in either dimention.
                                     ^^^^^^^^^
"dimension"

Also, please make it clear that WIDTH is measured in pixels.

> +Except for (box . WIDTH) case.

This is not a complete sentence, please clarify it.

> +  if (CONSP (arg)
> +      && EQ (XCAR (arg), Qbox)
> +      && RANGED_FIXNUMP (0, XCDR (arg), INT_MAX))
> +    {
> +      *width = XFIXNUM (XCDR (arg));

This calls XFIXNUM no less than 3 times.  I wonder if we could tweak
the code to do that only once.

> +      return FILLED_BOX_CURSOR;
> +    }
> +
>    if (EQ (arg, Qhollow))
>      return HOLLOW_BOX_CURSOR;

If the condition above is not fulfilled, you then make the code do 5
more gratuitous comparisons, before it falls back on
HOLLOW_BOX_CURSOR.  It is much better to return HOLLOW_BOX_CURSOR
right away if the conditions for FILLED_BOX_CURSOR weren't satisfied,
it makes the code which implements this feature much easier to read
and understand.

Re: Hollow cursor under images

[Evgeny Zajcev]

[-- Attachment #1: Type: text/plain, Size: 2889 bytes --]

пн, 27 янв. 2020 г. в 21:33, Eli Zaretskii <eliz@gnu.org>:

> > From: Evgeny Zajcev <lg.zevlg@gmail.com>
> > Date: Mon, 27 Jan 2020 15:54:56 +0300
> > Cc: Eli Zaretskii <eliz@gnu.org>, emacs-devel <emacs-devel@gnu.org>
> >
> > This patch implements (box . WIDTH) cursor type, allowing user to define
> his own notion for "Large" image
>
> Thanks.  This needs a NEWS entry and also an update for the ELisp
> manual.  See also a few comments below.
>
> > From 6406d198e9f3f5b2d93987ed0fd4fe22bd590624 Mon Sep 17 00:00:00 2001
> > From: Zajcev Evgeny <zevlg@yandex.ru>
> > Date: Mon, 27 Jan 2020 15:49:46 +0300
> > Subject: [PATCH] * Support for (box . WIDTH) `cursor-type'
> >
> > Before this commit, block cursor becomes hollow under "large" masked
> > images.  Notion for "large" was hardcoded to be image larger then
> > 32x32 in any dimension.
> >
> > This patch allows user to define his own notion for "large" image,
> > taking full control of block cursor look under masked images.
> >
> > With cursor-type equal to `box' cursor will always be block under
> > masked images.  This differs from former behavior for box cursor.
> > To get former behavior, set `cursor-type' to (box . 32)
>
> Please include in the log message a ChangeLog-style list of files and
> functions where you are making changes.
>
> > +  (box . WIDTH)   display a filled box cursor, but make it
> > +                  hollow if cursor is under masked image larger then
> > +                  WIDTH in either dimention.
>                                      ^^^^^^^^^
> "dimension"
>
> Also, please make it clear that WIDTH is measured in pixels.
>
> > +Except for (box . WIDTH) case.
>
> This is not a complete sentence, please clarify it.
>
> > +  if (CONSP (arg)
> > +      && EQ (XCAR (arg), Qbox)
> > +      && RANGED_FIXNUMP (0, XCDR (arg), INT_MAX))
> > +    {
> > +      *width = XFIXNUM (XCDR (arg));
>
> This calls XFIXNUM no less than 3 times.  I wonder if we could tweak
> the code to do that only once.
>
>
Just for my info.  Should not such tweaks be done by compiler optimizer?
No value is declared as volatile, it is pretty clear that there is no need
to call XFIXNUM 3 times.  It is written in sources just for the readability.

I'm afraid tweaking things in such way in source code will make code look
ugly and less obvious

> +      return FILLED_BOX_CURSOR;
> > +    }
> > +
> >    if (EQ (arg, Qhollow))
> >      return HOLLOW_BOX_CURSOR;
>
> If the condition above is not fulfilled, you then make the code do 5
> more gratuitous comparisons, before it falls back on
> HOLLOW_BOX_CURSOR.  It is much better to return HOLLOW_BOX_CURSOR
> right away if the conditions for FILLED_BOX_CURSOR weren't satisfied,
> it makes the code which implements this feature much easier to read
> and understand.
>


-- 
lg

[-- Attachment #2: Type: text/html, Size: 4023 bytes --]

Re: Hollow cursor under images

[Evgeny Zajcev]

[-- Attachment #1.1: Type: text/plain, Size: 2634 bytes --]

пн, 27 янв. 2020 г. в 21:33, Eli Zaretskii <eliz@gnu.org>:

> > From: Evgeny Zajcev <lg.zevlg@gmail.com>
> > Date: Mon, 27 Jan 2020 15:54:56 +0300
> > Cc: Eli Zaretskii <eliz@gnu.org>, emacs-devel <emacs-devel@gnu.org>
> >
> > This patch implements (box . WIDTH) cursor type, allowing user to define
> his own notion for "Large" image
>
> Thanks.  This needs a NEWS entry and also an update for the ELisp
> manual.  See also a few comments below.
>
> > From 6406d198e9f3f5b2d93987ed0fd4fe22bd590624 Mon Sep 17 00:00:00 2001
> > From: Zajcev Evgeny <zevlg@yandex.ru>
> > Date: Mon, 27 Jan 2020 15:49:46 +0300
> > Subject: [PATCH] * Support for (box . WIDTH) `cursor-type'
> >
> > Before this commit, block cursor becomes hollow under "large" masked
> > images.  Notion for "large" was hardcoded to be image larger then
> > 32x32 in any dimension.
> >
> > This patch allows user to define his own notion for "large" image,
> > taking full control of block cursor look under masked images.
> >
> > With cursor-type equal to `box' cursor will always be block under
> > masked images.  This differs from former behavior for box cursor.
> > To get former behavior, set `cursor-type' to (box . 32)
>
> Please include in the log message a ChangeLog-style list of files and
> functions where you are making changes.
>
> > +  (box . WIDTH)   display a filled box cursor, but make it
> > +                  hollow if cursor is under masked image larger then
> > +                  WIDTH in either dimention.
>                                      ^^^^^^^^^
> "dimension"
>
> Also, please make it clear that WIDTH is measured in pixels.
>
> > +Except for (box . WIDTH) case.
>
> This is not a complete sentence, please clarify it.
>
> > +  if (CONSP (arg)
> > +      && EQ (XCAR (arg), Qbox)
> > +      && RANGED_FIXNUMP (0, XCDR (arg), INT_MAX))
> > +    {
> > +      *width = XFIXNUM (XCDR (arg));
>
> This calls XFIXNUM no less than 3 times.  I wonder if we could tweak
> the code to do that only once.
>
> > +      return FILLED_BOX_CURSOR;
> > +    }
> > +
> >    if (EQ (arg, Qhollow))
> >      return HOLLOW_BOX_CURSOR;
>
> If the condition above is not fulfilled, you then make the code do 5
> more gratuitous comparisons, before it falls back on
> HOLLOW_BOX_CURSOR.  It is much better to return HOLLOW_BOX_CURSOR
> right away if the conditions for FILLED_BOX_CURSOR weren't satisfied,
> it makes the code which implements this feature much easier to read
> and understand.
>

I tried to fulfill all the review comments in this updated patch

Thanks

-- 
lg

[-- Attachment #1.2: Type: text/html, Size: 3609 bytes --]

[-- Attachment #2: 0001-Support-for-box-.-WIDTH-cursor-type.patch --]
[-- Type: text/x-patch, Size: 5863 bytes --]

From d4b74b56d7fb3fa1ac48d5049b18ac126e800d60 Mon Sep 17 00:00:00 2001
From: Zajcev Evgeny <zevlg@yandex.ru>
Date: Mon, 27 Jan 2020 15:49:46 +0300
Subject: [PATCH] Support for (box . WIDTH) `cursor-type'

* buffer.c (cursor-type): Add commentary about (box . WIDTH)
  cursor-type

* xdisp.c (get_specified_cursor_type): Check for `cursor-type' in form
  (box . WIDTH)

* xdisp.c (get_window_cursor_type): Check masked image size for
  (box . WIDTH) cursor-type.

* doc/emacs/display.texi, doc/emacs/display.texi: Add description
  for (box . WIDTH) `cursor-type'

* etc/NEWS: Add note about (box . WIDTH) `cursor-type'
---
 doc/emacs/display.texi  |  2 ++
 doc/lispref/frames.texi |  3 +++
 etc/NEWS                |  5 +++++
 src/buffer.c            |  5 +++++
 src/xdisp.c             | 28 ++++++++++++++--------------
 5 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/doc/emacs/display.texi b/doc/emacs/display.texi
index cb37ef4..bbfa938 100644
--- a/doc/emacs/display.texi
+++ b/doc/emacs/display.texi
@@ -1647,6 +1647,8 @@ Cursor Display
 the text shown under the cursor is drawn using the frame's background
 color.)  To change its shape, customize the buffer-local variable
 @code{cursor-type}; possible values are @code{box} (the default),
+@code{(box . @var{width})} (box cursor becoming a hollow box under
+masked images larger than @var{width} in either dimension),
 @code{hollow} (a hollow box), @code{bar} (a vertical bar), @code{(bar
 . @var{n})} (a vertical bar @var{n} pixels wide), @code{hbar} (a
 horizontal bar), @code{(hbar . @var{n})} (a horizontal bar @var{n}
diff --git a/doc/lispref/frames.texi b/doc/lispref/frames.texi
index 81a3dc6..67a4998 100644
--- a/doc/lispref/frames.texi
+++ b/doc/lispref/frames.texi
@@ -2219,6 +2219,9 @@ Cursor Parameters
 @table @code
 @item box
 Display a filled box.  (This is the default.)
+@item (box . @var{width})
+Display a filled box.  However, display it as a hollow box if point is
+under masked image larger than @var{width} in either dimension.
 @item hollow
 Display a hollow box.
 @item nil
diff --git a/etc/NEWS b/etc/NEWS
index dd33950..9cc0b1f 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -30,6 +30,11 @@ applies, and please also update docstrings as needed.
 \f
 * Changes in Emacs 28.1
 
+** Support for `(box . WIDTH)' cursor-type.  By default, `box' cursor
+always has a filled box shape.  Unless you specify cursor-type to be
+`(box . WIDTH)'.  In such case, cursor becomes a hollow box if the
+point is under masked image larger than `WIDTH' in any dimension.
+
 \f
 * Editing Changes in Emacs 28.1
 
diff --git a/src/buffer.c b/src/buffer.c
index 80eaa97..a490d5f 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -6247,6 +6247,9 @@ from (abs POSITION).  If POSITION is positive, point was at the front
   t               use the cursor specified for the frame
   nil             don't display a cursor
   box             display a filled box cursor
+  (box . WIDTH)   display a filled box cursor, but make it
+                  hollow if cursor is under masked image larger than
+                  WIDTH pixels in either dimension.
   hollow          display a hollow box cursor
   bar             display a vertical bar cursor with default width
   (bar . WIDTH)   display a vertical bar cursor with width WIDTH
@@ -6255,6 +6258,8 @@ from (abs POSITION).  If POSITION is positive, point was at the front
   ANYTHING ELSE   display a hollow box cursor
 
 WIDTH and HEIGHT can't exceed the frame's canonical character size.
+Except for (box . WIDTH) case, where WIDTH specifies the size of a
+masked image, not the size of the cursor.
 
 When the buffer is displayed in a non-selected window, the
 cursor's appearance is instead controlled by the variable
diff --git a/src/xdisp.c b/src/xdisp.c
index 3080f89..eb2bf1c 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -30692,14 +30692,6 @@ get_specified_cursor_type (Lisp_Object arg, int *width)
       return BAR_CURSOR;
     }
 
-  if (CONSP (arg)
-      && EQ (XCAR (arg), Qbar)
-      && RANGED_FIXNUMP (0, XCDR (arg), INT_MAX))
-    {
-      *width = XFIXNUM (XCDR (arg));
-      return BAR_CURSOR;
-    }
-
   if (EQ (arg, Qhbar))
     {
       *width = 2;
@@ -30707,11 +30699,16 @@ get_specified_cursor_type (Lisp_Object arg, int *width)
     }
 
   if (CONSP (arg)
-      && EQ (XCAR (arg), Qhbar)
       && RANGED_FIXNUMP (0, XCDR (arg), INT_MAX))
     {
       *width = XFIXNUM (XCDR (arg));
-      return HBAR_CURSOR;
+
+      if (EQ (XCAR (arg), Qbox))
+          return FILLED_BOX_CURSOR;
+      else if (EQ (XCAR (arg), Qbar))
+          return BAR_CURSOR;
+      else if (EQ (XCAR (arg), Qhbar))
+          return HBAR_CURSOR;
     }
 
   /* Treat anything unknown as "hollow box cursor".
@@ -30849,12 +30846,15 @@ get_window_cursor_type (struct window *w, struct glyph *glyph, int *width,
 	      struct image *img = IMAGE_OPT_FROM_ID (f, glyph->u.img_id);
 	      if (img != NULL && IMAGEP (img->spec))
 		{
-		  /* Arbitrarily, interpret "Large" as >32x32 and >NxN
+		  /* Interpret "large" as >WIDTHxWIDTH and >NxN
+                     where WIDTH is the value from cursor-type in form (box . WIDTH),
 		     where N = size of default frame font size.
-		     This should cover most of the "tiny" icons people may use.  */
+                     So, setting cursor-type to (box . 32) should cover most of
+                     the "tiny" icons people may use.  */
 		  if (!img->mask
-		      || img->width > max (32, WINDOW_FRAME_COLUMN_WIDTH (w))
-		      || img->height > max (32, WINDOW_FRAME_LINE_HEIGHT (w)))
+                      || (CONSP (BVAR (b, cursor_type))
+                          && img->width > max (*width, WINDOW_FRAME_COLUMN_WIDTH (w))
+                          && img->height > max (*width, WINDOW_FRAME_LINE_HEIGHT (w))))
 		    cursor_type = HOLLOW_BOX_CURSOR;
 		}
 	    }
-- 
2.7.4

Re: Hollow cursor under images

[Eli Zaretskii]

> From: Evgeny Zajcev <lg.zevlg@gmail.com>
> Date: Tue, 28 Jan 2020 11:46:15 +0300
> Cc: Alan Third <alan@idiocy.org>, emacs-devel <emacs-devel@gnu.org>
> 
>  > +  if (CONSP (arg)
>  > +      && EQ (XCAR (arg), Qbox)
>  > +      && RANGED_FIXNUMP (0, XCDR (arg), INT_MAX))
>  > +    {
>  > +      *width = XFIXNUM (XCDR (arg));
> 
>  This calls XFIXNUM no less than 3 times.  I wonder if we could tweak
>  the code to do that only once.
> 
> Just for my info.  Should not such tweaks be done by compiler optimizer?  No value is declared as volatile, it is
> pretty clear that there is no need to call XFIXNUM 3 times.

You forget the unoptimized build case.  And in any case, it is
un-economical and inelegant, IMO, to write such code.  Granted, that
is a stylistic preference of mine to some extent, so if you feel
strongly about that, I don't think I will fight you.

> I'm afraid tweaking things in such way in source code will make code look ugly and less obvious

I don't think so.  Here's what I'd do in this case:

  if (CONSP (arg))
    {
      Lisp_Object style = XCAR (arg);
      Lisp_Object lval = XCDR (arg);
      ptrdiff_t val = FIXNUMP (lval) ? XFIXNUM (lval) : -1;
      if (0 <= val && val < INT_MAX)
        {
	  *width = val;
	  if (EQ (style, Qbox))
	    return FILLED_BOX_CURSOR;
	  else if (EQ (style, Qbar))
	    return BAR_CURSOR;
	  else if (EQ (style, Qhbar))
	    return HBAR_CURSOR;
	}
    }

Btw, one other advantage of the above is that you can easily display
in the debugger each intermediate value used by this calculation,
while stepping through the code.  Again, a minor convenience, but
convenience nonetheless.

Re: Hollow cursor under images

[Eli Zaretskii]

> From: Evgeny Zajcev <lg.zevlg@gmail.com>
> Date: Tue, 28 Jan 2020 14:55:39 +0300
> Cc: Alan Third <alan@idiocy.org>, emacs-devel <emacs-devel@gnu.org>
> 
> I tried to fulfill all the review comments in this updated patch

Thanks.  A few minor comments below.

> * buffer.c (cursor-type): Add commentary about (box . WIDTH)
>   cursor-type

End this sentence with a period.

> * xdisp.c (get_specified_cursor_type): Check for `cursor-type' in form
>   (box . WIDTH)

Likewise.

> * xdisp.c (get_window_cursor_type): Check masked image size for
>   (box . WIDTH) cursor-type.

Changes in several functions in the same file should mention the file
name just once.  Like this:

  * xdisp.c (get_specified_cursor_type): Check for `cursor-type' in form
  '(box . WIDTH)'.
  ((get_window_cursor_type): Check masked image size for(box . WIDTH)
  cursor-type.

> * doc/emacs/display.texi, doc/emacs/display.texi: Add description
>   for (box . WIDTH) `cursor-type'
> 
> * etc/NEWS: Add note about (box . WIDTH) `cursor-type'

Period missing at end of each sentence.

> +@code{(box . @var{width})} (box cursor becoming a hollow box under
> +masked images larger than @var{width} in either dimension),

I think it's better to use "size", not 'width" here, because "width"
is closely associated with only one dimension, and you mean both.  Or
maybe even "min-size", for better mnemonic value.

> +** Support for `(box . WIDTH)' cursor-type.  By default, `box' cursor
> +always has a filled box shape.  Unless you specify cursor-type to be
> +`(box . WIDTH)'.  In such case, cursor becomes a hollow box if the
> +point is under masked image larger than `WIDTH' in any dimension.

The first sentence of a NEWS entry should be alone on the header
line.  That's because NEWS is visited in Outline mode, where users can
hide the text of the entry, leaving just the headings.  So leave only
the first sentence on the heading line, and move all the rest to the
body starting from the next line.

Also, we prefer quoting 'like this' in NEWS (and in log messages,
btw).

Re: Hollow cursor under images

[Evgeny Zajcev]

[-- Attachment #1.1: Type: text/plain, Size: 2393 bytes --]

вт, 28 янв. 2020 г. в 21:22, Eli Zaretskii <eliz@gnu.org>:

> > From: Evgeny Zajcev <lg.zevlg@gmail.com>
> > Date: Tue, 28 Jan 2020 14:55:39 +0300
> > Cc: Alan Third <alan@idiocy.org>, emacs-devel <emacs-devel@gnu.org>
> >
> > I tried to fulfill all the review comments in this updated patch
>
> Thanks.  A few minor comments below.
>
> > * buffer.c (cursor-type): Add commentary about (box . WIDTH)
> >   cursor-type
>
> End this sentence with a period.
>
> > * xdisp.c (get_specified_cursor_type): Check for `cursor-type' in form
> >   (box . WIDTH)
>
> Likewise.
>
> > * xdisp.c (get_window_cursor_type): Check masked image size for
> >   (box . WIDTH) cursor-type.
>
> Changes in several functions in the same file should mention the file
> name just once.  Like this:
>
>   * xdisp.c (get_specified_cursor_type): Check for `cursor-type' in form
>   '(box . WIDTH)'.
>   ((get_window_cursor_type): Check masked image size for(box . WIDTH)
>   cursor-type.
>
> > * doc/emacs/display.texi, doc/emacs/display.texi: Add description
> >   for (box . WIDTH) `cursor-type'
> >
> > * etc/NEWS: Add note about (box . WIDTH) `cursor-type'
>
> Period missing at end of each sentence.
>
> > +@code{(box . @var{width})} (box cursor becoming a hollow box under
> > +masked images larger than @var{width} in either dimension),
>
> I think it's better to use "size", not 'width" here, because "width"
> is closely associated with only one dimension, and you mean both.  Or
> maybe even "min-size", for better mnemonic value.
>
>
Yes! I like "size" name too


> > +** Support for `(box . WIDTH)' cursor-type.  By default, `box' cursor
> > +always has a filled box shape.  Unless you specify cursor-type to be
> > +`(box . WIDTH)'.  In such case, cursor becomes a hollow box if the
> > +point is under masked image larger than `WIDTH' in any dimension.
>
> The first sentence of a NEWS entry should be alone on the header
> line.  That's because NEWS is visited in Outline mode, where users can
> hide the text of the entry, leaving just the headings.  So leave only
> the first sentence on the heading line, and move all the rest to the
> body starting from the next line.
>
> Also, we prefer quoting 'like this' in NEWS (and in log messages,
> btw).
>

Here is the updated version, tried to fix all the formatting issues.

-- 
lg

[-- Attachment #1.2: Type: text/html, Size: 3389 bytes --]

[-- Attachment #2: 0001-Support-for-box-.-SIZE-cursor-type.patch --]
[-- Type: text/x-patch, Size: 5486 bytes --]

From be2b70bd3995f7dd333708ed4a973239b78e13cc Mon Sep 17 00:00:00 2001
From: Zajcev Evgeny <zevlg@yandex.ru>
Date: Mon, 27 Jan 2020 15:49:46 +0300
Subject: [PATCH] Support for (box . SIZE) `cursor-type'

To control minimum size of a masked image under which box cursor
becomes hollow.

* buffer.c (cursor-type): Add commentary about (box . SIZE)
  cursor-type.

* xdisp.c (get_specified_cursor_type): Check for `cursor-type' in form
  (box . SIZE).
  (get_window_cursor_type): Check masked image size for
  (box . SIZE) cursor-type.

* doc/emacs/display.texi, doc/emacs/display.texi: Add description
  for (box . SIZE) `cursor-type'.

* etc/NEWS: Add note about (box . SIZE) `cursor-type'.
---
 doc/emacs/display.texi  |  2 ++
 doc/lispref/frames.texi |  3 +++
 etc/NEWS                |  6 ++++++
 src/buffer.c            |  3 +++
 src/xdisp.c             | 28 ++++++++++++++--------------
 5 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/doc/emacs/display.texi b/doc/emacs/display.texi
index 8444aef..dc6fe3a 100644
--- a/doc/emacs/display.texi
+++ b/doc/emacs/display.texi
@@ -1654,6 +1654,8 @@ Cursor Display
 the text shown under the cursor is drawn using the frame's background
 color.)  To change its shape, customize the buffer-local variable
 @code{cursor-type}; possible values are @code{box} (the default),
+@code{(box . @var{SIZE})} (box cursor becoming a hollow box under
+masked images larger than @var{SIZE} pixels in either dimension),
 @code{hollow} (a hollow box), @code{bar} (a vertical bar), @code{(bar
 . @var{n})} (a vertical bar @var{n} pixels wide), @code{hbar} (a
 horizontal bar), @code{(hbar . @var{n})} (a horizontal bar @var{n}
diff --git a/doc/lispref/frames.texi b/doc/lispref/frames.texi
index 2bb505c..70ebe2e 100644
--- a/doc/lispref/frames.texi
+++ b/doc/lispref/frames.texi
@@ -2220,6 +2220,9 @@ Cursor Parameters
 @table @code
 @item box
 Display a filled box.  (This is the default.)
+@item (box . @var{SIZE})
+Display a filled box.  However, display it as a hollow box if point is
+under masked image larger than @var{SIZE} pixels in either dimension.
 @item hollow
 Display a hollow box.
 @item nil
diff --git a/etc/NEWS b/etc/NEWS
index 5395f2c..27ea895 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -64,6 +64,12 @@ It was declared obsolete in Emacs 27.1.
 \f
 * Changes in Emacs 28.1
 
+** Support for '(box . SIZE)' cursor-type.
+By default, 'box' cursor always has a filled box shape.  Unless you
+specify cursor-type to be '(box . SIZE)'.  In such case, cursor
+becomes a hollow box if the point is under masked image larger than
+'SIZE' pixels in any dimension.
+
 \f
 * Editing Changes in Emacs 28.1
 
diff --git a/src/buffer.c b/src/buffer.c
index 5c65d4d..cc7d4e4 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -6247,6 +6247,9 @@ from (abs POSITION).  If POSITION is positive, point was at the front
   t               use the cursor specified for the frame
   nil             don't display a cursor
   box             display a filled box cursor
+  (box . SIZE)    display a filled box cursor, but make it
+                  hollow if cursor is under masked image larger than
+                  SIZE pixels in either dimension.
   hollow          display a hollow box cursor
   bar             display a vertical bar cursor with default width
   (bar . WIDTH)   display a vertical bar cursor with width WIDTH
diff --git a/src/xdisp.c b/src/xdisp.c
index 68a504f..038b8e5 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -30741,14 +30741,6 @@ get_specified_cursor_type (Lisp_Object arg, int *width)
       return BAR_CURSOR;
     }
 
-  if (CONSP (arg)
-      && EQ (XCAR (arg), Qbar)
-      && RANGED_FIXNUMP (0, XCDR (arg), INT_MAX))
-    {
-      *width = XFIXNUM (XCDR (arg));
-      return BAR_CURSOR;
-    }
-
   if (EQ (arg, Qhbar))
     {
       *width = 2;
@@ -30756,11 +30748,16 @@ get_specified_cursor_type (Lisp_Object arg, int *width)
     }
 
   if (CONSP (arg)
-      && EQ (XCAR (arg), Qhbar)
       && RANGED_FIXNUMP (0, XCDR (arg), INT_MAX))
     {
       *width = XFIXNUM (XCDR (arg));
-      return HBAR_CURSOR;
+
+      if (EQ (XCAR (arg), Qbox))
+          return FILLED_BOX_CURSOR;
+      else if (EQ (XCAR (arg), Qbar))
+          return BAR_CURSOR;
+      else if (EQ (XCAR (arg), Qhbar))
+          return HBAR_CURSOR;
     }
 
   /* Treat anything unknown as "hollow box cursor".
@@ -30898,12 +30895,15 @@ get_window_cursor_type (struct window *w, struct glyph *glyph, int *width,
 	      struct image *img = IMAGE_OPT_FROM_ID (f, glyph->u.img_id);
 	      if (img != NULL && IMAGEP (img->spec))
 		{
-		  /* Arbitrarily, interpret "Large" as >32x32 and >NxN
+		  /* Interpret "large" as >SIZExSIZE and >NxN
+                     where SIZE is the value from cursor-type in form (box . SIZE),
 		     where N = size of default frame font size.
-		     This should cover most of the "tiny" icons people may use.  */
+                     So, setting cursor-type to (box . 32) should cover most of
+                     the "tiny" icons people may use.  */
 		  if (!img->mask
-		      || img->width > max (32, WINDOW_FRAME_COLUMN_WIDTH (w))
-		      || img->height > max (32, WINDOW_FRAME_LINE_HEIGHT (w)))
+                      || (CONSP (BVAR (b, cursor_type))
+                          && img->width > max (*width, WINDOW_FRAME_COLUMN_WIDTH (w))
+                          && img->height > max (*width, WINDOW_FRAME_LINE_HEIGHT (w))))
 		    cursor_type = HOLLOW_BOX_CURSOR;
 		}
 	    }
-- 
2.7.4

Re: Hollow cursor under images

[Eli Zaretskii]

> From: Evgeny Zajcev <lg.zevlg@gmail.com>
> Date: Mon, 3 Feb 2020 14:24:01 +0300
> Cc: Alan Third <alan@idiocy.org>, emacs-devel <emacs-devel@gnu.org>
> 
> Here is the updated version, tried to fix all the formatting issues.

Thanks, I pushed this to the master branch.

There were minor issues with formatting, which I fixed in the
follow-up commit; please see what I changed and try to follow this
style in the future.