Created attachment 208109 [details] [review]
Add some debug messages related to deletion

Created attachment 208110 [details] [review]
Remove some warnings when deleting machine

Remove two kinds of warnings when a Box is deleted:
- disconnect machine ui-state handler to not stop screenshot from
  happening
- do not attempt to undefine and remove storage in vm-creator if
  already done

The patch also adds some debugging.

Created attachment 208111 [details] [review]
First start experience improvement

Currently, when the user starts Boxes for the first time, he ends up
in an empty collection without message on what to do. Instead,
starting with the Wizard is slightly better. Proposed patch attached.

Note: there is no yet final decision from the design team on what
should be done, but only one line of the proposed patch is actually
implementing the first-time experience policy, and thus could easily
be changed if needed.

Created attachment 208112 [details] [review]
Do not mix draw_id and cursor_id

This avoid warnings if disconnecting wrong handler in remove_display()

Created attachment 208113 [details] [review]
Fix a connection race and code silliness

I noticed a bug when a VM is started, it may receive two connections
very quickly, but since channel events are asynchrnous, we would
disconnect and hide the display, even if the second connection
succeeded.

The current code is silly, trying to connect automatically when needed
when the _connect_display member is set.

Get rid of that, and make sure we try to connect only once. Do not
connect/disconnect/reconnect etc..

Review of attachment 208109 [details] [review]:

ACK otherwise!

::: src/app.vala
@@ -482,2 +482,2 @@
         Notificationbar.OKFunc undo = () => {
-            foreach (var selected in selected_items)
+            debug ("delete undo");

"Box deletion cancelled by user, re-adding to view.."

@@ -487,1 +489,2 @@
         Notificationbar.CancelFunc really_remove = () => {
+            debug ("delete really");

"Box deletion not cancelled by user, actually deleting it now.."

Review of attachment 208109 [details] [review]:

ACK otherwise!

::: src/app.vala
@@ -482,2 +482,2 @@
         Notificationbar.OKFunc undo = () => {
-            foreach (var selected in selected_items)
+            debug ("delete undo");

"Box deletion cancelled by user, re-adding to view.."

@@ -487,1 +489,2 @@
         Notificationbar.CancelFunc really_remove = () => {
+            debug ("delete really");

"Box deletion not cancelled by user, actually deleting it now.."

Review of attachment 208109 [details] [review]:

ACK otherwise!

::: src/app.vala
@@ -482,2 +482,2 @@
         Notificationbar.OKFunc undo = () => {
-            foreach (var selected in selected_items)
+            debug ("delete undo");

"Box deletion cancelled by user, re-adding to view.."

@@ -487,1 +489,2 @@
         Notificationbar.CancelFunc really_remove = () => {
+            debug ("delete really");

"Box deletion not cancelled by user, actually deleting it now.."

Review of attachment 208110 [details] [review]:

- disconnect machine ui-state handler to not stop screenshot from
happening


Wouldn't you agree that this part should be in a separate patch?

::: src/libvirt-machine.vala
@@ -229,2 +230,3 @@
     private void set_stats_enable (bool enable) {
         if (enable) {
+            debug ("enable stats");

"disabled statistics for '%s", name

Review of attachment 208110 [details] [review]:

Ack otherwise!

Review of attachment 208111 [details] [review]:

Good idea! ACK!

Review of attachment 208112 [details] [review]:

ACK!

::: src/display-page.vala
@@ -178,2 +178,3 @@
-        cursor_id = display.draw.connect (() => {
-            display.disconnect (cursor_id);
+        ulong draw_id = 0;
+        draw_id = display.draw.connect (() => {
+            display.disconnect (draw_id);

'if (draw_id == 0)' not needed?

Review of attachment 208113 [details] [review]:

Looks good, ACK if you have tested it.

::: src/machine.vala
@@ -64,1 +64,2 @@
             disconnected_id = _display.disconnected.connect (() => {
+                message ("display disconnected");

machine name in message might make it more useful