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 751106 - Don't toggle the star button and use a filled/unfilled icon instead of coloring it
Don't toggle the star button and use a filled/unfilled icon instead of colori...
Status: RESOLVED FIXED
Product: gnome-photos
Classification: Applications
Component: general
3.17.x
Other All
: Normal normal
: ---
Assigned To: GNOME photos maintainer(s)
GNOME photos maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2015-06-17 14:11 UTC by Allan Day
Modified: 2015-10-27 09:22 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Adwaita: Colour the star button when is toggled (1.04 KB, patch)
2015-08-04 10:41 UTC, Alessandro Bono
reviewed Details | Review
main-toolbar: Use symbolic icon instead of change the colour (3.66 KB, patch)
2015-10-12 18:40 UTC, Alessandro Bono
committed Details | Review
Adwaita: Make the favorite button icon work (807 bytes, patch)
2015-10-12 18:50 UTC, Alessandro Bono
committed Details | Review
Use a filled/unfilled icon instead of colour to denote favorites (3.65 KB, patch)
2015-10-22 08:34 UTC, Debarshi Ray
committed Details | Review
Screenshot of non-starred-symbolic (953.33 KB, image/png)
2015-10-22 08:52 UTC, Debarshi Ray
  Details
main-toolbar: Use a simple button instead of a toggle to denote favorites (1.41 KB, patch)
2015-10-22 11:50 UTC, Alessandro Bono
none Details | Review
Make favorite button consistent in selection toolbar (2.74 KB, patch)
2015-10-22 18:15 UTC, Alessandro Bono
committed Details | Review
main-toolbar: Use a simple button instead of a toggle to denote favorites (2.72 KB, patch)
2015-10-22 18:15 UTC, Alessandro Bono
none Details | Review
main-toolbar: Use a simple button instead of a toggle to denote favorites (2.81 KB, patch)
2015-10-22 18:24 UTC, Alessandro Bono
committed Details | Review

Description Allan Day 2015-06-17 14:11:00 UTC
The star button toggles when you press it, so that a pressed button indicates that a picture has been starred. This doesn't seem right to me, conceptually speaking. A pressed button doesn't say "starred". 

Changing the color of the icon would be a more effective way to communicate whether an image has been starred - grey for unstarred, and yellow for starred.
Comment 1 Fabien Bourigault 2015-07-17 11:59:26 UTC
I take it!
Comment 2 Debarshi Ray 2015-08-04 09:49:58 UTC
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.
Comment 3 Alessandro Bono 2015-08-04 10:41:48 UTC
Created attachment 308722 [details] [review]
Adwaita: Colour the star button when is toggled

The new colour is yellow
Comment 4 Debarshi Ray 2015-10-12 17:06:20 UTC
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?
Comment 5 Allan Day 2015-10-12 17:19:59 UTC
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.
Comment 6 Alessandro Bono 2015-10-12 18:39:42 UTC
(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/
Comment 7 Alessandro Bono 2015-10-12 18:40:17 UTC
Created attachment 313135 [details] [review]
main-toolbar: Use symbolic icon instead of change the colour
Comment 8 Alessandro Bono 2015-10-12 18:50:21 UTC
Created attachment 313136 [details] [review]
Adwaita: Make the favorite button icon work

This is a fix for stable branches
Comment 9 Debarshi Ray 2015-10-21 19:19:49 UTC
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.
Comment 10 Debarshi Ray 2015-10-22 08:33:50 UTC
Review of attachment 313135 [details] [review]:

Perfect. I pushed it after rebasing it on top of current master.
Comment 11 Debarshi Ray 2015-10-22 08:34:34 UTC
Created attachment 313844 [details] [review]
Use a filled/unfilled icon instead of colour to denote favorites
Comment 12 Debarshi Ray 2015-10-22 08:42:01 UTC
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?
Comment 13 Debarshi Ray 2015-10-22 08:52:26 UTC
Created attachment 313848 [details]
Screenshot of non-starred-symbolic
Comment 14 Lapo Calamandrei 2015-10-22 09:28:04 UTC
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".
Comment 15 Alessandro Bono 2015-10-22 11:50:57 UTC
Created attachment 313857 [details] [review]
main-toolbar: Use a simple button instead of a toggle to denote favorites
Comment 16 Debarshi Ray 2015-10-22 17:21:07 UTC
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.
Comment 17 Debarshi Ray 2015-10-22 17:22:52 UTC
We need to make the corresponding changes to the selection toolbar too.
Comment 18 Alessandro Bono 2015-10-22 18:15:00 UTC
Created attachment 313888 [details] [review]
Make favorite button consistent in selection toolbar
Comment 19 Alessandro Bono 2015-10-22 18:15:28 UTC
Created attachment 313889 [details] [review]
main-toolbar: Use a simple button instead of a toggle to denote favorites
Comment 20 Alessandro Bono 2015-10-22 18:24:28 UTC
Created attachment 313890 [details] [review]
main-toolbar: Use a simple button instead of a toggle to denote favorites
Comment 21 Debarshi Ray 2015-10-23 07:44:03 UTC
Review of attachment 313888 [details] [review]:

Looks good.
Comment 22 Debarshi Ray 2015-10-23 08:00:46 UTC
Review of attachment 313890 [details] [review]:

Thanks, Alessandro. Looks good now.