Thanks for the patches! General idea is fine, some comments below.
On 06/05/2013 03:40 PM, lagarcia@xxxxxxxxxxxxxxxxxx wrote:
> From: Leonardo Garcia <lagarcia@xxxxxxxxxx>
>
> ---
> ui/vmm-details.ui | 15 +++++++++++++++
> virtManager/baseclass.py | 5 +++--
> virtManager/details.py | 11 +++++++++--
> virtManager/engine.py | 46 +++++++++++++++++++++++++++++++++++++++++++++-
> 4 files changed, 72 insertions(+), 5 deletions(-)
>
> diff --git a/ui/vmm-details.ui b/ui/vmm-details.ui
> index ddb2a71..ea4b53e 100644
> --- a/ui/vmm-details.ui
> +++ b/ui/vmm-details.ui
> @@ -314,6 +314,21 @@
> </object>
> </child>
> <child>
> + <object class="GtkMenuItem" id="details-menu-delete">
> + <property name="visible">True</property>
> + <property name="can_focus">False</property>
> + <property name="label" translatable="yes">_Delete...</property>
> + <property name="use_underline">True</property>
> + <signal name="activate" handler="on_details_menu_delete_activate" swapped="no"/>
> + </object>
> + </child>
> + <child>
> + <object class="GtkSeparatorMenuItem" id="separator2">
> + <property name="visible">True</property>
> + <property name="can_focus">False</property>
> + </object>
> + </child>
> + <child>
> <object class="GtkMenuItem" id="details-menu-vm-screenshot">
> <property name="visible">True</property>
> <property name="can_focus">False</property>
> diff --git a/virtManager/baseclass.py b/virtManager/baseclass.py
> index 7bc7812..c3a093e 100644
> --- a/virtManager/baseclass.py
> +++ b/virtManager/baseclass.py
> @@ -194,8 +194,9 @@ class vmmGObjectUI(vmmGObject):
> self.close()
> vmmGObject.cleanup(self)
> self.builder = None
> - self.topwin.destroy()
> - self.topwin = None
> + if self.topwin:
> + self.topwin.destroy()
> + self.topwin = None
> self.uifile = None
> self.err = None
>
Hmm, I can see from the vmmGObjectUI code why this is needed but it shouldn't
be required in practice. I've pushed a cleanup now that should make this
unnecessary, so please drop this bit.
> diff --git a/virtManager/details.py b/virtManager/details.py
> index 8e927b1..1323232 100644
> --- a/virtManager/details.py
> +++ b/virtManager/details.py
> @@ -329,6 +329,7 @@ class vmmDetails(vmmGObjectUI):
> "action-exit-app": (GObject.SignalFlags.RUN_FIRST, None, []),
> "action-view-manager": (GObject.SignalFlags.RUN_FIRST, None, []),
> "action-migrate-domain": (GObject.SignalFlags.RUN_FIRST, None, [str, str]),
> + "action-delete-domain": (GObject.SignalFlags.RUN_FIRST, None, [str, str]),
> "action-clone-domain": (GObject.SignalFlags.RUN_FIRST, None, [str, str]),
> "details-closed": (GObject.SignalFlags.RUN_FIRST, None, []),
> "details-opened": (GObject.SignalFlags.RUN_FIRST, None, []),
> @@ -412,6 +413,7 @@ class vmmDetails(vmmGObjectUI):
> "on_details_menu_pause_activate": self.control_vm_pause,
> "on_details_menu_clone_activate": self.control_vm_clone,
> "on_details_menu_migrate_activate": self.control_vm_migrate,
> + "on_details_menu_delete_activate": self.control_vm_delete,
> "on_details_menu_screenshot_activate": self.control_vm_screenshot,
> "on_details_menu_view_toolbar_activate": self.toggle_toolbar,
> "on_details_menu_view_manager_activate": self.view_manager,
> @@ -559,8 +561,9 @@ class vmmDetails(vmmGObjectUI):
> for serial in self.serial_tabs:
> self._close_serial_tab(serial)
>
> - self.console.cleanup()
> - self.console = None
> + if self.console:
> + self.console.cleanup()
> + self.console = None
>
self.console should be unconditionally set, so I'm not sure why this is needed.
> self.vm = None
> self.conn = None
> @@ -1580,6 +1583,10 @@ class vmmDetails(vmmGObjectUI):
> self.emit("action-migrate-domain",
> self.vm.conn.get_uri(), self.vm.get_uuid())
>
> + def control_vm_delete(self, src_ignore):
> + self.emit("action-delete-domain",
> + self.vm.conn.get_uri(), self.vm.get_uuid())
> +
> def control_vm_screenshot(self, src_ignore):
> image = self.console.viewer.get_pixbuf()
>
> diff --git a/virtManager/engine.py b/virtManager/engine.py
> index 16ed552..7ab20e6 100644
> --- a/virtManager/engine.py
> +++ b/virtManager/engine.py
> @@ -48,6 +48,7 @@ from virtManager.create import vmmCreate
> from virtManager.host import vmmHost
> from virtManager.error import vmmErrorDialog
> from virtManager.systray import vmmSystray
> +from virtManager.delete import vmmDeleteDialog
>
> # Enable this to get a report of leaked objects on app shutdown
> # gtk3/pygobject has issues here as of Fedora 18
> @@ -95,6 +96,7 @@ class vmmEngine(vmmGObject):
> self.last_timeout = 0
>
> self.systray = None
> + self.delete_dialog = None
> self.application = Gtk.Application(
> application_id="com.redhat.virt-manager",
> flags=0)
> @@ -239,7 +241,9 @@ class vmmEngine(vmmGObject):
> return
>
> self.conns[hvuri]["windowDetails"][vmuuid].cleanup()
> - del(self.conns[hvuri]["windowDetails"][vmuuid])
> + if self.conns:
> + # The cleanup call above might end up emptying the conns dictionary
> + del(self.conns[hvuri]["windowDetails"][vmuuid])
>
How is this called after cleanup ?
> def _do_conn_changed(self, conn):
> if (conn.get_state() == conn.STATE_ACTIVE or
> @@ -373,6 +377,10 @@ class vmmEngine(vmmGObject):
> self.windowMigrate.cleanup()
> self.windowMigrate = None
>
> + if self.delete_dialog:
> + self.delete_dialog.cleanup()
> + self.delete_dialog = None
> +
> # Do this last, so any manually 'disconnected' signals
> # take precedence over cleanup signal removal
> for uri in self.conns:
> @@ -594,6 +602,7 @@ class vmmEngine(vmmGObject):
> obj.connect("action-exit-app", self.exit_app)
> obj.connect("action-view-manager", self._do_show_manager)
> obj.connect("action-migrate-domain", self._do_show_migrate)
> + obj.connect("action-delete-domain", self._do_delete_domain)
> obj.connect("action-clone-domain", self._do_show_clone)
> obj.connect("details-opened", self.increment_window_counter)
> obj.connect("details-closed", self.decrement_window_counter)
> @@ -984,3 +993,38 @@ class vmmEngine(vmmGObject):
> logging.debug("Resetting vm '%s'", vm.get_name())
> vmmAsyncJob.simple_async_noshow(vm.reset, [], src,
> _("Error resetting domain"))
> +
> + def _do_delete_domain(self, src, uri, uuid):
> + conn = self._lookup_conn(uri)
> + vm = conn.get_vm(uuid)
> + details_dialog = self._get_details_dialog(uri, uuid)
> +
> + if vm.is_active():
> + if not util.chkbox_helper(src, self.config.get_confirm_delrunningvm,
> + self.config.set_confirm_delrunningvm,
> + text1=_("Are you sure you want to force poweroff '%s'?" %
> + vm.get_name()),
> + text2=_("In order to delete a running VM, you first need to power "
> + "it off. This will immediately power off the VM without "
> + "shutting down the OS and may cause data loss.")):
> + return
> +
> + logging.debug("Forced power off of vm '%s in order to proceed with "
> + "its deletion'", vm.get_name())
> + def tmpcb(job, *args, **kwargs):
> + ignore = job
> + vm.destroy()
> + docb = tmpcb
> +
> + asyncjob = vmmAsyncJob(docb, [], _("Forcing VM Power off"),
> + _("Powering off the VM in order to proceed with its deletion."),
> + details_dialog.topwin, async=False, show_progress=True)
> + error, details = asyncjob.run()
> + if error is not None:
> + error = _("Error shutting down domain") + ": " + error
> + src.err.show_err(error, details=details)
> + return
> +
> + if not self.delete_dialog:
> + self.delete_dialog = vmmDeleteDialog()
> + self.delete_dialog.show(vm, details_dialog.topwin)
This bit looks fine, but please also change manager.py to use this as well,
rather than instantiate its own delete dialog.
Thanks,
Cole
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
