GNOME Bugzilla – Bug 769833
Add metadata to screenshots
Last modified: 2017-03-23 15:07:05 UTC
saving screenshots to PNGs using gdk-pixbuf makes saving extra metadata possible, such as the application that created the file.
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.
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).
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",
Created attachment 348115 [details] [review] cairo-display: Set pixbuf dpi This will allow GNOME Games to save aspect ratio with screenshot.
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).
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 ();
I agree that '_string' suffix is better in this situation.
Created attachment 348121 [details] [review] cairo-display: Set pixbuf dpi This will allow GNOME Games to save aspect ratio with screenshot.
Created attachment 348158 [details] [review] Take game title as argument when creating RetroRunner This will allow us to save title with screenshot's metadata.
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).
Review of attachment 348158 [details] [review]: LGTM.
Review of attachment 348159 [details] [review]: LGTM.
Review of attachment 348121 [details] [review]: LGTM.
Comment on attachment 348121 [details] [review] cairo-display: Set pixbuf dpi Attachment 348121 [details] pushed as 80214aa - cairo-display: Set pixbuf dpi
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
*** Bug 777771 has been marked as a duplicate of this bug. ***