bug#80281: 31.0.50; FR: Canvas object as display property

> From: Divya Ranjan Pattanaik <[email protected]>
> Cc: [email protected], Daniel Mendler <[email protected]>,
>  [email protected], [email protected], [email protected],
>  [email protected]
> Date: Sat, 01 Aug 2026 14:17:15 +0000
> 
> Hello Eli and others,
> 
> Daniel and I have been working on a revision of the canvas patch over the last few months, which we submit here. Several of Eli's and others' suggestions have been incorporated, such as the following:
> 
> - Creating a canvas image from Elisp via create-image or directly building the image spec
> - Canvas can be loaded from :file and :data via create-image
> - Supported and tested on multiple GNU/Linux ports (PGTK, X11, Lucid), MacOS and Windows.
> - The redrawing/refreshing logic of canvases has been made more robust. Specifically, if the frame is garbaged we wait for next full redisplay.
> 
> Apart from the above, the patch also contains a test suite, and more documentation. We have tried to keep the changes minimal and as non-invasive as possible.
> 
> Looking forward to hear your thoughts.

Thanks, my comments are below.  I'd appreciate review by Alan as well
(and anyone else who might have comments).

> +Once a canvas image object has been created and displayed, it can be
> +``refreshed'' via @code{canvas-refresh}. If an optional non-@code{nil}
> +argument is passed to @code{canvas-refresh} then Emacs will reload the
> +@code{:data} or @code{:file} before redrawing the canvas.

Please document canvas-refresh using @defun, as we do with every other
function in the manual.

> +@deftypefn Function uint32_t *canvas_data (emacs_env *@var{env}, emacs_value @var{canvas})
> +The function gives access to the pixel buffer of @var{canvas}.  The
> +pixel buffer is in row-major order with a size @var{width} * @var{height}.
> +The pixel format is ARGB32 on all platforms.  Return @code{NULL} in case
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Is this factually correct?  What about Windows, where the native
format is BGR AFAIU?  Or is the above correct only because Windows is
not currently supported?

> +Note that the pixel buffer is only valid as the canvas object is alive
                                            ^^
"as long as"

> +and its dimensions have not been changed. If it has changed, Emacs will

Please always leave two spaces between sentences, here and elsewhere
in the patch, including manuals, doc strings, comments, commit log
messages, etc.

> +@example
> +/* Access the pixel buffer */
> +uint32_t *pixel = env->canvas_data (env, canvas);
> +/* Write to the pixel buffer */
> +...
> +/* Redraw the canvas */
> +env->funcall(env, env->intern(env, "canvas-refresh"), 1, &canvas);
> +@end example
> +
> +Note that @code{canvas-refresh} has an optional boolean argument to
> +reload the @code{:data} vector or string from the canvas image
> +specification.  When called from dynamic modules, the argument should
> +usually be @code{nil}, thus not provided.

I find it a better convention to always provider optional arguments,
as Qnil.

> +Thus, one can simply define a module function that takes as an argument
> +a canvas image object, following the specification as laid down in
> +@xref{Canvas Images}, and then use the above code to manipulate the pixel
   ^^^^^
@ref, nor @xref.

> +extern uint32_t* canvas_data (Lisp_Object);

Our style is to put the '*' at the beginning of the identifier, like
this:

  extern uint32_t *canvas_data (Lisp_Object);

There are numerous places where you place the '*' as above, so please
make sure to fix all of them.  For example, this:

> +static uint32_t*
> +module_canvas_data (emacs_env *env, emacs_value canvas)

should be written like this:

  +static uint32_t *
  +module_canvas_data (emacs_env *env, emacs_value canvas)

> +struct canvas
> +{
> +  /* Linked list of canvases, see `canvas_list'.  */
> +  struct canvas *next;
> +  /* Pixel memory buffer in ARGB32 format across all platforms.  */
                               ^^^^^^
Same comment about Windows here.

>  	  /* Some animation-related data doesn't affect display, but
> -	     breaks the image cache.  Filter those out.  */
> -	  if (!(EQ (key, QCanimate_buffer)
> -		|| EQ (key, QCanimate_tardiness)
> -		|| EQ (key, QCanimate_position)))
> +	     breaks the image cache.  Furthermore for canvases do not
> +	     check the data for equality, such that data can be changed
> +	     via mutation to refresh the canvas. Filter these keys out.
                                               ^^
Two spaces here (and elsewhere in comments), please.

> +`redisplay' must be called after `canvas-refresh' such that the updated
> +canvas is displayed even if double buffering is enabled.  It can be
> +updated in a loop.
> +
> +Example:
> +
> +  (setq canvas (create-image (make-vector (* 100 100) 0) 'canvas t
> +                             :data-width 100 :data-height 100))
> +  (insert (propertize "#" 'display canvas))
> +  (dotimes (i (* 100 100))
> +    (aset (plist-get (cdr canvas) :data) i #xFF)
> +    (canvas-refresh canvas 'reload-data)
> +    (redisplay))
> +
> +When `canvas-refresh' is called from a timer or a command, `redisplay'
> +will be called implicitly after the timer or command.
> +
> +Example:
> +
> +  (let ((i 0))
> +    (run-at-time nil 0.03 (lambda ()
> +      (when (< i (* 100 100))
> +        (aset (plist-get (cdr canvas) :data) i #xFF)
> +        (incf i)
> +        (canvas-refresh canvas 'reload-data)))))  */)

Please move these examples and notes to the ELisp manual.

> +  for (int y = 0; y < w->current_matrix->nrows; ++y)
> +    {
> +      struct glyph_row *row = w->current_matrix->rows + y;
> +      if (row->enabled_p)
> +	{
> +	  int start = row->x;
> +	  for (int x = 0; x < row->used[TEXT_AREA]; ++x)
> +	    {
> +	      struct glyph *glyph = row->glyphs[TEXT_AREA] + x;
> +	      if (glyph->type == IMAGE_GLYPH)
> +		{
> +		  struct image* img = IMAGE_OPT_FROM_ID (f, glyph->u.img_id);
> +		  if (img && EQ (img->spec, spec))
> +		    {
> +		      prepare_image_for_display (f, img);
> +		      draw_glyphs (w, start, row, TEXT_AREA, x, x + 1, DRAW_NORMAL_TEXT, 0);
> +		    }
> +		}
> +	      start += glyph->pixel_width;

Can canvas images be displayed in the marginal area?  If so, why
doesn't the above redraw them as well?

> +   if (env->non_local_exit_check (env) != emacs_funcall_exit_return)
> +     env->non_local_exit_clear (env); // don't propagate the error to ERT

We don't use C++-style comments in C source files.

> +(defun test-canvas-gen-file (width height pixel)
> +  (let* ((bytes (unibyte-string (logand pixel #xff)
> +                                (logand (ash pixel -8) #xff)
> +                                (logand (ash pixel -16) #xff)
> +                                (logand (ash pixel -24) #xff)))
> +         (coding-system-for-write 'binary))
                                     ^^^^^^^
It is better to use 'no-conversion here.