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 664983 - Review and update artwork
Review and update artwork
Status: RESOLVED FIXED
Product: iagno
Classification: Applications
Component: general
git master
Other Linux
: Normal enhancement
: ---
Assigned To: iagno-maint
iagno-maint
Depends on:
Blocks:
 
 
Reported: 2011-11-27 23:09 UTC by Robert Ancell
Modified: 2014-09-16 13:39 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Update game information grid (13.84 KB, patch)
2014-09-14 13:37 UTC, Iulian Radu
needs-work Details | Review
Update background image. (143.06 KB, patch)
2014-09-14 18:54 UTC, Iulian Radu
committed Details | Review
Screenshot with both patches applied. (51.25 KB, image/png)
2014-09-14 18:55 UTC, Iulian Radu
  Details
Update game information grid according to the review. (156.88 KB, patch)
2014-09-15 13:41 UTC, Iulian Radu
reviewed Details | Review
Update artwork and move tiles out of the themes directory. (21.25 KB, patch)
2014-09-15 13:43 UTC, Iulian Radu
none Details | Review
Proof (47.06 KB, image/png)
2014-09-15 13:48 UTC, Iulian Radu
  Details
Update artwork and move tiles out of the themes directory. (21.53 KB, patch)
2014-09-15 14:00 UTC, Iulian Radu
reviewed Details | Review
Update game information grid (28.62 KB, patch)
2014-09-15 16:33 UTC, Iulian Radu
reviewed Details | Review
Update game information grid (28.42 KB, patch)
2014-09-15 18:47 UTC, Iulian Radu
committed Details | Review

Description Robert Ancell 2011-11-27 23:09:04 UTC
Review and update the artwork and make sure it is modern and consistent with the other GNOME Games.
Comment 1 Robert Ancell 2011-12-29 05:25:24 UTC
As part of the Vala rewrite the icons were updated to use new scalable versions.  The sun and moon looks the nicest, but it is complex and slow to render and the sun is not sharp on the image.  This should be improved.
Comment 2 Robert Ancell 2011-12-29 05:26:29 UTC
As part of the Vala rewrite the icons were updated to use new scalable versions.  The sun and moon looks the nicest, but it is complex and slow to render and the sun is not sharp on the image.  This should be improved.
Comment 3 Robert Ancell 2011-12-29 05:27:29 UTC
Some thought needs to be put into the board the tiles are placed on as it's currently unclear how big the grid is and where the player is allowed to play.
Comment 4 Tiffany Antopolski 2012-01-23 17:32:45 UTC
I would like to work on this.  I have spoken to gnome-design about 'mentoring' me with this, and possibly the board/grid layout problem also.
Comment 6 Robert Ancell 2013-10-14 22:18:50 UTC
More work from Allan:
https://raw.github.com/gnome-design-team/gnome-mockups/master/games/iagno/iagno.png
Comment 7 Iulian Radu 2014-09-13 21:52:21 UTC
(In reply to comment #6)
> More work from Allan:
> https://raw.github.com/gnome-design-team/gnome-mockups/master/games/iagno/iagno.png

Is this still the desired result? I see there is one more svg with different menu and play again button.

If so, I'd give it a try.
Comment 8 Michael Catanzaro 2014-09-14 01:23:59 UTC
Yeah, that screenshot is still the desired result, though it's missing the undo button in the header bar (which is already fine).
Comment 9 Iulian Radu 2014-09-14 12:44:07 UTC
When should the side menu should be displayed? When pressing "Start over"?

And what about the current Preferences menu and menu entry?
Comment 10 Michael Catanzaro 2014-09-14 13:23:33 UTC
We're discussing that in bug #664976; let's leave this bug for just the artwork.
Comment 11 Iulian Radu 2014-09-14 13:37:33 UTC
Created attachment 286166 [details] [review]
Update game information grid

For now, I updated the game information grid on the top right. I'll see what else I can do that only involves the artwork.

It's still one of my first patches and I'm sure I can do some things better. Can you take a look?
Comment 12 Iulian Radu 2014-09-14 18:54:13 UTC
Created attachment 286173 [details] [review]
Update background image.

Also updated image according to the mockup
Comment 13 Iulian Radu 2014-09-14 18:55:58 UTC
Created attachment 286174 [details]
Screenshot with both patches applied.
Comment 14 Michael Catanzaro 2014-09-15 00:40:38 UTC
Wow, good job!  Digital art skills are valuable in the GNOME community, since we only have a few talented artists.
Comment 15 Michael Catanzaro 2014-09-15 01:05:48 UTC
Review of attachment 286166 [details] [review]:

Awesome; this turned out a lot better than my (unposted) attempt to do the same last year.  The one thing I'm not totally sold on here is light.svg and dark.svg. In Allan's mockup, the same images are used in the scores table as in the game itself (they're just a bit smaller in the scores table). I think it'd look better to drop these separate images and use the normal game pieces instead.

Right now, the numerical score of the player to move is displayed in bold. That was previously needed to indicate the player to move, but now that we have an arrow instead, both scores should always be bold.

P.S. Don't be discouraged by the length of my comments below; this is actually pretty significant for your first contribution.

::: src/iagno.vala
@@ +30,2 @@
     private Gtk.Label dark_score_label;
+    private Gtk.Image light_icon;

Can these variables be declared in a smaller scope?  It looks like dark_icon and light_icon are only used in the function where you create them.

@@ +159,3 @@
         grid.vexpand = true;
         grid.hexpand = true;
         grid.set_column_spacing (8);

I think 12 pixels looks better for column spacing. (And for some reason, we seem to like multiples of 6.)

@@ +160,3 @@
         grid.hexpand = true;
         grid.set_column_spacing (8);
+        grid.set_row_spacing (8);

The mockup uses a lot more space here, and I think that looks better.  Try 18 or 24 pixels, as you prefer.

We also have an unfortunate alignment issue: shrink your window down to the minimum size, and you'll see there's a bit too much space between the board and the score area.  We want to somehow reduce that without also reducing the space between the board and the Start Over button. I couldn't find a very clean way to do this, but setting the margin-end property of the grid to 12 seemed to work well (at least when combined with my other spacing recommendations). I figured these out by playing with GtkInspector: press Ctrl+Shift+D inside Iagno (with a recent GTK+ 3.13, like something built from jhbuild) and you can play with all the properties of your widgets to see easily what looks best.

@@ +163,2 @@
         grid.show ();
         side_box.pack_start (grid, false, true, 0);

The 0 at the end is the amount of extra padding to add before the grid. I think it'd look a bit better to add 6 pixels, so that the top of the scoreboard seems to be slightly below the top of the grid, instead of slightly above.

@@ +164,3 @@
         side_box.pack_start (grid, false, true, 0);
 
+        var dark = GLib.Path.build_filename(DATA_DIRECTORY, "themes", "dark.svg");

Be careful to always leave a space before the opening parenthesis.

Also, GLib is imported by default, so don't specify this explicitly. It's assumed.

@@ +173,3 @@
         grid.attach (dark_score_label, 2, 0, 1, 1);
 
+        var light = GLib.Path.build_filename(DATA_DIRECTORY, "themes", "light.svg");

Space before opening parenthesis, and don't explicitly specify GLib.

@@ +322,3 @@
         new_game_action.set_enabled (game.can_undo (1));
 
+        var mark = GLib.Path.build_filename(DATA_DIRECTORY, "themes", "mark.svg");

Space, GLib.

@@ +323,3 @@
 
+        var mark = GLib.Path.build_filename(DATA_DIRECTORY, "themes", "mark.svg");
+        mark_icon = new Gtk.Image.from_file (mark);

Instead of re-creating the image each time, instead just two different images from the same file once (in the same place where you create dark_icon and light_icon), and here choose which one of those two to show and which to hide.

@@ +329,3 @@
+            grid.insert_column (0);
+            grid.attach (mark_icon, 0, 0, 1, 1);
+            mark_icon.show();

Space

@@ +339,3 @@
+            grid.insert_column (0);
+            grid.attach (mark_icon, 0, 1, 1, 1);
+            mark_icon.show();

Space
Comment 16 Michael Catanzaro 2014-09-15 01:07:12 UTC
Review of attachment 286166 [details] [review]:

Oh I found a bug. :)  I guess we need to store the new light.svg, dark.svg, and mark.svg in a different directory, since they're being picked up as themes that you can pick in Preferences -- to very buggy effect!
Comment 17 Michael Catanzaro 2014-09-15 01:10:39 UTC
Review of attachment 286166 [details] [review]:

::: data/Makefile.am
@@ +10,3 @@
+	dark.svg \
+	light.svg \
+	mark.svg

So instead of putting these in theme_DATA, move them up out of the themes folder and list them under dist_DATA (instead of theme_DATA).
Comment 18 Michael Catanzaro 2014-09-15 01:12:11 UTC
Review of attachment 286173 [details] [review]:

I couldn't find anything wrong with this. It looks a lot better!

For a follow-up patch, try thickening the lines of the grid, as shown in Allan's mockup.
Comment 19 Iulian Radu 2014-09-15 13:41:55 UTC
Created attachment 286200 [details] [review]
Update game information grid according to the review.

I was expecting a longer review. It's better this way so I can make sure I learn the DOs and DON'Ts from the very beginning. Thanks for your supportive words.
Comment 20 Iulian Radu 2014-09-15 13:43:52 UTC
Created attachment 286201 [details] [review]
Update artwork and move tiles out of the themes directory.

Please check this one, I'm not sure if what I did in the Makefile.am it's 100% ok.
Comment 21 Iulian Radu 2014-09-15 13:48:22 UTC
Created attachment 286203 [details]
Proof

I tried setting the GRID_WIDTH in game-view.vala to 2 but the lines turn purplish. Setting it to like 8, makes them white + black + purple or something and I cant figure it why. All the formulas seem to be using GRID_WIDTH so the positioning should change accordingly.
Comment 22 Iulian Radu 2014-09-15 14:00:59 UTC
Created attachment 286204 [details] [review]
Update artwork and move tiles out of the themes directory.

I forgot to add margin-end of 12. This should be ok now.
Comment 23 Michael Catanzaro 2014-09-15 15:10:31 UTC
(In reply to comment #21)
> I tried setting the GRID_WIDTH in game-view.vala to 2 but the lines turn
> purplish. Setting it to like 8, makes them white + black + purple or something
> and I cant figure it why. All the formulas seem to be using GRID_WIDTH so the
> positioning should change accordingly.

I guess we'll have to look more into this later. Thin is better than purple!
Comment 24 Michael Catanzaro 2014-09-15 15:21:36 UTC
Review of attachment 286200 [details] [review]:

::: src/iagno.vala
@@ +333,3 @@
             /* Translators: this is a 2 digit representation of the current score. */
             dark_score_label.set_markup ("<span font_weight='bold'>"+(_("%.2d").printf (game.n_dark_tiles))+"</span>");
+            light_score_label.set_markup ("<span font_weight='bold'>"+(_("%.2d").printf (game.n_light_tiles))+"</span>");

These don't need to be inside the conditional anymore, right?
Comment 25 Michael Catanzaro 2014-09-15 15:26:19 UTC
Review of attachment 286204 [details] [review]:

Instead of attaching a separate patch for this, could you please merge it into the previous patch using git rebase -i?

::: data/Makefile.am
@@ +5,3 @@
 
+distdir = $(datadir)/iagno
+dist_DATA = \

The dist_prefix just says to distribute the files; you don't need to define distdir to use it.

To be consistent with the rest of the file, how about:

imagedir = $(datadir)/iagno
image_DATA = \
...

And then you can include $(image_DATA) in EXTRA_DIST.

@@ +37,3 @@
 EXTRA_DIST = \
 	$(ui_DATA) \
+	$(dist_DATA) \

And this is redundant if you're using the dist prefix.
Comment 26 Iulian Radu 2014-09-15 16:33:18 UTC
Created attachment 286221 [details] [review]
Update game information grid

Done.
Comment 27 Michael Catanzaro 2014-09-15 17:40:22 UTC
Review of attachment 286221 [details] [review]:

Alright, great!

I notice that if I switch the tile set to sun and star, the scoreboard still shows the pieces from the black and white tile set. I guess this would be not be super easy to fix, though. We could do it by creating a folder for each theme inside the theme directory, and giving each folder the main theme image, and also its own dark.svg and light.svg.  You don't have to do that in this patch, though, if you'll file a bug for it.

::: src/iagno.vala
@@ +151,2 @@
         var side_box = new Gtk.Box (Gtk.Orientation.VERTICAL, 6);
+        side_box.margin_end = 12;

Can you try adding this on the grid instead of side_box? I think this is causing the Start Over button to get pushed too close to the game board.
Comment 28 Iulian Radu 2014-09-15 18:47:04 UTC
Created attachment 286227 [details] [review]
Update game information grid

Done.

I will file a bug for the scoreboard-theme problem so I can treat that separately. You can assign it to me, I really want to continue contributing.
Comment 29 Michael Catanzaro 2014-09-15 19:31:04 UTC
Review of attachment 286227 [details] [review]:

Great! That's what I was hoping for. :)
Comment 30 Michael Catanzaro 2014-09-15 22:22:19 UTC
Attachment 286173 [details] pushed as c42dbc0 - Update background image.
Attachment 286227 [details] pushed as 6543481 - Update game information grid
Comment 31 Arnaud B. 2014-09-16 03:27:24 UTC
I reopen, as there’s a little thing I dislike, concerning the new artworks of the information grid. The dark dot is quite cool, because it’s dark, so we don’t really see shadows. But the light dot has shadows, shadows that “reveals” it’s not a flat (reversible) piece of reversi, but a convex gravel of go… Allan’s designs for example didn’t have this problem. Probably just tweaking the gradient would be enough, but I didn’t test for now.
Comment 32 Michael Catanzaro 2014-09-16 13:39:25 UTC
Ideally we would update the pieces to look like Allan's. Anyway, this is covered by bug #710125.