Niall,
I'm OK with all the responses here.
Not further changes required from me.
- Dermot
On 12/21/11 01:23, Niall Power wrote:
On 12/21/11 04:40 AM, Dermot McCluskey wrote:
Niall,
Review of GUI section:
*
GUI: Darren, Dermot
---------------------------*
*
*usr/src/cmd/gui-install/src/Makefile
usr/src/cmd/gui-install/src/confirm_screen.py
186...
FYI - the app will never see disk.whole_disk set to True
in the VTOC world: if you select a disk with TargetController
and set use_whole_disk=True, it will add a default layout
and set disk.whole_disk = False before passing back to
the app.
Yes. We observed that. For GPT things are fortunately a lot simpler in
this case....
It looks like you are handling things slightly differently
for GPT?
Yes. We can set whole disk on GPT and ZFS will take care of making it
bootable and creating necessary special partitions such as EFI system.
All we need to for GPT whole_disk mode is to invoke zpool with the new
"-B" option that we asked the ZFS folks to add.
It means we can also correctly identify a whole disk installation in
the summary screen since whole disk will always be GPT on X86
Also, if you move 202-203 to before 185, then you can
combine the two separate "if" statements at lines 186 and 205.
You're right! The logic does look a bit overcomplicated :)
usr/src/cmd/gui-install/src/disk_panel.py
usr/src/cmd/gui-install/src/disk_screen.py
is line 147 needed, given line 150?
No, it's not. I think when I first wrote that code I didn't initialise
it in there and then changed it later and must have forgotten to
remove the declaration. I'll remove it.
usr/src/cmd/gui-install/src/fdisk_panel.py
126 builder.add_from_file(GLADE_DIR + "/" + FDISK_GLADE)
Why not do this in ScreenManager.__init__(), like all the other
Glade files? Also, connecting reset button(s) could be done
in ScreenManager, as before?
Same goes for gpt_panel.py.
Because I want to have fully self contained, blackbox like behaviour
for the two custom partitioning panels to the maximum extent possible.
Placing the the initialisation inside ScreenManager goes counter to
this because it creates references into the internal workings of
fdisk_panel and gpt_panel.
usr/src/cmd/gui-install/src/gpt_panel.py
Did you have a look at refactoring this code so some of it
could be shared with fdisk_panel.py, eg by moving it to
disk_panel.py?
It seems a lot of it is identical.
I did but I concluded that it would just cause more trouble than it
was worth. We went through several iterations of layout and
behavioural revisions to the GPT screen before arriving at the current
solution. From this point of view having a separate code base was
preferable, but
I did try to replicate the code as much as possible to make the two
more consistent and maintain bug parity ;)
Having common code in disk_panel.py would mean a change to one could
have impact on the other. The goal with disk_panel.py was simply
to define an interface that both gpt_panel and fdisk_panel should
implement so that the parent disk_screen doesn't need to care which
one it is talking to as long as the interfaces it implements are
consistent. The idea I had was to do something similar to Javas
"implements" interface.
I'd also like to structure the code in such a way that we can simply
discard Fdisk in the future, or even add a new partitioning scheme if
GPT doesn't last until my retirement :-)
usr/src/cmd/gui-install/src/screen_manager.py
usr/src/cmd/gui-install/xml/Makefile*
*usr/src/pkg/manifests/system-install-gui-install.mf *
*
*
I seriously wouldn't bother code reviewing these glade
auto-generated XML files but if you enjoy pulling your eyes out :)*
I took a quick look, anyway:
did you mean to remove/not add CDDL headers?
Good catch, thanks for looking anway. I will add those back in. Doh.
usr/src/cmd/gui-install/xml/fdisk-panel.xml
usr/src/cmd/gui-install/xml/gpt-panel.xml
usr/src/cmd/gui-install/xml/installationdisk.xml
375 - when renamingcustompartioningalignment
the "i" in partition was removed:custompartitonalignment
Wow - check out the laser eyes on you. Well spotted ;)
Thanks,
Niall
- Dermot
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
