Created attachment 348993 [details] [review]
display a cover for desktop games

desktop games do not display a cover
like they do in many other platforms

to fix this desktop game's title,icon
and mime type were used to generate a
cover through grillo

Review of attachment 348993 [details] [review]:

Can you link the bug adding PC support to the thegamesdb Grilo plugin?

Please read this https://wiki.gnome.org/Git/CommitMessages. :) You should tag the shortlog with the affected module and start it with a capital. Something like this for example: "desktop: Get the cover via Grilo"

For the rest of the message: no need to re-write what the bug is saying. The typical format is:
- if the shortlog doesn't already explain all that is important, add a paragraph describing the changes made to the code;
- if the reasoning of the changes isn't obvious, add a paragraph explaining why we need this change.

Here explaining that you use a GriloCover with the Desktop Entry mime type for desktop games would be sufficient.

::: plugins/desktop/src/desktop-plugin.vala
@@ +9,3 @@
 		return source;
 	}
+

We don't need this new line. :)

::: plugins/desktop/src/desktop-tracker-query.vala
@@ +53,3 @@
+		var media = new GriloMedia (title, MIME_TYPE));
+		var cover = new CompositeCover ({
+			new LocalCover (uri),

I'm not sure local covers make sense for desktop games… maybe just having a GriloCover is better. :o

Review of attachment 348993 [details] [review]:

.

Created attachment 349141 [details] [review]
desktop:Get the cover via Grillo

Using a Grillo cover with the Desktop
Entry mime type for desktop games

Adding PC support to the thegamesdb Grilo plugin

https://bugzilla.gnome.org/show_bug.cgi?id=780852

Review of attachment 349141 [details] [review]:

It's Grilo with one l. :)

Again, lines should feel as much as possible of the allowed 72 chars.

::: plugins/desktop/src/desktop-tracker-query.vala
@@ +58,3 @@
 		var runner = new CommandRunner (args, true);
 
+		games += new GenericGame (title, icon, media, runner);

I'm pretty sure this doesn't compile…

Created attachment 349297 [details] [review]
desktop:Get the cover via Grilo

Using a Grilo cover with the Desktop Entry mime type for desktop games.
New DesktopUid class gets the id via DesktopAppInfo and pass to grilo
to get the cover.

Couldn't test this due to an issue in my build.

Review of attachment 349297 [details] [review]:

Here we are doing two sequential but not really related things: we add the UID and we use it to get covers. It would be better to first add the UID in its own commit (as a reasoning for the addition, you could say that it will be used in the next commit), then to enable the covers in the second commit.

That being said, globally the patch looks good. :)

::: plugins/desktop/src/Makefile.am
@@ +35,3 @@
 	desktop-title.vala \
 	desktop-tracker-query.vala \
+    desktop-uid.vala\

Indentation is wrong: use a tab. Also a space is missing before the \.

::: plugins/desktop/src/desktop-tracker-query.vala
@@ +52,3 @@
+		var uid = new DesktopUid (app_info);
+		var media = new GriloMedia (title, MIME_TYPE);	
+        var cover = new GriloCover (media, uid);	

Ditto: use tabs.

::: plugins/desktop/src/desktop-uid.vala
@@ +9,3 @@
+
+	public string get_uid () throws Error {
+		return app_info.get_id ();

This is good, but I would prefer to `return @"desktop-$(app_info.get_id ())";` for consistency with the other UIDs.

Created attachment 349319 [details] [review]
desktop:Add brandnew DesktopUid class

New DesktopUid class gets the id via DesktopAppInfo and pass to grilo
to get the cover.This will be used in next commit.

Created attachment 349331 [details] [review]
desktop: Add brandnew DesktopUid class

New DesktopUid class gets the id via DesktopAppInfo and pass to grilo
to get the cover.This will be used in next commit.

Created attachment 349332 [details] [review]
desktop:Get the cover via Grilo

Using a Grilo cover with the Desktop Entry mime type for desktop games.

Review of attachment 349331 [details] [review]:

This code looks very good but I'm not sure the UID is constructed how we want it.

Take hedgewars.desktop, its ID would be hedgewars.desktop, hence the UID would be desktop-hedgewars.desktop. The repetition of "desktop" isn't a big deal but the ID shouldn't be used as a file name because of its suffix, also it would break the consistency with the other UIDs which use only lower case ASCII letters, digits and the - character.

We could try to remove the ".desktop" part or maybe better: hash the original ID with SHA256 to have something like "desktop-061f5c017b31fe8530f062be4cf141c4908d0d17bb8a9dea80e0980a3792451d" for the ID "hedgewars.desktop".

Any thought about that? :o

Regarding the commit message: "brandnew" is useless here, and the message doesn't really describe what this patch does, all it does is to add the class, right now it sounds like this precise commit is retrieving the cover via Grilo.

Created attachment 349550 [details] [review]
desktop: Add DesktopUid class

New DesktopUid class returns the id in SHA-256.This will be used in
next commit.

Created attachment 349551 [details] [review]
desktop: Get the cover via Grilo

Using a Grilo cover with the Desktop Entry mime type for desktop games.

Review of attachment 349550 [details] [review]:

A space is missing before "This" in the commit message.

Except very tiny stylistic errors, the code is good! :) I'll accept the patch once they are fixed and one we are sure the patch works.

::: plugins/desktop/src/desktop-uid.vala
@@ +10,3 @@
+
+	public string get_uid () throws Error {
+		uid = Checksum.compute_for_string (ChecksumType.SHA256, app_info.get_id(), -1);

No need to pass the optional -1 param here. Also return statements (and all sequence breaking statements, actually) should have an empty line before them so they pop out more.

@@ +11,3 @@
+	public string get_uid () throws Error {
+		uid = Checksum.compute_for_string (ChecksumType.SHA256, app_info.get_id(), -1);
+		return @"desktop-$(uid)";

No need for parenthesis for params which are just one word, so just $uid is fine here. :)

Review of attachment 349551 [details] [review]:

LGTM, I'll accept it once we check its works as expected and the errors we have are external.

Created attachment 349729 [details] [review]
desktop: Add DesktopUid class

New DesktopUid class returns the id in SHA-256.This was used in last
commit.

Created attachment 349730 [details] [review]
desktop: Add DesktopUid class

New DesktopUid class returns the id in SHA-256. This was used in last
commit.

The `DesktopUid` part of this was implemented separarately in https://gitlab.gnome.org/GNOME/gnome-games/merge_requests/54 and https://gitlab.gnome.org/GNOME/gnome-games/merge_requests/109. The remaining part is still relevant, except it probably needs a different MIME type.

We'll remove desktop platform this cycle, closing.