Re: [Qemu-devel] [PATCH 3/3] ui/cocoa.m: Add Reset and Power Down menu items to Machine menu

[Peter Maydell]

On 11 May 2015 at 23:18, Programmingkid <address@hidden> wrote:
> Adds Reset and Power Down menu items to the Machine menu.
>
> Signed-off-by: John Arbuckle <address@hidden>

This patch mostly looks good and pretty straightforward.
If you put it as #2 in this series then we can get it into
master even if we need to have another review-go-around
about the device eject/mediachange code.

>
> ---
>  ui/cocoa.m |   24 ++++++++++++++++++++++++
>  1 files changed, 24 insertions(+), 0 deletions(-)
>
> diff --git a/ui/cocoa.m b/ui/cocoa.m
> index 5e558ea..2c4a61a 100644
> --- a/ui/cocoa.m
> +++ b/ui/cocoa.m
> @@ -828,6 +828,8 @@ QemuCocoaView *cocoaView;
>  - (void)removePause;
>  - (void)ejectDeviceMedia:(id)sender;
>  - (void)changeDeviceMedia:(id)sender;
> +- (void)restartQemu:(id)sender;

"QEMU"

> +- (void)powerDown:(id)sender;

Why doesn't this method name have "QEMU" in it?

>  @end
>
>
>
>  @implementation QemuCocoaAppController
> @@ -1064,6 +1066,18 @@ QemuCocoaView *cocoaView;
>      }
>  }
>
>
>
> +/* Restarts QEMU */
> +- (void)restartQemu:(id)sender
> +{
> +    qemu_system_reset_request();
> +}
> +
> +/* Powers down the emulator */
> +- (void)powerDown:(id)sender
> +{
> +    qmp_system_powerdown(NULL);
> +}

This is inconsistent -- we should use qmp_system_reset(NULL),
which is then doing the same as the GTK UI and means we're
using QMP functions for both reset and powerdown.


> +
>  @end
>
>
>
>
>
> @@ -1202,6 +1216,15 @@ static void addDeviceMenuItems(NSMenu * menu)
>      }
>  }
>
>
>
> +// Adds the Reset and Power Down menu items to the specifed menu

"specified"

> +static void addResetPowerDownMenuItems(NSMenu* menu)
> +{
> +    [menu addItem: [NSMenuItem separatorItem]];
> +    [menu addItem: [[[NSMenuItem alloc] initWithTitle: @"Reset" action:
> @selector(restartQemu:) keyEquivalent: @""] autorelease]];
> +    [menu addItem: [[[NSMenuItem alloc] initWithTitle: @"Power Down"
> action: @selector(powerDown:) keyEquivalent: @""] autorelease]];
> +}
> +
> +
>  /*
>     Adds the Machine menu to the menu bar.
>     Has to be added separately because QEMU needs
> @@ -1222,6 +1245,7 @@ static void createMachineMenu()
>      [[NSApp mainMenu] insertItem: menuItem atIndex: 2]; // Insert after
> View menu
>      [[menu itemWithTitle: @"Resume"] setEnabled: NO];  // Disables the
> Resume menu item because it isn't needed right now.
>      addDeviceMenuItems(menu);
> +    addResetPowerDownMenuItems(menu);
>  }

thanks
-- PMM