Two patches to fix problems with the partition editor dialog box. For regular partitions (i.e., not LVM and not RAID), users could not uncheck 'Format' once they selected it. There were also some problems with UI flow between format, migrate, and resize. I've tried to clean that up with these patches.
LVM and RAID partition editing will probably need some small changes to go along with these changes, but I wanted to do those after these are reviewed.
Add wantFormat, wantMigrate, and wantResize to indicate whether or not the user actually wants a format (initialize), migrate, or resize operation to occur for the format. This is mainly for the UI components because right now we base the UI elements on the properties that indicate whether or an operation is possible, not if the user wants the operation done. For example, a format could be resizable, but the user might not want that. --- storage/formats/__init__.py | 30 ++++++++++++++++++++++++++++++ 1 files changed, 30 insertions(+), 0 deletions(-)
diff --git a/storage/formats/__init__.py b/storage/formats/__init__.py index 9ec0999..60be55e 100644 --- a/storage/formats/__init__.py +++ b/storage/formats/__init__.py @@ -149,6 +149,9 @@ class DeviceFormat(object): _minSize = 0 # minimum size in MB _dump = False _check = False + _wantFormat = False + _wantMigrate = False + _wantResize = False
def __init__(self, *args, **kwargs): """ Create a DeviceFormat instance. @@ -294,6 +297,15 @@ class DeviceFormat(object): self.device and os.path.exists(self.device))
+ def _getWantFormat(self): + return self._wantFormat + + def _setWantFormat(self, want): + self._wantFormat = want + + wantFormat = property(_getWantFormat, _setWantFormat, + "Initialize this format?") + @property def formattable(self): """ Can we create formats of this type? """ @@ -309,6 +321,15 @@ class DeviceFormat(object): """ Packages required to manage formats of this type. """ return self._packages
+ def _getWantResize(self): + return self._wantResize + + def _setWantResize(self, want): + self._wantResize = want + + wantResize = property(_getWantResize, _setWantResize, + "Resize this format?") + @property def resizable(self): """ Can formats of this type be resized? """ @@ -319,6 +340,15 @@ class DeviceFormat(object): """ Is this format type suitable for a boot partition? """ return self._bootable
+ def _getWantMigrate(self): + return self._wantMigrate + + def _setWantMigrate(self, want): + self._wantMigrate = want + + wantMigrate = property(_getWantMigrate, _setWantMigrate, + "Migrate this format?") + @property def migratable(self): """ Can formats of this type be migrated? """
On Mon, 2009-03-30 at 18:18 -1000, David Cantrell wrote:
Add wantFormat, wantMigrate, and wantResize to indicate whether or not the user actually wants a format (initialize), migrate, or resize operation to occur for the format. This is mainly for the UI components because right now we base the UI elements on the properties that indicate whether or an operation is possible, not if the user wants the operation done. For example, a format could be resizable, but the user might not want that.
We already have this information in the format classes. I don't like the idea of having multiple sources for any given piece of information as keeping them in sync is extra work, (not to mention error-prone).
storage/formats/__init__.py | 30 ++++++++++++++++++++++++++++++ 1 files changed, 30 insertions(+), 0 deletions(-)
diff --git a/storage/formats/__init__.py b/storage/formats/__init__.py index 9ec0999..60be55e 100644 --- a/storage/formats/__init__.py +++ b/storage/formats/__init__.py @@ -149,6 +149,9 @@ class DeviceFormat(object): _minSize = 0 # minimum size in MB _dump = False _check = False
- _wantFormat = False
wantFormat is equivalent to 'not self.exists'
- _wantMigrate = False
DeviceFormat already has a 'migrate' property for exactly this purpose.
- _wantResize = False
Isn't wantResize equivalent to 'self.currentSize != self.targetSize' ? Surely we already have this information.
Dave
def __init__(self, *args, **kwargs): """ Create a DeviceFormat instance.
@@ -294,6 +297,15 @@ class DeviceFormat(object): self.device and os.path.exists(self.device))
- def _getWantFormat(self):
return self._wantFormat
- def _setWantFormat(self, want):
self._wantFormat = want
- wantFormat = property(_getWantFormat, _setWantFormat,
"Initialize this format?")
- @property def formattable(self): """ Can we create formats of this type? """
@@ -309,6 +321,15 @@ class DeviceFormat(object): """ Packages required to manage formats of this type. """ return self._packages
- def _getWantResize(self):
return self._wantResize
- def _setWantResize(self, want):
self._wantResize = want
- wantResize = property(_getWantResize, _setWantResize,
"Resize this format?")
- @property def resizable(self): """ Can formats of this type be resized? """
@@ -319,6 +340,15 @@ class DeviceFormat(object): """ Is this format type suitable for a boot partition? """ return self._bootable
- def _getWantMigrate(self):
return self._wantMigrate
- def _setWantMigrate(self, want):
self._wantMigrate = want
- wantMigrate = property(_getWantMigrate, _setWantMigrate,
"Migrate this format?")
- @property def migratable(self): """ Can formats of this type be migrated? """
David Lehman wrote:
On Mon, 2009-03-30 at 18:18 -1000, David Cantrell wrote:
storage/formats/__init__.py | 30 ++++++++++++++++++++++++++++++ 1 files changed, 30 insertions(+), 0 deletions(-)
diff --git a/storage/formats/__init__.py b/storage/formats/__init__.py index 9ec0999..60be55e 100644 --- a/storage/formats/__init__.py +++ b/storage/formats/__init__.py @@ -149,6 +149,9 @@ class DeviceFormat(object): _minSize = 0 # minimum size in MB _dump = False _check = False
- _wantFormat = False
wantFormat is equivalent to 'not self.exists'
Is it true for existing partitions we want to format? I see the new want* properties more as a place to store information for UI. When I was thinking about how to fix UI edit flow (multiple editing) of existing partitions, I was trying another approach - to store original (in the sense of UI) format in device instance attribute or ui (in deviceinstance-keyed dict) and create the dialog based on actual device format and stored original format, but it started to become too clumsy and complicated.
I wanted to achieve that for existing partitions, re-edit would mean starting from the same point (preexisting format) so that you could e.g. decide to migreate (from original fs) instead of format when reediting the partition - this would also require canceling of format action planned in previous edit.
On Tue, 2009-03-31 at 19:20 +0200, Radek Vykydal wrote:
David Lehman wrote:
On Mon, 2009-03-30 at 18:18 -1000, David Cantrell wrote:
storage/formats/__init__.py | 30 ++++++++++++++++++++++++++++++ 1 files changed, 30 insertions(+), 0 deletions(-)
diff --git a/storage/formats/__init__.py b/storage/formats/__init__.py index 9ec0999..60be55e 100644 --- a/storage/formats/__init__.py +++ b/storage/formats/__init__.py @@ -149,6 +149,9 @@ class DeviceFormat(object): _minSize = 0 # minimum size in MB _dump = False _check = False
- _wantFormat = False
wantFormat is equivalent to 'not self.exists'
Is it true for existing partitions we want to format?
It should be. We probably do not handle correctly the case of re-editing a preexisting partition and unchecking 'format' after having checked it in the initial edit. In this case we should use the device tree to look up the ActionDestroyFormat and ActionCreateFormat and cancel them both.
I see the new want* properties more as a place to store information for UI. When I was thinking about how to fix UI edit flow (multiple editing) of existing partitions, I was trying another approach - to store original (in the sense of UI) format in device instance attribute or ui (in deviceinstance-keyed dict) and create the dialog based on actual device format and stored original format, but it started to become too clumsy and complicated.
I wanted to achieve that for existing partitions, re-edit would mean starting from the same point (preexisting format) so that you could e.g. decide to migreate (from original fs) instead of format when reediting the partition - this would also require canceling of format action planned in previous edit.
Right -- this is something we would need to do in any case if we have scheduled a format action that we do not want to actually execute. For new devices we can just schedule more actions and they get sorted out by DeviceTree.pruneActions, but it has the same effect.
Dave
Anaconda-devel-list mailing list Anaconda-devel-list@redhat.com https://www.redhat.com/mailman/listinfo/anaconda-devel-list
On 03/31/2009 08:01 AM, David Lehman wrote:
On Tue, 2009-03-31 at 19:20 +0200, Radek Vykydal wrote:
David Lehman wrote:
On Mon, 2009-03-30 at 18:18 -1000, David Cantrell wrote:
storage/formats/__init__.py | 30 ++++++++++++++++++++++++++++++ 1 files changed, 30 insertions(+), 0 deletions(-)
diff --git a/storage/formats/__init__.py b/storage/formats/__init__.py index 9ec0999..60be55e 100644 --- a/storage/formats/__init__.py +++ b/storage/formats/__init__.py @@ -149,6 +149,9 @@ class DeviceFormat(object): _minSize = 0 # minimum size in MB _dump = False _check = False
- _wantFormat = False
wantFormat is equivalent to 'not self.exists'
Is it true for existing partitions we want to format?
It should be. We probably do not handle correctly the case of re-editing a preexisting partition and unchecking 'format' after having checked it in the initial edit. In this case we should use the device tree to look up the ActionDestroyFormat and ActionCreateFormat and cancel them both.
I see the new want* properties more as a place to store information for UI. When I was thinking about how to fix UI edit flow (multiple editing) of existing partitions, I was trying another approach - to store original (in the sense of UI) format in device instance attribute or ui (in deviceinstance-keyed dict) and create the dialog based on actual device format and stored original format, but it started to become too clumsy and complicated.
I wanted to achieve that for existing partitions, re-edit would mean starting from the same point (preexisting format) so that you could e.g. decide to migreate (from original fs) instead of format when reediting the partition - this would also require canceling of format action planned in previous edit.
Right -- this is something we would need to do in any case if we have scheduled a format action that we do not want to actually execute. For new devices we can just schedule more actions and they get sorted out by DeviceTree.pruneActions, but it has the same effect.
What's funny is that the UI flow in the code wasn't entirely obvious to me until after I talked to dlehman in IRC about why the format was getting a new instance each time the dialog box ran. The problem I was trying to solve was what Radek was describing, but the wrong approach.
I've reworked the patch to remove the wantFormat, wantMigrate, and wantResize flags. I'm now using what we already had. The thing I had to do was modify how fmt_class() is being called so we carry over the values we need.
New patches will be posted soon, including fixes for RAID and LVM if I determine those are needed.
Dave and Radek, thanks for the feedback.
The format check mark now reflects what will happen. Correct some problems with the UI elements in the partition editor window. --- iw/partition_dialog_gui.py | 12 +++--- iw/partition_gui.py | 14 +++--- iw/partition_ui_helpers_gui.py | 92 +++++++++++++++++++++++---------------- 3 files changed, 67 insertions(+), 51 deletions(-)
diff --git a/iw/partition_dialog_gui.py b/iw/partition_dialog_gui.py index bb4a730..96ca9b6 100644 --- a/iw/partition_dialog_gui.py +++ b/iw/partition_dialog_gui.py @@ -215,10 +215,12 @@ class PartitionEditor: request = self.origrequest mountpoint = self.mountCombo.get_children()[0].get_text()
- if self.fsoptionsDict.has_key("formatcb") and \ - self.fsoptionsDict["formatcb"].get_active(): + if request.format.wantFormat: fmt_class = self.fsoptionsDict["fstypeCombo"].get_active_value() format = fmt_class(mountpoint=mountpoint) + format.wantFormat = request.format.wantFormat + format.exists = request.format.exists + format.device = request.format.device luksdev = None if self.fsoptionsDict.has_key("lukscb") and \ self.fsoptionsDict["lukscb"].get_active() and \ @@ -239,12 +241,10 @@ class PartitionEditor: request.weight = self.anaconda.platform.weight(mountpoint=mountpoint, fstype=request.format.type)
- if self.fsoptionsDict.has_key("migratecb") and \ - self.fsoptionsDict["migratecb"].get_active(): + if request.format.wantMigrate: actions.append(ActionMigrateFormat(request))
- if self.fsoptionsDict.has_key("resizecb") and \ - self.fsoptionsDict["resizecb"].get_active(): + if request.format.wantResize: size = self.fsoptionsDict["resizesb"].get_value_as_int()
try: diff --git a/iw/partition_gui.py b/iw/partition_gui.py index 6ce1b11..eaf0f87 100644 --- a/iw/partition_gui.py +++ b/iw/partition_gui.py @@ -714,10 +714,10 @@ class PartitionWindow(InstallWindow): self.tree[iter]['Size (MB)'] = "%Ld" % lv.size self.tree[iter]['PyObject'] = vg - if lv.format.type == "luks" and not lv.format.exists: + if format.type == "luks" and not format.wantFormat: # we're creating the LUKS header self.tree[iter]['Format'] = self.lock_pixbuf - elif not format.exists: + elif format.wantFormat: # we're creating a format on the device self.tree[iter]['Format'] = self.checkmark_pixbuf self.tree[iter]['IsFormattable'] = format.formattable @@ -770,9 +770,9 @@ class PartitionWindow(InstallWindow): if format.type: ptype = format.name if array.format.type == "luks" and \ - not array.format.exists: + array.format.wantFormat: self.tree[iter]['Format'] = self.lock_pixbuf - elif not format.exists: + elif format.wantFormat: self.tree[iter]['Format'] = self.checkmark_pixbuf self.tree[iter]['IsFormattable'] = format.formattable else: @@ -883,9 +883,9 @@ class PartitionWindow(InstallWindow):
if device and device.format and \ device.format.type == "luks" and \ - not device.format.exists: + device.format.wantFormat: self.tree[iter]['Format'] = self.lock_pixbuf - elif format and not format.exists: + elif format and format.wantFormat: self.tree[iter]['Format'] = self.checkmark_pixbuf if format and format.type: @@ -918,7 +918,7 @@ class PartitionWindow(InstallWindow): self.tree[iter]['Mount Point'] = ""
if device.format.type == "luks" and \ - not device.format.exists: + device.format.wantFormat: self.tree[iter]['Format'] = self.lock_pixbuf else: if format and format.type: diff --git a/iw/partition_ui_helpers_gui.py b/iw/partition_ui_helpers_gui.py index ede0200..20fdaba 100644 --- a/iw/partition_ui_helpers_gui.py +++ b/iw/partition_ui_helpers_gui.py @@ -35,6 +35,9 @@ from storage.formats import * import gettext _ = lambda x: gettext.ldgettext("anaconda", x)
+FLAG_FORMAT = 1 +FLAG_MIGRATE = 2 + class WideCheckList(checklist.CheckList): def toggled_item(self, data, row):
@@ -231,7 +234,10 @@ def mountptchangeCB(widget, fstypecombo): def resizeOptionCB(widget, resizesb): resizesb.set_sensitive(widget.get_active())
-def formatOptionResizeCB(widget, resizesb): +def formatOptionResizeCB(widget, data): + (resizesb, format) = data + format.wantResize = widget.get_active() + if widget.get_active(): lower = 1 else: @@ -249,9 +255,16 @@ def formatMigrateOptionCB(widget, data): if not sensitive: return
- (combowidget, mntptcombo, ofstype, lukscb, othercombo, othercb) = data + (combowidget, mntptcombo, format, lukscb, othercombo, othercb, flag) = data combowidget.set_sensitive(widget.get_active())
+ if flag == FLAG_FORMAT: + format.wantFormat = widget.get_active() + format.wantMigrate = False + elif flag == FLAG_MIGRATE: + format.wantFormat = False + format.wantMigrate = widget.get_active() + if othercb is not None: othercb.set_sensitive(not widget.get_active()) othercb.set_active(False) @@ -264,9 +277,9 @@ def formatMigrateOptionCB(widget, data): if not widget.get_active(): # set "Encrypt" checkbutton to match partition's initial state lukscb.set_active(lukscb.get_data("encrypted")) - lukscb.set_sensitive(0) + lukscb.set_sensitive(False) else: - lukscb.set_sensitive(1) + lukscb.set_sensitive(True)
# inject event for fstype menu if widget.get_active(): @@ -274,10 +287,10 @@ def formatMigrateOptionCB(widget, data): setMntPtComboStateFromType(fstype, mntptcombo) combowidget.grab_focus() else: - if isinstance(ofstype, type(ofstype)): - ofstype = type(ofstype) + if isinstance(format, type(format)): + format = type(format)
- setMntPtComboStateFromType(ofstype, mntptcombo) + setMntPtComboStateFromType(format, mntptcombo)
""" createPreExistFSOptionSection: given inputs for a preexisting partition, @@ -303,55 +316,58 @@ def createPreExistFSOptionSection(origrequest, maintable, row, mountCombo, else: origfs = origrequest.format
- formatcb = gtk.CheckButton(label=_("_Format as:")) - maintable.attach(formatcb, 0, 1, row, row + 1) - formatcb.set_active(not origfs.exists) - rc["formatcb"] = formatcb - - fstypeCombo = createFSTypeMenu(origfs, fstypechangeCB, - mountCombo, ignorefs=ignorefs) - fstypeCombo.set_sensitive(formatcb.get_active()) - maintable.attach(fstypeCombo, 1, 2, row, row + 1) - row += 1 - rc["fstypeCombo"] = fstypeCombo + if origfs.formattable: + formatcb = gtk.CheckButton(label=_("_Format as:")) + maintable.attach(formatcb, 0, 1, row, row + 1) + formatcb.set_active(origfs.wantFormat) + rc["formatcb"] = formatcb + + fstypeCombo = createFSTypeMenu(origfs, fstypechangeCB, + mountCombo, ignorefs=ignorefs) + fstypeCombo.set_sensitive(formatcb.get_active()) + maintable.attach(fstypeCombo, 1, 2, row, row + 1) + row += 1 + rc["fstypeCombo"] = fstypeCombo + else: + formatcb = None + fstypeCombo = None
if not formatcb.get_active() and not origfs.migrate: - mountCombo.set_data("prevmountable", origfs.mountable) + mountCombo.set_data("prevmountable", origfs.mountable)
# this gets added to the table a bit later on lukscb = gtk.CheckButton(_("_Encrypt"))
if origfs.migratable: - migratecb = gtk.CheckButton(label=_("Mi_grate filesystem to:")) - migratecb.set_active(origfs.migrate) + migratecb = gtk.CheckButton(label=_("Mi_grate filesystem to:")) + migratecb.set_active(origfs.wantMigrate)
- migtypes = [origfs.migrationTarget] + migtypes = [origfs.migrationTarget]
- maintable.attach(migratecb, 0, 1, row, row + 1) - migfstypeCombo = createFSTypeMenu(origfs, + maintable.attach(migratecb, 0, 1, row, row + 1) + migfstypeCombo = createFSTypeMenu(origfs, None, None, availablefstypes = migtypes) - migfstypeCombo.set_sensitive(migratecb.get_active()) - maintable.attach(migfstypeCombo, 1, 2, row, row + 1) - row = row + 1 + migfstypeCombo.set_sensitive(migratecb.get_active()) + maintable.attach(migfstypeCombo, 1, 2, row, row + 1) + row = row + 1 rc["migratecb"] = migratecb rc["migfstypeCombo"] = migfstypeCombo - migratecb.connect("toggled", formatMigrateOptionCB, + migratecb.connect("toggled", formatMigrateOptionCB, (migfstypeCombo, mountCombo, origfs, None, - fstypeCombo, formatcb)) + fstypeCombo, formatcb, FLAG_MIGRATE)) else: - migratecb = None - migfstypeCombo = None + migratecb = None + migfstypeCombo = None
- formatcb.connect("toggled", formatMigrateOptionCB, - (fstypeCombo, mountCombo, origfs, lukscb, - migfstypeCombo, migratecb)) + if formatcb: + formatcb.connect("toggled", formatMigrateOptionCB, + (fstypeCombo, mountCombo, origfs, lukscb, + migfstypeCombo, migratecb, FLAG_FORMAT))
if origrequest.resizable: resizecb = gtk.CheckButton(label=_("_Resize")) - resizecb.set_active(origrequest.resizable and \ - ((origrequest.targetSize != 0) and \ - (origrequest.targetSize != origrequest.currentSize))) + resizecb.set_active(origfs.resizable and origfs.wantResize) rc["resizecb"] = resizecb maintable.attach(resizecb, 0, 1, row, row + 1)
@@ -378,7 +394,7 @@ def createPreExistFSOptionSection(origrequest, maintable, row, mountCombo, resizeOptionCB(resizecb, resizesb) row = row + 1
- formatcb.connect("toggled", formatOptionResizeCB, resizesb) + formatcb.connect("toggled", formatOptionResizeCB, (resizesb, origfs))
if luksdev: lukscb.set_active(1)
anaconda-devel@lists.stg.fedoraproject.org