Hi Cole, comments inline... On 06/19/2009 03:39 PM, Cole Robinson wrote:
Ok, I'll change this. This is just about synchronizing my coding style with existing virt-manager codebase.On 06/18/2009 12:22 PM, Michal Novotny wrote:# HG changeset patch # User Michal Novotny<minov...@redhat.com> # Date 1245341781 -7200 # Node ID b599a65ca0615704a47e38eabcd2ff0947bcbaec # Parent a996e00b3bb6a99fdf9505c053bc1f6bee532d3b Make virt-manager remember last used pathsThis 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 )
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...
Well, the idea is good. I know what you mean and I'll use string values there to preserve most of those codes.- 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, 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?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.
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
_______________________________________________ et-mgmt-tools mailing list et-mgmt-tools@redhat.com https://www.redhat.com/mailman/listinfo/et-mgmt-tools
