GNOME Bugzilla – Bug 777987
Allow to have a sharp video output
Last modified: 2017-02-22 21:06:08 UTC
The way Games currently renders retro games is very blurry, it would be nice to give a sharp look to the video output, giving pixel all their blocky glory!
Created attachment 345298 [details] [review] Add video preferences and video filters
Created attachment 345299 [details] [review] cairo-display: Add display filters
Review of attachment 345299 [details] [review]: The patch globally looks good. ::: retro-gtk/video/cairo-display.vala @@ +3,2 @@ public class RetroGtk.CairoDisplay : Gtk.DrawingArea { + public enum Filter { What about having this in its own file? Maybe RetroGtk.VideoFilter. This way we could reuse it for other potential displays and it avoids having two depth of namespaces (RetroGtk.CairoDisplay.Filter). @@ +35,3 @@ + public void set_filter (Filter filter) { + this.filter = filter; We should request a redraw once the filter has been set. @@ +79,3 @@ + var source = cr.get_source (); + switch (filter) { + case Filter.SHARP: We put cases at the same indentation level as the switch statement. @@ +81,3 @@ + case Filter.SHARP: + source.set_filter (Cairo.Filter.NEAREST); + break; Please add an empty line before flow changing statements which are not alone in their block (to make it simple, put an empty line before beak, continue, return… if they are not alone). @@ +85,3 @@ + default: + source.set_filter (Cairo.Filter.FAST); + break; Ditto.
Created attachment 345427 [details] [review] video: Add video filter types
Created attachment 345428 [details] [review] cairo-display: Add sharp video filter
Created attachment 345471 [details] [review] Add VideoFilter
Created attachment 345472 [details] [review] cairo-display: Allow to set a video filter
Review of attachment 345471 [details] [review]: Ack.
Review of attachment 345472 [details] [review]: A log line is too long but I'll fix it.
Attachment 345471 [details] pushed as 6f0d5d5 - Add VideoFilter Attachment 345472 [details] pushed as 01b3f4f - cairo-display: Allow to set a video filter
Created attachment 345497 [details] [review] Add gsettings schema For now it defines various filters for video output.
Created attachment 345498 [details] [review] retro-runner: Use video filter from gsettings Add setup_video_filter() and connect it to gsettings.
Created attachment 345499 [details] [review] ui: Add video preferences Allow to set video filters in preferences window.
Created attachment 345500 [details] [review] ui: Add video preferences Allow to set video filters in preferences window.
Review of attachment 345497 [details] [review]: Maybe we could tag the short log with 'data: ', but it's not that important. :) It's good but the styling should be revised a bit. ::: data/org.gnome.Games.gschema.xml @@ +3,3 @@ + <!-- Corresponds to enum RetroGtk.VideoFilter --> + <enum id="org.gnome.Games.video-filters"> + <value nick="Smooth" value="0"/> I may be wrong but I think the nicks are not supposed to start with a capital (at least I didn't see one in Music's gschema. @@ +4,3 @@ + <enum id="org.gnome.Games.video-filters"> + <value nick="Smooth" value="0"/> + <value nick="Sharp" value="1"/> Ditto. @@ +6,3 @@ + <value nick="Sharp" value="1"/> + </enum> + Let's avoid empty lines in this file. @@ +13,3 @@ + <summary>Video filter</summary> + <description> + Various filters for the game video output. Are newlines and trailing spaces accepted in descriptions? In doubt and given how small it is it may be better to have it in one line. Maybe the description should describe the performed acction similarly to Music's gschema. It seems to be a good practice to describe the values in the description (see https://git.gnome.org/browse/gnome-music/tree/data/org.gnome.Music.gschema.xml).
Review of attachment 345498 [details] [review]: For information: the RetroGtk and Retro namespaces have been merged so you'll have to update this patch accordingly. About setup_video_filter(): - Usually we name callbacks similarly to the signal so it's clear it's a callback, for example here on_video_filter_changed() would blend better into the rest of the code (maybe it should be detailed in the coding style). - It may be safer to use a switch statement rather than a cast to handle potentially wrong values from the settings (using Filter.SMOOTH by default). It looks good otherwise. :)
Created attachment 346264 [details] [review] data: Add gsettings schema For now it defines various filters for video output. Updated according to the review.
Created attachment 346265 [details] [review] video-filter: Add from_string() Patch for retro-gtk Parse the string to get the corresponding filter. In case of failure, return the default one.
Created attachment 346275 [details] [review] retro-runner: Use video filter from gsettings Load video filter from gsettings and update when it is changed. Updated with head.
Review of attachment 346264 [details] [review]: LGTM.
Review of attachment 346265 [details] [review]: Style: - The indentation in from_string() is wrong. - The flow breaking instructions (return, break, continue…) must have an empty line separating it from the previous line except if they are the only one of their block (cf. the coding style documentation). The point of using strings is to avoid casts from the integer from the gschema to a Retro.VideoFilter, which are a bad practice. here we take the string but convert it into the integer from the gschema and then cast it to a Retro.Video: it completely defeats the point of using strings to avoid casting one integer to another. What about this? public static VideoFilter from_string (string value) { switch (value) { case "sharp": return VideoFilter.SHARP; case "smooth": default: return VideoFilter.SMOOTH; } This way if the order of the values in the gschema or VideoFilter happen to change, this function won't break. In the log: go to the line only if you run out of space or if you want to make a new paragraph.
Review of attachment 346275 [details] [review]: LGTM, except for a minor complain. ::: src/retro/retro-runner.vala @@ +247,3 @@ + private void on_video_filter_changed () { + video.set_filter (Retro.VideoFilter.from_string (settings.get_string ("video-filter"))); For readability reasons, we avoid encapsulating function calls into other function calls, there should be 3 lines here. :)
Created attachment 346284 [details] [review] video-filter: Add from_string() Parse the string to get the corresponding filter. In case of a failure, return the default one. Updated according to review.
Created attachment 346285 [details] [review] retro-runner: Use video filter from gsettings Load video filter from gsettings and update when it is changed. Updated according to review.
Review of attachment 346285 [details] [review]: LGTM.
Review of attachment 346284 [details] [review]: LGTM.
Review of attachment 345500 [details] [review]: It needs to be split up. Remember to mark the old patches as deprecated. :)
Created attachment 346469 [details] [review] ui: Add preference switch item This will be used in preferences ui to make options for switches, selectables and combo boxes.
Created attachment 346470 [details] [review] ui: Add video preference page Add a page to the preferences window to allow user to view and change video settings. This will be displayed in a subsequent commit.
Created attachment 346471 [details] [review] ui: Show video preference page Add video preference page in the preferences window and make it default.
Review of attachment 346469 [details] [review]: In the shortlog you can just say "ui: Add PreferencesSwitchItem" Besides that and the indentation problem it's good. ::: data/ui/preferences-switch-item.ui @@ +3,3 @@ + <template class="GamesPreferencesSwitchItem" parent="GtkBox"> + <property name="vexpand">false</property> + <property name="orientation">horizontal</property> There is an indentation problem, most of the lines have 4 more spaces than they should.
Review of attachment 346470 [details] [review]: LGTM.
Review of attachment 346471 [details] [review]: LGTM.
Comment on attachment 346284 [details] [review] video-filter: Add from_string() Attachment 346284 [details] pushed as cdca751 - video-filter: Add from_string()
Attachment 346264 [details] pushed as 181fa41 - data: Add gsettings schema Attachment 346285 [details] pushed as a34f351 - retro-runner: Use video filter from gsettings Attachment 346470 [details] pushed as 01c64c7 - ui: Add video preference page Attachment 346471 [details] pushed as 29b3bf9 - ui: Show video preference page