Michal Novotny wrote:
> Hi,
> I've modified my patch to match those requirements and hopefully they're
> already met now so please review this patch. In addition to saving those
> 5 directories (for image/media/save/restore and screenshot files) I
> found a bug that prevented showing existing PNG files in folders when
> saving screenshots and that has been fixed too (simple fix by changing
> "*.png" mask to "png" only because of asterisk and dot appended by
> util.browse_local() function itself).
>
Besides the change to browse_local() I mentioned in the other mail, just
a few nit picks inline.
> # HG changeset patch
> # User Michal Novotny <minovotn@xxxxxxxxxx>
> # Date 1245665443 -7200
> # Node ID 51fc9462316334d9360908c5cb713ccfc74de81b
> # Parent a996e00b3bb6a99fdf9505c053bc1f6bee532d3b
> Make virt-manager remember last used paths
>
> This patch makes virt-manager remember last used paths for disk images, saved
> snapshots, restored snapshots, media paths and also screenshot paths not to
> bother users with changing paths from the default location per HV technology.
> Useful when installing multiple domains and having all the media/image files
> in non-default locations.
>
> diff -r a996e00b3bb6 -r 51fc94623163 src/virt-manager.schemas.in
> --- a/src/virt-manager.schemas.in Thu Jun 18 10:54:21 2009 -0400
> +++ b/src/virt-manager.schemas.in Mon Jun 22 12:10:43 2009 +0200
> @@ -272,5 +272,70 @@
> <long>Whether to install a sound device for remote VMs or not</long>
> </locale>
> </schema>
> +
> + <schema>
> + <key>/schemas/apps/::PACKAGE::/paths/default-image-path</key>
> + <applyto>/apps/::PACKAGE::/paths/default-image-path</applyto>
> + <owner>::PACKAGE::</owner>
> + <type>string</type>
> + <default></default>
> +
> + <locale name="C">
> + <short>Default image path</short>
> + <long>Default path for choosing VM images</long>
> + </locale>
> + </schema>
> +
> + <schema>
> + <key>/schemas/apps/::PACKAGE::/paths/default-media-path</key>
> + <applyto>/apps/::PACKAGE::/paths/default-media-path</applyto>
> + <owner>::PACKAGE::</owner>
> + <type>string</type>
> + <default></default>
> +
> + <locale name="C">
> + <short>Default media path</short>
> + <long>Default path for choosing media</long>
> + </locale>
> + </schema>
> +
> + <schema>
> + <key>/schemas/apps/::PACKAGE::/paths/default-save-path</key>
> + <applyto>/apps/::PACKAGE::/paths/default-save-path</applyto>
> + <owner>::PACKAGE::</owner>
> + <type>string</type>
> + <default></default>
> +
> + <locale name="C">
> + <short>Default save domain path</short>
> + <long>Default path for saving VM snaphots</long>
> + </locale>
> + </schema>
> +
> + <schema>
> + <key>/schemas/apps/::PACKAGE::/paths/default-restore-path</key>
> + <applyto>/apps/::PACKAGE::/paths/default-restore-path</applyto>
> + <owner>::PACKAGE::</owner>
> + <type>string</type>
> + <default></default>
> +
> + <locale name="C">
> + <short>Default restore path</short>
> + <long>Default path for stored VM snapshots</long>
> + </locale>
> + </schema>
> +
> + <schema>
> + <key>/schemas/apps/::PACKAGE::/paths/default-screenshot-path</key>
> + <applyto>/apps/::PACKAGE::/paths/default-screenshot-path</applyto>
> + <owner>::PACKAGE::</owner>
> + <type>string</type>
> + <default></default>
> +
> + <locale name="C">
> + <short>Default screenshot path</short>
> + <long>Default path for saving screenshots from VMs</long>
> + </locale>
> + </schema>
> </schemalist>
> </gconfschemafile>
> diff -r a996e00b3bb6 -r 51fc94623163 src/virtManager/addhardware.py
> --- a/src/virtManager/addhardware.py Thu Jun 18 10:54:21 2009 -0400
> +++ b/src/virtManager/addhardware.py Mon Jun 22 12:10:43 2009 +0200
> @@ -678,7 +678,8 @@
>
> def browse_storage_file_address(self, src, ignore=None):
> textent = self.window.get_widget("storage-file-address")
> - folder = self.config.get_default_image_dir(self.vm.get_connection())
> + folder = self.config.get_default_directory(self.vm.get_connection(),
> + self.config.CONFIG_DIR_IMAGE)
> filename = self._browse_file(_("Locate or Create New Storage File"),
> textent, folder=folder,
> confirm_overwrite=True)
> diff -r a996e00b3bb6 -r 51fc94623163 src/virtManager/choosecd.py
> --- a/src/virtManager/choosecd.py Thu Jun 18 10:54:21 2009 -0400
> +++ b/src/virtManager/choosecd.py Mon Jun 22 12:10:43 2009 +0200
> @@ -20,6 +20,7 @@
> import gtk.glade
> import gobject
> import logging
> +import os.path
>
> import virtinst
>
> @@ -133,7 +134,9 @@
> pass
>
> def browse_fv_iso_location(self, ignore1=None, ignore2=None):
> - self._browse_file(_("Locate ISO Image"))
> + folder = self.config.get_default_directory(self.conn,
> + self.config.CONFIG_DIR_MEDIA)
> + self._browse_file(_("Locate ISO Image"), folder=folder)
>
> def populate_opt_media(self):
> try:
> @@ -146,14 +149,17 @@
>
> def set_storage_path(self, src, path):
> self.window.get_widget("iso-path").set_text(path)
> + folder = os.path.dirname(path)
> + self.config.set_default_directory(folder, self.config.CONFIG_DIR_MEDIA)
>
> - def _browse_file(self, dialog_name):
> + def _browse_file(self, dialog_name, folder):
> if self.storage_browser == None:
> - self.storage_browser = vmmStorageBrowser(self.config, self.conn)
> - #self.topwin)
> + self.storage_browser = vmmStorageBrowser(self.config, self.conn,
> + True)
> self.storage_browser.connect("storage-browse-finish",
> self.set_storage_path)
> - self.storage_browser.local_args = { "dialog_name": dialog_name }
> + self.storage_browser.local_args = { "dialog_name" : dialog_name,
> + "start_folder": folder }
> self.storage_browser.show(self.conn)
> return None
>
> diff -r a996e00b3bb6 -r 51fc94623163 src/virtManager/config.py
> --- a/src/virtManager/config.py Thu Jun 18 10:54:21 2009 -0400
> +++ b/src/virtManager/config.py Mon Jun 22 12:10:43 2009 +0200
> @@ -45,6 +45,13 @@
>
> class vmmConfig:
>
> + # GConf directory names for saving last used paths
> + CONFIG_DIR_IMAGE = "image"
> + CONFIG_DIR_MEDIA = "media"
> + CONFIG_DIR_SAVE = "save"
> + CONFIG_DIR_RESTORE = "restore"
> + CONFIG_DIR_SCREENSHOT = "screenshot"
> +
> CONSOLE_SCALE_NEVER = 0
> CONSOLE_SCALE_FULLSCREEN = 1
> CONSOLE_SCALE_ALWAYS = 2
> @@ -261,6 +268,36 @@
> self.conf.set_bool(self.conf_dir + "/vmlist-fields/network_traffic", state)
>
>
> + def get_default_directory(self, conn, _type):
> + if len(_type) == 0:
If somehow a non string/list value was passed in here (say, None), this
would backtrace. You can do something like
config_dirs = [CONFIG_DIR_IMAGE, CONFIG_DIR_MEDIA, ...]
...
if _type not in self.config_dirs:
logging.error(blah)
> + logging.error("Unknown type for get_default_directory")
> + return
> +
> + try:
> + path = self.conf.get_value(self.conf_dir + "/paths/default-%s-path"
> + % _type)
> + except:
> + path = None
> +
> + if not path or len(path) == 0:
The check for len(path) == 0 is redundant here: 'not path' will match
the zero length string.
> + if (_type == vmmConfig.CONFIG_DIR_IMAGE or
> + _type == vmmConfig.CONFIG_DIR_MEDIA):
> + path = self.get_default_image_dir(conn)
> + if (_type == vmmConfig.CONFIG_DIR_SAVE or
> + _type == vmmConfig.CONFIG_DIR_RESTORE):
> + path = self.get_default_save_dir(conn)
> +
> + logging.debug("get_default_directory(%s): returning %s" % (_type, path))
> + return path
> +
> + def set_default_directory(self, folder, _type):
> + if len(_type) == 0:
> + logging.error("Unknown type for set_default_directory")
> + return
> +
> + logging.debug("set_default_directory(%s): saving %s" % (_type, folder))
> + self.conf.set_value(self.conf_dir + "/paths/default-%s-path" % _type,
> + folder)
>
> def on_vmlist_domain_id_visible_changed(self, callback):
> self.conf.notify_add(self.conf_dir + "/vmlist-fields/domain_id", callback)
> diff -r a996e00b3bb6 -r 51fc94623163 src/virtManager/create.py
> --- a/src/virtManager/create.py Thu Jun 18 10:54:21 2009 -0400
> +++ b/src/virtManager/create.py Mon Jun 22 12:10:43 2009 +0200
> @@ -995,16 +995,22 @@
> nodetect_label.show()
>
> def browse_iso(self, ignore1=None, ignore2=None):
> + folder = self.config.get_default_directory(self.conn,
> + self.config.CONFIG_DIR_MEDIA)
> self._browse_file(_("Locate ISO Image"),
> - self.set_iso_storage_path)
> + self.set_iso_storage_path,
> + folder, True)
> self.window.get_widget("install-local-box").activate()
>
> def toggle_enable_storage(self, src):
> self.window.get_widget("config-storage-box").set_sensitive(src.get_active())
>
> def browse_storage(self, ignore1):
> + folder = self.config.get_default_directory(self.conn,
> + self.config.CONFIG_DIR_IMAGE)
> self._browse_file(_("Locate existing storage"),
> - self.set_disk_storage_path)
> + self.set_disk_storage_path,
> + folder, False)
>
> def toggle_storage_select(self, src):
> act = src.get_active()
> @@ -1014,9 +1020,13 @@
> self.window.get_widget("config-macaddr").set_sensitive(src.get_active())
>
> def set_iso_storage_path(self, ignore, path):
> + folder = os.path.dirname(path)
> + self.config.set_default_directory(folder, self.config.CONFIG_DIR_MEDIA)
> self.window.get_widget("install-local-box").child.set_text(path)
>
> def set_disk_storage_path(self, ignore, path):
> + folder = os.path.dirname( path )
> + self.config.set_default_directory(folder, self.config.CONFIG_DIR_IMAGE)
> self.window.get_widget("config-storage-entry").set_text(path)
>
> # Navigation methods
> @@ -1648,9 +1658,10 @@
> logging.exception("Error detecting distro.")
> self.detectedDistro = (None, None)
>
> - def _browse_file(self, dialog_name, callback, folder=None):
> + def _browse_file(self, dialog_name, callback, folder=None, is_media=False):
> if self.storage_browser == None:
> - self.storage_browser = vmmStorageBrowser(self.config, self.conn)
> + self.storage_browser = vmmStorageBrowser(self.config, self.conn,
> + is_media)
> self.storage_browser.connect("storage-browse-finish",
> callback)
> self.storage_browser.local_args = { "dialog_name": dialog_name,
> diff -r a996e00b3bb6 -r 51fc94623163 src/virtManager/details.py
> --- a/src/virtManager/details.py Thu Jun 18 10:54:21 2009 -0400
> +++ b/src/virtManager/details.py Mon Jun 22 12:10:43 2009 +0200
> @@ -31,6 +31,7 @@
> import os
> import socket
> import cairo
> +import os.path
>
> from virtManager.error import vmmErrorDialog
> from virtManager.addhardware import vmmAddHardware
> @@ -1408,13 +1409,22 @@
> def control_vm_screenshot(self, src):
> # If someone feels kind they could extend this code to allow
> # user to choose what image format they'd like to save in....
> + # Also _type was changed to "png" from "*.png" to show existing files
The change is correct, but no need to comment it in the code.
> path = util.browse_local(self.window.get_widget("vmm-details"),
> _("Save Virtual Machine Screenshot"),
> - _type = ("*.png", "PNG files"),
> + self.config.get_default_directory(
> + self.vm.get_connection(),
> + self.config.CONFIG_DIR_SCREENSHOT),
> + _type = ("png", "PNG files"),
> dialog_type = gtk.FILE_CHOOSER_ACTION_SAVE)
> if not path:
> return
>
> + # Save default screenshot directory
> + folder = os.path.dirname(path)
> + self.config.set_default_directory(folder,
> + self.config.CONFIG_DIR_SCREENSHOT)
> +
> filename = path
> if not(filename.endswith(".png")):
> filename += ".png"
> diff -r a996e00b3bb6 -r 51fc94623163 src/virtManager/engine.py
> --- a/src/virtManager/engine.py Thu Jun 18 10:54:21 2009 -0400
> +++ b/src/virtManager/engine.py Mon Jun 22 12:10:43 2009 +0200
> @@ -24,6 +24,7 @@
> import logging
> import gnome
> import traceback
> +import os.path
>
> from virtManager.about import vmmAbout
> from virtManager.connect import vmmConnect
> @@ -406,12 +407,17 @@
>
> path = util.browse_local(src.window.get_widget("vmm-details"),
> _("Save Virtual Machine"),
> - self.config.get_default_save_dir(con),
> + self.config.get_default_directory(con,
> + self.config.CONFIG_DIR_SAVE),
> dialog_type=gtk.FILE_CHOOSER_ACTION_SAVE)
>
> if not path:
> return
>
> + # Save default directory for saving VMs
> + folder = os.path.dirname(path)
> + self.config.set_default_directory(folder, self.config.CONFIG_DIR_SAVE)
> +
> progWin = vmmAsyncJob(self.config, self._save_callback, [vm, path],
> _("Saving Virtual Machine"))
> progWin.run()
> diff -r a996e00b3bb6 -r 51fc94623163 src/virtManager/manager.py
> --- a/src/virtManager/manager.py Thu Jun 18 10:54:21 2009 -0400
> +++ b/src/virtManager/manager.py Mon Jun 22 12:10:43 2009 +0200
> @@ -23,6 +23,7 @@
> import gtk.glade
> import logging
> import traceback
> +import os.path
>
> import sparkline
> import libvirt
> @@ -397,11 +398,16 @@
>
> path = util.browse_local(self.window.get_widget("vmm-manager"),
> _("Restore Virtual Machine"),
> - self.config.get_default_save_dir(conn))
> + self.config.get_default_directory(conn,
> + self.config.CONFIG_DIR_RESTORE))
>
> if not path:
> return
>
> + # Save last restore path
> + folder = os.path.dirname(path)
> + self.config.set_default_directory(folder, self.config.CONFIG_DIR_RESTORE)
> +
> if not conn.is_valid_saved_image(path):
> self.err.val_err(_("The file '%s' does not appear to be a "
> "valid saved machine image") % path)
> diff -r a996e00b3bb6 -r 51fc94623163 src/virtManager/storagebrowse.py
> --- a/src/virtManager/storagebrowse.py Thu Jun 18 10:54:21 2009 -0400
> +++ b/src/virtManager/storagebrowse.py Mon Jun 22 12:10:43 2009 +0200
> @@ -23,6 +23,7 @@
>
> import traceback
> import logging
> +import os.path
>
> import virtinst
>
> @@ -38,7 +39,7 @@
> gobject.TYPE_NONE, [str]),
> }
>
> - def __init__(self, config, conn):
> + def __init__(self, config, conn, is_media=False):
> self.__gobject_init__()
> self.window = gtk.glade.XML(config.get_glade_dir() + \
> "/vmm-storage-browse.glade",
> @@ -46,6 +47,7 @@
> domain="virt-manager")
> self.config = config
> self.conn = conn
> + self.is_media = is_media
> self.conn_signal_ids = []
>
> self.topwin = self.window.get_widget("vmm-storage-browse")
> @@ -240,6 +242,13 @@
> self._do_finish()
>
> def _do_finish(self, path=None):
> + if (not self.is_media and path is not None and
> + not path.startswith("/dev")):
> + # Save new default image directory
> + folder = os.path.dirname(path)
> + self.config.set_default_directory(folder,
> + self.config.CONFIG_DIR_IMAGE)
> +
> if not path:
> path = self.current_vol().get_path()
> self.emit("storage-browse-finish", path)
If we move all the gconf getting/setting into browse_local, the above
changes to the storage browser aren't necessary: the storage browser
will just need to be sure to specify 'conn', 'config', and
'browse_reason' when launching the local browser.
Thanks,
Cole
_______________________________________________
et-mgmt-tools mailing list
et-mgmt-tools@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/et-mgmt-tools
