I take it!

I seem to recall that the star icon would turn blue when pressed, but that doesn't happen any more. Not sure why. Instead I see a flickering blue when untoggling the icon. :/

Anyway, we need to fix this, and yellow sounds like a better colour than blue.

Created attachment 308722 [details] [review]
Adwaita: Colour the star button when is toggled

The new colour is yellow

Review of attachment 308722 [details] [review]:

Thanks for the patch, Alessandro. Looks good to me, but we need to get this reviewed by a designer / adwaita hacker.

::: data/Adwaita.css
@@ +1,3 @@
 @define-color theme_base_color #333333;
 @define-color theme_bg_color #393f3f;
+@define-color theme_selected_bg_color #bdaf3b;

From where did you get the colour values?

@@ +104,3 @@
 }
 
+.photos-favorite.button {

This fixes the flickering problem. I guess some Adwaita changes broke this some releases ago. Can you split this hunk into a separate patch so that we can backport it to stable branches?

Sorry, I've been meaning to say - perhaps we could used the filled/unfilled variants of the icon, rather than changing the colour (probably better for accessibility!)

The icons are starred-symbolic and non-starred-symbolic.

(In reply to Debarshi Ray from comment #4)
> 
> From where did you get the colour values?
From a picture (the last one) in Allan's post:
https://blogs.gnome.org/aday/2015/07/01/photos-future-plans/

Created attachment 313135 [details] [review]
main-toolbar: Use symbolic icon instead of change the colour

Created attachment 313136 [details] [review]
Adwaita: Make the favorite button icon work

This is a fix for stable branches

Review of attachment 313136 [details] [review]:

Thanks Alessandro, it looks good. I pushed it to master [*] and gnome-3-18. It should probably be in gnome-3-16 too, but I can't check right now.

[*] Although we are not going to really use it in master, I still prefer to have it there because it makes it easier to understand what was going on.

Review of attachment 313135 [details] [review]:

Perfect. I pushed it after rebasing it on top of current master.

Created attachment 313844 [details] [review]
Use a filled/unfilled icon instead of colour to denote favorites

Am I the only one who thinks that the non-starred-symbolic looks a bit odd? I agree that it is the right thing to use here, but it looks a bit odd to me. At this size, the white border of the star looks dotted or something.

Allan, Jimmac:
I guess you also want a normal button, not a toggle, right?

Created attachment 313848 [details]
Screenshot of non-starred-symbolic

I think the non starred icon doesn't work without previous knowledge of the starred one. I mean I see a star in a button, it doesn't look "turned off".

Created attachment 313857 [details] [review]
main-toolbar: Use a simple button instead of a toggle to denote favorites

Review of attachment 313857 [details] [review]:

Thanks, Alessandro.

::: src/photos-main-toolbar.c
@@ -463,1 @@
   g_signal_handlers_unblock_by_func (priv->favorite_button, photos_main_toolbar_favorite_button_clicked, self);

The blocking/unblocking was only needed due to the set_active call. They are useless now. Please remove them.

We need to make the corresponding changes to the selection toolbar too.

Created attachment 313888 [details] [review]
Make favorite button consistent in selection toolbar

Created attachment 313889 [details] [review]
main-toolbar: Use a simple button instead of a toggle to denote favorites

Created attachment 313890 [details] [review]
main-toolbar: Use a simple button instead of a toggle to denote favorites

Review of attachment 313888 [details] [review]:

Looks good.

Review of attachment 313890 [details] [review]:

Thanks, Alessandro. Looks good now.