Created attachment 217337 [details] [review]
Unify deletion of machines a bit

This fixes the regression of ghost item in collection view when VM
creation is canceled at the 'review' page.

Ah, is that somehow a regression from 674209?

(In reply to comment #2)
> Ah, is that somehow a regression from 674209?

Yeah, I think that combined with libvirt event issue cause I did test cancellation against those patches and there was no ghost VMs. Its also possible I did address this issue and then lost the fix in one of the several rebases..

Review of attachment 217337 [details] [review]:

::: src/vm-creator.vala
@@ +124,2 @@
                 domain.disconnect (stopped_id);
+                App.app.delete_machine (machine);

no need for volume.delete() anymore?

(In reply to comment #4)
> Review of attachment 217337 [details] [review]:
> 
> ::: src/vm-creator.vala
> @@ +124,2 @@
>                  domain.disconnect (stopped_id);
> +                App.app.delete_machine (machine);
> 
> no need for volume.delete() anymore?

app.delete_machine() calls machine.delete and that does volume.delete().

(In reply to comment #5)

> app.delete_machine() calls machine.delete and that does volume.delete().

I fail to see how.. :(

(In reply to comment #6)
> (In reply to comment #5)
> 
> > app.delete_machine() calls machine.delete and that does volume.delete().
> 
> I fail to see how.. :(

In this page:

src/app.vala:195
   public void delete_machine (Machine machine, bool by_user = true) {
        machine.delete (by_user);
        collection.remove_item (machine);
    }

And then:

http://git.gnome.org/browse/gnome-boxes/tree/src/libvirt-machine.vala#n377

Admitted that this 'by-user' is a bit confusing. Its more like 'we are doing this ourselves and not reacting to a domain deletion from another app'. Suggestion for better name?

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > 
> > > app.delete_machine() calls machine.delete and that does volume.delete().
> > 
> > I fail to see how.. :(
> 
> In this page:

s/page/patch/ :)

(In reply to comment #7)
> 
> http://git.gnome.org/browse/gnome-boxes/tree/src/libvirt-machine.vala#n377

ah correct, it would be worth a comment perhaps :)

thanks

Review of attachment 217337 [details] [review]:

ack

Attachment 217337 [details] pushed as e0cfff8 - Unify deletion of machines a bit