Created attachment 228599 [details] [review]
screenshot: support non-absolute paths when saving screenshots

If a non-absolute path is passed to the screenshot methods, treat it as
a basename for the output image, and automatically try to save it in
$XDG_PICTURES_DIR, falling back to $HOME if it doesn't exist.

Created attachment 228603 [details] [review]
screenshot: don't fire flash if we failed to take picture

Review of attachment 228599 [details] [review]:

To me, the screenshot API is a tiny internal API that's supposed to be used by smart people; it's a way to grab a pixbuf for a place, since the compositor knows more than other people.

It's not supposed to be a smart API that does the right thing. The filename passed here is supposed to be correct.

Review of attachment 228603 [details] [review]:

This obviously makes sense.

Review of attachment 228599 [details] [review]:

::: src/shell-screenshot.c
@@ +102,3 @@
+      idx++;
+    }
+  while (g_file_test (real_path, G_FILE_TEST_EXISTS));

This is racy as well ;)
You need to use creat() and below...

@@ +153,3 @@
+    status = CAIRO_STATUS_FILE_NOT_FOUND;
+  else
+    status = cairo_surface_write_to_png (screenshot_data->image, filename);

... use cairo_surface_write_to_png_stream() to write to the fd you opened.

(In reply to comment #3)
> It's not supposed to be a smart API that does the right thing. The filename
> passed here is supposed to be correct.

If you pass a "correct" filename, you have a race. Take 2 screenshots quickly, and you'll end up with one overwriting the other.

(In reply to comment #6)
> If you pass a "correct" filename, you have a race. Take 2 screenshots quickly,
> and you'll end up with one overwriting the other.

That's because you passed the same filename twice. The code doesn't (and shouldn't) have any logic to determine that. Is keeping track of in-flight screenshot requests that hard?

(In reply to comment #7)
> (In reply to comment #6)
> > If you pass a "correct" filename, you have a race. Take 2 screenshots quickly,
> > and you'll end up with one overwriting the other.
> 
> That's because you passed the same filename twice. The code doesn't (and
> shouldn't) have any logic to determine that. Is keeping track of in-flight
> screenshot requests that hard?

It's shoddy engineering, and means that 2 applications capturing screenshots could clash. Given the amount of code, I'd rather this was done right. Otherwise I'm happy for gnome-settings-daemon to use D-Bus' FD passing to get the data back. Otherwise, I don't see the point. The "tracking" of in-flight screenshots will be fragile, and more code than the one being added to gnome-shell.

What about a screenshot giving you a tmpfile back, which you can move or copy to somewhere appropriate?

(In reply to comment #9)
> What about a screenshot giving you a tmpfile back, which you can move or copy
> to somewhere appropriate?

In the target directory? sure. Not in the temp directory, it might not be in the same partition, and would just add more IO for no reason.

Created attachment 228609 [details] [review]
screenshot: support non-absolute paths when saving screenshots

--

Open stream directly when checking for file existence, to avoid races.

I don't think this warrants changing the API if we can avoid it (we would also need to port gnome-screenshot over...).

This new patch should get rid of the races.

Comment on attachment 228603 [details] [review]
screenshot: don't fire flash if we failed to take picture

Attachment 228603 [details] pushed as 123fb35 - screenshot: don't fire flash if we failed to take picture

Created attachment 231150 [details] [review]
screenshot: support non-absolute paths when saving screenshots

-

Fixes a double free.

Created attachment 231151 [details] [review]
screenshot: change API to return the filename used for saving

Since we also support passing a basename now, clients might be
interested in knowing the path used to save the file.
Add an out argument to the interface for that.

Created attachment 231152 [details] [review]
screenshot: move to a separate interface

Since we're breaking API already, take this as an occasion to use a
separate interface for all the screenshot-related methods. The interface
name is org.gnome.Shell.Screenshot.
Internally, move all the related code to screenshot.js.

Created attachment 231153 [details] [review]
screenshot: document the Screenshot interface

Use gdbus-codegen to generate a docbook for the screenshot interface,
and add it to our gtk-doc.

Created attachment 231154 [details] [review]
screenshot: move to a separate interface

--

Correct patch.

Review of attachment 231154 [details] [review]:

This needs to go before the documentation commit. Otherwise, looks fine.

Review of attachment 231153 [details] [review]:

OK.

Review of attachment 231150 [details] [review]:

::: src/shell-screenshot.c
@@ +75,3 @@
+/* called in an I/O thread */
+static GOutputStream *
+get_unique_path_stream (const gchar *path,

Put the unique path part in a separate commit, please. I'll critique the rest of it there.

Review of attachment 231151 [details] [review]:

OK.

Created attachment 232547 [details] [review]
screenshot: support non-absolute paths when saving screenshots

--

Split the unique filename part out of this.

Created attachment 232548 [details] [review]
screenshot: change API to return the filename used for saving

--

Updated for new patchset.

Created attachment 232549 [details] [review]
screenshot: save to an unique path when using a basename

When a basename is passed, avoid filename conflicts. The unique path is
returned by the API.

Review of attachment 232547 [details] [review]:

::: src/shell-screenshot.c
@@ +85,3 @@
+
+  if (ptr != NULL)
+    real_filename = g_strndup (filename, ptr - filename);

This a bit silly to remove the .png and then add it again. I'd do:

    if (g_str_has_suffix (filename, ".png"))
      {
        real_path = g_build_filename (path, filename, NULL);
      }
    else
      {
        gchar *name = g_strdup_printf (real_filename);
        real_path = g_build_Filename (path, name, NULL);
        g_free (name);
      }

Review of attachment 232548 [details] [review]:

OK.

Review of attachment 232549 [details] [review]:

OK.

Review of attachment 232549 [details] [review]:

Note that I'm not overly happy with the API, but I don't see an easy way to get something like mkstemp but in the directory we want.

(In reply to comment #26)
> Review of attachment 232547 [details] [review]:
> 
> ::: src/shell-screenshot.c
> @@ +85,3 @@
> +
> +  if (ptr != NULL)
> +    real_filename = g_strndup (filename, ptr - filename);
> 
> This a bit silly to remove the .png and then add it again. I'd do:

Indeed it looks silly if you take this patch alone, but it makes more sense after we start ensuring there's an unique path, since we possibly need to add a counter in before the file extension.

Review of attachment 232547 [details] [review]:

OK, then.

Attachment 231153 [details] pushed as e96867f - screenshot: document the Screenshot interface
Attachment 231154 [details] pushed as 57bd43b - screenshot: move to a separate interface
Attachment 232547 [details] pushed as acba0e4 - screenshot: support non-absolute paths when saving screenshots
Attachment 232548 [details] pushed as dcad22b - screenshot: change API to return the filename used for saving
Attachment 232549 [details] pushed as 3a4e595 - screenshot: save to an unique path when using a basename