GNOME Bugzilla – Bug 757554
Starring button behavior differs slightly from other starring
Last modified: 2016-01-06 20:42:17 UTC
Created attachment 314776 [details] photos starring behaviour The behavior of the button for starring behaves slightly different in Photos and Maps. In Maps it becomes pressed, while in Photos it gets pressed, but then goes back to normal state.
Created attachment 314777 [details] maps starring behaviour
I feel the Photos one is slightly nicer.
So this is about what type of GtkButton the favourite button in mapBubble.js should be, it should be a nice first task for someone wanting to contribute to Maps.
This should require changes in both src/mapBubble.js and data/ui/map-bubble.ui I think, to change the type of the favorite button. See: https://developer.gnome.org/gtk3/stable/GtkButton.html https://developer.gnome.org/gtk3/stable/GtkToggleButton.html Guide on commit messages: https://wiki.gnome.org/Git/CommitMessages
Created attachment 318317 [details] [review] A fix for starring button behaviour that behaves different from other starring button. Button used in the maps and photo for starring was toogleGtk Button,that remains pressed if it is pressed to make a place favourite, that is different from other starring button.so in order to change that behaviour i delete some line in src/mapBubble.js and add some line that does fix this issues. if somthing more needed please let me know.
Review of attachment 318317 [details] [review]: Thanks for your work, really appreciated! I think I would also re-think the commit message. The message too large and too detailed. Maybe something like: mapBubble: Use GtkButton for favorite button And you can explain the reason in the commit message following lines (be consistent with Photos), that'd be enough. Some comments about the patch below. ::: src/mapBubble.js @@ +116,3 @@ button.active = isFavorite; + image.icon_name = 'non-starred-symbolic'; You must set the image.icon_name according the value of isFavorite (isFavorite = true -> starred-symbolic, isFavorite = false -> non-starred-symbolic). Also, you must not set the button's active property. @@ +120,2 @@ + button.connect('clicked', (function() { + if (image.icon_name == 'non-starred-symbolic') { Try to always use strict equal comparison (===). (Would it be better to use placeStore.exists to make this check).
(In reply to Damián Nohales from comment #6) > Review of attachment 318317 [details] [review] [review]: > > Thanks for your work, really appreciated! > > I think I would also re-think the commit message. The message too large and > too detailed. Maybe something like: > > mapBubble: Use GtkButton for favorite button > > And you can explain the reason in the commit message following lines (be > consistent with Photos), that'd be enough. > > Some comments about the patch below. > > ::: src/mapBubble.js > @@ +116,3 @@ > button.active = isFavorite; > > + image.icon_name = 'non-starred-symbolic'; > > You must set the image.icon_name according the value of isFavorite > (isFavorite = true -> starred-symbolic, isFavorite = false -> > non-starred-symbolic). > > Also, you must not set the button's active property. > > @@ +120,2 @@ > + button.connect('clicked', (function() { > + if (image.icon_name == 'non-starred-symbolic') { > > Try to always use strict equal comparison (===). > > (Would it be better to use placeStore.exists to make this check). isFavorite is the value of PlaceStore.exist if we see in mapBubble.js file what if i use isFavorite in function.
Review of attachment 318317 [details] [review]: ::: src/mapBubble.js @@ +120,2 @@ + button.connect('clicked', (function() { + if (image.icon_name == 'non-starred-symbolic') { Hmmm... I prefer to use the placeStore.exists method here and also above when setting the initial icon_name value instead of taking care of updating a variable. (Please go to the patch review page to respond review comments, so your comments appear in the review page).
Created attachment 318379 [details] [review] mapBubble: use GtkButton for favorite button updated patch of the above.
Review of attachment 318379 [details] [review]: Thanks for updating! About the commit message, it's not necessary to mention that you're modifying the Maps application, that's obvious (you are committing to Maps repo). Also "change favorite button in maps and photo" sounds like you're changing Photos in this commit, which is not true. I think something like "This is to keep consistency with gnome-photos favorite button appearance.". And please capitalize the "use" word in the commit message first line (sorry, my mistake :D) Some trivial comments about the code: ::: src/mapBubble.js @@ +121,3 @@ + button.connect('clicked', (function() { + if (placeStore.exists(this._place, + PlaceStore.PlaceType.FAVORITE)){ This if is not well indented, also please add a space between ) and { @@ +124,3 @@ image.icon_name = 'non-starred-symbolic'; placeStore.removePlace(this._place, + PlaceStore.PlaceType.FAVORITE); Align this line with "this._place" @@ +130,1 @@ PlaceStore.PlaceType.FAVORITE); Same here.
Created attachment 318382 [details] [review] Use GtkButton for favorite button updated patch with indentation fixes of previous patch.
Review of attachment 318382 [details] [review]: Thanks! Looks good, there's still a little nit of indentation in the if, but I'm going to fix it on push.