Created attachment 146265 [details] [review]
[StTheme] Add functions to dynamically load/unload stylesheets

For implementing extensions, we want the ability to add a stylesheet
dynamically, and unload it as well.

Created attachment 146266 [details] [review]
Add global.log, global.logError functions

These log to a display in lookingGlass; rough equivalent of
the Firebug "console.log".

Created attachment 146267 [details] [review]
Add extensionSystem

Comment on attachment 146265 [details] [review]
[StTheme] Add functions to dynamically load/unload stylesheets

>+  if (!g_file_get_contents (filename, &contents ,&length, error))
>+    return NULL;

Fix the comma before &length while you're there.

>+st_theme_load_stylesheet (StTheme    *theme,

this will at least leak memory and maybe worse if two extensions load the same stylesheet. (Although the existing code already has that problem if we @import the same stylesheet more than once...)

>+  application_stylesheet = parse_stylesheet (theme->application_stylesheet, NULL);
>+  theme_stylesheet = parse_stylesheet (theme->theme_stylesheet, NULL);
>+  default_stylesheet = parse_stylesheet (theme->default_stylesheet, NULL);

>+                  import_rule->sheet = parse_stylesheet (filename, NULL);

These probably need to pass a GError and g_warn or something on failure. Otherwise CSS-loading problems just turn into silent failure.

>- *Evaluates if a given additional selector matches an style node.
>+ *Evaluates if a given additional selec;tor matches an style node.

typo

Comment on attachment 146266 [details] [review]
Add global.log, global.logError functions

>+        this.text.clutter_text.line_wrap = true;

meh. StLabel should mirror ClutterText's properties or something.

>+    _onMappedNotify: function() {

it would be nice if it continued updating while it was visible too...

>+        for (let i = 0; i < Main._errorLogStack.length; i++) {

Although the contents of this loop are pretty simple, it would be safer to copy Main._errorLogStack to another variable first, and then iterate that, just in case something inside the loop causes an error to be logged... :)

(You could even just have a Main.getErrorLogStack() method, that returned and cleared Main._errorLogStack, and then I also wouldn't have to frown at the fact that you're modifying another module's private variables.)

>+    // Now monkey patch utility functions into the global proxy;
>+    // This is easier and faster than indirecting down into global
>+    // if we want to call back up into JS.
>+    window.global.logError = _logError;
>+    window.global.log = _logDebug;

You can just say "global" rather than "window.global" at that point.

Is there any reason to want "global.logError" rather than "Main.logError" though?

>+function _log(category, msg) {
>+    _errorLogStack.push({offset: new Date().getTime() - _startDate.getTime(),
>+                         category: category,
>+                         message: msg });
>+}

it would be swell if this used "Array.slice(arguments, 1).join(' ')" for the message, so you could say log("foo:", x, y, z)

(In reply to comment #4)

> >+st_theme_load_stylesheet (StTheme    *theme,
> 
> this will at least leak memory and maybe worse if two extensions load the same
> stylesheet. (Although the existing code already has that problem if we @import
> the same stylesheet more than once...)

Well, this isn't intended for direct use by an extension in the normal case; we automatically load an extension stylesheet from the well-known filename "stylesheet.css" in the extension directory, which will be a different full path for each one.

Created attachment 148080 [details] [review]
[StTheme] Add functions to dynamically load/unload stylesheets

For implementing extensions, we want the ability to add a stylesheet
dynamically, and unload it as well.

Created attachment 148081 [details] [review]
Add global.log, global.logError functions

These log to a display in lookingGlass; rough equivalent of
the Firebug "console.log".

Created attachment 148082 [details] [review]
Add extensionSystem

Fundamentals for loading extensions from a well known directory.

Created attachment 148083 [details] [review]
[lookingGlass] Port more to CSS

Created attachment 148084 [details] [review]
[lookingGlass] Add primitive extensions list

In the future this should be more like the Firefox Addons UI
probably with enable/disable/update checking, but stub it out
here for now.

Comment on attachment 148080 [details] [review]
[StTheme] Add functions to dynamically load/unload stylesheets

st_theme_load_stylesheet() calls insert_stylesheet(), but st_theme_unload_stylesheet() doesn't undo that, so even after unloading, the style sheet will still be in the hash tables.

Comment on attachment 148081 [details] [review]
Add global.log, global.logError functions

looks good. it seems like having a timestamp might be more useful than having an offset though?

Comment on attachment 148083 [details] [review]
[lookingGlass] Port more to CSS

yay css!

Review of attachment 148082 [details] [review]:

Looks like a nice simple way of doing extensions. My only reservation is that unifying the manifest and the main Javascript file into a single JS file could conceivably cause problems if you installed an extension that was crashing by importing some buggy piece of C code; we could start in a emergency mode with all extensions disabled, but we couldn't show their names in the UI, because importing the extension.js would potentially trigger the problems. Would it make sense to make the manifest a JSON file separate from the main.js?

Some comments in detail.

::: data/sample-extension.js
@@ +4,3 @@
+
+const EXTENSION = {
+    name: __INSERT_NAME_HERE__,

Any sort of template like this should include comments that document the expected form (capitalization, sentence fragment or real sentence, etc.) of Name and Description, or the end result will be inconsistency.

@@ +7,3 @@
+    /* You must have a unique identifier for your extension.
+     * The simplest way to get one is to use the 'uuidgen' Unix utility.
+     * A uuid looks like this: 3636ab59-5f9c-4dc3-ba94-a637ef8741a8

Of the two styles of uuid supported by Firefox, I find 'firebug@software.joehewitt.com' a lot more self-documenting than '{e4a8a97b-f2ed-450b-b12d-ee082ba24781}' [that's GreaseMonkey, if it wasn't immediately obvious :-)]

::: js/ui/extensionSystem.js
@@ +13,3 @@
+    while ((info = fileEnum.next_file(null)) != null) {
+        let displayName = info.get_display_name();
+        allFiles[displayName] = info;

This feels like an abuse of Gio - it doesn't matter too much here - if you have multiple files that have the same display name in your extension you get what you deserve, but it seems if we write some "load a couple of files from a directory code", we should have some simple way of doing it that is generally correct, and not correct only making certain assumptions.

@@ +17,3 @@
+    fileEnum.close(null);
+    let baseErrorString = 'While loading extension from "' + dir.get_parse_name() + '": ';
+    let mainJs = allFiles['extension.js'];

If the file is extension.js the variable should be extensionJs.

@@ +38,3 @@
+    let extensionModule;
+    try {
+        global.add_extension_importer("imports.ui.extensionSystem.extensions", extensionName, dir.get_path());

I don't think gjs has relative imports, so I'd have to write:

 let Utils = imports.ui.extensionSystem.extensions.breadcrumbs.Utils;

? Why not shorten that to 'imports.extensions.breadcrumbs.Utils' ?

@@ +54,3 @@
+    //if (!meta.uuid) {
+    //    global.logError(baseErrorString + 'missing \'uuid\' property in EXTENSION');
+    //}

Don't see a point in having an *optional* uuid. The uuid seems like where we'd want to use for an "enabled" GConf key off of too.

@@ +71,3 @@
+function loadExtensions() {
+    let configPath = GLib.get_user_config_dir();
+    let extensionsPath = GLib.build_filenamev([configPath, 'gnome-shell', 'extensions']);

As discussed in person, I think we want to allow for system installed extensions from the get-go. The example I gave Colin was a company using specialized software (CAD, animation, etc.) that needed some customizations to the gnome-shell workflow, and wanted to put an extension on all their users computers. Distro packaging of extensions is another use case, though I don't know how much we want to emphasize that versus a user-installed model.

@@ +74,3 @@
+    let extensionsDir = Gio.file_new_for_path(extensionsPath);
+    try {
+        extensionsDir.make_directory_with_parents(null);

I don't like a loadExtensions function creating a directory as a side effect

@@ +76,3 @@
+        extensionsDir.make_directory_with_parents(null);
+    } catch (e) {
+        // Ignore existence errors

And what about typos, etc? Unfortunately, Javascript makes it hard to catch specific erorrs, and obviously we don't have the GError support for error codes yet either in gjs, but I think catch (e) { /* ignore everything without logging */; } should be stylistically verboten.

@@ +85,3 @@
+            continue;
+        let path = GLib.build_filenamev([extensionsPath, info.get_name()]);
+        loadExtension(Gio.file_new_for_path(path));

loadExtension(Gio.file_get_child(extensionsDir, info.get_name()));

::: src/shell-global.c
@@ +520,3 @@
 
+gboolean
+shell_global_add_extension_importer (ShellGlobal *global,

Function docs would be nice; I was able to decipher the operation, but it's not exactly obvious.

@@ +558,3 @@
+                  G_IO_ERROR,
+                  G_IO_ERROR_FAILED,
+                  "%s", "Target object is not object");

Why bother with the GError if the errors that are going to be produced are human-unreadable?

Review of attachment 148084 [details] [review]:

Looks like a fine start except for one unused chunk of code.

::: js/ui/lookingGlass.js
@@ +351,3 @@
+
+    _createNewExtension: function() {
+        let scriptPath = global.datadir + "createExtension.js";

I didn't see this JS in your patches (and this function is unused). Extension creation doesn't seem hard enough to me to really require automation - a page on the wiki should be fine.

@@ +356,3 @@
+            proc.run();
+        } catch (e) {
+        }

See comment on other patch about catch (e) {}

Comment on attachment 148081 [details] [review]
Add global.log, global.logError functions

Attachment 148081 [details] pushed as a4405be - Add global.log, global.logError functions

(In reply to comment #15)
> Review of attachment 148082 [details] [review]:

> Any sort of template like this should include comments that document the
> expected form (capitalization, sentence fragment or real sentence, etc.) of
> Name and Description, or the end result will be inconsistency.

I'm going to kill the in-tree template for now I think, in favor of a commented version on the wiki.

> 
> ::: js/ui/extensionSystem.js
> @@ +13,3 @@
> +    while ((info = fileEnum.next_file(null)) != null) {
> +        let displayName = info.get_display_name();
> +        allFiles[displayName] = info;
> 
> This feels like an abuse of Gio - it doesn't matter too much here - if you have
> multiple files that have the same display name in your extension you get what
> you deserve, but it seems if we write some "load a couple of files from a
> directory code", we should have some simple way of doing it that is generally
> correct, and not correct only making certain assumptions.

Hm, you could only get multiple files with the same display name if they were identical except for one or more runs of invalid unicode.  I'm not very concerned about this.  Ideally we'd skip files with invalid unicode...I could search for the replacement character I guess.
 
> I don't think gjs has relative imports, so I'd have to write:
> 
>  let Utils = imports.ui.extensionSystem.extensions.breadcrumbs.Utils;
> 
> ? Why not shorten that to 'imports.extensions.breadcrumbs.Utils' ?

I'll try; I think this might not work unless "imports.extensions" is itself a valid module.
 
> I don't like a loadExtensions function creating a directory as a side effect

I'd like it to exist if the shell's been run once, to make finding the place to add extensions more failsafe.  I guess it could be an explicit init() method called from main.js or the like?
 
> @@ +76,3 @@
> +        extensionsDir.make_directory_with_parents(null);
> +    } catch (e) {
> +        // Ignore existence errors
> 
> And what about typos, etc? 

What typos?  This is a hardcoded dir in the source.

> Unfortunately, Javascript makes it hard to catch
> specific erorrs, and obviously we don't have the GError support for error codes
> yet either in gjs, but I think catch (e) { /* ignore everything without logging
> */; } should be stylistically verboten.

Yes, it would be nice if we could catch specific errors.
> @@ +558,3 @@
> +                  G_IO_ERROR,
> +                  G_IO_ERROR_FAILED,
> +                  "%s", "Target object is not object");
> 
> Why bother with the GError if the errors that are going to be produced are
> human-unreadable?

You're saying that the error message is unclear?  That's definitely true, but trying to explain the problem would be too much bother; It shouldn't happen in practice.  This error is basically just an index into the source code.  I suppose for errors like this it'd be nice to have some sort of error allocation scheme "E005105" or the like.  

I need to set the GError though because the one above it *can* throw in practice (it's where we parse the user code).

(In reply to comment #18)

> > ::: js/ui/extensionSystem.js
> > @@ +13,3 @@
> > +    while ((info = fileEnum.next_file(null)) != null) {
> > +        let displayName = info.get_display_name();
> > +        allFiles[displayName] = info;
> > 
> > This feels like an abuse of Gio - it doesn't matter too much here - if you have
> > multiple files that have the same display name in your extension you get what
> > you deserve, but it seems if we write some "load a couple of files from a
> > directory code", we should have some simple way of doing it that is generally
> > correct, and not correct only making certain assumptions.
> 
> Hm, you could only get multiple files with the same display name if they were
> identical except for one or more runs of invalid unicode.  I'm not very
> concerned about this.  Ideally we'd skip files with invalid unicode...I could
> search for the replacement character I guess.

I think you are missing my point - if the only way of using Gio conveniently from Javascript is by a method that the Gio maintainer (Alex) is very clear is incorrect, then we have a problem.
 
> > I don't like a loadExtensions function creating a directory as a side effect
> 
> I'd like it to exist if the shell's been run once, to make finding the place to
> add extensions more failsafe.  I guess it could be an explicit init() method
> called from main.js or the like?

That would be OK.
 
> > @@ +76,3 @@
> > +        extensionsDir.make_directory_with_parents(null);
> > +    } catch (e) {
> > +        // Ignore existence errors
> > 
> > And what about typos, etc? 
> 
> What typos?  This is a hardcoded dir in the source.

Typos in the source code - if you wrote 

 extensionsDir.make_directory_with_pparents(null);

You'd have never have found the problem.

> > Unfortunately, Javascript makes it hard to catch
> > specific erorrs, and obviously we don't have the GError support for error codes
> > yet either in gjs, but I think catch (e) { /* ignore everything without logging
> > */; } should be stylistically verboten.
> 
> Yes, it would be nice if we could catch specific errors.

We either have to add the error codes, or search for string matches. (That obviously is bad for internationalization, so maybe you just need to fix the gjs bug.) Catching all errors without logging is not OK.

> > @@ +558,3 @@
> > +                  G_IO_ERROR,
> > +                  G_IO_ERROR_FAILED,
> > +                  "%s", "Target object is not object");
> > 
> > Why bother with the GError if the errors that are going to be produced are
> > human-unreadable?
> 
> You're saying that the error message is unclear?  That's definitely true, but
> trying to explain the problem would be too much bother; It shouldn't happen in
> practice.  This error is basically just an index into the source code.  I
> suppose for errors like this it'd be nice to have some sort of error allocation
> scheme "E005105" or the like.  

If it can't happen, then I'd say just g_error().
 
> I need to set the GError though because the one above it *can* throw in
> practice (it's where we parse the user code).

Are you sure? I think it doesn't get parsed until you do extensionModule = extensions[extensionName].extension;

(In reply to comment #19)
> Typos in the source code - if you wrote 
> 
>  extensionsDir.make_directory_with_pparents(null);
> 
> You'd have never have found the problem.

And for the record, I totally did that while hacking on the sidebar widgets, and it was incredibly confusing until I figured it out. (And then changed the catch{} clauses to all use logError.)

Created attachment 148931 [details] [review]
Add extensionSystem

Consumer documentation will live at http://live.gnome.org/GnomeShell/Extensions

In terms of implementation; basically we load extensions from the well-known
directories.  Add a GConf key to disable extensions by uuid.  There is a new
option --create-extension for the gnome-shell script which takes a bit of
interactive input, sets up some sample files, and launches gedit.

No extensions UI in this patch; that will come later.

https://bugzilla.gnome.org/show_bug.cgi?id=599661

ext

Comment on attachment 148931 [details] [review]
Add extensionSystem

inc updated patch

Created attachment 148942 [details] [review]
Add extensionSystem

Consumer documentation will live at http://live.gnome.org/GnomeShell/Extensions

In terms of implementation; basically we load extensions from the well-known
directories.  Add a GConf key to disable extensions by uuid.  There is a new
option --create-extension for the gnome-shell script which takes a bit of
interactive input, sets up some sample files, and launches gedit.

No extensions UI in this patch; that will come later.

Created attachment 148943 [details] [review]
Port more to CSS

Created attachment 148944 [details] [review]
[St] Implement text-decoration: [underline|strikethrough]

Move CSS handling of StLabel and StButton for their underlying
ClutterText objects into st_private, and implement support for
the underline and strikethrough St text-decoration properties.

Overline isn't implemented for lack of a corresponding Pango
attribute, and blink, well...

Created attachment 148945 [details] [review]
Make link.js into a St.Button, delete unused link imports

It's actually totally unused at the moment, but a future patch will use it.

Created attachment 148946 [details] [review]
Add primitive extensions list

In the future this should be more like the Firefox Addons UI
probably with enable/disable/update checking, but stub it out
here for now.

(In reply to comment #19)
> > Hm, you could only get multiple files with the same display name if they were
> > identical except for one or more runs of invalid unicode.  I'm not very
> > concerned about this.  Ideally we'd skip files with invalid unicode...I could
> > search for the replacement character I guess.
> 
> I think you are missing my point - if the only way of using Gio conveniently
> from Javascript is by a method that the Gio maintainer (Alex) is very clear is
> incorrect, then we have a problem.

This looks like the only unaddressed comment here...not sure how to proceed.  We're just testing this list for specific file names, it's not the case that we'll fail on seeing invalid utf8 files.

If we were instead looking for any file ending in .js, maybe then you need to pick some behavior (skip them, throw error etc.), but here we really just skip.

I don't see how this is specific to JS either, I would probably write similar code in c.

(In reply to comment #28)
> (In reply to comment #19)
> > > Hm, you could only get multiple files with the same display name if they were
> > > identical except for one or more runs of invalid unicode.  I'm not very
> > > concerned about this.  Ideally we'd skip files with invalid unicode...I could
> > > search for the replacement character I guess.
> > 
> > I think you are missing my point - if the only way of using Gio conveniently
> > from Javascript is by a method that the Gio maintainer (Alex) is very clear is
> > incorrect, then we have a problem.
> 
> This looks like the only unaddressed comment here...not sure how to proceed. 
> We're just testing this list for specific file names, it's not the case that
> we'll fail on seeing invalid utf8 files.
> 
> If we were instead looking for any file ending in .js, maybe then you need to
> pick some behavior (skip them, throw error etc.), but here we really just skip.
> 
> I don't see how this is specific to JS either, I would probably write similar
> code in c.

I don't really understand why I'd write such code in JS or C. Could easily be:

+    let metadata = allFiles['metadata.json'];
+    if (!metadata) {
+        global.logError(baseErrorString + 'Missing metadata.json');
+        return;
+    }
+
+    let metadataFile = dir.get_child('metadata.json');

    let metadataFile = dir.get_child('metadata.json');
    if (!metadataFile.query_exists()) P
        global.logError(baseErrorString + 'Missing metadata.json');
        return;
    }

Etc. There certainly is a point that given enough files that you want to test for in a directory, it's more efficient to readdir() the directory instead of stat'ing files. If you were at that limit (I don't think you are close to it), then I think you could simply use .get_name() rather than the get_display_name() that you are using here ... gjs_string_from_filename()/gjs_to_from_filename() should take ASCII to ASCII and should produce something hashable (if not interpretable) for non-ASCII results.

That would require fixing:

      <method name="get_name" c:identifier="g_file_info_get_name">
        <return-value transfer-ownership="none">
          <type name="utf8" c:type="char*"/>
        </return-value>
      </method>

(In reply to comment #29)
> 
>     let metadataFile = dir.get_child('metadata.json');
>     if (!metadataFile.query_exists()) P
>         global.logError(baseErrorString + 'Missing metadata.json');
>         return;
>     }

Good idea, thanks!  Fixed.

Attachment 148084 [details] pushed as 2cb4dfb - [lookingGlass] Add primitive extensions list
Attachment 148942 [details] pushed as aa9d351 - Add extensionSystem
Attachment 148943 [details] pushed as c4a49b4 - Port more to CSS
Attachment 148944 [details] pushed as 55fbb9d - [St] Implement text-decoration: [underline|strikethrough]
Attachment 148945 [details] pushed as c0ff006 - Make link.js into a St.Button, delete unused link imports