On Fri, Sep 25, 2009 at 01:49:44PM -1000, David Cantrell wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Comments inline.
>
> On Wed, 16 Sep 2009, Joel Granados Moreno wrote:
>
>> The reason behind this change is to make the code ready for LVM and
>> RAID.
>>
>> * iw/partition_gui.py (Slice, Stripe, StripeGraph): New classes that
>> replace the old ones. These new clases do not depend on partition
>> objects. The only accept offsets and lengths.
>> (Slice.eventHandler): Introduce a oneClick and doubleClick call back
...
>> - if self.partedPartition.type & parted.PARTITION_EXTENDED:
>> - return ""
>> - if self.partedPartition.type & parted.PARTITION_FREESPACE:
>> - rc = "Free\n"
>> - else:
>> - rc = "%s\n" % (self.partedPartition.getDeviceNodeName().split("/")[-1],)
>> - rc = rc + "%Ld MB" % (self.partedPartition.getSize(unit="MB"),)
>> - return rc
>> -
>> - def update(self):
>> - disk = self.parent.getDisk()
>> - (cylinders, heads, sectors) = disk.device.biosGeometry
>> - totalSectors = float(heads * sectors * cylinders)
>> -
>> - # XXX hack but will work for now
>> - if gtk.gdk.screen_width() > 640:
>> - width = CANVAS_WIDTH_800
>> - else:
>> - width = CANVAS_WIDTH_640
>> + return True
>>
>> - # If it's a very, very small partition then there's no point in trying
>> - # cut off a piece of the parent disk's stripe for it.
>> - if totalSectors == 0:
>> - return
>> + def put_on_canvas(self):
>
> Style: All other methods in these classes are named with CamelCase, except
> this one.
>
Done
>> + pgroup = self.parent.getGroup()
>> + self.group = pgroup.add(gnomecanvas.CanvasGroup)
>> + self.box = self.group.add(gnomecanvas.CanvasRect)
>> + self.group.connect("event", self.eventHandler)
>> + canvas_text = self.group.add(gnomecanvas.CanvasText,
>> + font="sans", size_points=8)
....
>> - width = CANVAS_WIDTH_640
>> -
>> - group.add(gnomecanvas.CanvasRect, x1=0.0, y1=10.0, x2=width,
>> + def put_on_canvas(self, yoff):
>
> Style again here.
yep, done.
>
>> + """
>> + returns the yposition after drawhing this stripe.
>> +
>> + """
....
>> + # Define the default colors per partition type.
>> + self.part_type_colors = \
>> + {"sel_logical": "cornsilk1", "unsel_logical": "white",
>> + "sel_extended": "cornsilk1", "unsel_extended": "white",
>> + "sel_normal": "cornsilk1", "unsel_normal": "white",
>> + "sel_freespace": "grey88", "unsel_freespace": "grey88"}
>
> The color settings here... perhaps they should be provided by some sort of
> theme class that lives somewhere else. We may want to adjust these things
> later on for RHEL or if Fedora decides to change themes. Other distributions
> might like an easier way to change the theme settings for the storage UI.
> Just a thought.
Yes, This is definitely a good idea. Something for the future though.
>
>> + if drive:
>> + self.setDisplayed(drive)
>> +
>> + def _createStripe(self, drive):
>> + # Create the stripe
>> + drivetext = _("Drive %s (%-0.f MB) (Model: %s)") % (drive.path,
>> + drive.size, drive.model)
>> + stripe = Stripe(self.canvas, drivetext, self.editCB, obj = drive)
>> +
...
>> + # Create the start and length for the slice.
>> + xoffset = float(part.geometry.start) \
>> + / float(drive.partedDevice.length)
>> + xlength = float(part.geometry.length) \
>> + / float(drive.partedDevice.length)
>
> Style: Division across lines leads to the weird \ / stuff and I'd rather go
> beyond 80 characters on the line than have the line continuation marker next
> to the division operator.
Hrm. I still very much dislike the 80 character barrier being exceeded.
I'll enclose them in parentheses and get rid of the "\ /".
>
>> +
>> + # We need to use the self.storage objects not the partedDisk ones.
>> + # The free space has not storage object.
>> + if part.type != parted.PARTITION_FREESPACE:
>> + partName = devicePathToName(part.getDeviceNodeName())
>> + o_part = self.storage.devicetree.getDeviceByName(partName)
>> + dcCB = self.editCB
...
>>
>> def deleteCB(self, widget):
>> """ Right now we can say that if the device is partitioned we
>> @@ -1477,7 +1535,8 @@ class PartitionWindow(InstallWindow):
>> self.treeView.connect('row-activated', self.treeActivateCB)
>> self.treeViewSelection = self.treeView.get_selection()
>> self.treeViewSelection.connect("changed", self.treeSelectCB)
>> - self.diskStripeGraph = DiskStripeGraph(self.tree, self.editCB)
>> + self.diskStripeGraph = DiskStripeGraph(self.tree, self.editCB,
>> + storage = self.storage)
>> self.populate(initial = 1)
>>
>> # Create the top scroll window
>>
>
> I didn't see any issues as I walked through this code. Aside from the style
> comments and the theme comment, the rest looks fine.
>
--
Joel Andres Granados
Brno, Czech Republic, Red Hat.
Attachment:
pgpbqf2SywabD.pgp
Description: PGP signature
_______________________________________________ Anaconda-devel-list mailing list Anaconda-devel-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/anaconda-devel-list
