GNOME Bugzilla – Bug 700780
Allow undoing automatic deletion of VMs
Last modified: 2016-03-31 13:22:07 UTC
We automatically delete a (live) VM when it shutsdown w/o writing anything to the disk. Since its at least theoretically possible for our detection code to be wrong about nothing being installed on the disk or some regression(s) can cause this to trigger and deletion of VM could easily mean loss of important user data, I think it would be better if we display a notification for automatically deleted VMs as well that allows user to 'undo' the deletion.
Created attachment 270511 [details] [review] Adds undo capability for automatically deleted boxes. If there is anything wrong with this patch, write it and I will correct it. I created it via git format-patch.
(In reply to comment #1) > Created an attachment (id=270511) [details] [review] > Adds undo capability for automatically deleted boxes. > > If there is anything wrong with this patch, write it and I will correct it. Yes, that is the very usual workflow. If you notice, there is a separate 'review' link to make inline comments to your patch. > I > created it via git format-patch. That is good but for making things easier for yourself, do give git-bz a try: http://git.fishsoup.net/cgit/git-bz/
Review of attachment 270511 [details] [review]: First, its a pretty good patch considering that you are not familiar with Vala and its your first contribution. ::: src/app.vala @@ +334,3 @@ + public void delete_machine_undoable (Machine machine, bool by_user = true) { + var message = _("Box '%s' has been deleted").printf (machine.name); * I think we need a more specific message here: "Live box '%s' deleted automatically." * I don't think this function belongs here, it should be in VMCreator class, not just because its the only user of this function but also because the message is very specific to its usecase. @@ +343,3 @@ + + Notification.CancelFunc really_remove = () => { + debug ("Box deletion, deleting now"); The is a C&P of a bad English from a french friend. :) Lets write: "User didn't cancel deletion. Deleting now.."
I am currently thinking about merging the delete_machine_undoable with remove_selected items and an additional optional message parameter (defaulting to "Box '%s' has been deleted." or "Boxes ..."). This could be used by more than one function and it would be cleaner for possible future uses of this more general undoable deletion. As for the message text itself - I plan to take a look at these language files so that we can get a translated message there. Wouldn't this be better anyway? I'll change the debug message.
(In reply to comment #4) > I am currently thinking about merging the delete_machine_undoable with > remove_selected items and an additional optional message parameter (defaulting > to "Box '%s' has been deleted." or "Boxes ..."). This could be used by more > than one function and it would be cleaner for possible future uses of this more > general undoable deletion. Yes, that sounds better indeed but we don't want a default in here as there is only two callers and they both have different needs so defaults doesn't make sense in this case. > As for the message text itself - I plan to take a look at these language files > so that we can get a translated message there. Wouldn't this be better anyway? Yes, at least all strings that are shown to user in UI are supposed to be translated but you don't need to look into language files. Just mark it for translation using _() function. As you can see the existing messages are already marked for translation and the plural form needs to be dealt a bit differently using ngettext. > I'll change the debug message. Cool. BTW, next time you reply to inline review comments, please make use of the 'review' link to do that instead. Not mandatory though so its ok if you forget, I'll just keep reminding you. :)
Created attachment 270575 [details] [review] Review improvements and removed auto-deletion on every startup. Since one caller has a List<CollectionItem> and the other just a machine and overloading is not possible in vala I decided to put the delete_machine_undoable to libvirt-machine as you suggested. I also found a bug in the old fix: When undoing the deletion of a live machine and restarting gnome-boxes the live machine will be auto-deleted on startup of the application. (And the user has to undo it again.) I corrected this with this patch so a user can have a live vm if he wants it. (And has to undo the deletion only once.) (I'll try out git bz as soon I get rid of these python issues with it. Thanks for the suggestion.)
(In reply to comment #6) > Created an attachment (id=270575) [details] [review] > Review improvements and removed auto-deletion on every startup. > > Since one caller has a List<CollectionItem> and the other just a machine You can create a List<CollectionItem> and add the only machine (which is fine since a machine is also a CollectionItem) into it. > and > overloading is not possible in vala Overloading is possible in vala but you can't change the signature of the method. > I decided to put the > delete_machine_undoable to libvirt-machine as you suggested. I don't think we want that. I think your previous suggestion was better and it can be implemented (see above). > I also found a bug in the old fix: When undoing the deletion of a live machine > and restarting gnome-boxes the live machine will be auto-deleted on startup of > the application. (And the user has to undo it again.) I corrected this with > this patch so a user can have a live vm if he wants it. (And has to undo the > deletion only once.) Ah good but that is not a bug in existing code since there is no 'undo' of automatic deletion. :) > (I'll try out git bz as soon I get rid of these python issues with it. Thanks > for the suggestion.) Sure. :)
Created attachment 270588 [details] [review] Added undo possibility for automatically deleted live boxes. Here is the new version. I just dont like this solution that much from a performance standpoint. I do not like allocating a whole list instead of passing a machine much. But I agree that this is a practical OOP solution. Just to clarify: I really meant the bug is in my fix, not in your existent code.
(In reply to comment #8) > Created an attachment (id=270588) [details] [review] > Added undo possibility for automatically deleted live boxes. > > Here is the new version. > > I just dont like this solution that much from a performance standpoint. I do > not like allocating a whole list instead of passing a machine much. You are assuming that the allocation is expensive, which its not. Even if it was, its not an issue here since we are not calling this function often enough. > But I agree > that this is a practical OOP solution. Exactly. > Just to clarify: I really meant the bug is in my fix, not in your existent > code. Ah ok. :)
Review of attachment 270588 [details] [review]: I must admit this task turned out to be not as trivial as I thought but that always happens. :) Just don't think you are not doing good because you are. Appart fro the inline comments, some comments on commit log: * you don't want the commit log to say which version of the patch it is (you can say that in the comment section when submitting the patch) * Try you best to follow these guidelines: https://wiki.gnome.org/Git/CommitMessages * commit log shouldn't talk of differences between the patch and its previous versions (e.g the last line does that). * Our preferred way of bug references is a URL as the last line (with an empty line above it), which is the default behavior of git-bz. ::: src/app.vala @@ +482,3 @@ } + //private List<CollectionItem> delete_storage; Remove this redundant line please. There is others below too, same goes for those. @@ +483,3 @@ + //private List<CollectionItem> delete_storage; + public void delete_machine_undoable (owned List<CollectionItem> machines, string message) { This function is dealing with items (even though currently an itme can only be a machine) so we don't want 'machine' in the name of function or parameter name. Also the function removes multiple machines so you want to use plural. @@ +491,3 @@ + debug ("Box deletion cancelled by user. Re-adding to view"); + foreach (var machine in machines) { + var mach = machine as Machine; if 'machine' is named item (as it should be), you won't have to give a weird name like 'mach' here. :) ::: src/libvirt-machine.vala @@ +12,3 @@ + * automatically on every application start. + */ + public bool just_created { get; set; default = false; } This will not be around if user quits Boxes (i-e the value is not persistent). What you gotta do is to make the machine as installed in the libvirt configuration so vm-creator doesn't touch it anymore. What you need to do from vm-creator is to see if machine deletion was undone by user and if so, call VMConfigurator.post_install_setup on it. You'll have to modify your new machine removal function in App. It should take optional function argument, that if passed will notify caller of what happened in the end. Just a function with a boolean argument would do the trick. ::: src/vm-creator.vala @@ +164,3 @@ machine.disconnect (state_changed_id); install_media.clean_up (); + // If the user presses undo do not delete it automatically anymore I don't think this comment fits in here.
(In reply to comment #10) > Review of attachment 270588 [details] [review]: > > I must admit this task turned out to be not as trivial as I thought but that > always happens. :) Just don't think you are not doing good because you are. Thanks. Where would be the fun if there is no challange? I learned a lot about vala during this process. (This language just gets better.) > Appart fro the inline comments, some comments on commit log: > > * you don't want the commit log to say which version of the patch it is (you > can say that in the comment section when submitting the patch) > * Try you best to follow these guidelines: > https://wiki.gnome.org/Git/CommitMessages > * commit log shouldn't talk of differences between the patch and its previous > versions (e.g the last line does that). > * Our preferred way of bug references is a URL as the last line (with an empty > line above it), which is the default behavior of git-bz. Thanks. I'll reread the page. I resolved the python issues and try out git bz for the next patch. > ::: src/app.vala > @@ +482,3 @@ > } > > + //private List<CollectionItem> delete_storage; > > Remove this redundant line please. There is others below too, same goes for > those. I'm sorry. I missed this during my just-bofore-upload-review. I have to get more accurate. > @@ +483,3 @@ > > + //private List<CollectionItem> delete_storage; > + public void delete_machine_undoable (owned List<CollectionItem> machines, > string message) { > > This function is dealing with items (even though currently an itme can only be > a machine) so we don't want 'machine' in the name of function or parameter > name. Also the function removes multiple machines so you want to use plural. Done in my local version. This comes with the next patch. > @@ +491,3 @@ > + debug ("Box deletion cancelled by user. Re-adding to view"); > + foreach (var machine in machines) { > + var mach = machine as Machine; > > if 'machine' is named item (as it should be), you won't have to give a weird > name like 'mach' here. :) Also done locally. > ::: src/libvirt-machine.vala > @@ +12,3 @@ > + * automatically on every application start. > + */ > + public bool just_created { get; set; default = false; } > > This will not be around if user quits Boxes (i-e the value is not persistent). > What you gotta do is to make the machine as installed in the libvirt > configuration so vm-creator doesn't touch it anymore. What you need to do from > vm-creator is to see if machine deletion was undone by user and if so, call > VMConfigurator.post_install_setup on it. > > You'll have to modify your new machine removal function in App. It should take > optional function argument, that if passed will notify caller of what happened > in the end. Just a function with a boolean argument would do the trick. Currently I do not know how I can notify a caller in vala. (And I didn't came up with the right queries for the search engine.) I would create a public delegate wich can be passed optionally and is called by undo. > ::: src/vm-creator.vala > @@ +164,3 @@ > machine.disconnect (state_changed_id); > install_media.clean_up (); > + // If the user presses undo do not delete it automatically > anymore > > I don't think this comment fits in here. This vanishes with the provious paragraphs implicitly anyway.
(In reply to comment #11) > (In reply to comment #10) > > Review of attachment 270588 [details] [review] [details]: > > > > I must admit this task turned out to be not as trivial as I thought but that > > always happens. :) Just don't think you are not doing good because you are. > Thanks. Where would be the fun if there is no challange? I learned a lot about > vala during this process. (This language just gets better.) Cool. Glad to see you have the right attitude. > > ::: src/app.vala > > @@ +482,3 @@ > > } > > > > + //private List<CollectionItem> delete_storage; > > > > Remove this redundant line please. There is others below too, same goes for > > those. > I'm sorry. I missed this during my just-bofore-upload-review. I have to get > more accurate. No worries, we all make mistakes, especially when new to a project. > > ::: src/libvirt-machine.vala > > @@ +12,3 @@ > > + * automatically on every application start. > > + */ > > + public bool just_created { get; set; default = false; } > > > > This will not be around if user quits Boxes (i-e the value is not persistent). > > What you gotta do is to make the machine as installed in the libvirt > > configuration so vm-creator doesn't touch it anymore. What you need to do from > > vm-creator is to see if machine deletion was undone by user and if so, call > > VMConfigurator.post_install_setup on it. > > > > You'll have to modify your new machine removal function in App. It should take > > optional function argument, that if passed will notify caller of what happened > > in the end. Just a function with a boolean argument would do the trick. > Currently I do not know how I can notify a caller in vala. (And I didn't came > up with the right queries for the search engine.) > > I would create a public delegate wich can be passed optionally and is called by > undo. Sounds good but please do check if there isn't already a delegate defined by GLib or Gio that we can use here.
Created attachment 271003 [details] [review] vm-creator: undo option for live vm autodeletion If a live VM shuts down without writing anything to the disk, it is deleted automatically. This changeset allows the user to undo this.
Review of attachment 271003 [details] [review]: ::: src/app.vala @@ +32,3 @@ public signal void item_selected (CollectionItem item); + public delegate void undo_callback (owned List<CollectionItem> machines); * This is a type so you use camelcase name: UndoCallback. Actually I'd suggest the name 'UndoNotifyCallback' so its clear that it only notifies you of an undo operation rather than expect you to do the undo. * The machines argument is very much redundant here as the caller of delete_machines_undoable already has access to the machines as its the one that passed it in the first place. @@ +485,3 @@ + /** + * Deletes machines and allows the user to undo this (all at once). I think "(all at once)" part is only confusing here but no issue. @@ +491,3 @@ + * @param callback: a function for further undo / do actions + */ + public void delete_machines_undoable (owned List<CollectionItem> machines, As pointed out in the last review, machines => items. @@ +493,3 @@ + public void delete_machines_undoable (owned List<CollectionItem> machines, + string message, + undo_callback? tidy_up = null) { * tidy_up isn't what the callback is about so very misleading. Just call it undo_notify_callback. * I think this needs to be declare as 'owned' just like Notificationbar.display_for_action's callback arguments. @@ +505,3 @@ + // pass back the ownership + if (tidy_up != null) + tidy_up ((owned) machines); you don't need to pass back the ownership here. You probably want to make the callback argument owned though. @@ +536,1 @@ + delete_machines_undoable ((owned) selected_items, message); No need for (owned) here. ::: src/vm-creator.vala @@ +167,3 @@ + Boxes.App.undo_callback tidy_up = (machines) => { + debug ("Live box deletion cancelled. Invoking post installation setup..."); + foreach (var item in machines) { you don't need to iterate on the list or access the returned list. We know that we are only passing one machine and you have access to that machine from the lambda (lambdas can access the local context). @@ +173,3 @@ + }; + + App.app.delete_machines_undoable ((owned) machines, _("Live box '%s' has been deleted " (owned) should not be needed here but only for the callback. @@ +174,3 @@ + + App.app.delete_machines_undoable ((owned) machines, _("Live box '%s' has been deleted " + + "automatically.").printf (machine.name), We want the string to be all together in the translation files so you can't divide this translated string like this. Just do: var msg = _("Live box '%s' has been deleted automatically.").printf (machine.name); App.app.delete_machines_undoable (machines, msg, (owned) undo_callback);
Review of attachment 271003 [details] [review]: ::: src/app.vala @@ +32,3 @@ public signal void item_selected (CollectionItem item); + public delegate void undo_callback (owned List<CollectionItem> machines); * I wanted to think about the name anyway. I'll use yours. * And here comes the ownership problem. If I don't pass the ownership to the function, weird things happen: (gnome-boxes:14395): Boxes-DEBUG: app.vala:510: User did not cancel deletion. Deleting now... [1] 14395 segmentation fault (core dumped) G_MESSAGES_DEBUG=Boxes jhbuild run gnome-boxes or (gnome-boxes:14704): Boxes-DEBUG: app.vala:500: Box deletion cancelled by user. Re-adding to view [1] 14704 segmentation fault (core dumped) G_MESSAGES_DEBUG=Boxes jhbuild run gnome-boxes If I use ownership, everything works smoothly. (And I have to pass it back if the vm is not deleted as the code in this patch does.) This also affects the manual deletion of any vm. This behaviour matches my understanding of valas ownership before my short discussion with you. (Now it's a bit chaotic. - The understanding.) @@ +485,3 @@ + /** + * Deletes machines and allows the user to undo this (all at once). I removed this. @@ +491,3 @@ + * @param callback: a function for further undo / do actions + */ + public void delete_machines_undoable (owned List<CollectionItem> machines, I thought you just meant the iterators, not the list. I will change it. ::: src/vm-creator.vala @@ +167,3 @@ + Boxes.App.undo_callback tidy_up = (machines) => { + debug ("Live box deletion cancelled. Invoking post installation setup..."); + foreach (var item in machines) { As for the local context, this depends on the ownership question. (See my comment on yours in app.vala.) I have a version that uses the object in the local scope but it fails: (gnome-boxes:17344): Boxes-DEBUG: vm-creator.vala:169: Live box deletion cancelled. Invoking post installation setup... [1] 17344 segmentation fault (core dumped) G_MESSAGES_DEBUG=Boxes jhbuild run gnome-boxes (I have checked for null pointing before.) Note: in this version the ownership of the object is transferred to delete_machines_undoable, because the code never reaches this function in the current version if I do not do this. As for iterating the list: I wanted this to be as general as possible. Code lives. @@ +174,3 @@ + + App.app.delete_machines_undoable ((owned) machines, _("Live box '%s' has been deleted " + + "automatically.").printf (machine.name), Good to know. Thanks, I'll correct this too.
Review of attachment 271003 [details] [review]: ::: src/app.vala @@ +32,3 @@ public signal void item_selected (CollectionItem item); + public delegate void undo_callback (owned List<CollectionItem> machines); * Did you try with owned passing of the callbacks as I suggested instead? * That is crash and for crashes, the important bit is the backtrace. ::: src/vm-creator.vala @@ +167,3 @@ + Boxes.App.undo_callback tidy_up = (machines) => { + debug ("Live box deletion cancelled. Invoking post installation setup..."); + foreach (var item in machines) { I think making the callback arguments owned would help here.
Created attachment 271016 [details] [review] vm-creator: undo option for live vm autodeletion If a live VM shuts down without writing anything to the disk, it is deleted automatically. This changeset allows the user to undo this.
Review of attachment 271016 [details] [review]: Please do not review this patch. It is just to show what I have done experimentally. I tried your suggestion. It seems to work if the caller provides a callback. If there is no callback (manual deletion) the program crashes. Here is a backtrace: http://pastebin.com/9avG07pa
Created attachment 271075 [details] [review] vm-creator: undo option for live vm autodeletion If a live VM shuts down without writing anything to the disk, it is deleted automatically. This changeset allows the user to undo this. I hope I didnt forget anything. This patch is growing quite fast. * In my test cases it worked every time. * I pass back the object because the reference at least in this case doesnt exist anymore in the local context of the lambda. Passing the lambda owned does not work. * I pass back the ownership via the callback in case the callback does something asynchronously and needs a counted reference. (I know that this is not the case here, but this is more general and has no drawbacks as I see this.) * I named the parameter of undo_notify_callback "machines" and not "items" since this name is used in the local context. If you want this to be "items" I would suggest "local_items" so that the scope difference is clear.
Review of attachment 271075 [details] [review]: Almost there. ::: src/app.vala @@ +36,3 @@ + * + * @attention the ownership for items is required since GLib.List is a compact class and there is no reference + * counting for this class. Passing it back allows the caller to make further use of the reference asynchronously. Good work with adding this comment here but I think the last senetence is redundant as this delegate at least does no 'passing back'. @@ +495,3 @@ + * @param items the list of machines + * @param message The message to be shown together with the undo button + * @param callback a function for further undo actions I would say: "optional function that, if provided is called after cancellation of deletion on user action." ::: src/vm-creator.vala @@ +167,3 @@ + Boxes.App.UndoNotifyCallback undo_notify_callback = (machines) => { + debug ("Live box deletion cancelled. Invoking post installation setup..."); + var item = machines.first().data as LibvirtMachine; you already have the 'machine' from the enclosing scope so this is redundant.
Review of attachment 271075 [details] [review]: You commented about my valadoc compatible commenting style above functions. Since this applies to both patches, I'll comment it just here. * I see no drawback in using valadoc conform comments. * Since this style is widely used for documentation I find this rather clear and nice. When I read code I recognize these styles and use these comments to get a rought overview over the program. * Some IDEs even parse these documentation formatted headers and show it with suggestions or when reading the code on mouseover. No one knows when this comes for vala, but I think it will. * If the project grows and one decides to generate a documentation you have them there. ::: src/app.vala @@ +36,3 @@ + * + * @attention the ownership for items is required since GLib.List is a compact class and there is no reference + * counting for this class. Passing it back allows the caller to make further use of the reference asynchronously. As I understand the situation: the _ownership_ is passed to delete_machines_undoable and then passed "back" to the UndoNotifyCallback function. @@ +495,3 @@ + * @param items the list of machines + * @param message The message to be shown together with the undo button + * @param callback a function for further undo actions No objections. ::: src/vm-creator.vala @@ +167,3 @@ + Boxes.App.UndoNotifyCallback undo_notify_callback = (machines) => { + debug ("Live box deletion cancelled. Invoking post installation setup..."); + var item = machines.first().data as LibvirtMachine; As I understand this, when leaving the scope the reference to machine from the scope is deleted. Therefore it is not available anymore when the undo_notify_callback is called. (This is the reason, why I passed back the list and the local one cannot be used.) In fact I tested using the local reference and got some of these assertion failures __tmp#__ != NULL (that is the failed assertion, not a statement thats true). So there is probably a NULL reference if I try to access the machine from the scope around.
Review of attachment 271075 [details] [review]: ::: src/app.vala @@ +36,3 @@ + * + * @attention the ownership for items is required since GLib.List is a compact class and there is no reference + * counting for this class. Passing it back allows the caller to make further use of the reference asynchronously. But from the POV of this delegate, there is no 'passing back', you are only transfering ownership. Also as I explained in great detail, the 'items' callback is useless here. ::: src/vm-creator.vala @@ +167,3 @@ + Boxes.App.UndoNotifyCallback undo_notify_callback = (machines) => { + debug ("Live box deletion cancelled. Invoking post installation setup..."); + var item = machines.first().data as LibvirtMachine; The scope might be deleted but vala keeps the needed references around. You might need to pass the ownership of the callback to delet_machines though.
Created attachment 271243 [details] [review] vm-creator: undo option for live vm autodeletion I left the items parameter for the delegate even if its not needed in this case. Maybe somewhen someone will somehow need it - and then its there.
(In reply to comment #23) > Created an attachment (id=271243) [details] [review] > vm-creator: undo option for live vm autodeletion > > I left the items parameter for the delegate even if its not needed in this > case. Maybe somewhen someone will > somehow need it - and then its there. When its needed, i can be very easily added. Since its very unlikely that it will be needed, lets not have it.
Created attachment 271249 [details] [review] vm-creator: undo option for live vm autodeletion If a live VM shuts down without writing anything to the disk, it is deleted automatically. This changeset allows the user to undo this.
Review of attachment 271249 [details] [review]: Until the exception is provided for string break, I'll not merge this yet. ::: src/vm-creator.vala @@ +167,3 @@ + Boxes.App.UndoNotifyCallback undo_notify_callback = () => { + debug ("Live box deletion cancelled. Invoking post installation setup..."); + set_post_install_config(machine); space before '(' but its not an issue. I'll just correct it when pushing. @@ +170,3 @@ + }; + + var msg = _("Live box '%s' has been deleted automatically.").printf (machine.name); I still haven't got the OK for string freeze break but I'm hoping i can get it for a single string. :)
Pushed the patch with some minor changes.