Re: x-export-frames for non-Cairo builds

[Eli Zaretskii]

> From: Clément Pit-Claudel <address@hidden>
> Date: Fri, 26 Jan 2018 18:13:28 -0500
> 
> Understood, thanks.  Here's a first draft of the patch.  I have a few 
> questions:

Thanks, and apologies for a long delay in responding.

> * Is there a way to wrap 'attributes: noreturn' in a preprocessor directive? 
> I used an auxilliary C function instead because of that.

I don't understand why you needed that attribute.  Can you elaborate?

In general, the auxiliary function is short enough to make the
function redundant: just put its body inside Fexport_frame.

> * Is it OK to reuse `frame' after calling `decode_window_system_frame 
> (frame)'?

Yes.  That function doesn't change its argument, it only extracts a C
pointer from it.

> * What would be the proper way to save the output of Fx_export_frame (there's 
> a FIXME at that point in the code)?  I could either use a_write on the actual 
> string data, or add further branches to x_cr_export_frame to write to disk 
> directly.

Just open a file and write the data into it.

> * x_cr_export_frame does a number of things that I don't understand too well:
>     specbind (Qredisplay_dont_pause, Qt);
>     redisplay_preserve_echo_area (31);
>     block_input ();
>     record_unwind_protect (x_cr_destroy, make_save_ptr (cr));
>     x_clear_area (f, 0, 0, width, height);
>     expose_frame (f, 0, 0, width, height);
>   Do I need any of these in x_gtk3_export_frame?

These measures make sure the frame is fully visible and completely
redrawn, before you export it, so I think you do need them.

The call to record_unwind_protect is needed because otherwise the
data allocated by cairo_create will not be released if the function
signals an error, or the user types C-g.  If none of the objects
created inside x_gtk3_export_frame need to be released (the code
indicates they don't, but maybe you forgot something), then you don't
need a similar call to record_unwind_protect.

> * The docs of gdk_pixbuf_get_from_window say that "If the window you’re 
> obtaining data from is partially obscured by other windows, then the contents 
> of the pixbuf areas corresponding to the obscured regions are undefined".  Is 
> there a way I can check for that?

I think the above measures eliminate this problem by calling
expose_frame.

> I made it possible for the function to return a string instead of writing to 
> disk to save time (I'm hoping to make Emacs screencasts).  One issue is that 
> the cast to an Emacs string is still quite slow (quick benchmarks: I did 200 
> screenshots with xwd, with my new function saving a png and then a bmp into a 
> string, and with my new function saving a png and then a bmp to disk: 1.82s, 
> 5.1s, 1.84s, 5.2s, 0.7s[!]).  Is there a trick I could use to make image 
> capture faster?  Could I store the GdkPixbuf directly into an Emacs image and 
> save it later?

Sorry, I don't know enough about GTK programming to answer that.

> On 2018-01-26 09:45, Stefan Monnier wrote:
> > And which code to use should be based on dispatching on the frame type
> > (so it can work even if we have several different kinds of frame types:
> 
> I'm not sure I understood this right.  Does the patch below do what you had 
> in mind?

I believe so.

Some more comments below.

> 
> Cheers,
> Clément.
> 
> [2:text/x-patch Hide Save:export-frame.patch (6kB)]
> 
> diff --git a/src/frame.c b/src/frame.c
> index 9b56080..f0c0d34 100644
> --- a/src/frame.c
> +++ b/src/frame.c
> @@ -3508,6 +3508,67 @@ bottom edge of FRAME's display.  */)
>  
>    return Qt;
>  }
> +
> +#ifdef HAVE_WINDOW_SYSTEM
> +static
> +#if ! (defined HAVE_GTK3 || defined USE_CAIRO)
> +_Noreturn
> +#endif
> +Lisp_Object
> +export_frame (Lisp_Object frame, Lisp_Object type, Lisp_Object fname)
> +{
> +  struct frame *f = decode_window_system_frame (frame);
> +
> +  if (!FRAME_VISIBLE_P (f))
> +    error ("Frames to be exported must be visible");
> +
> +  if (FRAME_OBSCURED_P (f))
> +    error ("Frames to be exported must not be obscured");
> +
> +  if (!NILP(fname))
> +      CHECK_STRING(fname);

The Cairo code in x-export-frames also does these checks, so why not
call x_cr_export_frame directly?

> +#ifdef USE_CAIRO
> +      // FIXME: save output when FNAME is non-nil
> +      /* Question: Is it OK to reuse FRAME here? */
> +      return Fx_export_frames(frame, type);
> +#elif HAVE_GTK3
> +      return x_gtk3_export_frame(f, type, fname);
> +#endif

Here and elsewhere please make sure you leave a blank between the name
of a function/macro and the opening parenthesis.

> +      /* Fall through */
> +    case output_initial:
> +    case output_termcap:
> +    case output_w32:
> +    case output_msdos_raw:
> +    case output_ns:
> +    default:
> +      error ("Unsupported toolkit");

I think implementation for text-mode frames should not be too hard:
you could reuse some of the code in dump-frame-glyph-matrix and its
subroutines.  The result should be a text file, of course.

Also "Unsupported toolkit" is slightly misleading, it would be better
to say something like

  "This command does not yet support frames of type %s"

with a suitable string replacing %s.

> +DEFUN ("export-frame", Fexport_frame,
> +       Sexport_frame, 0, 3, 0,
> +       doc: /* Capture a screenshot of FRAME in TYPE format.
> +Available formats depend on the graphic toolkit in use.

This last sentence should be moved to after the description of TYPE.
(And I would call the argument FORMAT instead.)

> +Currently, this function only works with Cairo and GTK3.
> +
> +FRAME must be a live, visible, and unobscured frame.  It defaults to

Partially obscured frames are okay, right?  If so, the doc string
should tell that.

> +If FNAME is non-nil, save the resulting capture under FNAME; if
> +FNAME is nil, return the captured data as a unibyte string.  */)

This never says explicitly that FNAME is a file name.  Either tell
that or call the argument FILE.

Also, "save in file FNAME" is better; "under" is a strange way of
saying this.