[Spice-devel] [Spice-commits] 6 commits - meson.build src/map-file src/spice-glib-sym-file src/spice-gtk-session.c src/spice-session.c src/spice-session.h src/spice-session-priv.h

[Jakub Janku]

On Wed, Sep 9, 2020 at 5:27 PM Frediano Ziglio <fziglio at redhat.com> wrote:
>
> > Hi
>
> > On Wed, Sep 9, 2020 at 6:45 PM Jakub Janku < jjanku at redhat.com > wrote:
>
> > > On Wed, Sep 9, 2020 at 4:36 PM Frediano Ziglio < fziglio at redhat.com >
> > > wrote:
> >
> > > >
> >
> > > > > On Wed, Sep 9, 2020 at 4:16 PM Frediano Ziglio < fziglio at redhat.com >
> > > > > wrote:
> >
> > > > > >
> >
> > > > > > > > commit 4b9092b96b8da946ff3d17922b0fcf225c5dc81f
> >
> > > > > > >
> >
> > > > > > > > Author: Jakub Janků < jjanku at redhat.com >
> >
> > > > > > >
> >
> > > > > > > > Date: Sat May 23 16:28:52 2020 +0200
> >
> > > > > > >
> >
> > > > > >
> >
> > > > > > > > session: make spice_session_get_webdav_server() public
> >
> > > > > > >
> >
> > > > > >
> >
> > > > > > > > It will be necessary to access the webdav server from
> >
> > > > > > > > spice-gtk-session.c
> >
> > > > > > >
> >
> > > > > > > > which isn't compiled with spice-session-priv.h, so make
> >
> > > > > > >
> >
> > > > > > > > spice_session_get_webdav_server() public.
> >
> > > > > > >
> >
> > > > > >
> >
> > > > > > > I haven't looked at the whole series. Wouldn't it make sense to
> > > > > > > make
> > > > > > > it a
> >
> > > > > > > read-only property instead?
> >
> > > > > >
> >
> > > > > > It sounds reasonable for me.
> >
> > > > > > Jakub ?
> >
> > > > > >
> >
> > > > >
> >
> > > > > I agree.
> >
> > > > >
> >
> > > > > Revert the commits please. I'll reopen the merge request once I have it
> >
> > > > > ready.
> >
> > > > >
> >
> > > > > Cheers,
> >
> > > > > Jakub
> >
> > > > >
> >
> > > >
> >
> > > > To be honest I don't see the need to revert commits, it's just a change
> >
> > > > from public to private.
> >
>
> > > Ok, so should I open a separate MR?
> >
>
> > > To make sure that I didn't misunderstand it: the suggestion is to keep
> >
> > > spice_session_get_webdav_server() private and install a new
> >
> > > SpiceSession read-only property "webdav", correct?
> >
>
> > yes (the main motivation is to avoid adding new library symbols, and
> > properties can be looked up at runtime, which may avoid bumping dependencies
> > in some cases)
>
> Oh, I though the idea was making the new property private to in the future it could be removed if not needed anymore.
> You can achieve the dynamic resolution using dlsym if needed using library symbols.
> It's not that easy to look the property dynamically, to avoid warnings you have to use g_object_class_find_property first.
> For a "get" between spice-gtk and spice-glib the current solution is easier and consistent, there are already multiple spice_session_get_* functions.

Most of the getters are in spice-session-priv.h though. In
spice-session.h, there's only three of them.
So perhaps it would be more consistent to access the phodav server
using properties, as Marc-André suggested.

Anyway, here's the possible patch
https://gitlab.freedesktop.org/jjanku/spice-gtk/-/commit/048a150f8088d9fd643ba9987c8fba9e83494d94

Jakub
>
> Frediano
>