Re: [PATCH 3/4] ui/icons: Use bundle mechanism

[Akihiko Odaki]

Hi,

Reverting commit e31746ecf8dd2f25f687c94ac14016a3ba5debfc solves the
problem only for cocoa and introduces another problem. (For others:
see 
797ADA26-0366-447F-85F0-5E27DC534479@gmail.com/">https://lore.kernel.org/qemu-devel/797ADA26-0366-447F-85F0-5E27DC534479@gmail.com/
for the context.) The fix for cocoa basically comes for free if you
fix the problem for other displays, and that's what this patch does.

Honestly I don't really like the nature of the code in the "configure"
script. It is a duplicate of ui/icons/meson.build and people may
overlook it when updating ui/icons/meson.build. I blindly followed
what the script does for pc-bios, but I can improve it for icons and
pc-bios by moving the responsibility to meson if it makes sense.

Regards,
Akihiko Odaki

On Fri, Jul 9, 2021 at 3:52 AM Programmingkid <programmingkidx@gmail.com> wrote:
>
>
> > On Jul 8, 2021, at 1:25 PM, Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
> >
> > Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
> > ---
> > configure   | 10 ++++++++++
> > meson.build |  3 +--
> > ui/cocoa.m  | 20 +++++++++++---------
> > ui/gtk.c    |  8 +++++---
> > ui/sdl2.c   | 18 +++++++++++-------
> > 5 files changed, 38 insertions(+), 21 deletions(-)
> >
> > diff --git a/configure b/configure
> > index cff5a8ac280..a9f746849ec 100755
> > --- a/configure
> > +++ b/configure
> > @@ -5058,6 +5058,16 @@ for f in $UNLINK ; do
> >     fi
> > done
> >
> > +for icon_extension in bmp png ; do
> > +  for icon_file in $source_path/ui/icons/qemu_*.$icon_extension ; do
> > +    icon_basename=${icon_file%.$icon_extension}
> > +    icon_name=${icon_basename#$source_path/ui/icons/qemu_}
> > +    icon_dir=qemu-bundle/share/icons/hicolor/$icon_name/apps
> > +    symlink $icon_file $icon_dir/qemu.$icon_extension
> > +  done
> > +done
> > +
> > +symlink $source_path/ui/icons/qemu.svg 
> > qemu-bundle/share/icons/hicolor/scalable/apps/qemu.svg
> > symlink ../../pc-bios qemu-bundle/share/qemu
> >
> > (for i in $cross_cc_vars; do
> > diff --git a/meson.build b/meson.build
> > index 44adc660e23..6d90fe92bf1 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -1200,8 +1200,7 @@ config_host_data.set_quoted('CONFIG_QEMU_CONFDIR', 
> > get_option('prefix') / qemu_c
> > config_host_data.set_quoted('CONFIG_QEMU_BUNDLE_DATADIR', qemu_datadir)
> > config_host_data.set_quoted('CONFIG_QEMU_DESKTOPDIR', get_option('prefix') 
> > / qemu_desktopdir)
> > config_host_data.set_quoted('CONFIG_QEMU_FIRMWAREPATH', 
> > get_option('qemu_firmwarepath'))
> > -config_host_data.set_quoted('CONFIG_QEMU_HELPERDIR', get_option('prefix') 
> > / get_option('libexecdir'))
> > -config_host_data.set_quoted('CONFIG_QEMU_ICONDIR', get_option('prefix') / 
> > qemu_icondir)
> > +config_host_data.set_quoted('CONFIG_QEMU_BUNDLE_ICONDIR', qemu_icondir)
> > config_host_data.set_quoted('CONFIG_QEMU_LOCALEDIR', get_option('prefix') / 
> > get_option('localedir'))
> > config_host_data.set_quoted('CONFIG_QEMU_LOCALSTATEDIR', 
> > get_option('prefix') / get_option('localstatedir'))
> > config_host_data.set_quoted('CONFIG_QEMU_MODDIR', get_option('prefix') / 
> > qemu_moddir)
> > diff --git a/ui/cocoa.m b/ui/cocoa.m
> > index 9f72844b079..d459dfaf595 100644
> > --- a/ui/cocoa.m
> > +++ b/ui/cocoa.m
> > @@ -1477,15 +1477,17 @@ - (void)make_about_window
> >     NSRect picture_rect = NSMakeRect(x, y, picture_width, picture_height);
> >
> >     /* Make the picture of QEMU */
> > -    NSImageView *picture_view = [[NSImageView alloc] initWithFrame:
> > -                                                     picture_rect];
> > -    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];
> > +    char *qemu_image_path_c = find_bundle(CONFIG_QEMU_BUNDLE_ICONDIR 
> > "/hicolor/512x512/apps/qemu.png");
> > +    if (qemu_image_path_c) {
> > +        NSString *qemu_image_path = [NSString 
> > stringWithUTF8String:qemu_image_path_c];
> > +        g_free(qemu_image_path_c);
> > +        NSImageView *picture_view = [[NSImageView alloc] initWithFrame:
> > +                                                         picture_rect];
> > +        NSImage *qemu_image = [[NSImage alloc] 
> > initWithContentsOfFile:qemu_image_path];
> > +        [picture_view setImage: qemu_image];
> > +        [picture_view setImageScaling: NSImageScaleProportionallyUpOrDown];
> > +        [superView addSubview: picture_view];
> > +    }
> >
> >     /* Make the name label */
> >     NSBundle *bundle = [NSBundle mainBundle];
> > diff --git a/ui/gtk.c b/ui/gtk.c
> > index 98046f577b9..e250f9e18a0 100644
> > --- a/ui/gtk.c
> > +++ b/ui/gtk.c
> > @@ -2198,9 +2198,11 @@ static void gtk_display_init(DisplayState *ds, 
> > DisplayOptions *opts)
> >     s->opts = opts;
> >
> >     theme = gtk_icon_theme_get_default();
> > -    dir = get_relocated_path(CONFIG_QEMU_ICONDIR);
> > -    gtk_icon_theme_prepend_search_path(theme, dir);
> > -    g_free(dir);
> > +    dir = find_bundle(CONFIG_QEMU_BUNDLE_ICONDIR);
> > +    if (dir) {
> > +        gtk_icon_theme_prepend_search_path(theme, dir);
> > +        g_free(dir);
> > +    }
> >     g_set_prgname("qemu");
> >
> >     s->window = gtk_window_new(GTK_WINDOW_TOPLEVEL);
> > diff --git a/ui/sdl2.c b/ui/sdl2.c
> > index a203cb0239e..7b7ed9f9065 100644
> > --- a/ui/sdl2.c
> > +++ b/ui/sdl2.c
> > @@ -877,15 +877,19 @@ static void sdl2_display_init(DisplayState *ds, 
> > DisplayOptions *o)
> >     }
> >
> > #ifdef CONFIG_SDL_IMAGE
> > -    dir = get_relocated_path(CONFIG_QEMU_ICONDIR 
> > "/hicolor/128x128/apps/qemu.png");
> > -    icon = IMG_Load(dir);
> > +    dir = find_bundle(CONFIG_QEMU_BUNDLE_ICONDIR 
> > "/hicolor/128x128/apps/qemu.png");
> > +    if (dir) {
> > +        icon = IMG_Load(dir);
> > +    }
> > #else
> >     /* Load a 32x32x4 image. White pixels are transparent. */
> > -    dir = get_relocated_path(CONFIG_QEMU_ICONDIR 
> > "/hicolor/32x32/apps/qemu.bmp");
> > -    icon = SDL_LoadBMP(dir);
> > -    if (icon) {
> > -        uint32_t colorkey = SDL_MapRGB(icon->format, 255, 255, 255);
> > -        SDL_SetColorKey(icon, SDL_TRUE, colorkey);
> > +    dir = find_bundle(CONFIG_QEMU_BUNDLE_ICONDIR 
> > "/hicolor/32x32/apps/qemu.bmp");
> > +    if (dir) {
> > +        icon = SDL_LoadBMP(dir);
> > +        if (icon) {
> > +            uint32_t colorkey = SDL_MapRGB(icon->format, 255, 255, 255);
> > +            SDL_SetColorKey(icon, SDL_TRUE, colorkey);
> > +        }
> >     }
> > #endif
> >     g_free(dir);
> > --
> > 2.30.1 (Apple Git-130)
> >
>
> This is a lot of code for just loading an icon. I think it would be best to 
> simply revert commit e31746ecf8dd2f25f687c94ac14016a3ba5debfc instead.
>
> Thank you.
>