GNOME Bugzilla – Bug 710306
support snapshots
Last modified: 2016-03-31 13:22:07 UTC
(from Montreal summit session) Boxes should support live snapshots of vms when the unterlying stack supports it. We discussed keeping it fairly simple and minimal: - Allow taking a snapshot at any time. Should probably allow to add a label or note to it - Allow to roll back to a snapshot - There has to be a way to list available snapshots - Maybe there could be a way to delete or drop old snapshots Allowing to run from a snapshot while keeping the newer changes around makes things a lot more complicated, because you get different branches, and suddenly need a graph editor to keep track of the changes. Therefore, we decided to not allow this, at least initially.
Created attachment 282071 [details] [review] properties: Add snapshots page This will later be used to show the snapshots of a VM
Created attachment 282073 [details] [review] libvirt-machine: Delete snapshots with machine libvirt does not allow do delete/undefine domains that have snapshot metadata attached. Sine creating snapshots will be possible, we need to pass GVir.DomainDeleteFlags.SNAPSHOTS_METADATA to the delete() call of the domain, so the user can even delete domains with snapshots.
Created attachment 282074 [details] [review] Add SnapshotListRow This will be used in the snapshots page of the vm properties and features a way to delete, rename or revert to the snapshot it represents.
Created attachment 282075 [details] [review] properties: Unify margins of property widgets They were only added on the left of each configuration widget which makes horizontally expanding widgets look unbalanced.
Created attachment 282076 [details] [review] properties: Use a GtkScrolledWindow for the pages This is needed for property pages that contain either very high widgets or a dynamic number of widgets, e.g. the coming snapshots page.
Created attachment 282077 [details] [review] LibvirtMachine: Add snapshot API Add API to fetch, get and create snapshots which will be used in later commits.
Created attachment 282078 [details] [review] properties: Implement the snapshots page The snapshot page lists all the snapshot available for the domain and the user is able to delete, rename or revert to them.
Created attachment 282144 [details] [review] machine: Don't switch away from the properties If the user reverts to a snapshots and the domain is running, it will temporarily be paused, causing its display to disconnect. We normally switch to the collections view, but let's not to it if the user is currently in the properties view since the state change is just normal when reverting to a snapshot.
Review of attachment 282071 [details] [review]: Fine apart from one nitpick: missing '.' at the end of commit log description.
Review of attachment 282073 [details] [review]: Looks good apart from a typo in description: not allow do -> not allow to
Review of attachment 282074 [details] [review]: nitpick: vm -> VM. I probably will have more comments but those can wait till next iteration. :) ::: data/ui/snapshot-list-row.ui @@ +2,3 @@ +<interface> + + <menu id="more_menu"> Isn't this more like a actions_menu? @@ +42,3 @@ + + + Kudos for inserting empty lines to make it easier to read but one empty line separate is enough I think. :) @@ +114,3 @@ + + <packing> + <property name="name">edit_name</property> * There is a 'name' child property? Shouldn't you use give an id to the box instead? * I'd add '_box' suffix to name/id. ::: src/snapshot-list-row.vala @@ +1,3 @@ +// This file is part of GNOME Boxes. License: LGPLv2+ + + Redundant line. @@ +4,3 @@ +[GtkTemplate (ui = "/org/gnome/Boxes/ui/snapshot-list-row.ui")] +private class Boxes.SnapshotListRow : Gtk.ListBoxRow { + private static const string STACK_DEFAULT = "default"; 'static' is redundant/implied for 'const'. @@ +6,3 @@ + private static const string STACK_DEFAULT = "default"; + private static const string STACK_EDIT_NAME = "edit_name"; + [GtkChild] Would prefer a empty line separate static and non-static members. @@ +7,3 @@ + private static const string STACK_EDIT_NAME = "edit_name"; + [GtkChild] + private Gtk.Revealer revealer; Maybe more descriptive names for 'relealer' and 'stack' so its easy to tell what they are for? If descriptive names don't help, some comments (preferably in the UI file) would help too. @@ +15,3 @@ + private Gtk.Entry name_entry; + + /** index of the snapshot in the list */ We prefer single-line '//' in Vala code. @@ +68,3 @@ + } + + public override bool draw (Cairo.Context ct) { * A comment here explaining why need to override draw would be nice. * Are you sure we can't achieve the same with CSS? The UI doesn't need to look the the perfect replica of the mockups btw. We can do fine-tuning later. @@ +110,3 @@ + + [GtkCallback] + private void save_name_button_clicked_cb () { We tend to use 'on_save_name_button_clicked' form for callbacks. @@ +151,3 @@ + }; + + var message = _("%s deleted.").printf (snapshot.get_name ()); * I think we want to have a more descriptive/specific message: "Snapshot \"%s\" deleted." ? * Name is the free form text? I can't remember this cause in case of domains, name is more like an ID and title is user visible name. @@ +177,3 @@ + var parent = this.get_parent (); + + if (parent == null || !(parent is Gtk.ListBox)) { Coding-style: No braces for single blocks. @@ +181,3 @@ + } + + var siblings = ((Gtk.Container)parent).get_children (); a space between ) and parent would be nicer. :) @@ +183,3 @@ + var siblings = ((Gtk.Container)parent).get_children (); + this.index = siblings.index (this); + this.parent_size = (int)siblings.length (); same here
Review of attachment 282075 [details] [review]: Unify -> remove?
Review of attachment 282074 [details] [review]: forgot to update status
Review of attachment 282076 [details] [review]: I don't think we want this for all pages. How about we only put snapshots page in scrolled window?
Review of attachment 282077 [details] [review]: I don't think we need this abstraction here. LibvirtMachine already has lots of code, would be nice not to add more to it. Besides all properties related code is in LibvirtMachineProperties so if we want this, it probably fits more in there. ::: src/libvirt-machine.vala @@ +48,3 @@ + config.set_name (new GLib.DateTime.now_local ().format ("%y-%m-%e-%H-%M-%S")); + config.set_description (config.get_name ()); + return domain.create_snapshot (config, 0); coding-style: empty line before return.
Review of attachment 282078 [details] [review]: Looks good apart from these non-functional issues. ::: src/i-properties-provider.vala @@ +62,3 @@ + +private class Boxes.SnapshotsProperty : Boxes.Property { This is not generic enough to be put here. The other Property subclasses are here cause they are kinda generic. Lets put this in its own file. @@ +73,3 @@ + list.selection_mode = Gtk.SelectionMode.NONE; + list.set_size_request (-1, 250); + list.set_sort_func ((a, b) => { would be nice to break this out into a named function. @@ +77,3 @@ + GVirConfig.DomainSnapshot conf2; + try { + conf1 = ((SnapshotListRow)a).snapshot.get_config (0); use 'as' keyword to cast wherever possible. @@ +80,3 @@ + conf2 = ((SnapshotListRow)b).snapshot.get_config (0); + } catch (GLib.Error e) { + critical (e.message); i don't think its so critical. Just a warning is enough but you want to provide context in the message: "Failed to fetch snapshot: %s". @@ +84,3 @@ + } + if (conf1.get_creation_time () < conf2.get_creation_time ()) + return -1; these two lines can go into 'try' block above and then you don't need to declare the conf variables beforehand and just use 'var'. Less LOCs and more readable code. @@ +88,3 @@ + }); + + create_snapshot_button.clicked.connect (() => { this too! The less nesting and LOCs in a function, the better! @@ +106,3 @@ + machine.fetch_snapshots.end (res); + } catch (GLib.Error e) { + critical (e.message); Same here: warning and some context. @@ +108,3 @@ + critical (e.message); + } + GLib.List<GVir.DomainSnapshot> snapshots = machine.get_snapshots (); Use var if possible. @@ +110,3 @@ + GLib.List<GVir.DomainSnapshot> snapshots = machine.get_snapshots (); + + unowned GLib.List<GVir.DomainSnapshot> l =snapshots; space after '=' @@ +111,3 @@ + + unowned GLib.List<GVir.DomainSnapshot> l =snapshots; + while (l != null) { you can use foreach. You gotta think in vala when coding for Boxes. :) @@ +248,3 @@ + + protected Boxes.SnapshotsProperty add_snapshots_property (ref List<Boxes.Property> list, + LibvirtMachine machine) { coding style nitpick: Align the names too: TypeA a, TypeBCA b,
Review of attachment 282144 [details] [review]: Suggestion for better shortlog: machine: Don't switch away from properties on display disconnection Its slightly longer than recommended 50 chars but its better to be clear as long as we don't break the limit: 72 chars. ::: src/machine.vala @@ +77,3 @@ value != MachineState.RUNNING && + value != MachineState.UNKNOWN && + App.app.ui_state != UIState.PROPERTIES) { Lets keep UNKNOWN to be last to be checked.
Review of attachment 282074 [details] [review]: ::: data/ui/snapshot-list-row.ui @@ +114,3 @@ + + <packing> + <property name="name">edit_name</property> The 'name' child property is for use with GtkStack's visible_child_name property. I usually use that if I want "modes" in a GtkStack instead of referring to the actual child ID (i.e. in this case there's normal mode and edit-name mode), but I'm also fine with giving the containers IDs and using visible_child. ::: src/snapshot-list-row.vala @@ +68,3 @@ + } + + public override bool draw (Cairo.Context ct) { A comment would be fine by me of course. Overriding the draw function is needed for making the path(s) look connected. GtkListBoxRow reserves some space for the focus ring we can't draw onto if we just use e.g. a widget for the indicator on the left of a SnapshoListRow. @@ +151,3 @@ + }; + + var message = _("%s deleted.").printf (snapshot.get_name ()); Oh, this line is actually wrong, we should also show the description here (which is totally free-form, yes).
Review of attachment 282074 [details] [review]: ::: data/ui/snapshot-list-row.ui @@ +114,3 @@ + + <packing> + <property name="name">edit_name</property> Ah ok. Looking at other UI files, I've named them simply after the widget or used 'WIDGETNAME-page'. BTW, you should still give them an ID for consistency.
Seems we never put the design page link in here. :( Well, better late than never: https://wiki.gnome.org/Design/Apps/Boxes/Snapshots
(In reply to comment #14) > Review of attachment 282076 [details] [review]: > > I don't think we want this for all pages. How about we only put snapshots page > in scrolled window? All the property widgets get put into a GtkGrid and that grid has the margins (so it does not expand over the entire area). So if just one page has a GtkScrolledWindow around it, there will still be the margins, i.e. the scrollbar won't be on the very right. (I mean, it's possible to do it just for the properties page, but I'm not sure it'd worth the hazzle provided that it doesn't make any difference for the other pages)
(In reply to comment #21) > (In reply to comment #14) > > Review of attachment 282076 [details] [review] [details]: > > > > I don't think we want this for all pages. How about we only put snapshots page > > in scrolled window? > > All the property widgets get put into a GtkGrid and that grid has the margins > (so it does not expand over the entire area). So if just one page has a > GtkScrolledWindow around it, there will still be the margins, i.e. the > scrollbar won't be on the very right. (I mean, it's possible to do it just for > the properties page, but I'm not sure it'd worth the hazzle provided that it > doesn't make any difference for the other pages) Ah ok. Fair enough. P.S. Please try to always use the review link/page for replying to inline comments so context is clear.
(In reply to comment #15) > Review of attachment 282077 [details] [review]: > > I don't think we need this abstraction here. LibvirtMachine already has lots of > code, would be nice not to add more to it. Besides all properties related code > is in LibvirtMachineProperties so if we want this, it probably fits more in > there. That works all out so far, but since create_snapshot is called from SnapshotListRow, there has to be a function in LibvirtMachine for it *or* the LibvirtMachineProperties in LibvirtMachine have to be public. Which of the two did you have in mind or am I missing something?
(In reply to comment #23) > (In reply to comment #15) > > Review of attachment 282077 [details] [review] [details]: > > > > I don't think we need this abstraction here. LibvirtMachine already has lots of > > code, would be nice not to add more to it. Besides all properties related code > > is in LibvirtMachineProperties so if we want this, it probably fits more in > > there. > > > That works all out so far, but since create_snapshot is called from > SnapshotListRow, there has to be a function in LibvirtMachine for it *or* the > LibvirtMachineProperties in LibvirtMachine have to be public properties becoming public sounds fine to me.
Created attachment 282849 [details] [review] libvirt-machine: Delete snapshots with machine libvirt does not allow to delete/undefine domains that have snapshot metadata attached. Sine creating snapshots will be possible, we need to pass GVir.DomainDeleteFlags.SNAPSHOTS_METADATA to the delete() call of the domain, so the user can even delete domains with snapshots.
Created attachment 282850 [details] [review] Add SnapshotListRow This will be used in the snapshots page of the VM properties and features a way to delete, rename or revert to the snapshot it represents.
Created attachment 282852 [details] [review] properties: Remove margins of property widgets They were only added on the left of each configuration widget which makes horizontally expanding widgets look unbalanced.
Created attachment 282853 [details] [review] properties: Use a GtkScrolledWindow for the pages This is needed for property pages that contain either very high widgets or a dynamic number of widgets, e.g. the coming snapshots page.
Created attachment 282854 [details] [review] LibvirtMachineProperties: Add snapshot API Add API to fetch, get and create snapshots which will be used in later commits.
Created attachment 282855 [details] [review] properties: Implement the snapshots page The snapshot page lists all the snapshot available for the domain and the user is able to delete, rename or revert to them.
Created attachment 282856 [details] [review] machine: Don't switch away from properties on display disconnection If the user reverts to a snapshots and the domain is running, it will temporarily be paused, causing its display to disconnect. We normally switch to the collections view, but let's not to it if the user is currently in the properties view since the state change is just normal when reverting to a snapshot.
Review of attachment 282849 [details] [review]: ack
Review of attachment 282850 [details] [review]: Looks fine apart form these minor nitpicks. ::: data/gtk-style.css @@ +14,3 @@ +.boxes-snapshot-list-row { + border-bottom: 1px solid #000; Lasse will not like more hard-coding of colors. :) Fine by me. He or someone can remove all hard codings at the same time. ::: src/snapshot-list-row.vala @@ +20,3 @@ + private int parent_size; + + public GVir.DomainSnapshot snapshot; We define public API before private as coding style. @@ +69,3 @@ + } + + // Need to override this in order to draw the indicators and their connections properly and what does 'properly' mean? :) @@ +171,3 @@ + _("_Undo"), + (owned)undo, + (owned)really_remove); space b/w ')' and identifier.
Review of attachment 282852 [details] [review]: Isn't this needed to have some space between property names and sidebar?
Review of attachment 282853 [details] [review]: ack
Review of attachment 282854 [details] [review]: Good otherwise ::: src/libvirt-machine-properties.vala @@ +593,3 @@ + config.set_description (config.get_name ()); + + return machine.domain.create_snapshot (config, 0); Shouldn't this be async?
Review of attachment 282855 [details] [review]: Not everyone would know about the design so 'Implement the' -> 'Add' in shortlog. ::: src/libvirt-machine.vala @@ +23,3 @@ public bool importing { get { return vm_creator != null && vm_creator is VMImporter; } } + public LibvirtMachineProperties properties; I'd put this in separate patch. ::: src/snapshots-property.vala @@ +1,3 @@ +// This file is part of GNOME Boxes. License: LGPLv2+ + +private class Boxes.SnapshotsProperty : Boxes.Property { Following the convention set by your own previous patches, I'd put addition of this class as separate patch. :) @@ +7,3 @@ + public SnapshotsProperty (LibvirtMachine machine) { + var vbox = new Gtk.Box (Gtk.Orientation.VERTICAL, 0); + // Need to call the base constructor here so we can use the `this` pointer later its not exactly a pointer on vala level and its typical to call base () on top of the constructor so this comment is pretty much redundant. @@ +8,3 @@ + var vbox = new Gtk.Box (Gtk.Orientation.VERTICAL, 0); + // Need to call the base constructor here so we can use the `this` pointer later + base (null, vbox, null); I'd put an empty line here and above base() so its clear where chain-up is happening. @@ +64,3 @@ + new_row.reveal (); + } catch (GLib.Error e) { + warning ("Could not create snapshot: %s", e.message); I think a notification to user is needed here (though we shouldn't include details/exact error in that one).
Review of attachment 282856 [details] [review]: * Typo: "let's not to it" -> "let's not do it". * I assume you have tested that after this patch, display gets reconnected after machine comes back to running?
(In reply to comment #34) > Review of attachment 282852 [details] [review]: > > Isn't this needed to have some space between property names and sidebar? All the GtkGrids get a margin here: https://git.gnome.org/browse/gnome-boxes/tree/src/properties.vala#n65 This is what it looks like with that patch applied: http://i.imgur.com/S3BgcIp.png (the border on the right is a little hard to see but you get the idea)
Review of attachment 282854 [details] [review]: ::: src/libvirt-machine-properties.vala @@ +593,3 @@ + config.set_description (config.get_name ()); + + return machine.domain.get_snapshots (); I posted these patches right at the Boxes BoF so they still block the main loop (for creating and reverting). I planned to make those async (and show some progress indication) in later patches.
Review of attachment 282852 [details] [review]: Fair enough
Review of attachment 282854 [details] [review]: ::: src/libvirt-machine-properties.vala @@ +593,3 @@ + config.set_description (config.get_name ()); + + return machine.domain.create_snapshot (config, 0); OK. When you do, please update existing (unmerged) patches wherever possible.
Created attachment 283265 [details] [review] Add SnapshotListRow This will be used in the snapshots page of the VM properties and features a way to delete, rename or revert to the snapshot it represents.
Created attachment 283267 [details] [review] LibvirtMachineProperties: Add snapshot API Add API to fetch, get and create snapshots which will be used in later commits.
Created attachment 283268 [details] [review] LibvirtMachine: Make the properties public Creating, fetching, etc. of snapshots is now part of the LibvirtMachineProperties and to be able to create snapshots from outside the machine, we need access to its properties.
Created attachment 283269 [details] [review] Add SnapshotsProperty The SnapshotsProperty provides a UI to view, create, delete, rename and revert to snapshots of the given LibvirtMachine.
Created attachment 283270 [details] [review] properties: Implement the snapshots page The snapshot page lists all the snapshot available for the domain and the user is able to delete, rename or revert to them.
Created attachment 283271 [details] [review] DisplayPage: Add a way to switch the display widget without showing it If the user is in the properties page and the machine's display reconnects, we need to replace the display page's widget with the new display without showing it.
Created attachment 283272 [details] [review] machine: Don't switch away from properties on display disonnection If the user reverts to a snapshots and the domain is running, it will temporarily be paused, causing its display to disconnect. We normally switch to the collections view, but let's not to it if the user is currently in the properties view since the state change is just normal when reverting to a snapshot.
Review of attachment 283265 [details] [review]: Looks good otherwise. ::: data/gtk-style.css @@ +14,3 @@ +.boxes-snapshot-list-row { + border-bottom: 1px solid #000; what happens if you don't set colors here? Do widgets look totally different from the mockup? ::: src/snapshot-list-row.vala @@ +131,3 @@ + private void revert_to_activated (GLib.SimpleAction action, GLib.Variant? v) { + run_in_thread.begin (() => { + snapshot.revert_to (0); Nope, we shouldn't need to use run_in_thread here but rather we want snapshots_revert_to_async here. @@ +138,3 @@ + } catch (GLib.Error e) { + warning (e.message); + machine.window.notificationbar.display_error (_("Revert failed")); Noun of 'revert' is not 'revert' but 'reversion'. I'd be a bit more specific: Failed to apply snapshot "%s" to "%s". @@ +145,3 @@ + + private void delete_activated (GLib.SimpleAction action, GLib.Variant? v) { + Notification.OKFunc undo = () => { I'd define these two function variables before they are used below. i-e before machine.window.notificationbar.display_for_action. @@ +155,3 @@ + this.snapshot.delete (0); + } catch (GLib.Error e) { + critical ("Error while deleting snapshot %s: %s", not really that critical. Rest of boxes can continue functioning fine. I'll use warning here. @@ +159,3 @@ + e.message); + } + parent_container.queue_draw (); we need to redraw even if deletion fails? @@ +167,3 @@ + snapshot_identifier = config.get_description (); + } catch (GLib.Error e) { + critical ("Could not get config of snapshot %s: %s", * should be a 'warning'. * 'config' -> 'configuration'. @@ +201,3 @@ + return; + + var siblings = ((Gtk.Container) parent).get_children (); use 'as' to cast whenever possible.
Review of attachment 283267 [details] [review]: ack
Review of attachment 283268 [details] [review]: From shorlog it sounds like you are making all properties public. How about: Make 'properties' field public?
Review of attachment 283269 [details] [review]: looks good otherwise. ::: src/snapshots-property.vala @@ +32,3 @@ + var snapshots = machine.properties.get_snapshots (); + + foreach (var snap in snapshots) { * snap -> snapshot @@ +54,3 @@ + } + + return 1; since this is only happening if 'if' condition above is not true, i'd put that in an else branch above. @@ +57,3 @@ + } + + private async void on_create_snapshot_button_clicked () { Signal handler is supposed to be sync so this is likely to case problems later if its not already. I'll use a lambda when connecting above and rename it to just 'create_snapshot' to not suggest its a signal handler callback. @@ +60,3 @@ + try { + GVir.DomainSnapshot new_snapshot = null; + yield run_in_thread (() => { We shouldn't need run_in_thread here and create_snapshot should be async. If some caller needs sync version, you can add a sync variant too with "_sync" suffix. @@ +69,3 @@ + } catch (GLib.Error e) { + warning ("Could not create snapshot: %s", e.message); + machine.window.notificationbar.display_error (_("Snapshot creation failed")); I'll include title of VM in the message and have it a full sentence with '.' at the end.
Review of attachment 283270 [details] [review]: ::: src/i-properties-provider.vala @@ +184,3 @@ + + protected Boxes.SnapshotsProperty add_snapshots_property (ref List<Boxes.Property> list, + LibvirtMachine machine) { As I said before, this is not generic enough to belong here. ::: src/libvirt-machine-properties.vala @@ +266,3 @@ + case PropertiesPage.SNAPSHOTS: + add_snapshots_property (ref list, machine); + break; empty line before 'break'.
Review of attachment 283271 [details] [review]: 'Add a' and 'the ' could be dropped to keep shortlog short and 'without' can be abbreviated 'w/o'. ::: src/display-page.vala @@ +101,3 @@ } + public void show_display (Boxes.Display display, Widget widget, bool show_page = true) { instead of complicating this method and making it do something that is not very much according to the name, how about adding a new method that only switches the display and using that from this function?
Review of attachment 283272 [details] [review]: Same typo as last time: "not to it" -> "not do it".
Review of attachment 283265 [details] [review]: ::: data/gtk-style.css @@ +14,3 @@ +.boxes-snapshot-list-row { + border-bottom: 1px solid #000; Without any color: http://i.imgur.com/asyqg9a.png Using @borders wich gets exported from Adwaita: http://i.imgur.com/gu6e3Yg.png The latter looks basically the same so depending on what you are after, it might work. ::: src/snapshot-list-row.vala @@ +131,3 @@ + private void revert_to_activated (GLib.SimpleAction action, GLib.Variant? v) { + run_in_thread.begin (() => { + snapshot.revert_to (0); Need to add that to libvirt-glib first. Does the same apply to create_snapshot? And delete?
Review of attachment 283265 [details] [review]: ::: data/gtk-style.css @@ +14,3 @@ +.boxes-snapshot-list-row { + border-bottom: 1px solid #000; I think both are fine. As I said we don't necessarily need to be completely 100% consistent with mockup. ::: src/snapshot-list-row.vala @@ +131,3 @@ + private void revert_to_activated (GLib.SimpleAction action, GLib.Variant? v) { + run_in_thread.begin (() => { + snapshot.revert_to (0); Yeah. As I said on IRC yesterday, every libvirt call involves IPC and therefore need to be ideally async.
Created attachment 283579 [details] [review] Add SnapshotListRow This will be used in the snapshots page of the VM properties and features a way to delete, rename or revert to the snapshot it represents.
Created attachment 283580 [details] [review] LibvirtMachine: Make the 'properties' field public Creating, fetching, etc. of snapshots is now part of the LibvirtMachineProperties and to be able to create snapshots from outside the machine, we need access to its properties.
Created attachment 283581 [details] [review] Add SnapshotsProperty The SnapshotsProperty provides a UI to view, create, delete, rename and revert to snapshots of the given LibvirtMachine.
Created attachment 283582 [details] [review] properties: Add the snapshots page The snapshot page lists all the snapshot available for the domain and the user is able to delete, rename or revert to them.
Created attachment 283583 [details] [review] DisplayPage: Way to switch display widget w/o showing it If the user is in the properties page and the machine's display reconnects, we need to replace the display page's widget with the new display without showing it.
Created attachment 283584 [details] [review] machine: Don't switch away from properties on display disonnection If the user reverts to a snapshots and the domain is running, it will temporarily be paused, causing its display to disconnect. We normally switch to the collections view, but let's not do it if the user is currently in the properties view since the state change is just normal when reverting to a snapshot.
Review of attachment 283579 [details] [review]: Looking good otherwise. ::: data/gtk-style.css @@ +18,3 @@ + +.boxes-snapshot-list-row.indicator { + background-color: #666; The question in my previous review was about all the hard-coding of colors here. :) ::: src/snapshot-list-row.vala @@ +168,3 @@ + } catch (GLib.Error e) { + warning ("Error while deleting snapshot %s: %s", snapshot.get_name (), e.message); + } Since you removed parent_container.queue_draw (); from here, I'm guessing its not needed at all in the end?
Review of attachment 283580 [details] [review]: ack
Review of attachment 283581 [details] [review]: Good otherwise. ::: src/snapshots-property.vala @@ +58,3 @@ + + private void on_create_snapshot_button_clicked () { + machine.properties.create_snapshot.begin ((obj, res) => { I tend to avoid using lambdas for more than a few LOC which is why I suggested in previous review: "I'll use a lambda when connecting above and rename it to just 'create_snapshot' to not suggest its a signal handler callback."
Review of attachment 283582 [details] [review]: Good other than that. ::: src/i-properties-provider.vala @@ +182,3 @@ return property; } + redundant change.
Review of attachment 283583 [details] [review]: good otherwise. ::: src/display-page.vala @@ +108,3 @@ + } + + public void set_display (Boxes.Display display, Widget widget) { hmm.. not a very appropriate name. Sounds like its a setter. How about show_display_only ? Better name suggestions welcome!
Review of attachment 283584 [details] [review]: ack
Review of attachment 283579 [details] [review]: ::: data/gtk-style.css @@ +18,3 @@ + +.boxes-snapshot-list-row.indicator { + background-color: #666; Oh! :/ we could use @theme_fg_color and @insensitive_fg_color to get http://i.imgur.com/b4SV4KY.png ::: src/snapshot-list-row.vala @@ +168,3 @@ + } catch (GLib.Error e) { + warning ("Error while deleting snapshot %s: %s", snapshot.get_name (), e.message); + } Oh, I think that got lost somewhere.
Review of attachment 283579 [details] [review]: ::: data/gtk-style.css @@ +18,3 @@ + +.boxes-snapshot-list-row.indicator { + background-color: #666; Seems fine to me. :)
Review of attachment 283583 [details] [review]: ::: src/display-page.vala @@ +108,3 @@ + } + + public void set_display (Boxes.Display display, Widget widget) { I don't think about adding a widget and calling show() on it as "showing" it if it still stays invisible for the user. Something like replace_display would be better IMO but if you want to go for show_display_only, thats fine with me.
Review of attachment 283583 [details] [review]: ::: src/display-page.vala @@ +108,3 @@ + } + + public void set_display (Boxes.Display display, Widget widget) { true, lets go with that then. :)
Created attachment 283653 [details] [review] Add SnapshotListRow This will be used in the snapshots page of the VM properties and features a way to delete, rename or revert to the snapshot it represents.
Created attachment 283654 [details] [review] Add SnapshotsProperty The SnapshotsProperty provides a UI to view, create, delete, rename and revert to snapshots of the given LibvirtMachine. The lambda became a little longer because of the progress indication (which would need to go into the upper lambda for the button). I didn't know if it's ok to blow up *that* lambda instead of the one in the clicked handler so I just left it like that for now.
Created attachment 283656 [details] [review] properties: Add the snapshots page The snapshot page lists all the snapshot available for the domain and the user is able to delete, rename or revert to them.
Created attachment 283657 [details] [review] DisplayPage: Way to switch display widget w/o showing it If the user is in the properties page and the machine's display reconnects, we need to replace the display page's widget with the new display without showing it.
Created attachment 283658 [details] [review] machine: Don't switch away from properties on display disonnection If the user reverts to a snapshots and the domain is running, it will temporarily be paused, causing its display to disconnect. We normally switch to the collections view, but let's not do it if the user is currently in the properties view since the state change is just normal when reverting to a snapshot.
Created attachment 283659 [details] [review] machine: Don't show error msg on disconnect in props State changes to STOPPED are completely normal if the user reverts to a snapshot that was taken when the machine was stopped, so don't show any error message in this case and don't switch to the collections view.
Created attachment 283660 [details] [review] PropertiesToolbar: Don't go back to non-running VMs In some cases, the user can be in the properties view and the VM can change into STOPPED state. This can for example happen if the user reverts to a snapshot that has been taken when the VM was STOPPED (the snapshot is 'shutoff'). If that's the case, don't switch back to the display page of a non-running VM but show the collections view instead.
Created attachment 283662 [details] [review] SnapshotListRow: Show progress when reverting Since reverting to a snapshot can take a while when the VM is running, we also need to show progress indication in that case.
Review of attachment 283653 [details] [review]: sorry found an issue again. :) ::: src/snapshot-list-row.vala @@ +23,3 @@ + + private Boxes.LibvirtMachine machine; + private Gtk.Container? parent_container = null; This needs to be weak ref to avoid cyclic dep.
Review of attachment 283653 [details] [review]: good otherwise
Review of attachment 283654 [details] [review]: ::: src/snapshots-property.vala @@ +20,3 @@ + + + Redundant empty lines. @@ +34,3 @@ + create_snapshot_button.icon_widget = icon_img; + + machine.properties.fetch_snapshots.begin (null, (obj, res) => { launching this async from here could mean that we call other functions below before this one finishes fetching snapshots. Lets make this constructor async and yield in here instead. Although I'd do this after initializing the UI. @@ +61,3 @@ + progress_label.get_style_context ().add_class (Gtk.STYLE_CLASS_DIM_LABEL); + progress_box.pack_start (progress_label, false, false); + snapshot_stack.add (progress_box); with so much UI definition code, would be nice to define this all from UI file. Its fine for now but could you add this task to your TODO just after this bug? @@ +86,3 @@ + machine.window.sidebar.sensitive = false; + } + machine.properties.create_snapshot.begin ((obj, res) => { you are still using a lambda here. :( @@ +95,3 @@ + } catch (GLib.Error e) { + machine.window.notificationbar.display_error (_("Failed to create snapshot of %s") + .printf (machine.name)); we don't divide long lines like this. In this case lets use a temporary variable instead: var msg = _("...").printf (machine.name); machine.window.notificationbar.display_error (msg);
Review of attachment 283656 [details] [review]: ack
Review of attachment 283657 [details] [review]: ack
Review of attachment 283658 [details] [review]: still ack
Review of attachment 283659 [details] [review]: "Ignore disconnection during props" would be a more generic and appropriate shortlog.
Review of attachment 283660 [details] [review]: As I said on IRC yesterday, this would be very unexpected behaviour from users' POV. We should probably either disable 'back' button or show a spinner in display page instead.
Review of attachment 283662 [details] [review]: From log, I thought this adds a progressbar (or spinner) but seems it only adds a message. I think we should also show a spinner (maybe next to the message?) and also we should show message for other actions too (at least creation) even if they are typically not as slow operation as reverting. ::: src/snapshot-list-row.vala @@ +24,3 @@ private Boxes.LibvirtMachine machine; private Gtk.Container? parent_container = null; + private SnapshotsProperty property; Instead of giving this class a ref to property, I'd rather this class have a boolean property that our property listens to and show/hide 'reverting' message appropriately. @@ +145,3 @@ + } + property.show_progress_message (_("Reverting to %s...").printf (snapshot_name)); + machine.window.sidebar.sensitive = false; I don't think this change belongs here.
(In reply to comment #91) > Review of attachment 283662 [details] [review]: > > From log, I thought this adds a progressbar (or spinner) but seems it only adds > a message. I think we should also show a spinner (maybe next to the message?) > and also we should show message for other actions too (at least creation) even > if they are typically not as slow operation as reverting. show_progress_message will set the text of the progress_label and switch the GtkStack in the SnapshotsProperty to the "progress page" which has a GtkSpinner with a GtkLabel beneath it. Creating also shows the same progress page, see the SnapshotsProperty patch.
(In reply to comment #88) > Review of attachment 283658 [details] [review]: > > still ack You have never reviewed this before. This is about the snapshot being taken when the machine was stopped. Reverting to such a snapshot will cause the machine to go into STOPPED state, not just PAUSED.
(In reply to comment #90) > Review of attachment 283660 [details] [review]: > > As I said on IRC yesterday, this would be very unexpected behaviour from users' > POV. We should probably either disable 'back' button or show a spinner in > display page instead. So if the machine is STOPPED after reverting to a snapshot (and was RUNNING before), it should automatically be started?
Review of attachment 283654 [details] [review]: ::: src/snapshots-property.vala @@ +34,3 @@ + create_snapshot_button.icon_widget = icon_img; + + machine.properties.fetch_snapshots.begin (null, (obj, res) => { But async constructors can only be called with yield, so that would mean that add_snapshots_property in libvirt-machine-properties.vala has to be async so we would be asynchronously add an element to the GList of properties and I don't think that'll work out very well.
(In reply to comment #92) > (In reply to comment #91) > > Review of attachment 283662 [details] [review] [details]: > > > > From log, I thought this adds a progressbar (or spinner) but seems it only adds > > a message. I think we should also show a spinner (maybe next to the message?) > > and also we should show message for other actions too (at least creation) even > > if they are typically not as slow operation as reverting. > show_progress_message will set the text of the progress_label and switch the > GtkStack in the SnapshotsProperty to the "progress page" which has a GtkSpinner > with a GtkLabel beneath it. Creating also shows the same progress page, see the > SnapshotsProperty patch. ok cool, i think my objection was on the terms used. 'progress' -> 'activity' would make me feel better. :) (In reply to comment #93) > (In reply to comment #88) > > Review of attachment 283658 [details] [review] [details]: > > > > still ack > > You have never reviewed this before. This is about the snapshot being taken > when the machine was stopped. Reverting to such a snapshot will cause the > machine to go into STOPPED state, not just PAUSED. I think you got it confused or maybe the log message is very confusing. From the log it seems to be about handling the temporary PAUSED state when reverting and thats something I clearly recall reviewing. (In reply to comment #94) > (In reply to comment #90) > > Review of attachment 283660 [details] [review] [details]: > > > > As I said on IRC yesterday, this would be very unexpected behaviour from users' > > POV. We should probably either disable 'back' button or show a spinner in > > display page instead. > > So if the machine is STOPPED after reverting to a snapshot (and was RUNNING > before), it should automatically be started? I would think so, yes.
Review of attachment 283654 [details] [review]: ::: src/snapshots-property.vala @@ +34,3 @@ + create_snapshot_button.icon_widget = icon_img; + + machine.properties.fetch_snapshots.begin (null, (obj, res) => { well fetch_snapshots does something very similar (if not the same) doesn't it? What you can do here then is to make other methods (that assume this call to be successfuly done already) to do this on the fly. Check db loading code in os-database.vala.
> > (In reply to comment #93) > > (In reply to comment #88) > > > Review of attachment 283658 [details] [review] [details] [details]: > > > > > > still ack > > > > You have never reviewed this before. This is about the snapshot being taken > > when the machine was stopped. Reverting to such a snapshot will cause the > > machine to go into STOPPED state, not just PAUSED. > > I think you got it confused or maybe the log message is very confusing. From > the log it seems to be about handling the temporary PAUSED state when reverting > and thats something I clearly recall reviewing. WTF, yes looks like it, sorry.
Review of attachment 283654 [details] [review]: ::: src/snapshots-property.vala @@ +34,3 @@ + create_snapshot_button.icon_widget = icon_img; + + machine.properties.fetch_snapshots.begin (null, (obj, res) => { fetch_snapshots actually creates a completely new GList and doesn't work with the existing one. I also don't really understand why this matters here, nothing after the fetch_snapshots call here assumes it to be done?
Review of attachment 283654 [details] [review]: ::: src/snapshots-property.vala @@ +34,3 @@ + create_snapshot_button.icon_widget = icon_img; + + machine.properties.fetch_snapshots.begin (null, (obj, res) => { Ah, you only use the snapshots after you have fetched it. All good then. I think you also want to put this async call in a separate method. The lesser the nesting of code, the more readable it is.
Created attachment 283927 [details] [review] props toolbar: Make back button public To keep the user from going backwards to a possible unconnected display, we need to make the back button insensitive when longer running snapshot operations are going on.
Created attachment 283928 [details] [review] Add SnapshotListRow This will be used in the snapshots page of the VM properties and features a way to delete, rename or revert to the snapshot it represents.
Created attachment 283929 [details] [review] Add SnapshotsProperty The SnapshotsProperty provides a UI to view, create, delete, rename and revert to snapshots of the given LibvirtMachine.
Created attachment 283930 [details] [review] machine: Ignore disconnetion during props State changes to STOPPED are completely normal if the user reverts to a snapshot that was taken when the machine was stopped, so don't show any error message in this case and don't switch to the collections view.
Review of attachment 283927 [details] [review]: Kudos of putting a lot of details but its missing the most important detail: how the change is needed by what you described. I'll just write something like: This change is needed by a following patch where we need to access back button from CLASS_NAME. ::: data/ui/properties-toolbar.ui @@ +11,2 @@ <child> + <object class="GtkButton" id="back_button"> this change is not covered by commit log. It should probably be a separate patch anyway. ::: src/topbar.vala @@ +17,3 @@ public UIState ui_state { get; protected set; } + [GtkChild] + public PropertiesToolbar props_toolbar; same for changes here.
Review of attachment 283928 [details] [review]: Good apart from some nitpicks. ::: data/gtk-style.css @@ +23,3 @@ + +.boxes-snapshot-list-row.indicator.active { + background-color: @theme_fg_color; Looking at the screenshot you showed today, these might need adjusting but we can do that later. ::: src/snapshot-list-row.vala @@ +134,3 @@ + + private void revert_to_activated (GLib.SimpleAction action, GLib.Variant? v) { + I'll put this empty line below following 2 declarations. @@ +136,3 @@ + + string snapshot_name; + GVirConfig.DomainSnapshotDomainState snapshot_state; I'll do: var snapshot_name = snapshot.get_name (); var snapshot_state = GVirConfig.DomainSnapshotDomainState.NOSTATE; saves two lines from catch below. :) @@ +173,3 @@ + + private void delete_activated (GLib.SimpleAction action, GLib.Variant? v) { + string snapshot_identifier; same for this one. Use var and initialize it already to fallback value.
Review of attachment 283929 [details] [review]: Good apart from one minor nitpick. ::: src/snapshots-property.vala @@ +84,3 @@ + + private async void create_snapshot () { + var show_progress = (machine.state == Machine.MachineState.RUNNING); one more progress -> activity to go :)
Review of attachment 283930 [details] [review]: typo: disconnetion -> disconnection. Good otherwise.
Review of attachment 283928 [details] [review]: ::: data/gtk-style.css @@ +23,3 @@ + +.boxes-snapshot-list-row.indicator.active { +.boxes-snapshot-list-row.indicator { This is just the indicator scaling (i.e. the rect/line stuff on the left). The problem from the screenshots is more that each BoxesPropertyWidget gets a .content-view applied which has @content_view_bg as a background, which is #292929 and a GtkListBox also gets #292929 so this is either a bug in Adwaita or we need to work around it in some other way. But the indicator styling is not the problem.
Review of attachment 283928 [details] [review]: ::: data/gtk-style.css @@ +23,3 @@ + +.boxes-snapshot-list-row.indicator.active { +.boxes-snapshot-list-row.indicator { Correction: The CSS here is still OK, but the GtkListView problem should be handled by packing it inside a GtkFrame.
Created attachment 284110 [details] [review] Add SnapshotListRow This will be used in the snapshots page of the VM properties and features a way to delete, rename or revert to the snapshot it represents.
Created attachment 284111 [details] [review] PropertiesToolbar: Make the back button public To do that, rename it from 'back' to 'back_button' to make the name more desriptive. This will be needed in a later patch to access the PropertiesToolbar's back_button from within SnapshotsProperty.
Created attachment 284112 [details] [review] Topbar: Make props_toolbar public This will be used in a later patch to access the props_toolbar's back button from SnapshotsProperty.
Created attachment 284113 [details] [review] Add SnapshotsProperty The SnapshotsProperty provides a UI to view, create, delete, rename and revert to snapshots of the given LibvirtMachine.
Created attachment 284114 [details] [review] machine: Don't switch away from properties on display disconnection If the user reverts to a snapshots and the domain is running, it will temporarily be paused, causing its display to disconnect. We normally switch to the collections view, but let's not do it if the user is currently in the properties view since the state change is just normal when reverting to a snapshot.
Review of attachment 284110 [details] [review]: ack ::: src/snapshot-list-row.vala @@ +170,3 @@ + + private void delete_activated (GLib.SimpleAction action, GLib.Variant? v) { + string snapshot_identifier; forgot this one but no biggie at all.
Review of attachment 284111 [details] [review]: ok :)
Review of attachment 284112 [details] [review]: ack
Review of attachment 284113 [details] [review]: Good without this change too. ::: src/snapshots-property.vala @@ +86,3 @@ + + private async void create_snapshot () { + var indicate_activity = (machine.state == Machine.MachineState.RUNNING); Just thought of it now. Instead of this local variable and show/hide methods below, why not simply use a boolean prop that shows activity when set and hides when unset?
Review of attachment 284114 [details] [review]: still ack
Review of attachment 284113 [details] [review]: ::: src/snapshots-property.vala @@ +86,3 @@ + + private async void create_snapshot () { + var indicate_activity = (machine.state == Machine.MachineState.RUNNING); You mean a boolean property together with a set_activity_label function? Or how would I set the activity_label's text?
Review of attachment 284113 [details] [review]: ::: src/snapshots-property.vala @@ +86,3 @@ + + private async void create_snapshot () { + var indicate_activity = (machine.state == Machine.MachineState.RUNNING);
Review of attachment 284113 [details] [review]: ::: src/snapshots-property.vala @@ +86,3 @@ + + private async void create_snapshot () { + var indicate_activity = (machine.state == Machine.MachineState.RUNNING); That would work I guess. Should I change that or is it too late now? :)
Review of attachment 284113 [details] [review]: ::: src/snapshots-property.vala @@ +86,3 @@ + + private async void create_snapshot () { + var indicate_activity = (machine.state == Machine.MachineState.RUNNING); no, its not late at all.
Created attachment 284126 [details] [review] properties: Add snapshots page This will later be used to show the snapshots of a VM.
Created attachment 284127 [details] [review] libvirt-machine: Delete snapshots with machine libvirt does not allow to delete/undefine domains that have snapshot metadata attached. Sine creating snapshots will be possible, we need to pass GVir.DomainDeleteFlags.SNAPSHOTS_METADATA to the delete() call of the domain, so the user can even delete domains with snapshots.
Created attachment 284128 [details] [review] Add SnapshotListRow This will be used in the snapshots page of the VM properties and features a way to delete, rename or revert to the snapshot it represents.
Created attachment 284129 [details] [review] properties: Remove margins of property widgets They were only added on the left of each configuration widget which makes horizontally expanding widgets look unbalanced.
Created attachment 284130 [details] [review] properties: Use a GtkScrolledWindow for the pages This is needed for property pages that contain either very high widgets or a dynamic number of widgets, e.g. the coming snapshots page.
Created attachment 284131 [details] [review] LibvirtMachineProperties: Add snapshot API Add API to fetch, get and create snapshots which will be used in later commits.
Created attachment 284132 [details] [review] LibvirtMachine: Make the 'properties' field public Creating, fetching, etc. of snapshots is now part of the LibvirtMachineProperties and to be able to create snapshots from outside the machine, we need access to its properties.
Created attachment 284133 [details] [review] PropertiesToolbar: Make the back button public To do that, rename it from 'back' to 'back_button' to make the name more desriptive. This will be needed in a later patch to access the PropertiesToolbar's back_button from within SnapshotsProperty.
Created attachment 284134 [details] [review] Topbar: Make props_toolbar public This will be used in a later patch to access the props_toolbar's back button from SnapshotsProperty.
Created attachment 284135 [details] [review] Add SnapshotsProperty The SnapshotsProperty provides a UI to view, create, delete, rename and revert to snapshots of the given LibvirtMachine.
Created attachment 284136 [details] [review] properties: Add the snapshots page The snapshot page lists all the snapshot available for the domain and the user is able to delete, rename or revert to them.
Created attachment 284137 [details] [review] DisplayPage: Way to switch display widget w/o showing it If the user is in the properties page and the machine's display reconnects, we need to replace the display page's widget with the new display without showing it.
Created attachment 284138 [details] [review] machine: Don't switch away from properties on display disconnection If the user reverts to a snapshots and the domain is running, it will temporarily be paused, causing its display to disconnect. We normally switch to the collections view, but let's not do it if the user is currently in the properties view since the state change is just normal when reverting to a snapshot.
Created attachment 284139 [details] [review] machine: Ignore disconnection during props State changes to STOPPED are completely normal if the user reverts to a snapshot that was taken when the machine was stopped, so don't show any error message in this case and don't switch to the collections view.
Hi, I tested the patches a bit. Great work by the way! There are some issues with some recent additions you might not have tested: Topbar: you dont want the user to change any setting so you disable the settings page switcher. However there's a new control element in the topbar where you can change the VMs title. Maybe you should take care of that too if thats relevant. The spinner withe the "Creating snapshot" text is not centered vertically for me. I think it would make sense to do that. There are some other design decisions I'm still doubting but I'll probably file bugs for them. I think its worth to look at the new HIG, section Patterns, subsection Lists. (https://people.gnome.org/~tobiasmue/hig3/lists.html) You know I want the cake and eat it :) Apart from the general design things: when I revert to a snapshot I sometimes get the spinner and after 10 minutes of waiting I kill Boxes because I'm inpatient. I may note that I'm using a live VM because I dont have any installed right now for testing. This seems a bit nondeterministic to me but I had this problem several times. Dont have time for more testing right now. When I revert to a version and then snapshot again the snapshot gets appended to the end of the linear tree. So it gets the wrong parent. I hope this helps avoiding future bugs.
(In reply to comment #139) > Hi, > > I tested the patches a bit. Great work by the way! Cool, thanks for testing! Good that you found some issues. Fortunately most of them are not anything that should block the merging of this work. Perhaps we should file a separate bugs for each of the issues you found after this is merged?
I'd not ship this yet. "when I revert to a snapshot I sometimes get the spinner and after 10 minutes of waiting" And I think some more testing would be useful too. I still plan to do that but I cant guarantee its in time. Zeeshan: you told me on chat you had tested and some issues? Whats your result?
Attachment 284126 [details] pushed as 09919e8 - properties: Add snapshots page Attachment 284127 [details] pushed as 6aef7bb - libvirt-machine: Delete snapshots with machine Attachment 284128 [details] pushed as bfb4c17 - Add SnapshotListRow Attachment 284129 [details] pushed as 2e8245d - properties: Remove margins of property widgets Attachment 284130 [details] pushed as 3c9ff8c - properties: Use a GtkScrolledWindow for the pages Attachment 284131 [details] pushed as 8686cd2 - LibvirtMachineProperties: Add snapshot API Attachment 284132 [details] pushed as a09fb70 - LibvirtMachine: Make the 'properties' field public Attachment 284133 [details] pushed as 4704021 - PropertiesToolbar: Make the back button public Attachment 284134 [details] pushed as 1f34c1d - Topbar: Make props_toolbar public Attachment 284135 [details] pushed as ec406d1 - Add SnapshotsProperty Attachment 284136 [details] pushed as 1e1030b - properties: Add the snapshots page Attachment 284137 [details] pushed as 662a0a9 - DisplayPage: Way to switch display widget w/o showing it Attachment 284138 [details] pushed as cb1a3cc - machine: Don't switch away from properties on display disconnection Attachment 284139 [details] pushed as 7c91133 - machine: Ignore disconnection during props