Re: [Qemu-devel] [PATCH v2] ui/cocoa.m: Adds device menu items to Machine menu

[Programmingkid]

On Jun 14, 2015, at 1:11 PM, Peter Maydell wrote:

> On 18 May 2015 at 17:23, Programmingkid <address@hidden> wrote:
>> Adds all removable devices to the Machine menu as a Change and Eject menu
>> item pair. ide-cd0 would have a "Change ide-cd0..." and "Eject ide-cd0"
>> menu items.
>> 
>> Signed-off-by: John Arbuckle <address@hidden>
> 
> I'm afraid this one still needs a bit more work on the
> detail, though I think it's OK in general approach.
> 
>> ---
>> Replace NSRunAlertPanel() with QEMU_Alert().
>> Free currentDevice variable after finished using it.
>> Add "Removable Media" text to the top of devices menu items for easier
>> identification.
>> Replace depreciated code.
>> 
>> ui/cocoa.m |  140
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 files changed, 140 insertions(+), 0 deletions(-)
>> 
>> diff --git a/ui/cocoa.m b/ui/cocoa.m
>> index 559058b..3acde50 100644
>> --- a/ui/cocoa.m
>> +++ b/ui/cocoa.m
>> @@ -30,6 +30,7 @@
>> #include "ui/input.h"
>> #include "sysemu/sysemu.h"
>> #include "qmp-commands.h"
>> +#include "sysemu/blockdev.h"
>> 
>> 
>> 
>> #ifndef MAC_OS_X_VERSION_10_5
>> #define MAC_OS_X_VERSION_10_5 1050
>> @@ -242,7 +243,27 @@ static int cocoa_keycode_to_qemu(int keycode)
>>     return keymap[keycode];
>> }
>> 
>> 
>> 
>> +/* Displays an alert dialog box with the specified message */
>> +static void QEMU_Alert(NSString *message)
>> +{
>> +    NSAlert *alert;
>> +    alert = [NSAlert alertWithMessageText:message
>> +                    defaultButton:@"OK"
>> +                  alternateButton:nil
>> +                      otherButton:nil
>> +        informativeTextWithFormat:@""];
> 
> This gives a deprecation warning on 10.10:
> 
> /Users/pm215/src/qemu/ui/cocoa.m:250:22: warning:
>      
> 'alertWithMessageText:defaultButton:alternateButton:otherButton:informativeTextWithFormat:'
> is deprecated: first
>      deprecated in OS X 10.10 - Use -init instead [-Wdeprecated-declarations]
>    alert = [NSAlert alertWithMessageText:message
>                     ^
> /System/Library/Frameworks/AppKit.framework/Headers/NSAlert.h:70:1: note:
>      
> 'alertWithMessageText:defaultButton:alternateButton:otherButton:informativeTextWithFormat:'
> has been explicitly marked
>      deprecated here
> + (NSAlert *)alertWithMessageText:(NSString *)message
> defaultButton:(NSString *)defaultButton alternateButton:(NSStri...
> ^

Apple is becoming depreciation crazy. Ok, will fix this problem.


>> +    [alert runModal];
>> +}
>> 
>> 
> 
>> +/* Displays a dialog box asking the user to select an image file to load.
>> + * Uses sender's tag value to figure out which drive to use.
>> + */
>> +- (void)changeDeviceMedia:(id)sender
>> +{
>> +    /* Find the drive name */
>> +    NSString * drive;
>> +    drive = [sender representedObject];
>> +    if(drive == nil) {
>> +        NSBeep();
>> +        QEMU_Alert(@"Could not find drive!");
>> +        return;
>> +    }
>> +
>> +    /* Display the file open dialog */
>> +    NSOpenPanel * openPanel;
>> +    openPanel = [NSOpenPanel openPanel];
>> +    [openPanel setCanChooseFiles: YES];
>> +    [openPanel setAllowsMultipleSelection: NO];
>> +    [openPanel setAllowedFileTypes: [NSArray arrayWithObjects: @"iso",
>> @"cdr", @"img", @"dmg", nil]];
> 
> This list is too short (no qcow2, among other things) and doesn't match
> the one we use for the initial image. Ideally we should use the same
> code for "let user pick initial disk image" and "let user pick new
> image for this drive".

Is this the complete list you want: iso, cdr, img, dmg, cow, qcow, qcow2, vmdk, 
cloop, vpc, vdi

> 
>> +    if([openPanel runModal] == NSFileHandlingPanelOKButton) {
>> +        NSString * file = [[openPanel filenames] objectAtIndex: 0];
> 
> Another deprecation warning:
> /Users/pm215/src/qemu/ui/cocoa.m:1112:39: warning: 'filenames' is
> deprecated: first deprecated in OS X 10.6
>      [-Wdeprecated-declarations]
>        NSString * file = [[openPanel filenames] objectAtIndex: 0];
>                                      ^
> /System/Library/Frameworks/AppKit.framework/Headers/NSOpenPanel.h:51:1:
> note: 'filenames' has been explicitly marked
>      deprecated here
> - (NSArray *)filenames NS_DEPRECATED_MAC(10_0, 10_6);
> ^
> 

Why Apple why? Ok, I will see what I can do for this issue.

> 
> 
>> +        Error *err = NULL;
>> +        qmp_change_blockdev([drive cStringUsingEncoding:
>> NSASCIIStringEncoding], [file cStringUsingEncoding: NSASCIIStringEncoding],
>> "raw", &err);
>> +        handleAnyDeviceErrors(err);
>> +    }
>> +}
>> +
>> @end
>> 
>> 
>> 
>> 
>> 
>> @@ -1260,6 +1329,71 @@ static void add_console_menu_entries(void)
>>     }
>> }
>> 
>> 
>> 
>> +/* Make menu items for all removable devices.
>> + * Each device is given an 'Eject' and 'Change' menu item.
>> + */
>> +static void addRemovableDevicesMenuItems()
>> +{
>> +    NSMenu *menu;
>> +    NSMenuItem *menuItem;
>> +    BlockInfoList *currentDevice;
>> +    NSString *deviceName;
>> +
>> +    currentDevice = qmp_query_block(NULL);
>> +    if(currentDevice == NULL) {
>> +        NSBeep();
>> +        QEMU_Alert(@"Failed to query for block devices!");
>> +        return;
>> +    }
>> +
>> +    menu = [[[NSApp mainMenu] itemWithTitle:@"Machine"] submenu];
>> +
>> +    // Add a separator between related groups of menu items
>> +    [menu addItem:[NSMenuItem separatorItem]];
>> +
>> +    // Set the attributes to the "Removable Media" menu item
>> +    NSString *titleString = @"Removable Media";
>> +    NSMutableAttributedString *attString=[[NSMutableAttributedString alloc]
>> initWithString:titleString];
>> +    NSColor *newColor = [NSColor blackColor];
>> +    NSFontManager *fontManager = [NSFontManager sharedFontManager];
>> +    NSFont *font = [fontManager fontWithFamily:@"Helvetica"
>> +
>> traits:NSBoldFontMask|NSItalicFontMask
>> +                                          weight:0
>> +                                            size:14];
>> +    [attString addAttribute:NSFontAttributeName value:font
>> range:NSMakeRange(0, [titleString length])];
>> +    [attString addAttribute:NSForegroundColorAttributeName value:newColor
>> range:NSMakeRange(0, [titleString length])];
>> +    [attString addAttribute:NSUnderlineStyleAttributeName value:[NSNumber
>> numberWithInt: 1] range:NSMakeRange(0, [titleString length])];
> 
> This italicised line looks really odd in the menu -- is it really
> the way apple's UI guidelines recommend doing this sort of thing?
> (It seems like quite a lot of effort to have to go to.)

It required a huge amount of effort on my part. But it did look really good, so 
I really hope you let me keep it :)

> 
>> +
>> +    // Add the "Removable Media" menu item
>> +    menuItem = [NSMenuItem new];
>> +    [menuItem setAttributedTitle: attString];
>> +    [menuItem setEnabled: NO];
>> +    [menu addItem: menuItem];
>> +
>> +    /* Loop thru all the block devices in the emulator */
>> +    while (currentDevice) {
>> +        deviceName = [[NSString stringWithFormat: @"%s",
>> currentDevice->value->device] retain];
>> +
>> +        if(currentDevice->value->removable) {
>> +            menuItem = [[NSMenuItem alloc] initWithTitle: [NSString
>> stringWithFormat: @"Change %s...", currentDevice->value->device]
>> +                                                  action:
>> @selector(changeDeviceMedia:)
>> +                                           keyEquivalent: @""];
>> +            [menu addItem: menuItem];
>> +            [menuItem setRepresentedObject: deviceName];
>> +            [menuItem autorelease];
>> +
>> +            menuItem = [[NSMenuItem alloc] initWithTitle: [NSString
>> stringWithFormat: @"Eject %s", currentDevice->value->device]
>> +                                                  action:
>> @selector(ejectDeviceMedia:)
>> +                                           keyEquivalent: @""];
>> +            [menu addItem: menuItem];
>> +            [menuItem setRepresentedObject: deviceName];
>> +            [menuItem autorelease];
>> +        }
>> +        currentDevice = currentDevice->next;
>> +    }
>> +    free(currentDevice);
> 
> currentDevice will usually be NULL here because you've used
> it as your iterator variable. In any case this
> isn't the right way to free the pointer you get back from
> qmp_query_block() -- you need qapi_free_BlockInfoList().

qapi_free_BlockInfoList(currentDevice);
This is what you want? 


> 
>> +}
>> +
>> void cocoa_display_init(DisplayState *ds, int full_screen)
>> {
>>     COCOA_DEBUG("qemu_cocoa: cocoa_display_init\n");
>> @@ -1283,4 +1417,10 @@ void cocoa_display_init(DisplayState *ds, int
>> full_screen)
>>      * menu entries for them.
>>      */
>>     add_console_menu_entries();
>> +
>> +    /* Give all removable devices a menu item.
>> +     * Has to be called after QEMU has started to
>> +     * find out what removable devices it has.
>> +     */
>> +    addRemovableDevicesMenuItems();
>> }
>> --
>> 1.7.5.4
> 
> thanks
> -- PMM

Thank you for taking the time to review my patches. I will implement your 
changes as soon as I receive your reply.