[blivet:rhel7/master] Set device.format.label field close to where we read it (#1056139)
Vratislav Podzimek
vpodzime at redhat.com
Thu Jan 23 05:38:41 UTC 2014
On Wed, 2014-01-22 at 09:58 -0500, Anne Mulhern wrote:
>
>
>
> ----- Original Message -----
> > From: "Vratislav Podzimek" <vpodzime at redhat.com>
> > To: "anaconda patch review" <anaconda-patches at lists.fedorahosted.org>
> > Sent: Wednesday, January 22, 2014 3:31:32 AM
> > Subject: Re: [blivet:rhel7/master] Set device.format.label field close to where we read it (#1056139)
> >
> > On Tue, 2014-01-21 at 12:07 -0500, mulhern wrote:
> > > Resolves: rhbz#1056139
> > >
> > > Previously, somewhere between where this was set and where it was first
> > > read
> > > in _populate_right_side it was occasionally being reset to None. This
> > > should have been impossible as the code between is very simple and it
> > > should
> > > be impossible for it to have this side-effect. Nonetheless, it was observed
> > > to happen.
> > >
> > > Signed-off-by: mulhern <amulhern at redhat.com>
> > > ---
> > > pyanaconda/ui/gui/spokes/custom.py | 10 ++++++----
> > > 1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/pyanaconda/ui/gui/spokes/custom.py
> > > b/pyanaconda/ui/gui/spokes/custom.py
> > > index b6bbf7c..82e020f 100644
> > > --- a/pyanaconda/ui/gui/spokes/custom.py
> > > +++ b/pyanaconda/ui/gui/spokes/custom.py
> > > @@ -1693,9 +1693,6 @@ class CustomPartitioningSpoke(NormalSpoke,
> > > StorageChecker):
> > > else:
> > > use_dev = device
> > >
> > > - if hasattr(device.format, "label") and device.format.label is
> > > None:
> > > - device.format.label = ""
> > > -
> > > if hasattr(use_dev, "req_disks") and not use_dev.exists:
> > > self._device_disks = use_dev.req_disks[:]
> > > else:
> > > @@ -1734,7 +1731,12 @@ class CustomPartitioningSpoke(NormalSpoke,
> > > StorageChecker):
> > > self._mountPointEntry.set_text(getattr(device.format,
> > > "mountpoint", "") or "")
> > > fancy_set_sensitive(self._mountPointEntry,
> > > device.format.mountable)
> > >
> > > - self._labelEntry.set_text(getattr(device.format, "label", ""))
> > > + if hasattr(device.format, "label"):
> > > + if device.format.label is None:
> > > + device.format.label = ""
> > > + self._labelEntry.set_text(device.format.label)
> > > + else:
> > > + self._labelEntry.set_text("")
> > > fancy_set_sensitive(self._labelEntry, not device.format.exists)
> > Looks good to me, thanks! Just out of curiosity -- if device.format
> > doesn't have the 'label' attribute, does it make sense to just set
> > labelEntry to "" or should it be made insensitive?
> >
> > --
> > Vratislav Podzimek
> >
> > Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic
> >
> > _______________________________________________
> > anaconda-patches mailing list
> > anaconda-patches at lists.fedorahosted.org
> > https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
> >
>
> There is so much that is going on with interactions between labels and formats that it seems better to make the _labelEntry
> sensitivity dependent only on
> a property that will hold true for all formats and set it only in one place.
> Then cope with the user's final choice when they press the button below all those fields.
>
> Otherwise, with previous implementation, they get into situations where they feel they need to set the filesystem type to one they don't want in order to reset the label
> to what it was originally so that it won't be changed, then change it back to the filesystem type they actually want. Little do they
> know that actually the label will be ignored in this case, so if they hadn't already saved their previous changes, they are OK.
> And there are lots of other, even more complicated issues, like this one.
>
> I thought that existence was a property that wouldn't change if the user changed the format, but I now realize that that is almost certainly false.
>
> I believe that it would be very possible for the disk to be already formatted for one filesystem and then if the user changes the format, exists == True would change to exists == False.
>
> So, I'm going to be working on that problem, separately in association with the original labeling bug, #1038590, but push this fix.
Sounds good to me, thanks!
--
Vratislav Podzimek
Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic
More information about the anaconda-patches
mailing list