[et-mgmt-tools] [PATCH] virt-manager: Make virt-manager remember last image directory
Michal Novotny
minovotn at redhat.com
Mon Jun 22 07:53:37 UTC 2009
Hi Cole,
comments inline...
On 06/19/2009 03:39 PM, Cole Robinson wrote:
> On 06/18/2009 12:22 PM, Michal Novotny wrote:
>
>> # HG changeset patch
>> # User Michal Novotny<minovotn at redhat.com>
>> # Date 1245341781 -7200
>> # Node ID b599a65ca0615704a47e38eabcd2ff0947bcbaec
>> # 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.
>>
>>
>
> Comments inline:
>
>
>> diff -r a996e00b3bb6 -r b599a65ca061 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 Thu Jun 18 18:16:21 2009 +0200
>>
>
> <snip>
>
> This all looks fine.
>
>
>> diff -r a996e00b3bb6 -r b599a65ca061 src/virtManager/addhardware.py
>> --- a/src/virtManager/addhardware.py Thu Jun 18 10:54:21 2009 -0400
>> +++ b/src/virtManager/addhardware.py Thu Jun 18 18:16:21 2009 +0200
>> @@ -678,7 +678,7 @@
>>
>> 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(), "image")
>> filename = self._browse_file(_("Locate or Create New Storage File"),
>> textent, folder=folder,
>> confirm_overwrite=True)
>> diff -r a996e00b3bb6 -r b599a65ca061 src/virtManager/choosecd.py
>> --- a/src/virtManager/choosecd.py Thu Jun 18 10:54:21 2009 -0400
>> +++ b/src/virtManager/choosecd.py Thu Jun 18 18:16:21 2009 +0200
>> @@ -20,6 +20,7 @@
>> import gtk.glade
>> import gobject
>> import logging
>> +import os.path
>>
>> import virtinst
>>
>> @@ -133,7 +134,8 @@
>> 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, "media" )
>> + self._browse_file(_("Locate ISO Image"), folder=folder)
>>
>> def populate_opt_media(self):
>> try:
>> @@ -146,14 +148,16 @@
>>
>> 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, "media" )
>>
>>
>
> I'd prefer if function calls look like:
>
> func(arg1, arg2)
>
> rather than
>
> func( arg1, arg2 )
>
Ok, I'll change this. This is just about synchronizing my coding style
with existing virt-manager codebase.
> The former is more inline with existing code.
>
> Also, I like all new code to try and stay under the 80 column length. If it's
> too ugly to make it fit, don't worry, but that isn't the common case.
>
>
Ok, fine. I'll rewrite it to fit on 80 columns per line...
>> - 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.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 b599a65ca061 src/virtManager/config.py
>> --- a/src/virtManager/config.py Thu Jun 18 10:54:21 2009 -0400
>> +++ b/src/virtManager/config.py Thu Jun 18 18:16:21 2009 +0200
>> @@ -261,6 +261,30 @@
>> self.conf.set_bool(self.conf_dir + "/vmlist-fields/network_traffic", state)
>>
>>
>> + def get_default_directory(self, conn, _type):
>> + if _type not in ["image", "media", "save", "restore", "screenshot"]:
>> + logging.error("Unknown type for get_default_directory: %s" % _type)
>> + 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:
>> + if _type == "image" or _type == "media":
>> + path = self.get_default_image_dir(conn)
>> + if _type == "save":
>> + path = self.get_default_save_dir(conn)
>> +
>> + return path
>> +
>> + def set_default_directory(self, folder, _type):
>> + if _type not in ["image", "media", "save", "restore", "screenshot"]:
>> + logging.error("Unknown type for set_default_directory: %s" % _type)
>> + return
>> +
>> + self.conf.set_value( self.conf_dir + "/paths/default-%s-path" % _type, folder)
>>
>>
>
> Rather than have the user pass a string in like "image", "media", ..., how
> about defining some constants in the config class:
>
> CONFIG_DIR_IMAGE = 1
> CONFIG_DIR_SAVE = 2
> ...
>
> (They can even be the string values you use above anyways). Then have a
> separate function which converts that to the requested gconf path. This way
> you don't have to duplicate the ["image", "media", ...] whitelist in two
> places, and I think will make the calling code more readable.
>
>
Well, the idea is good. I know what you mean and I'll use string values
there to preserve most of those codes.
> Also, to simplify a lot of the code, you could push down the gconf fetching/
> storing into util.browse_local: add an argument which takes one of the above
> CONFIG_DIR values, sets the start folder, and then records the directory that
> the user chooses. That way, all callers would need to pass one value, and
> wouldn't need to play with the result.
>
Well, I don't know what do you mean by that. Do you mean to import gconf
directly into util.py and directly call gconf in browse_local? You wrote
something about altering util.browse_local() ... could you provide me
the prototype you mean?
>
>> 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 b599a65ca061 src/virtManager/create.py
>> --- a/src/virtManager/create.py Thu Jun 18 10:54:21 2009 -0400
>> +++ b/src/virtManager/create.py Thu Jun 18 18:16:21 2009 +0200
>> @@ -995,16 +995,20 @@
>> nodetect_label.show()
>>
>> def browse_iso(self, ignore1=None, ignore2=None):
>> + folder = self.config.get_default_directory( self.conn, "media")
>> self._browse_file(_("Locate ISO Image"),
>> - self.set_iso_storage_path)
>> + self.set_iso_storage_path,
>> + folder)
>> 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, "image")
>> self._browse_file(_("Locate existing storage"),
>> - self.set_disk_storage_path)
>> + self.set_disk_storage_path,
>> + folder)
>>
>> def toggle_storage_select(self, src):
>> act = src.get_active()
>> @@ -1014,9 +1018,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, "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, "image")
>> self.window.get_widget("config-storage-entry").set_text(path)
>>
>> # Navigation methods
>> diff -r a996e00b3bb6 -r b599a65ca061 src/virtManager/details.py
>> --- a/src/virtManager/details.py Thu Jun 18 10:54:21 2009 -0400
>> +++ b/src/virtManager/details.py Thu Jun 18 18:16:21 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
>> @@ -1410,11 +1411,17 @@
>> # user to choose what image format they'd like to save in....
>> path = util.browse_local(self.window.get_widget("vmm-details"),
>> _("Save Virtual Machine Screenshot"),
>> + self.config.get_default_directory(self.vm.get_connection(),
>> + "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, "screenshot" )
>> +
>> filename = path
>> if not(filename.endswith(".png")):
>> filename += ".png"
>> diff -r a996e00b3bb6 -r b599a65ca061 src/virtManager/engine.py
>> --- a/src/virtManager/engine.py Thu Jun 18 10:54:21 2009 -0400
>> +++ b/src/virtManager/engine.py Thu Jun 18 18:16:21 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,16 @@
>>
>> 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, "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, "save")
>> +
>> progWin = vmmAsyncJob(self.config, self._save_callback, [vm, path],
>> _("Saving Virtual Machine"))
>> progWin.run()
>> diff -r a996e00b3bb6 -r b599a65ca061 src/virtManager/manager.py
>> --- a/src/virtManager/manager.py Thu Jun 18 10:54:21 2009 -0400
>> +++ b/src/virtManager/manager.py Thu Jun 18 18:16:21 2009 +0200
>> @@ -23,6 +23,7 @@
>> import gtk.glade
>> import logging
>> import traceback
>> +import os.path
>>
>> import sparkline
>> import libvirt
>> @@ -397,11 +398,15 @@
>>
>> 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, "restore"))
>>
>> if not path:
>> return
>>
>> + # Save last restore path
>> + folder = os.path.dirname( path )
>> + self.config.set_default_directory( folder, "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)
>>
>
> Thanks! This will be useful to have.
> Cole
>
Ok, no problem.
Michal
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/et-mgmt-tools/attachments/20090622/42f93686/attachment.htm>
More information about the et-mgmt-tools
mailing list