Re: [Qemu-devel] [PATCH 3/3] gtk: add devices menu to allow changing removable block devices

[Anthony Liguori]

Kevin Wolf <address@hidden> writes:

> Am 02.05.2013 um 15:41 hat Anthony Liguori geschrieben:
>> Kevin Wolf <address@hidden> writes:
>> 
>> > Am 26.04.2013 um 21:43 hat Anthony Liguori geschrieben:
>> >> +static void gd_block_device_menu_update(BlockDeviceMenu *bdm, BlockInfo 
>> >> *info)
>> >> +{
>> >> +    bool value;
>> >> +    const char *label = _("<No media>");
>> >> +
>> >> +    value = info->has_inserted && !info->locked;
>> >
>> > Shouldn't the actual value of info->inserted play a role as well?
>> 
>> inserted contains information about the inserted disk but doesn't
>> contain a boolean to indicate that the device is inserted.
>> 
>> My understanding is that the existance of the inserted structure is what
>> indicates that the device is inserted.
>
> Sorry, my bad, I should have looked up the schema. I was indeed assuming
> that it's a boolean.
>
>> >> +static void gd_enum_disk(const char *path, const char *proptype, void 
>> >> *opaque)
>> >> +{
>> >> +    GtkDisplayState *s = opaque;
>> >> +    Object *obj;
>> >> +    char *block_id;
>> >> +
>> >> +    obj = object_resolve_path(path, NULL);
>> >> +    g_assert(obj != NULL);
>> >> +
>> >> +    block_id = object_property_get_str(obj, proptype, NULL);
>> >> +    if (strcmp(block_id, "") != 0) {
>> >> +        BlockDeviceMenu *bdm;
>> >> +        DiskType disk_type;
>> >> +        char *type;
>> >> +        char *desc = NULL;
>> >> +
>> >> +        type = object_property_get_str(obj, "type", NULL);
>> >> +
>> >> +        if (strcmp(type, "ide-cd") == 0 || strcmp(type, "ide-hd") == 0) {
>> >> +            desc = object_property_get_str(obj, "drive-id", NULL);
>> >> +        } else {
>> >> +            desc = g_strdup(type);
>> >> +        }
>> >
>> > Ugh. Comparing the device name to an incomplete set of strings here and
>> > then figuring out for each what the specific way for this device is to
>> > create a nice string sounds like a bad idea.
>> >
>> > Why can't all devices just expose a property with a human-readable
>> > string? We'll need it for more than just the disk change menus.
>> 
>> I thought about this, there are a few concerns.  The first is that you
>> might lose consistency across devices.  The second is i18n.
>
> What's the kind of consistency that you lose? I guess I could see the
> point (even if not agree) if it was about creating the string here vs.
> in each device, as the centralised strings would be more likely to use
> the same pattern, but you already have this part in the IDE device.
>
> The i18n point I don't buy. Avoiding i18n by choosing cryptic device
> names that can't be translated isn't a good strategy. But if you do have
> translations, it doesn't matter whether you have them in the GUI or in
> the device itself, except that the latter could be used outside the
> GTK frontend, too.
>
>> I would like to show USB device separately from IDE devices (even if
>> it's a USB CDROM).  I want the menu to look something like this:
>> 
>> QEMU DVD-ROM QM003 ->
>> Floppy Disk        ->
>> ---------------------
>> USB Devices        ->
>>                       USB Tablet                       ->
>>                       -----------------------------------
>>                       Description of USB Host Device 1 ->
>>                       Description of USB Host Device 2 ->
>>                       Description of USB Host Device 3 ->
>> 
>> Such that you can also do USB host device pass through via the menus.
>> 
>> From an i18n point of view, I would expect 'Floppy Disk' to be
>> translated.  I wouldn't expect 'QEMU DVD-ROM QM003' to be translated
>> though since this is how the device is described within the guest.
>
> Note that there can be two floppy drives. Currently both will show up as
> "isa-fdc".

This is a bug.  My intention was to use block_id as the description, not
type.

Regards,

Anthony Liguori