[virt-tools-list] [virt-manager PATCH 3/7] details: Show attached disk information in scsi controller page

[Cole Robinson]

On 11/06/2017 07:52 AM, Lin Ma wrote:
> It helps users to recognize controllers <-> disks mapping relationship.
> 
> Signed-off-by: Lin Ma <lma at suse.com>
> ---
>  ui/details.ui          | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  virtManager/details.py | 25 ++++++++++++++++++++++++-
>  2 files changed, 72 insertions(+), 1 deletion(-)
> 
> diff --git a/virtManager/details.py b/virtManager/details.py
> index 22e0786e..a9e0cb79 100644
> --- a/virtManager/details.py
> +++ b/virtManager/details.py
> @@ -1042,6 +1042,16 @@ class vmmDetails(vmmGObjectUI):
>          uiutil.init_combo_text_column(combo, 1)
>          combo.set_active(-1)
>  
> +        attached_disk_list = self.widget("scsi-disk-list")
> +        model = Gtk.ListStore(str)
> +        attached_disk_list.set_model(model)
> +        attached_disk_list.set_headers_visible(False)
> +        diskTextCol = Gtk.TreeViewColumn()
> +        disk_txt = Gtk.CellRendererText()
> +        diskTextCol.pack_start(disk_txt, True)
> +        diskTextCol.add_attribute(disk_txt, 'text', 0)
> +        attached_disk_list.append_column(diskTextCol)
> +
>  

Can you generalize all the naming here? This UI could easily be reused
for all attached controller devices, let's not add 'disk' into all the
naming that just needs to be changed later. So "controller-device-list"
for the UI name, just name the variables combo/model/col/text here since
it's just init code.

Also in the UI change the label from 'Disks:' to 'Devices:'

>      ##########################
>      # Window state listeners #
> @@ -2995,17 +3005,30 @@ class vmmDetails(vmmGObjectUI):
>          if not dev:
>              return
>  
> +        self.widget("disk-list-label").set_visible(False)
> +        self.widget("scsi-disk-box").set_visible(False)

Change these names too. And you should be able to use
uiutil.set_grid_row_visible(self.widget("XXX"), False) to hide the UI

>          can_remove = True
>          if self.vm.get_xmlobj().os.is_x86() and dev.type == "usb":
>              can_remove = False
>          if dev.type == "pci":
>              can_remove = False
>          if dev.type == "scsi":
> +            model = self.widget("scsi-disk-list").get_model()
> +            model.clear()
>              for disk in self.vm.get_disk_devices(inactive=True):
>                  if (dev.type == disk.bus and
>                      dev.index == disk.address.controller):

Should have mentioned in the first patch. I think you should be checking
disk.address.type here instead of disk.bus. Doesn't matter in practice
but it more correctly identifies what you are checking for.

Also if you move this to a function in VirtualDeviceAddress like
compare_controller, we can use a central place to store logic for other
devices as well.

>                      can_remove = False
> -                    break
> +                    name = _label_for_device(disk)
> +                    bus = disk.address.bus
> +                    target = disk.address.target
> +                    unit = disk.address.unit
> +                    infoStr = ("%s:  bus = %s, target = %s, unit = %s" %
> +                               (name, bus, target, unit))
> +                    model.append([infoStr])

Can we just use _label_for_device instead of inventing a new label
format? If that's no sufficient and the full address is desired I'd
rather put some address pretty printing into virtinst and use that instead.

> +            self.widget("disk-list-label").set_visible(not can_remove)
> +            self.widget("scsi-disk-box").set_visible(not can_remove)
> +

I say always show the UI for scsi controllers, even if they have no
disks attached.

Thanks,
Cole