GNOME Bugzilla – Bug 730258
New default thumbnail
Last modified: 2016-03-31 13:22:00 UTC
(From DX hackfest, berlin) I think jimmc wanted to make a new default thumbnail for VMs that do not have an own. Also currently the thumbnail goes completely black when hovering.
(In reply to comment #0) > (From DX hackfest, berlin) > > I think jimmc wanted to make a new default thumbnail for VMs that do not have > an own. Also currently the thumbnail goes completely black when hovering. Jimmac?
(In reply to Zeeshan Ali (Khattak) from comment #1) > (In reply to comment #0) > > (From DX hackfest, berlin) > > > > I think jimmc wanted to make a new default thumbnail for VMs that do not have > > an own. Also currently the thumbnail goes completely black when hovering. > > Jimmac? Jimmac? :) I recall you pointed me to some symbolic icon I can use here?
I don't recall the conversation anymore :) While I agree a black thumbnail is to be avoided, having a symbolic emblem on it isn't helping to identify a VM either. A VM that's turned off was meant to have the last non-black image shown desaturated.
(In reply to Jakub Steiner from comment #3) > I don't recall the conversation anymore :) While I agree a black thumbnail > is to be avoided, having a symbolic emblem on it isn't helping to identify a > VM either. A VM that's turned off was meant to have the last non-black image > shown desaturated. Really? But then you can't differentiate it from saved VMs.
hmmk, perhaps the icon you're looking for is system-shutdown-symbolic then.
(In reply to Jakub Steiner from comment #5) > hmmk, perhaps the icon you're looking for is system-shutdown-symbolic then. Yeah. I think we should show the OS's logo if its known/available.
Created attachment 305641 [details] Testings icons and logos as default thumbnails I made some tests for a new default thumbnail. Please note that not all screenshot have been taken with the Windows box favorited. On the left column you can see Boxes' current state, and on the right columns some test of what to do with the default thumbnail. On these tests, the OS's logo is used to suggest that a box is currently stopped, defaulting for boxes whose logo couldn't be found to either a "shutdown" icon or Boxes' logo. Adding a "shutdown" emblem have also been tested. I'm not very fond of the emblem: it conflicts with the "favorite" emblem and is not very readable on Ubuntu's logo. Using a big "shutdown" icon with the OS' logo is weird, if logos or the shutdown icons are used, we should go for fully the one or the other, but not mixing them: it looks inconsistent. I like the bottom right test, it's different enough from the "running" and "suspended" states, it doesn't conflict with the emblems, it gives more information that just a black box and it's consistent with itself. =)
When emblems were discussed, they were meant as 16x16px emblems overlaid on the default rectangle. Similar to these: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/videos/thumbnails.png
These black boxes look great to show the box's state! Even the black ones. Jimmac: Do you have any idea how to show a favorited box's state without having the emblems conflicting?
Created attachment 305905 [details] [review] util-app: Add paint_framed_thumbnail() This is needed to draw a better thumbnail for stopped boxes to better differenciate them from the others.
Created attachment 305906 [details] [review] collection-view: Improve stopped machine thumbnail This draws a frame containing a 'system-shudown' symbolic icon as the thumbnail for stopped boxes. This is needed to better differenciate stopped boxes from the others.
Review of attachment 305905 [details] [review]: Thanks for adding rationale but: * I'll rephrase to keep it very clear that we don't yet use this function (in this patch). Something like "We'll use this in the following patch to...". * When it's not obvious (which is the case here), you want to summarize the change before rationale. ::: src/util-app.vala @@ +516,3 @@ } + + public Gdk.Pixbuf? paint_framed_thumbnail (Gdk.Pixbuf? picture, bool with_background = true) { Let's call it pain_framed_picture instead. @@ +517,3 @@ + + public Gdk.Pixbuf? paint_framed_thumbnail (Gdk.Pixbuf? picture, bool with_background = true) { + var surface = new Cairo.ImageSurface (Cairo.Format.ARGB32, Machine.SCREENSHOT_WIDTH, Machine.SCREENSHOT_HEIGHT); Don't like this function accessing Machine's constants, i-e assuming this to be a thumbnail. If its in untils, its supposed to be generic. Please pass these as arguments. @@ +532,3 @@ + if (picture != null) { + Gdk.cairo_set_source_pixbuf (cr, picture, 0, 0); + // FIXME Set a rounded rectangle as a mask I'd add a bit more explanation here why that's a problem and why it's ok to ignore for now. @@ +534,3 @@ + // FIXME Set a rounded rectangle as a mask + cr.paint (); + } Seems to me that this function is actually performing two different tasks depending on which arg is passed and which arg is omitted. I think you want two different functions and a third one for common code to draw the border. @@ +539,3 @@ + cr.set_source_rgb (0x3b / 255.0, 0x3c / 255.0, 0x38 / 255.0); + cr.set_line_width (1.0); + // FIXME The rectangle's width is diminished by 1 because Gd.MainView doesn't show the last column of pixels. A bug on libgd and link here?
Review of attachment 305906 [details] [review]: I really like the result :) * "Summarize the change itself in the shortlog, not the bug or effects/benefits (that goes in description)." from https://wiki.gnome.org/Git/CommitMessages * "the others" -> "running ones with (mostly) black thumbnails (e.g full screened terminal windows)" ::: src/collection-view.vala @@ +81,3 @@ + case Machine.MachineState.FORCE_STOPPED: + case Machine.MachineState.STOPPED: + var frame = paint_framed_thumbnail (null); if we are only going to use this method in only one of its configurations, I don't think this method should handle any other configruations. @@ +82,3 @@ + case Machine.MachineState.STOPPED: + var frame = paint_framed_thumbnail (null); + pixbuf = add_central_emblem_icon (frame, "system-shutdown-symbolic", BIG_EMBLEM_SIZE); I'd rather just add one method to draw stopped VM thumbnail. @@ +113,3 @@ } + private Gdk.Pixbuf add_central_emblem_icon (Gdk.Pixbuf pixbuf, string icon_name, int size) { central -> centered
Created attachment 306106 [details] [review] util-app: Add paint_empty_frame() Add the paint_empty_frame() function, allowing to draw a rounded rectangle with a border to use as a machine's thumbnail. This will be used in next commit to draw a better thumbnail for stopped boxes to better differenciate them from the others.
Created attachment 306107 [details] [review] collection-view: Stopped boxes show shutdown icon Draw a frame containing a 'system-shudown' symbolic icon as the thumbnail for stopped boxes. This is needed to better differenciate stopped boxes from running ones with (mostly) black thumbnails (e.g full screened terminal windows).
Review of attachment 306107 [details] [review]: Good otherwise. ::: src/collection-view.vala @@ +91,3 @@ + pixbuf = machine.pixbuf; + break; + } how about we add a is_stopped() method below (similar to is_running) and just do: var pixbuf = is_stopped ()? paint_stopped_machine_thumbnail () : machine.pixbuf;
Review of attachment 306106 [details] [review]: I wouldn't mention the usage in the first para that describes the change but only in the 2nd one for justification. ::: src/util-app.vala @@ +516,3 @@ } + + public Gdk.Pixbuf? paint_empty_frame (int width, int height, bool with_background = true) { To make these generic enough to be in utils, I'd also add color params. @@ +518,3 @@ + public Gdk.Pixbuf? paint_empty_frame (int width, int height, bool with_background = true) { + var surface = new Cairo.ImageSurface (Cairo.Format.ARGB32, width, height); + I'd remove this empty line. @@ +546,3 @@ + private void rounded_rectangle (Cairo.Context cr, double x, double y, double width, double height) { + var r = 2.0; // Radius + var degrees = Math.PI / 180.0; Why convert when you can use a constant value? Degrees is more natural for humans so this is not making it more readable IMHO.
Created attachment 306119 [details] [review] util-app: Add paint_empty_frame() Add the paint_empty_frame() function, allowing to draw a rounded rectangle with a border. This will be used in next commit to draw a better thumbnail for stopped boxes to better differenciate them from running ones with (mostly) black thumbnails (e.g full screened terminal windows).
Created attachment 306120 [details] [review] collection-view: Stopped boxes show shutdown icon Draw a frame containing a 'system-shudown' symbolic icon as the thumbnail for stopped boxes. This is needed to better differenciate stopped boxes from running ones with (mostly) black thumbnails (e.g full screened terminal windows).
Attachment 306119 [details] pushed as 3a6620a - util-app: Add paint_empty_frame() Attachment 306120 [details] pushed as 0a61fdb - collection-view: Stopped boxes show shutdown icon
*** Bug 751708 has been marked as a duplicate of this bug. ***
I think i closed this too early. The committed patches only solve the case of stopped boxes.
Created attachment 306412 [details] [review] collection-view: Add a default thumbnail Add paint_default_machine_thumbnail() to paint a framed 'computer' emblem as a machine's default thumbnail. This is needed to make a machine's default thumbnail consistent with the style of the stopped machines' thumbnail.
Created attachment 306413 [details] [review] machine: Remove fallback thumbnail Remove the draw_fallback_vm() static method and the default_fallback static attribute. This is needed to let CollectionView handle a machine's default thumbnail and to make it consistent with the style of the stopped machines' thumbnails.
These patches have been pushed for review, but it may be better to commit these after https://bugzilla.gnome.org/attachment.cgi?id=306411&action=edit so we can use paint_empty_machine_thumbnail() in paint_default_machine_thumbnail().
Review of attachment 306412 [details] [review]: ::: src/collection-view.vala @@ +87,3 @@ var pixbuf = is_stopped (machine) ? paint_stopped_machine_thumbnail () : machine.pixbuf; + pixbuf = pixbuf ?? paint_default_machine_thumbnail (); how come you are not removing the existing code that draws the default thumbnail?
Review of attachment 306413 [details] [review]: Oh! I only now realized that you have been moving pixbuf creation code from machine to colletion-view. I think this belongs in Machine class. If you don't want to further complicate this class, feel free to add a new class for pixbuf creation etc but collection-view shouldn't be doing this.
Created attachment 306471 [details] [review] Add MachineThumbnailer class This is needed by next commits to move thumbnail creation out of CollectionView and hence make it simpler.
Created attachment 306472 [details] [review] collection-view: Use MachineThumbnailer Use the MachineThumbnailer class to draw thumbnails instead of its own methods. This is needed to make the CollectionView class simpler, especially as the thumbnail drawing code is expected to get more complex in next commits.
Created attachment 306473 [details] [review] machine-thumbnailer: Add get_default_thumbnail() Add get_default_thumbnail() to paint a framed 'computer' emblem as the machine's default thumbnail. This is needed to make a machine's default thumbnail consistent with the style of the stopped machines' thumbnail.
Created attachment 306474 [details] [review] machine: Remove fallback thumbnail Remove the draw_fallback_vm() static method and the default_fallback static attribute. This is needed to let MachineThumbnailer handle a machine's default thumbnail and to make it consistent with the style of the stopped machines' thumbnails.
Review of attachment 306471 [details] [review]: Looks good but should be squashed with next patch since it's about moving code. That way it's easy to track which code was moved with a good diffing tool.
Review of attachment 306472 [details] [review]: Looks good otherwise. ::: src/collection-view.vala @@ +88,1 @@ + var thumbnailer = item.get_data<MachineThumbnailer> ("thumbnailer"); Would be a good opportunity to move this simply initialization to Machine class in this patch already so that we don't have to add this get_data/set_data game.
Review of attachment 306473 [details] [review]: ack
Review of attachment 306474 [details] [review]: * This should be squashed with last patch for same reason as the second and first patch should be squashed. * "handle a machine's" -> "handle the machine's".
Review of attachment 306472 [details] [review]: ::: src/collection-view.vala @@ +88,1 @@ + var thumbnailer = item.get_data<MachineThumbnailer> ("thumbnailer"); I'm not sure, I really dislike the idea to add view related stuff to a class which clearly is part on the model (Machine), that's why I did it this way.
Created attachment 306522 [details] [review] collection-view: Move thumbnailer to own class Add the MachineThumbnailer class to handle rendering a machine's thumbnail. This is needed to move thumbnail creation out of CollectionView, hence making it simpler, especially as the thumbnail drawing code is expected to get more complex in next commits.
Created attachment 306523 [details] [review] machine-thumbnailer: Draw fallback thumbnail Add MachineThumbnailer.get_default_thumbnail() to paint a framed 'computer' emblem as the machine's default thumbnail and remove Machine.draw_fallback_vm() and Machine.default_fallback. This is needed to let MachineThumbnailer handle the machine's default thumbnail and to make it consistent with the style of the stopped machines' thumbnails.
Review of attachment 306522 [details] [review]: Patch itself looks good. * "Move thumbnailer" -> "Move thumbnailing" * "Add the" -> "Add. * "This is needed to move thumbnail creation out of CollectionView", actually not since this patch itself is already doing it. While it's not exactly a problem, you should always first attempt to justify change on it's own as part of the general desire to keep patches atomic. This patch is on it's own good as a refactoring/cleaning one. Not saying that you shouldn't mention how it also helps any following patches as well.
Review of attachment 306523 [details] [review]: ack
Review of attachment 306522 [details] [review]: ::: src/machine-thumbnailer.vala @@ +20,3 @@ + public Gdk.Pixbuf thumbnail { + get { + _thumbnail = is_stopped (machine) ? get_stopped_thumbnail () : machine.pixbuf; * Actually this is a bit weird getter. If you're always going to draw the thumbnail on every read, there is no point of having _thumbnail around. * You should check for changes in machine's state and screenshot and update the thumbnail accordingly, instead.
Created attachment 306580 [details] [review] collection-view: Move thumbnailing to own class Move the responsability of rendering a machine's thumbnail out of CollectionView and to the new MachineThumbnailer class. This is needed to make CollectionView simpler, especially as the thumbnail drawing code is expected to get more complex in next commits.
Created attachment 306581 [details] [review] machine-thumbnailer: Draw fallback thumbnail Add MachineThumbnailer.get_default_thumbnail() to paint a framed 'computer' emblem as the machine's default thumbnail and remove Machine.draw_fallback_vm() and Machine.default_fallback. This is needed to let MachineThumbnailer handle the machine's default thumbnail and to make it consistent with the style of the stopped machines' thumbnails.
Review of attachment 306580 [details] [review]: ::: src/machine-thumbnailer.vala @@ +40,3 @@ + } + + private void update_config_connection () { you don't need to have this two layer approach to keep your categories notification in sync since box config doesn't change. @@ +61,3 @@ + } + + private bool is_stopped (Machine machine) { * This should go in Machine class (similar to is_on()). * can't we just use !machine.is_on() ? ::: src/machine.vala @@ +8,2 @@ public Boxes.CollectionSource source; + // Needs to be a property so the thumbnailer can monitor the config. As I pointed out in another patch's review, we don't want such comments. Instead putting this a separate patch with this as commit log would be more helpful. @@ +8,3 @@ public Boxes.CollectionSource source; + // Needs to be a property so the thumbnailer can monitor the config. + public Boxes.BoxConfig config { get; set; } I don't think config ever changes in Machine's lifetime.
Review of attachment 306581 [details] [review]: ack if nothing's changed.
Review of attachment 306580 [details] [review]: ::: src/machine-thumbnailer.vala @@ +40,3 @@ + } + + private void update_config_connection () { I do because config isn't set by the machine's constructor and the thumbnailer is in the machine and set byt its constructor, by the time the thumbnailer is constructed there is no machine.config, hence I have to monitor machine.config to be hable to connect to machine.config.categories once there is a config. The other way to do it would be to make this method public to let the machine call it but that would be weaker and more error prone. @@ +61,3 @@ + } + + private bool is_stopped (Machine machine) { Correct, I'll edit that. =) ::: src/machine.vala @@ +8,3 @@ public Boxes.CollectionSource source; + // Needs to be a property so the thumbnailer can monitor the config. + public Boxes.BoxConfig config { get; set; } As explained in the other comment: it does, it is not set by the machine's constructor.
Review of attachment 306580 [details] [review]: ::: src/machine-thumbnailer.vala @@ +40,3 @@ + } + + private void update_config_connection () { Another method would be to ensure that box config is created *before* thumbnailer. Can you have a look into that?
Review of attachment 306580 [details] [review]: ::: src/machine-thumbnailer.vala @@ +61,3 @@ + } + + private bool is_stopped (Machine machine) { About using is_on: as strange as it sound it's not that simple as there are states which are neither on or off, for example the 'unknwon' state for which we may want another thumbnail like the default one or another one stating an error (an explamation mark for example) or that it state is unkown (a question mark maybe).
Created attachment 306629 [details] [review] machine: Constructor init display config Move the call to create_display_config() from subclasses' constructors to Machine's constructor. This is needed to ensure that Machine.config is set in Machine's constructor and it will be used by next commits.
Created attachment 306630 [details] [review] machine: Add 'is_stopped' prop This is be needed by next commits to easily check if a machine is stopped to draw a special thumbnail for them.
Created attachment 306631 [details] [review] collection-view: Move thumbnailing to own class Move the responsability of rendering a machine's thumbnail out of CollectionView and to the new MachineThumbnailer class. This is needed to make CollectionView simpler, especially as the thumbnail drawing code is expected to get more complex in next commits.
Created attachment 306632 [details] [review] machine-thumbnailer: Draw fallback thumbnail Add MachineThumbnailer.get_default_thumbnail() to paint a framed 'computer' emblem as the machine's default thumbnail and remove Machine.draw_fallback_vm() and Machine.default_fallback. This is needed to let MachineThumbnailer handle the machine's default thumbnail and to make it consistent with the style of the stopped machines' thumbnails.
Review of attachment 306580 [details] [review]: ::: src/machine-thumbnailer.vala @@ +61,3 @@ + } + + private bool is_stopped (Machine machine) { that's the only state and we have always treated as 'on' state so that's not a problem.
Review of attachment 306629 [details] [review]: "it will be used by next commits" -> "subclasses will assume config to be initaized after base constructor returns in the following patches".
Review of attachment 306630 [details] [review]: As I said previously we don't need this and if we want to add it, we want to be consistent and keep it a method like is_on() and friends.
Review of attachment 306631 [details] [review]: Looks fine part from nick. ::: src/machine.vala @@ +228,3 @@ create_display_config (uuid); + + // This need to be set after the 'config' prop has been set. need -> needs
Review of attachment 306632 [details] [review]: ack
This depends on https://bugzilla.gnome.org/show_bug.cgi?id=752007
Review of attachment 306630 [details] [review]: As I explained, we need it because !is_on =/= is_stopped (using !is_on screw the thumbnails up), also you stated earlier that the is_* methods should be properties, so we should keep this patch as is and turn the other is_* methods into properties first.
Created attachment 306909 [details] [review] machine: Constructor init display config Move the call to create_display_config() from subclasses' constructors to Machine's constructor. This is needed to ensure that Machine.config is set in Machine's constructor and subclasses will assume config to be initalized after base constructor returns in the following patches.
Created attachment 306910 [details] [review] machine: Add 'is_stopped' prop This is be needed by next commits to easily check if a machine is stopped to draw a special thumbnail for them.
Created attachment 306911 [details] [review] collection-view: Move thumbnailing to own class Move the responsability of rendering a machine's thumbnail out of CollectionView and to the new MachineThumbnailer class. This is needed to make CollectionView simpler, especially as the thumbnail drawing code is expected to get more complex in next commits.
Created attachment 306912 [details] [review] machine-thumbnailer: Draw fallback thumbnail Add MachineThumbnailer.get_default_thumbnail() to paint a framed 'computer' emblem as the machine's default thumbnail and remove Machine.draw_fallback_vm() and Machine.default_fallback. This is needed to let MachineThumbnailer handle the machine's default thumbnail and to make it consistent with the style of the stopped machines' thumbnails.
Review of attachment 306630 [details] [review]: Not really. You talked of states that aren't "on" but only provided one example: unknown. As I said, that state has simply been assumed to be 'running'. Such a state is more a bug in some layer and normally do not happen so there is no point is having special emblems etc for it. Let's continue to assume this state to be same as 'running'.
Review of attachment 306630 [details] [review]: After deeper investigation, usage of "machine.is_stopped" would have to be replaced by "!machine.is_on && machine.state && machine.state != Machine.MachineState.SAVED" at least, "!machine.is_on && machine.state != Machine.MachineState.UNKNOWN && machine.state != Machine.MachineState.SAVED" being better. Using "!machine.is_on" or "!machine.is_on && machine.state != Machine.MachineState.UNKNOWN" doesn't work.
Review of attachment 306630 [details] [review]: ah ok, in that case this patch is justified. I guess I was wrong about UNKNOWN being treated as 'on' rather?
Review of attachment 306630 [details] [review]: I don't know about UNKOWN, but what is certain is that SAVED is neither "on" or "stopped" ("saved" is a different subset of "!on/off" than "stopped").
Review of attachment 306630 [details] [review]: SAVED is not part of is_on for obvious reasons but is_stopped would be only about stopped and forced stopped states and that is the only state we want to draw this special thumbnail for. SAVED state shows last thumbnail of VM greyed out.
Review of attachment 306909 [details] [review]: ack
Review of attachment 306910 [details] [review]: ack
Review of attachment 306911 [details] [review]: ack
Review of attachment 306912 [details] [review]: Actually the last para makes it sound like this patch is only laying the foundation for the actual change. I'll try to fix while pushing..
Created attachment 307035 [details] [review] machine: Constructor init display config Move the call to create_display_config() from subclasses' constructors to Machine's constructor. This is needed to ensure that Machine.config is set in Machine's constructor and subclasses will assume config to be initalized after base constructor returns in the following patches.
Created attachment 307036 [details] [review] machine: Add 'is_stopped' prop This is be needed by next commits to easily check if a machine is stopped to draw a special thumbnail for them.
Created attachment 307037 [details] [review] collection-view: Move thumbnailing to own class Move the responsability of rendering a machine's thumbnail out of CollectionView and to the new MachineThumbnailer class. This is needed to make CollectionView simpler, especially as the thumbnail drawing code is expected to get more complex in next commits.
Created attachment 307038 [details] [review] machine-thumbnailer: Draw fallback thumbnail Add MachineThumbnailer.get_default_thumbnail() to paint a framed 'computer' emblem as the machine's default thumbnail and remove Machine.draw_fallback_vm() and Machine.default_fallback. This is needed to let MachineThumbnailer handle the machine's default thumbnail and to make it consistent with the style of the stopped machines' thumbnails.
Review of attachment 306912 [details] [review]: While editing the log, I realized also that first paragraph is translating code changes to English rather than summarizing the change.
Attachment 307035 [details] pushed as 906afc2 - machine: Constructor init display config Attachment 307036 [details] pushed as 292855a - machine: Add 'is_stopped' prop Attachment 307037 [details] pushed as d78869d - collection-view: Move thumbnailing to own class Attachment 307038 [details] pushed as 13e1055 - machine-thumbnailer: Draw fallback thumbnail