On Tue, Mar 1, 2016 at 10:54 PM, Jonathon Jongsma <jjongsma@xxxxxxxxxx> wrote:
> On Fri, 2016-02-26 at 23:38 +0100, Fabiano Fidêncio wrote:
>> Let's take advantage of GResource for loading ui files in a better and
>> cleaner way than virt_viewer_util_load_ui() was doing.
>> It also brings the benefit, at least for developers, of being able to
>> test ui changes without having to "make install" virt-viewer.
>>
>> Signed-off-by: Fabiano Fidêncio <fidencio@xxxxxxxxxx>
>> ---
>> src/Makefile.am | 25 ++++++++++++++++++----
>> src/virt-viewer-about.xml | 1 -
>> src/virt-viewer-app.c | 10 +++++++++
>> src/virt-viewer-util.c | 50 ++++++------------------------------------
>> -
>> src/virt-viewer-window.c | 20 +++++++++++++----
>> src/virt-viewer.gresource.xml | 19 ++++++++++++++++
>> 6 files changed, 73 insertions(+), 52 deletions(-)
>> create mode 100644 src/virt-viewer.gresource.xml
>>
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index f42a7bf..ce31f08 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -18,8 +18,10 @@ builderxml_DATA = \
>>
>> EXTRA_DIST = \
>> $(builderxml_DATA) \
>> + $(resource_files) \
>> virt-viewer-enums.c.etemplate \
>> virt-viewer-enums.h.etemplate \
>> + virt-viewer.gresource.xml \
>> $(NULL)
>>
>> ENUMS_FILES = \
>> @@ -27,15 +29,32 @@ ENUMS_FILES = \
>> $(NULL)
>>
>> BUILT_SOURCES = \
>> + virt-viewer-resources.h \
>> + virt-viewer-resources.c \
>> virt-viewer-enums.h \
>> virt-viewer-enums.c \
>> $(NULL)
>>
>> -$(BUILT_SOURCES): %: %.etemplate $(ENUMS_FILES)
>> - $(AM_V_GEN)$(GLIB_MKENUMS) --template $^ | \
>> +resource_files = $(shell glib-compile-resources --sourcedir=$(srcdir) -
>> -generate-dependencies $(srcdir)/virt-viewer.gresource.xml)
>> +virt-viewer-resources.c: $(srcdir)/virt-viewer.gresource.xml
>> $(resource_files)
>> + glib-compile-resources --target=$@ --sourcedir=$(srcdir) --generate
>> -source --c-name virt_viewer $(srcdir)/virt-viewer.gresource.xml
>> +virt-viewer-resources.h: $(srcdir)/virt-viewer.gresource.xml
>> $(resource_files)
>> + glib-compile-resources --target=$@ --sourcedir=$(srcdir) --generate
>> -header --c-name virt_viewer $(srcdir)/virt-viewer.gresource.xml
>> +
>> +virt-viewer-enums.h: %: virt-viewer-enums.h.etemplate $(ENUMS_FILES)
>> + $(AM_V_GEN)$(GLIB_MKENUMS) --template $^ | \
>> + sed -e 's/VIRT_TYPE_VIEWER/VIRT_VIEWER_TYPE/' \
>> + -e 's,#include "$(srcdir)/,#include ",' > $@
>> +
>> +virt-viewer-enums.c: %: virt-viewer-enums.c.etemplate $(ENUMS_FILES)
>> + $(AM_V_GEN)$(GLIB_MKENUMS) --template $^ | \
>> sed -e 's/VIRT_TYPE_VIEWER/VIRT_VIEWER_TYPE/' \
>> -e 's,#include "$(srcdir)/,#include ",' > $@
>
>
> This duplication seems a shame. Is there a reason we can't continue using a
> pattern rule?
Hmm. Maybe? I'm not sure, to be honest.
>
>>
>> +CLEANFILES = \
>> + $(BUILT_SOURCES) \
>> + $(NULL)
>> +
>> libvirt_viewer_la_SOURCES = \
>> $(BUILT_SOURCES) \
>> virt-viewer-util.h \
>> @@ -185,8 +204,6 @@ if OS_WIN32
>> remote_viewer_LDFLAGS += -Wl,--subsystem,windows
>> endif
>>
>> -AM_CPPFLAGS = -DPACKAGE_DATADIR=\""$(pkgdatadir)"\"
>> -
>> VIRT_VIEWER_RES = virt-viewer.rc virt-viewer.manifest
>> ICONDIR = $(top_builddir)/icons
>> MANIFESTDIR = $(srcdir)
>> diff --git a/src/virt-viewer-about.xml b/src/virt-viewer-about.xml
>> index 16db3c2..287c68d 100644
>> --- a/src/virt-viewer-about.xml
>> +++ b/src/virt-viewer-about.xml
>> @@ -36,7 +36,6 @@ Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
>> 02111-1307 USA
>> Marc-André Lureau
>> </property>
>> <property name="translator_credits" translatable="yes">The Fedora
>> Translation Team</property>
>> - <property name="logo_icon_name">virt-viewer</property>
>> <signal name="delete-event" handler="virt_viewer_app_about_delete"
>> swapped="no"/>
>> <signal name="response" handler="virt_viewer_app_about_close"
>> swapped="no"/>
>> <child internal-child="vbox">
>> diff --git a/src/virt-viewer-app.c b/src/virt-viewer-app.c
>> index 3fa80a5..68fe533 100644
>> --- a/src/virt-viewer-app.c
>> +++ b/src/virt-viewer-app.c
>> @@ -52,6 +52,7 @@
>> #endif
>>
>> #include "virt-viewer-app.h"
>> +#include "virt-viewer-resources.h"
>> #include "virt-viewer-auth.h"
>> #include "virt-viewer-window.h"
>> #include "virt-viewer-session.h"
>> @@ -115,6 +116,7 @@ struct _VirtViewerAppPrivate {
>> gchar *clipboard;
>> GtkWidget *preferences;
>> GtkFileChooser *preferences_shared_folder;
>> + GResource *resource;
>> gboolean direct;
>> gboolean verbose;
>> gboolean enable_accel;
>> @@ -1717,6 +1719,11 @@ virt_viewer_app_dispose (GObject *object)
>> g_hash_table_unref(tmp);
>> }
>>
>> + if (priv->resource != NULL) {
>> + g_resources_unregister(priv->resource);
>> + priv->resource = NULL;
>> + }
>> +
>
> I'm quite sure that this is not actually necessary, since you did not pass -
> -manual-register to the glib-compile-resources script. The generated code makes
> use of 'constructor' functions to register these static resources automatically.
>
>> g_clear_object(&priv->session);
>> g_free(priv->title);
>> priv->title = NULL;
>> @@ -1863,6 +1870,9 @@ virt_viewer_app_on_application_startup(GApplication
>> *app)
>>
>> G_APPLICATION_CLASS(virt_viewer_app_parent_class)->startup(app);
>>
>> + self->priv->resource = virt_viewer_get_resource();
>> + g_resources_register(self->priv->resource);
>> +
>
> same as above
>
Well, didn't work without doing this.
You can remove this part and see if it works for you, but wasn't the case here.
>> virt_viewer_app_set_debug(opt_debug);
>> virt_viewer_app_set_fullscreen(self, opt_fullscreen);
>>
>> diff --git a/src/virt-viewer-util.c b/src/virt-viewer-util.c
>> index 76b61a1..ce81e06 100644
>> --- a/src/virt-viewer-util.c
>> +++ b/src/virt-viewer-util.c
>> @@ -41,6 +41,8 @@
>>
>> #include "virt-viewer-util.h"
>>
>> +#define VIRT_VIEWER_RESOURCE_PREFIX "/org/virt-manager/virt-viewer"
>> +
>> GQuark
>> virt_viewer_error_quark(void)
>> {
>> @@ -49,53 +51,15 @@ virt_viewer_error_quark(void)
>>
>> GtkBuilder *virt_viewer_util_load_ui(const char *name)
>> {
>> - struct stat sb;
>> GtkBuilder *builder;
>> - GError *error = NULL;
>> -
>> - builder = gtk_builder_new();
>> - if (stat(name, &sb) >= 0) {
>> - gtk_builder_add_from_file(builder, name, &error);
>> - } else {
>> - gchar *path = g_build_filename(PACKAGE_DATADIR, "ui", name, NULL);
>> - gboolean success = (gtk_builder_add_from_file(builder, path, &error)
>> != 0);
>> - if (error) {
>> - if (!(error->domain == G_FILE_ERROR && error->code ==
>> G_FILE_ERROR_NOENT))
>> - g_warning("Failed to add ui file '%s': %s", path, error
>> ->message);
>> - g_clear_error(&error);
>> - }
>> - g_free(path);
>> -
>> - if (!success) {
>> - const gchar * const * dirs = g_get_system_data_dirs();
>> - g_return_val_if_fail(dirs != NULL, NULL);
>> -
>> - while (dirs[0] != NULL) {
>> - path = g_build_filename(dirs[0], PACKAGE, "ui", name, NULL);
>> - if (gtk_builder_add_from_file(builder, path, NULL) != 0) {
>> - g_free(path);
>> - break;
>> - }
>> - g_free(path);
>> - dirs++;
>> - }
>> - if (dirs[0] == NULL)
>> - goto failed;
>> - }
>> - }
>> + gchar *resource = g_strdup_printf("%s/%s",
>> + VIRT_VIEWER_RESOURCE_PREFIX,
>> + name);
>>
>> - if (error) {
>> - g_error("Cannot load UI description %s: %s", name,
>> - error->message);
>> - g_clear_error(&error);
>> - goto failed;
>> - }
>> + builder = gtk_builder_new_from_resource(resource);
>>
>> + g_free(resource);
>> return builder;
>> - failed:
>> - g_error("failed to find UI description file");
>> - g_object_unref(builder);
>> - return NULL;
>> }
>>
>> int
>> diff --git a/src/virt-viewer-window.c b/src/virt-viewer-window.c
>> index 8ce34ca..2c46c06 100644
>> --- a/src/virt-viewer-window.c
>> +++ b/src/virt-viewer-window.c
>> @@ -1030,11 +1030,24 @@ G_MODULE_EXPORT void
>> virt_viewer_window_menu_help_about(GtkWidget *menu G_GNUC_UNUSED,
>> VirtViewerWindow *self)
>> {
>> - GtkBuilder *about = virt_viewer_util_load_ui("virt-viewer-about.xml");
>> + GtkBuilder *about;
>> + GtkWidget *dialog;
>> + GdkPixbuf *icon;
>> +
>> + about = virt_viewer_util_load_ui("virt-viewer-about.xml");
>> +
>> + dialog = GTK_WIDGET(gtk_builder_get_object(about, "about"));
>>
>> - GtkWidget *dialog = GTK_WIDGET(gtk_builder_get_object(about, "about"));
>> gtk_about_dialog_set_version(GTK_ABOUT_DIALOG(dialog), VERSION BUILDID);
>>
>> + icon = gdk_pixbuf_new_from_resource("/org/virt-manager/virt
>> -viewer/icons/48x48/virt-viewer.png", NULL);
>
> VIRT_VIEWER_RESOURCE_PREFIX could be used here.
>
>> + if (icon != NULL) {
>> + gtk_about_dialog_set_logo(GTK_ABOUT_DIALOG(dialog), icon);
>> + g_object_unref(icon);
>> + } else {
>> + gtk_about_dialog_set_logo_icon_name(GTK_ABOUT_DIALOG(dialog), "virt
>> -viewer");
>> + }
>> +
>> gtk_window_set_transient_for(GTK_WINDOW(dialog),
>> GTK_WINDOW(self->priv->window));
>>
>> @@ -1066,8 +1079,7 @@ virt_viewer_window_toolbar_setup(VirtViewerWindow *self)
>> g_signal_connect(button, "clicked",
>> G_CALLBACK(virt_viewer_window_menu_file_quit), self);
>>
>> /* USB Device selection */
>> - button = gtk_image_new_from_icon_name("virt-viewer-usb",
>> - GTK_ICON_SIZE_INVALID);
>> + button = gtk_image_new_from_resource("/org/virt-manager/virt
>> -viewer/icons/24x24/virt-viewer-usb.png");
>
> and here
>
>> button = GTK_WIDGET(gtk_tool_button_new(button, NULL));
>> gtk_tool_button_set_label(GTK_TOOL_BUTTON(button), _("USB device
>> selection"));
>> gtk_tool_item_set_tooltip_text(GTK_TOOL_ITEM(button), _("USB device
>> selection"));
>> diff --git a/src/virt-viewer.gresource.xml b/src/virt-viewer.gresource.xml
>> new file mode 100644
>> index 0000000..596889a
>> --- /dev/null
>> +++ b/src/virt-viewer.gresource.xml
>> @@ -0,0 +1,19 @@
>> +<?xml version="1.0" encoding="UTF-8"?>
>> +<gresources>
>> + <gresource prefix="/org/virt-manager/virt-viewer">
>> + <file>remote-viewer-connect.xml</file>
>> + <file>virt-viewer-about.xml</file>
>> + <file>virt-viewer-auth.xml</file>
>> + <file>virt-viewer-guest-details.xml</file>
>> + <file>virt-viewer-preferences.xml</file>
>> + <file>virt-viewer-vm-connection.xml</file>
>> + <file>virt-viewer.xml</file>
>> + <file alias="icons/16x16/virt-viewer.png">../icons/16x16/virt
>> -viewer.png</file>
>> + <file alias="icons/22x22/virt-viewer.png">../icons/22x22/virt
>> -viewer.png</file>
>> + <file alias="icons/24x24/virt-viewer.png">../icons/24x24/virt
>> -viewer.png</file>
>> + <file alias="icons/24x24/virt-viewer-usb.png">../icons/24x24/virt-viewer
>> -usb.png</file>
>> + <file alias="icons/32x32/virt-viewer.png">../icons/32x32/virt
>> -viewer.png</file>
>> + <file alias="icons/48x48/virt-viewer.png">../icons/48x48/virt
>> -viewer.png</file>
>> + <file alias="icons/256x256/virt-viewer.png">../icons/256x256/virt
>> -viewer.png</file>
>> + </gresource>
>> +</gresources>
>
>
> Reviewed-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
>
> _______________________________________________
> virt-tools-list mailing list
> virt-tools-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/virt-tools-list
I will address your (and Eduardo) comments and submit a v2.
Best Regards,
--
Fabiano Fidêncio
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
