GNOME Bugzilla – Bug 751478
Favorites icon not visible on white thumbnail
Last modified: 2018-01-11 10:24:41 UTC
Created attachment 306071 [details] The favorites emblem casting a shadow on the thumbnail The favorites emblem is invisible on a white thumbnail. We could add a shadow under it to make it pop out on white thumbnails and appear as regular otherwise as shown in the attachment. The downside of this approach is that a (probably complex) blurring function would have to be included in Boxes, which is not desirable.
One idea would be to choose the color of symbolic icon based on thumbnail's colors. We already have a pixbuf_energy() method in Machine class that you could use here.
Created attachment 324344 [details] [review] machine-thumbnailer: Make favorites icon visible on any thumbnail The favorites icon is white by default and this causes it to be hard to notice on thumbnails have a similar color. In order to fix this, select the emblem color by evaluating the luminance of the area.
Review of attachment 324344 [details] [review]: So I assume we can't re-use pixbuf energy here as suggested in comment#1?
Created attachment 324507 [details] [review] machine-thumbnailer: Make favorites icon visible on any thumbnail The favorites icon is white by default and this causes it to be hard to notice on thumbnails that have a similar color. In order to fix this, select the emblem color by evaluating the pixbuf energy of the emblem area. White is noticeable on small energies and dark orange on the others.
Review of attachment 324507 [details] [review]: Have you tested this patch? ::: src/machine-thumbnailer.vala @@ +8,3 @@ public const Gdk.RGBA CENTERED_EMBLEM_COLOR = { 0xbe / 255.0, 0xbe / 255.0, 0xbe / 255.0, 1.0 }; public const Gdk.RGBA SMALL_EMBLEM_COLOR = { 1.0, 1.0, 1.0, 1.0 }; + public const Gdk.RGBA SMALL_EMBLEM_COLOR_ORANGE = { 1.0, 115.0, 255.0, 1.0 }; Not sure orange is a good choice here. Why not just grey? @@ +169,3 @@ + emblem = icon_info.load_symbolic (SMALL_EMBLEM_COLOR_ORANGE); + } else { + var icon_info = theme.lookup_icon (icon_name, emblem_size, Gtk.IconLookupFlags.FORCE_SIZE); Seems this call is the same in both cases so you want to move it outside the 'if' and hence also remove the {}. @@ +173,3 @@ + } + } catch (GLib.Error error) { + warning (@"Unable to get icon '$icon_name': $(error.message)"); If you read rest of the code, you'll find that @ strings are rarely used (and if they are, it's from very old times). Would prefer if you followed that convention for consistency. Talking of which, we've been using the form "Failed to..." rather than "Unable to..". ::: src/machine.vala @@ +426,3 @@ /* Calculates the average energy intensity of a pixmap. * Being a square of an 8bit value this is a 16bit value. */ + public int pixbuf_energy (Gdk.Pixbuf pixbuf) { * Turning a method public is counted as a separate change, hence such a change would go into it's own patch. * Since this is now going to be used by others and it has nothing to do with Machine (especially it's state), it should be moved to util-app.vala at the same time.
Created attachment 324566 [details] [review] machine-thumbnailer: Make favorites icon visible on any thumbnail The favorites icon is white by default and this causes it to be hard to notice on thumbnails that have a similar color. In order to fix this, select the emblem color by evaluating the pixbuf energy of the emblem area. White is noticeable on pictures with small energes and dark grey on the others.
Review of attachment 324566 [details] [review]: * Does the shortlog fit under 50 chars? * Shortlog should summarize the change, not the effects/benefits. * My question stands about whether or not your tested this patch to work against both non-white and white thumbnails. ::: src/machine-thumbnailer.vala @@ +8,3 @@ public const Gdk.RGBA CENTERED_EMBLEM_COLOR = { 0xbe / 255.0, 0xbe / 255.0, 0xbe / 255.0, 1.0 }; + public const Gdk.RGBA SMALL_EMBLEM_COLOR_WHITE = { 1.0, 1.0, 1.0, 1.0 }; + public const Gdk.RGBA SMALL_EMBLEM_COLOR_GREY = { 200.0, 200.0, 200.0, 1.0 }; Now that you (rightfully) changed name of existing constant as well, I came to think that naming constants based on their values isn't a good idea (since we can decide to change the values later on without having to change all their usage in the code and hence the reason to define them here rather than just hardcoding them). Name them by their functions. I'd like you to come up with the names as a homework but if you can't, I'll help you out. :) @@ +161,3 @@ var emblemed = pixbuf.copy (); + + var temp = emblem.copy (); I prefer descriptive names. @@ +164,3 @@ + pixbuf.copy_area (offset_x, offset_y, emblem_size, emblem_size, temp, 0, 0); + + var icon_info = theme.lookup_icon (icon_name, emblem_size, Gtk.IconLookupFlags.FORCE_SIZE); Why are we doing all this and not just modifying the emblem = icon_info.load_symbolic (SMALL_EMBLEM_COLOR); line above? ::: src/machine.vala @@ -426,3 @@ - /* Calculates the average energy intensity of a pixmap. - * Being a square of an 8bit value this is a 16bit value. */ - private int pixbuf_energy (Gdk.Pixbuf pixbuf) { Please see the comment in my previous review about separate change always going into separate patch. We strictly follow that rule so you need to really get into that. :)
> ::: src/machine-thumbnailer.vala > @@ +8,3 @@ > public const Gdk.RGBA CENTERED_EMBLEM_COLOR = { 0xbe / 255.0, 0xbe / > 255.0, 0xbe / 255.0, 1.0 }; > + public const Gdk.RGBA SMALL_EMBLEM_COLOR_WHITE = { 1.0, 1.0, 1.0, 1.0 }; > + public const Gdk.RGBA SMALL_EMBLEM_COLOR_GREY = { 200.0, 200.0, 200.0, > 1.0 }; > > Now that you (rightfully) changed name of existing constant as well, I came > to think that naming constants based on their values isn't a good idea > (since we can decide to change the values later on without having to change > all their usage in the code and hence the reason to define them here rather > than just hardcoding them). Name them by their functions. I'd like you to > come up with the names as a homework but if you can't, I'll help you out. :) I don't really know what names to use or what you're expecting here.
*** Bug 764287 has been marked as a duplicate of this bug. ***
(In reply to Radu Stochitoiu from comment #8) > > ::: src/machine-thumbnailer.vala > > @@ +8,3 @@ > > public const Gdk.RGBA CENTERED_EMBLEM_COLOR = { 0xbe / 255.0, 0xbe / > > 255.0, 0xbe / 255.0, 1.0 }; > > + public const Gdk.RGBA SMALL_EMBLEM_COLOR_WHITE = { 1.0, 1.0, 1.0, 1.0 }; > > + public const Gdk.RGBA SMALL_EMBLEM_COLOR_GREY = { 200.0, 200.0, 200.0, > > 1.0 }; > > > > Now that you (rightfully) changed name of existing constant as well, I came > > to think that naming constants based on their values isn't a good idea > > (since we can decide to change the values later on without having to change > > all their usage in the code and hence the reason to define them here rather > > than just hardcoding them). Name them by their functions. I'd like you to > > come up with the names as a homework but if you can't, I'll help you out. :) > > I don't really know what names to use or what you're expecting here. 1. It would be easier to discuss/answer if you kindly reply inline to inline comments in splinter view (Review link). 2. Since I tried to explain, it would be easier if you tell me which part of my comments you found confusing and I can try to explain them better/more.
(In reply to Radu Stochitoiu from comment #8) > > ::: src/machine-thumbnailer.vala > > @@ +8,3 @@ > > public const Gdk.RGBA CENTERED_EMBLEM_COLOR = { 0xbe / 255.0, 0xbe / > > 255.0, 0xbe / 255.0, 1.0 }; > > + public const Gdk.RGBA SMALL_EMBLEM_COLOR_WHITE = { 1.0, 1.0, 1.0, 1.0 }; > > + public const Gdk.RGBA SMALL_EMBLEM_COLOR_GREY = { 200.0, 200.0, 200.0, > > 1.0 }; > > > > Now that you (rightfully) changed name of existing constant as well, I came > > to think that naming constants based on their values isn't a good idea > > (since we can decide to change the values later on without having to change > > all their usage in the code and hence the reason to define them here rather > > than just hardcoding them). Name them by their functions. One thing that might confuse you that I see here is the term 'functions'. I meant function as in functionality, rather than subroutine. I hope that helps.
I still don't understand what their functionality would be beside being "white" or "grey". Do you mean that I should just define a non-const SMALL_EMBLEM_COLOR which I modify anytime I need it?
(In reply to Radu Stochitoiu from comment #12) > I still don't understand what their functionality would be beside being > "white" or "grey". You are using one for low energy thumbnail and the other for high energy thumbnail (IOW one for dark and one for light thumbnail).
What about other comments on the patch?
Ok, so using LOW and HIGH instead of GREY and WHITE, having a separate patch for moving pixbuf_energy, having descripting names for variables and not passing shortlog size or row length should be alright? Is there anything else about this bug?
(In reply to Radu Stochitoiu from comment #15) > Ok, so using LOW and HIGH instead of GREY and WHITE, LOW what? SMALL_LOW_ENERGY_EMBLEM_COLOR and SMALL_HIGH_ENERGY_EMBLEM_COLOR would be more specific but still not too long. > having a separate patch > for moving pixbuf_energy, having descripting names for variables and not > passing shortlog size or row length should be alright? Is there anything > else about this bug? Just address all review comments.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gnome-boxes/issues/62.