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 730409 - Support resource:/// URL's in GNOME_SHELL_JS envvar
Support resource:/// URL's in GNOME_SHELL_JS envvar
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2014-05-19 23:23 UTC by Owen Taylor
Modified: 2014-05-20 00:48 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Support resource:/// URL's in GNOME_SHELL_JS envvar (1.68 KB, patch)
2014-05-19 23:23 UTC, Owen Taylor
committed Details | Review

Description Owen Taylor 2014-05-19 23:23:37 UTC
Here's a patch that special cases resource: in GNOME_SHELL_JS - a much
shorter solution would be:

 g_regex_split_simple ("(?<!(?:^|:)resource):", ...)

(untested.) My thought in writing it this way was to avoid magic line noise.

I think special casing rather than escaping makes sense based on personal
experience - it took me a while and adding debugging prints to figure out
what was wrong with resource:///org/gnome/shell:<mypath>.
Comment 1 Owen Taylor 2014-05-19 23:23:41 UTC
Created attachment 276809 [details] [review]
Support resource:/// URL's in GNOME_SHELL_JS envvar

It can be useful to augment the shell's search path by doing

 GNOME_SHELL_JS=resource:///org/gnome/shell:<mypath>

But this doesn't work because resource: is split off. Special
case path elements that are just 'resource' and recombine
them with the next element.
Comment 2 Jasper St. Pierre (not reading bugmail) 2014-05-19 23:49:14 UTC
Review of attachment 276809 [details] [review]:

::: src/shell-global.c
@@ +291,3 @@
+
+      /* 'resource' is not a useful search path element, so assume resource:<something>
+       * is a resource source path element, and recombine accordingly */

I'd say something like:

"The naive g_strsplit above will split `resource:///foo/bar/` into `resource`, `///foo/bar/`. Combine these back together by looking for a literal `resource` in the array."

@@ +302,3 @@
+              search_path[i++] = NULL;
+              g_free (search_path[i]);
+              search_path[i++] = NULL;

It took me a second to figure this wasn't a copy/paste error. I don't know if it's more confusing, but:

    g_free (search_path[i]);
    g_free (search_path[i + 1]);
    i += 2;

@@ +311,3 @@
+
+          search_path[j++] = out;
+        }

It took me a bit to make sure that the last item was always NULL. I don't know if it's any clearer, but having:

    /* Make sure the array is NULL-terminated */
    search_path[j] = NULL;

and removing the NULL sets above might be clearer. Feel free to ignore.
Comment 3 Jasper St. Pierre (not reading bugmail) 2014-05-19 23:50:07 UTC
Review of attachment 276809 [details] [review]:

(Also, this is just a marking for now so I don't forget, but this might want to be added into gjs as GJS_SEARCH_PATH too at some point. Seems like it could be useful for apps.)
Comment 4 Owen Taylor 2014-05-20 00:48:24 UTC
Pushed with all your suggestions - they were at least as readable to me,
and if they were also more readable to you, then they had to be better :-)

Attachment 276809 [details] pushed as 5d11941 - Support resource:/// URL's in GNOME_SHELL_JS envvar