Created attachment 199794 [details] [review]
lang: Add new "Class" framework adapted from MooTools

The current strategy of using __proto__ and _init to implement prototype chains
is non-standard and error-prone. Use something a bit more automatic to help us
along.

Additionally, when patches come for inheriting from gi objects, we can use the
existing framework to hoist ourselves without too much effort.

Review of attachment 199794 [details] [review]:

Definitely an improvement over the current situation, and I'm looking forward to port shell code to this framework.
Just some small comments here and there.

Plus a feature request: can this be merged with import.signals, so that all classes get signals out of the box (like GObject derived classes)?

::: modules/lang.js
@@ +134,3 @@
+
+function _super() {
+    if (!this.$caller)

I don't think we use this naming convention anywhere. What about python-style __caller__ and __super__?

@@ +144,3 @@
+
+    if (!previous)
+        throw new Error("The method '" + name + "' has no superclass");

TypeError?

@@ +170,3 @@
+    let proto = new klazz();
+    delete klazz.$prototyping;
+    return proto;

This is a horrible hack, and needed for conformance with ES 3.
But we use JS with extensions, so we can just create an object with the right prototype, by setting prototype. Or, if you want to respect specs, use Object.create().

@@ +174,3 @@
+
+function Class(params) {
+    if (typeof params === 'function')

I don't think this style (passing just the constructor) is worth using.

@@ +178,3 @@
+
+    var newClass = function() {
+        if (newClass.$prototyping)

Once getInstance() is killed, this can be dropped as well.

@@ +201,3 @@
+    }
+
+    for (let prop in this) {

Uhm? Class() has no prototype property, so a new Class object has no property to be copied.

Semi-digression...

A while ago I was trying to improve Object.toString() so that it didn't just say "[object Object]" for all JS types. I originally though it could make use of object.constructor.name:

    gjs> function Foo() {}
    gjs> foo = new Foo()
    [object Object]
    gjs> foo.constructor.name
    "Foo"

But that breaks the way we do things:

    gjs> function Bar() {}
    gjs> Bar.prototype = {}
    gjs> bar = new Bar()
    [object Object]
    gjs> bar.constructor.name
    "Object"

(because the object literal we assign to Bar.prototype implicitly has the property "constructor: Object", which then gets inherited by all Bars. If you manually include "constructor: Bar" in the prototype, then it works right)

With Lang.Class:

    gjs> Baz = new Lang.Class({})
    gjs> baz = new Baz()
    [object Object]
    gjs> baz.constructor.name
    ""

I guess you'd have to pass a name to Lang.Class to have it be able to set things up to work right...

Created attachment 199844 [details] [review]
lang: Add new "Class" framework adapted from MooTools

The current strategy of using __proto__ and _init to implement prototype chains
is non-standard and error-prone. Use something a bit more automatic to help us
along.

Additionally, when patches come for inheriting from gi objects, we can use the
existing framework to hoist ourselves without too much effort.



The "$" naming is because ECMA-262 says that "$" should be a convention for "internal". Obviously, this isn't globally used (see jQuery for an example), but I figured that this._super would be more likely to collide than this.$super. Changed to __super__, we can put it back if wanted.

Things we can do to solve the toString problem:

    function Foo() {
    }

    Lang.makeClass(Foo, {
        extends: Bar,

        _init: function() {
        }
    });

--

    const Foo = Lang.makeClass('Foo', {
        extends: Bar, // ...
    });

--

    const Foo = Lang.makeClass(
        name: 'Foo',
        extends: Bar, // ...
    });

--

    Lang.makeAndInstallClass('Foo', {
        extends: Bar,
    });

(this one inserts something into the current scope. Possible, but I don't like it)

--

Note sure if these are "better". But yes, to have a better toString(), we need to have the name somewhere.

Review of attachment 199844 [details] [review]:

::: modules/lang.js
@@ +188,3 @@
+    }
+
+    for (let prop in this) {

I reinstate my comment. What is this for?
Is it meant for something like
MetaClass = new Lang.Class({ extends: Lang.Class, myClassMethod: function() { } })
Class = new MetaClass();
Class.myClassMethod()
?

(In reply to comment #5)
> Things we can do to solve the toString problem:
>
> [...]
> --
> 
>     const Foo = Lang.makeClass('Foo', {
>         extends: Bar, // ...
>     });
> 
> --
> 
>     const Foo = Lang.makeClass(
>         name: 'Foo',
>         extends: Bar, // ...
>     });
> 

I'd go with the one of these (and personally prefer the second). After all, we'll have an explicit name property in any case when we start subclassing GObject, so better be prepared.
(Btw, I prefer "new Lang.Class" to "Lang.makeClass". It reinforces the idea that classes are "type objects" like in Python, and makes it possible to do "Foo instanceof Lang.Class").

(In reply to comment #6)
> Review of attachment 199844 [details] [review]:
> 
> ::: modules/lang.js
> @@ +188,3 @@
> +    }
> +
> +    for (let prop in this) {
> 
> I reinstate my comment. What is this for?

In MooTools, it copies all things that are on the 'Class' object to be on the objects themselves. We don't put anything on the 'Class' object prototype, so I'll remove it (sorry, I forgot).

Other thing to think about: "super" and "extends" are technically ES262 "Future Reserved Keywords", so we really shouldn't be using them as property names, although it works. How do people about staying more true to MooTools and using "Extends" and "parent"?

(In reply to comment #8)
> Other thing to think about: "super" and "extends" are technically ES262 "Future
> Reserved Keywords", so we really shouldn't be using them as property names,
> although it works. How do people about staying more true to MooTools and using
> "Extends" and "parent"?

I'm ok with parent, but Extends with a capital E is against all our style rules.
What about a positional parameter, like:

MyClass = new Lang.Class(MyBase, {
   _init: function () { }
});

?

It has the advantage that it looks more to the ideal
class MyClass : MyBase {
  function _init() {
  }
}
(that btw will hopefully appear in ES6)

Created attachment 199861 [details] [review]
lang: Add new "Class" framework adapted from MooTools

The current strategy of using __proto__ and _init to implement prototype chains
is non-standard and error-prone. Use something a bit more automatic to help us
along.

Additionally, when patches come for inheriting from gi objects, we can use the
existing framework to hoist ourselves without too much effort.



> What about a positional parameter, like:
> 
> MyClass = new Lang.Class(MyBase, {
>    _init: function () { }
> });
> 
> ?
> 
> It has the advantage that it looks more to the ideal
> class MyClass : MyBase {
>   function _init() {
>   }
> }
> (that btw will hopefully appear in ES6)

What about a class with no base? Detect whether the parameter is an object or a function and switch on that? Should we make 'name' a positional parameter as well?

I'm attaching a patch that gets us back to MooTools style and adds a ton more tests.

If we wanted to be super evil, though:

    const MyClass = new Lang.Class({
        __proto__: MyBase,

        _init: function() {
        }
    });

Review of attachment 199861 [details] [review]:

I don't like capitals for Name/Extends, but other than that, it looks good to me.

Attachment 199861 [details] pushed as bb7272c - lang: Add new "Class" framework adapted from MooTools


OK. This is pushed as-is. If we want to refine the framework further, now would be the itme to do so.