After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 766636 - Add grl.resource()
Add grl.resource()
Status: RESOLVED OBSOLETE
Product: grilo
Classification: Other
Component: lua
unspecified
Other All
: Normal normal
: ---
Assigned To: grilo-maint
grilo-maint
Depends on:
Blocks:
 
 
Reported: 2016-05-18 23:05 UTC by Bastien Nocera
Modified: 2018-09-24 09:43 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
lua-factory: Fix grl.fetch API doc (961 bytes, patch)
2016-05-18 23:05 UTC, Bastien Nocera
committed Details | Review
lua-factory: Fix GResource leak (1.97 KB, patch)
2016-05-18 23:05 UTC, Bastien Nocera
committed Details | Review
lua-factory: Make _load_goa_data() a private function (1.61 KB, patch)
2016-05-18 23:05 UTC, Bastien Nocera
committed Details | Review
lua-factory: Add grl.resource() (7.31 KB, patch)
2016-05-18 23:05 UTC, Bastien Nocera
reviewed Details | Review

Description Bastien Nocera 2016-05-18 23:05:31 UTC
.
Comment 1 Bastien Nocera 2016-05-18 23:05:36 UTC
Created attachment 328158 [details] [review]
lua-factory: Fix grl.fetch API doc
Comment 2 Bastien Nocera 2016-05-18 23:05:42 UTC
Created attachment 328159 [details] [review]
lua-factory: Fix GResource leak

When registering and deregistering a source, we should make sure that
its resources get unregistered as well.
Comment 3 Bastien Nocera 2016-05-18 23:05:48 UTC
Created attachment 328160 [details] [review]
lua-factory: Make _load_goa_data() a private function

It's only used inside the grl-lua-library.c source.
Comment 4 Bastien Nocera 2016-05-18 23:05:54 UTC
Created attachment 328161 [details] [review]
lua-factory: Add grl.resource()

To allow sources to load files inside a private resources bundle.

So, for a grl-foo.lua source to load the "public" GResources, which
will get registered so that the application can access it, you'd name
the resource file grl-foo.gresource

And to load the private GResources data that only the source itself can
access, you'd name the resource file grl-foo-private.gresource
Comment 5 Victor Toso 2016-05-19 05:45:01 UTC
Review of attachment 328158 [details] [review]:

ok
Comment 6 Victor Toso 2016-05-19 05:45:36 UTC
Review of attachment 328159 [details] [review]:

great
Comment 7 Victor Toso 2016-05-19 05:46:23 UTC
Review of attachment 328160 [details] [review]:

sure
Comment 8 Victor Toso 2016-05-19 06:12:25 UTC
Review of attachment 328161 [details] [review]:

Looks good. A unit test for this would be great :)
I'll mark as reviewed so you can double check if leaks on failure.

::: src/lua-factory/grl-lua-factory.c
@@ +452,3 @@
   resource = NULL;
+  source->priv->private_resource = priv_resource;
+  priv_resource = NULL;

I don't see where both resources are freed if we `goto bail` below.. we probably want to g_clear_object (&source) there.
Comment 9 Adrien Plazas 2016-05-19 08:02:10 UTC
Is it possible to return nil if the resource doesn't exist or fails to load? Trying to load a non existing resource crashes.
Comment 10 Adrien Plazas 2016-05-19 09:35:33 UTC
Is there any way to configure a plugin with resource URIs from the application so the plugin can use these as sources?

Currently, using resources from the application doesn't work.
Comment 11 Bastien Nocera 2016-05-19 10:25:14 UTC
(In reply to Adrien Plazas from comment #9)
> Is it possible to return nil if the resource doesn't exist or fails to load?
> Trying to load a non existing resource crashes.

It's supposed to return nil already. Backtrace and test case please.

(In reply to Adrien Plazas from comment #10)
> Is there any way to configure a plugin with resource URIs from the
> application so the plugin can use these as sources?
> 
> Currently, using resources from the application doesn't work.

No. Otherwise that means that the source can access any GResource data from other sources.
Comment 12 Bastien Nocera 2016-05-19 19:57:41 UTC
Attachment 328158 [details] pushed as 4fdd9fd - lua-factory: Fix grl.fetch API doc
Attachment 328159 [details] pushed as fb8a8d6 - lua-factory: Fix GResource leak
Attachment 328160 [details] pushed as fd67192 - lua-factory: Make _load_goa_data() a private function
Comment 13 Victor Toso 2017-01-22 13:01:12 UTC
(In reply to Victor Toso from comment #8)
> ::: src/lua-factory/grl-lua-factory.c
> @@ +452,3 @@
>    resource = NULL;
> +  source->priv->private_resource = priv_resource;
> +  priv_resource = NULL;
> 
> I don't see where both resources are freed if we `goto bail` below.. we
> probably want to g_clear_object (&source) there.

Besides this, I don't see a reason to hold this patch.
Comment 14 Bastien Nocera 2017-01-22 14:26:58 UTC
(In reply to Victor Toso from comment #13)
> (In reply to Victor Toso from comment #8)
> > ::: src/lua-factory/grl-lua-factory.c
> > @@ +452,3 @@
> >    resource = NULL;
> > +  source->priv->private_resource = priv_resource;
> > +  priv_resource = NULL;
> > 
> > I don't see where both resources are freed if we `goto bail` below.. we
> > probably want to g_clear_object (&source) there.
> 
> Besides this, I don't see a reason to hold this patch.

We're waiting for the crash report from Adrien :/
Comment 15 GNOME Infrastructure Team 2018-09-24 09:43:42 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/grilo/issues/88.