Re: [PATCH] ui/cocoa: Do not rely on the first argument

[Peter Maydell <peter.maydell@linaro.org>]

On Tue, 23 Feb 2021 at 11:06, Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
>
> The first argument of the executable was used to get its path, but it is
> not reliable because the executer can specify any arbitrary string. Use the
> interfaces provided by QEMU and the platform to get those paths.
>
> This change also changes the icon shown in the "about" window to QEMU's
> cute one.

This patch seems to be doing two things at once:
 * we get the executable path via the OSX APIs rather than using argv[0]

 * we get an image from the icon directory rather than using the
   executable's built-in icon

Please don't put more than one change in the same patch: it makes it
harder to review.

Could you split this up so that there is one patch per change
(ie make it a 2-patch series), please ?

> @@ -1401,18 +1402,13 @@ - (void)make_about_window
>      y = about_height - picture_height - 10;
>      NSRect picture_rect = NSMakeRect(x, y, picture_width, picture_height);
>
> -    /* Get the path to the QEMU binary */
> -    NSString *binary_name = [NSString stringWithCString: gArgv[0]
> -                                      encoding: NSASCIIStringEncoding];
> -    binary_name = [binary_name lastPathComponent];
> -    NSString *program_path = [[NSString alloc] initWithFormat: @"%@/%@",
> -    [[NSBundle mainBundle] bundlePath], binary_name];
> -
>      /* Make the picture of QEMU */
>      NSImageView *picture_view = [[NSImageView alloc] initWithFrame:
>                                                       picture_rect];
> -    NSImage *qemu_image = [[NSWorkspace sharedWorkspace] iconForFile:
> -                                                         program_path];
> +    char *qemu_image_path_c = get_relocated_path(CONFIG_QEMU_ICONDIR "/hicolor/512x512/apps/qemu.png");
> +    NSString *qemu_image_path = [NSString stringWithUTF8String:qemu_image_path_c];
> +    g_free(qemu_image_path_c);
> +    NSImage *qemu_image = [[NSImage alloc] initWithContentsOfFile:qemu_image_path];
>      [picture_view setImage: qemu_image];
>      [picture_view setImageScaling: NSImageScaleProportionallyUpOrDown];
>      [superView addSubview: picture_view];
> @@ -1427,9 +1423,7 @@ - (void)make_about_window
>      [name_label setBezeled: NO];
>      [name_label setDrawsBackground: NO];
>      [name_label setAlignment: NSTextAlignmentCenter];
> -    NSString *qemu_name = [[NSString alloc] initWithCString: gArgv[0]
> -                                            encoding: NSASCIIStringEncoding];
> -    qemu_name = [qemu_name lastPathComponent];
> +    NSString *qemu_name = [[[NSBundle mainBundle] executablePath] lastPathComponent];

The API docs for NSBundle mainBundle say it can return nil and you
always have to check the return value.

https://developer.apple.com/documentation/foundation/nsbundle/1410786-mainbundle

It's not clear to me what the fallback should be if it does return nil...

>      [name_label setStringValue: qemu_name];
>      [superView addSubview: name_label];
>
> --
> 2.24.3 (Apple Git-128)

thanks
-- PMM