[virt-tools-list] [PATCH 2/2] details: Add USB redirection support in console viewer

[Guannan Ren]

On 06/25/2013 12:14 AM, Leonardo Augusto Guimarães Garcia wrote:
> Comments below.
>
>
>>
>>       def _init_widget(self):
>>           self.set_grab_keys()
>> @@ -594,6 +596,22 @@ class SpiceViewer(Viewer):
>>               return
>>           self.display.set_property("scaling", scaling)
>>
>> +    def get_usb_widget(self):
>> +
>> +        # The @format positional parameters are the following:
>> +        # 1 '%s' manufacturer
>> +        # 2 '%s' product
>> +        # 3 '%s' descriptor (a [vendor_id:product_id] string)
>> +        # 4 '%d' bus
>> +        # 5 '%d' address
>> +
>> +        usb_device_description_fmt = _("%s %s %s at %d-%d")
> This is the default string. We can pass None instead of redefining it 
> here.

        Yeah, I tried it before,  the python binding API doesn't accept 
None for this argument.
        widget = SpiceClientGtk.UsbDeviceWidget.new(self.session, None)
        Error like: TypeError: Argument 1 does not allow None as a value

>> + spice_usbdev_widget, spice_usb_device,
>> +                                   errstr):
>> +        ignore_widget = spice_usbdev_widget
>> +        ignore_device = spice_usb_device
>> +
>> +        self.err.show_err(_("USB redirection error"),
>> +                         text2=str(errstr),
>> +                         async=False)
>> +
>> +    def control_vm_usb_redirection(self, src):
>> +        ignore = src
>> +        spice_usbdev_dialog = self.err
> I don't think we need this variable here as you are referencing 
> self.err directly below. You should either use self.err or 
> spice_usbdev_dialog in bellow references.

            the variable just help to make code more readable no other 
meanings.


>> +
>> +        spice_usbdev_widget = self.console.viewer.get_usb_widget()
>> +        if not spice_usbdev_widget:
>> +            self.err.show_err(_("Error initialize spice USB device 
>> widget"))
> Error initializing spice USB device widget
>> +            return
>> +
>> +        spice_usbdev_widget.connect("connect-failed",
>> + self.spice_usbdev_rediret_error)
>> +        spice_usbdev_widget.show()
> I think this is not necessary. When you call show on the info dialog, 
> all the widgets within it will be shown.

         Actually, we need this, dialog.show_all() will take this 
effect, but in error.py we use dialog.show() or nothing if sync is true.

         Thanks for this review.

         Guannan