> From: Evgeny Zajcev <address@hidden>
> Date: Mon, 27 Jan 2020 15:54:56 +0300
> Cc: Eli Zaretskii <address@hidden>, emacs-devel <address@hidden>
>
> 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 <address@hidden>
> 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.