Re: [Qemu-devel] [PATCH 1/3] ui/cocoa.m: adds Machine menu with pause and resume menu items

[Peter Maydell]

On 11 May 2015 at 23:12, Programmingkid <address@hidden> wrote:
> Adds a Machine menu to the Macintosh interface that pause and resume menu

"with pause and resume menu items"

> items.
> These items can either pause or resume execution of the guest operating
> system.
>
> Signed-off-by: John Arbuckle <address@hidden>

This mostly looks good. A minor note: your commit messages would
fit better with the usual style if you phrased them as "Add foo",
"Improve bar", etc, rather than "Adds", "Improves", etc. Wrapping
lines between 70-75 characters would be nice too (you can see the
lines have wrapped badly above, for instance).

Could you rebase these patches on top of my cocoa.next branch,
please?

> ---
>  ui/cocoa.m |   79
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 79 insertions(+), 0 deletions(-)
>
> diff --git a/ui/cocoa.m b/ui/cocoa.m
> index d37c29b..7a9c194 100644
> --- a/ui/cocoa.m
> +++ b/ui/cocoa.m
> @@ -29,6 +29,7 @@
>  #include "ui/console.h"
>  #include "ui/input.h"
>  #include "sysemu/sysemu.h"
> +#include "qmp-commands.h"
>
>
>
>  #ifndef MAC_OS_X_VERSION_10_4
>  #define MAC_OS_X_VERSION_10_4 1040
> @@ -64,6 +65,7 @@ static int last_buttons;
>
>
>
>  int gArgc;
>  char **gArgv;
> +NSTextField * pauseLabel;

No need for a space after the '*'.

>  // keymap conversion
>  int keymap[] =
> @@ -801,6 +803,10 @@ QemuCocoaView *cocoaView;
>  - (void)toggleFullScreen:(id)sender;
>  - (void)showQEMUDoc:(id)sender;
>  - (void)showQEMUTec:(id)sender;
> +- (void)pauseQemu:(id)sender;
> +- (void)resumeQemu: (id) sender;

Style nit -- can you be consistent with the spacing here?
Also QEMU should be all-caps.

> +/*
> +   Adds the Machine menu to the menu bar.
> +   Has to be added separately because QEMU needs
> +   to be running to determine used items.
> +*/

 /* This is the usual
  * format for comments that
  * span multiple lines.
  */

> +static void createMachineMenu()
> +{
> +    NSMenu * menu;
> +    NSMenuItem * menuItem;
> +
> +    // Machine menu
> +     menu = [[NSMenu alloc] initWithTitle: @"Machine"];
> +    [menu setAutoenablesItems: NO];
> +    [menu addItem: [[[NSMenuItem alloc] initWithTitle: @"Pause" action:
> @selector(pauseQemu:) keyEquivalent: @""] autorelease]];
> +    [menu addItem: [[[NSMenuItem alloc] initWithTitle: @"Resume" action:
> @selector(resumeQemu:) keyEquivalent: @""] autorelease]];
> +    menuItem = [[[NSMenuItem alloc] initWithTitle: @"Machine" action:nil
> keyEquivalent:@""] autorelease];
> +    [menuItem setSubmenu:menu];
> +    [[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.
> +}

If we use the approach I described in the previous patch
where all the static parts of menus are created initially
and then dynamic parts added later, we can do this all
in the same place as the other menus are created.

thanks
-- PMM