[virt-tools-list] [virt-viewer v2 3/3] util: allow loading an in-tree ui file

[Jonathon Jongsma]

On Thu, 2016-02-25 at 16:24 +0100, Fabiano Fidêncio wrote:
> On Thu, Feb 25, 2016 at 2:26 PM, Fabiano Fidêncio <fidencio at redhat.com> wrote:
> > On Thu, Feb 25, 2016 at 2:19 PM, Daniel P. Berrange <berrange at redhat.com>
> > wrote:
> > > On Thu, Feb 25, 2016 at 02:03:40PM +0100, Fabiano Fidêncio wrote:
> > > > This commits allows to load ui files without the need to install
> > > > virt-viewer. It helps a lot developers testing changes in the ui files
> > > > and also avoid abortions like [0] when running remote-viewer from the
> > > > source tree and without having any previous installation.
> > > > 
> > > > [0]:
> > > > fidenci at cat ~/src/upstream/virt-viewer $ ./src/remote-viewer
> > > > 
> > > > (remote-viewer:29951): virt-viewer-ERROR **: failed to find UI
> > > > description file
> > > > Trace/breakpoint trap (core dumped)
> > > > 
> > > > Signed-off-by: Fabiano Fidêncio <fidencio at redhat.com>
> > > > ---
> > > >  src/Makefile.am        |  2 +-
> > > >  src/virt-viewer-util.c | 35 +++++++++++++++++++++++++++++++----
> > > >  2 files changed, 32 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/src/Makefile.am b/src/Makefile.am
> > > > index 171a141..ac7177d 100644
> > > > --- a/src/Makefile.am
> > > > +++ b/src/Makefile.am
> > > > @@ -187,7 +187,7 @@ if OS_WIN32
> > > >  remote_viewer_LDFLAGS += -Wl,--subsystem,windows
> > > >  endif
> > > > 
> > > > -AM_CPPFLAGS = -DPACKAGE_DATADIR=\""$(pkgdatadir)"\"
> > > > +AM_CPPFLAGS = -DPACKAGE_DATADIR=\""$(pkgdatadir)"\" 
> > > > -DSOURCE_DIR=\""$(abs_srcdir)"\"
> > > 
> > > Embedding the source build directory inside binaries is generally
> > > bad practice IMHO.
> > 
> > Thanks, that's exactly the kind of feedback I was expecting for.
> > 
> > > 
> > > > +static gboolean
> > > > +virt_viewer_util_try_to_load_ui_from_non_system_data_dirs(GtkBuilder
> > > > *builder,
> > > > +                                                          const gchar
> > > > *name,
> > > > +                                                          GError
> > > > **error)
> > > > +{
> > > > +    gboolean ret;
> > > > +    gchar *path;
> > > > +
> > > > +    /*
> > > > +     * Firstly, try to load the ui file from the $srcdir.
> > > > +     * It allows developers to test changes done in the ui file without
> > > > +     * installing virt-viewer.
> > > > +     */
> > > > +    path = g_build_filename(SOURCE_DIR, name, NULL);
> > > 
> > > Just create a relative path here eg
> > > 
> > >  g_build_filename(".", "src", name, NULL);
> > > 
> > 
> > Sure, I'll re-work it and send a v3.
> > It must work considering VPATHs, let's see what I can come up with ...
> 
> Just for the record, this is Daniel's suggestion:
> 
> 10:56 <  fidencio> danpb: just to be sure, any suggestion for getting
> the right filename when building virt-viewer using VPATH?
> 10:59 <     danpb> simplest approach would be to check  ../src and ./src
> 10:59 <     danpb> that works well enough for people using a vpath in
> immediate subdir
> 

What about using GResource instead of distributing separate ui files, etc? We
depend on new enough glib now, I think.


> 
> > 
> > > > +    ret = virt_viewer_util_add_to_builder_from_file(builder, path,
> > > > error);
> > > >      g_free(path);
> > > > 
> > > > +    if (!ret) {
> > > > +        /*
> > > > +         * And only then try to load the ui file from the $pkgdatadir.
> > > > +         */
> > > > +        path = g_build_filename(PACKAGE_DATADIR, "ui", name, NULL),
> > > > +        ret = virt_viewer_util_add_to_builder_from_file(builder, path,
> > > > error);
> > > > +        g_free(path);
> > > > +    }
> > > > +
> > > >      return ret;
> > > 
> > > Regards,
> > > Daniel
> > > --
> > > > : 
> > > > http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ 
> > > > :|
> > > > : 
> > > > http://libvirt.org              -o-             http://virt-manager.org 
> > > > :|
> > > > : 
> > > > http://autobuild.org       -o-         http://search.cpan.org/~danberr/ 
> > > > :|
> > > > : 
> > > > http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc 
> > > > :|
> > 
> > Best Regards,
> > --
> > Fabiano Fidêncio
> 
> _______________________________________________
> virt-tools-list mailing list
> virt-tools-list at redhat.com
> https://www.redhat.com/mailman/listinfo/virt-tools-list