After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 769833 - Add metadata to screenshots
Add metadata to screenshots
Status: RESOLVED FIXED
Product: gnome-games
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: GNOME Games maintainers
GNOME Games maintainers
: 777771 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2016-08-13 12:38 UTC by Bastien Nocera
Modified: 2017-03-23 15:07 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
retro: Save metadata with screenshots (2.20 KB, patch)
2017-03-15 17:46 UTC, Mateusz Sieczko
needs-work Details | Review
cairo-display: Set pixbuf dpi (1.30 KB, patch)
2017-03-16 19:18 UTC, Mateusz Sieczko
none Details | Review
Save metadata with screenshots (6.05 KB, patch)
2017-03-16 19:19 UTC, Mateusz Sieczko
none Details | Review
cairo-display: Set pixbuf dpi (1.53 KB, patch)
2017-03-16 21:20 UTC, Mateusz Sieczko
committed Details | Review
Take game title as argument when creating RetroRunner (18.48 KB, patch)
2017-03-17 10:28 UTC, Mateusz Sieczko
committed Details | Review
retro: Save metadata with screenshots (2.16 KB, patch)
2017-03-17 10:28 UTC, Mateusz Sieczko
committed Details | Review

Description Bastien Nocera 2016-08-13 12:38:16 UTC
saving screenshots to PNGs using gdk-pixbuf makes saving extra metadata possible, such as the application that created the file.
Comment 1 Adrien Plazas 2017-02-09 15:45:58 UTC
What metadata could be nice to save? I can think of:
- the game's title
- the time when the screenshot has been taken
- the aspect ratio to render the screenshot

I'm pretty sure there are other interesting metadata.
Comment 2 Mateusz Sieczko 2017-03-15 17:46:27 UTC
Created attachment 348021 [details] [review]
retro: Save metadata with screenshots

This include game title, platform name, creation time and Gnome Games
as application used to create screenshot.

"Title" filed is included for portability with tools that displays data
from PNG's tEXt chunk and it combines "Game Title" and "Platform"
fields. It doesn't have mush sense for screenshots, and separate fields
are better (think about "Song Title" and "Album" in mp3/ogg metadata)
but they are non-standard and some tools may not display them (eg.
Nautilus).
Comment 3 Adrien Plazas 2017-03-15 19:34:33 UTC
Review of attachment 348021 [details] [review]:

In the commit message: GNOME should be in uppercase.

Despite the title problem and some styling issues, this patch looks very good and works well for what I tested. Congrats. :)

::: src/retro/retro-runner.vala
@@ +511,2 @@
 		var pixbuf = video.pixbuf;
+

No need to add this newline.

@@ +520,3 @@
+		var uri = media.uri;
+
+		var title = new FilenameTitle (uri).get_title ();

We can't assume that the title is retrieved from the filname (it's not the case for MAME and PlayStation games for example), it would be better to get the title as a construction parameter.

Also we forbid chaining function calls as splitting them improves maintainability and allow us to better check a faulty return.

@@ +529,3 @@
+		// non-standard fields as allowed by PNG specification.
+		pixbuf.save (screenshot_path, "png",
+					 "tEXt::Software",

All of these lines don't follow out indentation rules: we use tabs for indentation and space for alignment, so here it would be 2 tabs and 13 spaces (some editors may reformat them automatically to another style).

Also putting the kay and the value on the same line would be just fine in that case, that would improve readability IMO. :)

@@ +530,3 @@
+		pixbuf.save (screenshot_path, "png",
+					 "tEXt::Software",
+					     "Gnome Games",

GNOME is written an uppercase.

@@ +532,3 @@
+					     "Gnome Games",
+					 "tEXt::Title",
+					     "Screenshot of %s on %s".printf (title, platform_name),

For non-translatable strings we favorite string templates for better readability, so it would be something like:
@"Screenshot of $title on $platform_name",
Comment 4 Mateusz Sieczko 2017-03-16 19:18:36 UTC
Created attachment 348115 [details] [review]
cairo-display: Set pixbuf dpi

This will allow GNOME Games to save aspect ratio with screenshot.
Comment 5 Mateusz Sieczko 2017-03-16 19:19:02 UTC
Created attachment 348116 [details] [review]
Save metadata with screenshots

This include game title, platform name, creation time, aspect ratio, and
GNOME Games as application used to create screenshot.

"Title" filed is included for portability with tools that displays data
from PNG's tEXt chunk and it combines "Game Title" and "Platform"
fields. It doesn't have mush sense for screenshots, and separate fields
are better (think about "Song Title" and "Album" in mp3/ogg metadata)
but they are non-standard and some tools may not display them (eg.
Nautilus).
Comment 6 Adrien Plazas 2017-03-16 19:57:55 UTC
Review of attachment 348115 [details] [review]:

I still have some styling nitpicks, but again: good job! :)

::: retro-gtk/video/cairo-display.vala
@@ +40,3 @@
+		// Because we only need this for aspect ratio and gdk-pixbuf saves dpi
+		// as integer, multiply by large value to increase precision.
+		var factor = 1000000.0;

A private const in the class would be better to store the scaling factor (with a more explicit name as it would be moved out of the place where it's used). Something like:
private const float Y_DPI = 1000000.0;

The comment would have to go with the definition of the const. :)

@@ +41,3 @@
+		// as integer, multiply by large value to increase precision.
+		var factor = 1000000.0;
+		var x_dpif = aspect_ratio * factor;

Short names are avoided for better readability and maintainance (code is written once but read many times :) ).

A nitpick and your version is perfectly correct, but as the "proper representation" of the DPIs are the float maybe it's better to specify _string to the string transformations than f or _float to the originals. What do you think about this?
var x_dpi = aspect_ratio * Y_DPI;
var x_dpi_string = x_dpi.to_string ();
var y_dpi_string = Y_DPI.to_string ();
Comment 7 Mateusz Sieczko 2017-03-16 21:19:28 UTC
I agree that '_string' suffix is better in this situation.
Comment 8 Mateusz Sieczko 2017-03-16 21:20:28 UTC
Created attachment 348121 [details] [review]
cairo-display: Set pixbuf dpi

This will allow GNOME Games to save aspect ratio with screenshot.
Comment 9 Mateusz Sieczko 2017-03-17 10:28:32 UTC
Created attachment 348158 [details] [review]
Take game title as argument when creating RetroRunner

This will allow us to save title with screenshot's metadata.
Comment 10 Mateusz Sieczko 2017-03-17 10:28:39 UTC
Created attachment 348159 [details] [review]
retro: Save metadata with screenshots

This include game title, platform name, creation time, aspect ratio, and
GNOME Games as application used to create screenshot.

"Title" filed is included for portability with tools that displays data
from PNG's tEXt chunk and it combines "Game Title" and "Platform"
fields. It doesn't have mush sense for screenshots, and separate fields
are better (think about "Song Title" and "Album" in mp3/ogg metadata)
but they are non-standard and some tools may not display them (eg.
Nautilus).
Comment 11 Adrien Plazas 2017-03-17 10:36:15 UTC
Review of attachment 348158 [details] [review]:

LGTM.
Comment 12 Adrien Plazas 2017-03-17 10:38:07 UTC
Review of attachment 348159 [details] [review]:

LGTM.
Comment 13 Adrien Plazas 2017-03-17 10:38:20 UTC
Review of attachment 348121 [details] [review]:

LGTM.
Comment 14 Adrien Plazas 2017-03-17 14:08:02 UTC
Comment on attachment 348121 [details] [review]
cairo-display: Set pixbuf dpi

Attachment 348121 [details] pushed as 80214aa - cairo-display: Set pixbuf dpi
Comment 15 Adrien Plazas 2017-03-20 09:53:23 UTC
Attachment 348158 [details] pushed as 03a0063 - Take game title as argument when creating RetroRunner
Attachment 348159 [details] pushed as d235712 - retro: Save metadata with screenshots
Comment 16 Adrien Plazas 2017-03-23 15:07:05 UTC
*** Bug 777771 has been marked as a duplicate of this bug. ***