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 700347 - Add support for Gtk widget templates
Add support for Gtk widget templates
Status: RESOLVED FIXED
Product: gjs
Classification: Bindings
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gjs-maint
gjs-maint
Depends on:
Blocks: 728301
 
 
Reported: 2013-05-14 22:26 UTC by Giovanni Campagna
Modified: 2014-09-29 11:19 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Implement GTypeClass and GTypeInterface arguments (14.12 KB, patch)
2013-05-14 22:26 UTC, Giovanni Campagna
none Details | Review
Stop exposing GType structures as such (5.04 KB, patch)
2013-05-14 22:26 UTC, Giovanni Campagna
none Details | Review
Gtk: add override to handle GtkWidget templates (6.67 KB, patch)
2013-05-14 22:26 UTC, Giovanni Campagna
needs-work Details | Review
[PATCH] object: add a custom hook to run at instance_init (3.09 KB, patch)
2013-07-26 00:43 UTC, Colin Walters
needs-work Details | Review
Implement GTypeClass and GTypeInterface arguments (13.92 KB, patch)
2014-04-16 07:49 UTC, Bastien Nocera
none Details | Review
Stop exposing GType structures as such (5.08 KB, patch)
2014-04-16 07:49 UTC, Bastien Nocera
none Details | Review
Implement GTypeClass and GTypeInterface arguments (14.02 KB, patch)
2014-09-16 16:27 UTC, Bastien Nocera
none Details | Review
Stop exposing GType structures as such (5.17 KB, patch)
2014-09-16 16:27 UTC, Bastien Nocera
none Details | Review
object: add a custom hook to run at instance_init (3.33 KB, patch)
2014-09-16 16:28 UTC, Bastien Nocera
none Details | Review
Gtk: add override to handle GtkWidget templates (6.54 KB, patch)
2014-09-16 16:28 UTC, Bastien Nocera
none Details | Review
Gtk: add override to handle GtkWidget templates (6.56 KB, patch)
2014-09-26 14:48 UTC, Bastien Nocera
none Details | Review
Implement GTypeClass and GTypeInterface arguments (14.08 KB, patch)
2014-09-26 15:54 UTC, Bastien Nocera
committed Details | Review
Stop exposing GType structures as such (5.25 KB, patch)
2014-09-26 15:55 UTC, Bastien Nocera
committed Details | Review
object: add a custom hook to run at instance_init (3.33 KB, patch)
2014-09-26 15:55 UTC, Bastien Nocera
committed Details | Review
Gtk: add override to handle GtkWidget templates (6.56 KB, patch)
2014-09-26 15:56 UTC, Bastien Nocera
none Details | Review
Gtk: add override to handle GtkWidget templates (6.42 KB, patch)
2014-09-29 11:02 UTC, Bastien Nocera
committed Details | Review

Description Giovanni Campagna 2013-05-14 22:26:40 UTC
I know, I know, we should not add more custom "convenience"
things and overrides to gjs. But Gtk templates are really
handy when you have custom widgets (which almost every app
has).
In any case, the Gtk specific part of this bug is an override
just for ease of implementation. It could be just a custom
meta class that you call directly (and that apps would copy
around as needed), as long as the first two patches are merged.
Also, this will not introduce any Gtk dependency whatsoever.
The override module is simply ignored if Gtk is not available,
and there are no automated test cases.
Comment 1 Giovanni Campagna 2013-05-14 22:26:42 UTC
Created attachment 244257 [details] [review]
Implement GTypeClass and GTypeInterface arguments

It is common in the GObject world to have non trivial functionality
exposed as GObjectClass/GTypeClass methods, to be used from
class_init. One example is the GtkWidget template system.
This commit implements marshalling of GType structures as
JS classes (constructors), going through the associated GType.
Comment 2 Giovanni Campagna 2013-05-14 22:26:47 UTC
Created attachment 244258 [details] [review]
Stop exposing GType structures as such

Having methods exposed in GType structures like they were boxeds
forces one to use a horrible hack through prototype to call them.
Instead, have those methods installed on the JS classes like
static methods. A hack is still required to call them on subclasses,
but it's smaller.
Comment 3 Giovanni Campagna 2013-05-14 22:26:52 UTC
Created attachment 244259 [details] [review]
Gtk: add override to handle GtkWidget templates

Add a new override module for Gtk, which provides a custom metaclass
to all GtkWidget derived classes.
This metaclass accepts an additional parameter, Template, which is either
a resource:/// URI or a bare XML string, and takes care of calling
gtk_widget_class_set_template() and gtk_widget_init_template()
Comment 4 Colin Walters 2013-07-26 00:43:46 UTC
Created attachment 250163 [details] [review]
[PATCH] object: add a custom hook to run at instance_init
Comment 5 Giovanni Campagna 2014-04-16 07:23:26 UTC
*** Bug 728301 has been marked as a duplicate of this bug. ***
Comment 6 Bastien Nocera 2014-04-16 07:49:37 UTC
Created attachment 274422 [details] [review]
Implement GTypeClass and GTypeInterface arguments

It is common in the GObject world to have non trivial functionality
exposed as GObjectClass/GTypeClass methods, to be used from
class_init. One example is the GtkWidget template system.
This commit implements marshalling of GType structures as
JS classes (constructors), going through the associated GType.
Comment 7 Bastien Nocera 2014-04-16 07:49:43 UTC
Created attachment 274423 [details] [review]
Stop exposing GType structures as such

Having methods exposed in GType structures like they were boxeds
forces one to use a horrible hack through prototype to call them.
Instead, have those methods installed on the JS classes like
static methods. A hack is still required to call them on subclasses,
but it's smaller.
Comment 8 Bastien Nocera 2014-04-16 07:51:12 UTC
Comment on attachment 244259 [details] [review]
Gtk: add override to handle GtkWidget templates

Doesn't apply anymore.
Comment 9 Bastien Nocera 2014-04-16 08:07:27 UTC
With the first 2 patches applied, my test case from bug 728301 (after applying Giovanni's recommended change) kind of works. Running 'Gom.Resource.set_table('items');' works. But it seems to only apply to one instance of the class.

After the "const Item = new Lang.Class()" call:
Gom-Message: pkey: url
Gom-Message: class: 0x12bf7f0

When trying to use an instance of Item with "let item = new Item();":
Gom-Message: pkey: 
Gom-Message: klass: 0x12c1800

Could it be that the class is getting finalized?
Comment 10 Bastien Nocera 2014-09-16 16:27:50 UTC
Created attachment 286310 [details] [review]
Implement GTypeClass and GTypeInterface arguments

It is common in the GObject world to have non trivial functionality
exposed as GObjectClass/GTypeClass methods, to be used from
class_init. One example is the GtkWidget template system.
This commit implements marshalling of GType structures as
JS classes (constructors), going through the associated GType.
Comment 11 Bastien Nocera 2014-09-16 16:27:56 UTC
Created attachment 286311 [details] [review]
Stop exposing GType structures as such

Having methods exposed in GType structures like they were boxeds
forces one to use a horrible hack through prototype to call them.
Instead, have those methods installed on the JS classes like
static methods. A hack is still required to call them on subclasses,
but it's smaller.
Comment 12 Bastien Nocera 2014-09-16 16:28:02 UTC
Created attachment 286312 [details] [review]
object: add a custom hook to run at instance_init

This allows to run code that requires to be inside GTypeInstance
init(), such as gtk_widget_init_template()
Comment 13 Bastien Nocera 2014-09-16 16:28:07 UTC
Created attachment 286313 [details] [review]
Gtk: add override to handle GtkWidget templates

Add a new override module for Gtk, which provides a custom metaclass
to all GtkWidget derived classes.
This metaclass accepts an additional parameter, Template, which is either
a resource:/// URI or a bare XML string, and takes care of calling
gtk_widget_class_set_template() and gtk_widget_init_template()
Comment 14 Bastien Nocera 2014-09-16 16:31:48 UTC
I've rebased all 4 patches. Unfortunately the first one fails to pass its test suite, probably due to other changes in gjs.

This is the backtrace of the crash:
  • #0 __strcmp_ssse3
    at ../sysdeps/x86_64/strcmp.S line 210
  • #1 _g_base_info_find_method
    at girepository/gifunctioninfo.c line 73
  • #2 boxed_new_resolve
    at gi/boxed.cpp line 144
  • #3 js::GetPropertyHelper(JSContext*, JS::Handle<JSObject*>, JS::Handle<long>, unsigned int, JS::MutableHandle<JS::Value>)
    from /lib64/libmozjs-24.so
  • #4 Interpret(JSContext*, js::RunState&)
    from /lib64/libmozjs-24.so
  • #5 js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::AbstractFramePtr, JS::Value*)
    from /lib64/libmozjs-24.so
  • #6 js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*)
    from /lib64/libmozjs-24.so
  • #7 JS::Evaluate(JSContext*, JS::Handle<JSObject*>, JS::CompileOptions, unsigned short const*, unsigned long, JS::Value*)
    from /lib64/libmozjs-24.so
  • #8 JS::Evaluate(JSContext*, JS::Handle<JSObject*>, JS::CompileOptions, char const*, unsigned long, JS::Value*)
    from /lib64/libmozjs-24.so
  • #9 gjs_eval_with_scope
    at gjs/jsapi-util.cpp line 1347
  • #10 gjs_context_eval
    at gjs/context.cpp line 647
  • #11 gjs_context_eval_file
    at gjs/context.cpp line 699
  • #12 test
    at installed-tests/gjs-unit.cpp line 111
  • #13 test_case_run
    at gtestutils.c line 2059
  • #14 g_test_run_suite_internal
    at gtestutils.c line 2120
  • #15 g_test_run_suite_internal
    at gtestutils.c line 2131
  • #16 g_test_run_suite
    at gtestutils.c line 2184
  • #17 g_test_run
    at gtestutils.c line 1488
  • #18 main
    at installed-tests/gjs-unit.cpp line 229

And the valgrind log:
==3039== Invalid read of size 1
==3039==    at 0x4C2FBB3: strcmp (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==3039==    by 0x676E3E4: _g_base_info_find_method (gifunctioninfo.c:73)
==3039==    by 0x4E6170D: ??? (boxed.cpp:144)
==3039==    by 0x7CFB759: ??? (in /usr/lib64/libmozjs-24.so)
==3039==    by 0x7BAB3A6: ??? (in /usr/lib64/libmozjs-24.so)
==3039==    by 0x7BACD48: ??? (in /usr/lib64/libmozjs-24.so)
==3039==    by 0x7BADC32: ??? (in /usr/lib64/libmozjs-24.so)
==3039==    by 0x7C5B3FC: JS::Evaluate(JSContext*, JS::Handle<JSObject*>, JS::CompileOptions, unsigned short const*, unsigned long, JS::Value*) (in /usr/lib64/libmozjs-24.so)
==3039==    by 0x7C5B50B: JS::Evaluate(JSContext*, JS::Handle<JSObject*>, JS::CompileOptions, char const*, unsigned long, JS::Value*) (in /usr/lib64/libmozjs-24.so)
==3039==    by 0x4E58F60: gjs_eval_with_scope (jsapi-util.cpp:1347)
==3039==    by 0x4E533F2: gjs_context_eval (context.cpp:647)
==3039==    by 0x4E53597: gjs_context_eval_file (context.cpp:699)
==3039==  Address 0x4c103000 is not stack'd, malloc'd or (recently) free'd

I'm guessing that the code in boxed.cpp needs some changes to accommodate the new feature.
Comment 15 Bastien Nocera 2014-09-26 14:48:12 UTC
Created attachment 287165 [details] [review]
Gtk: add override to handle GtkWidget templates

Add a new override module for Gtk, which provides a custom metaclass
to all GtkWidget derived classes.
This metaclass accepts an additional parameter, Template, which is either
a resource:/// URI or a bare XML string, and takes care of calling
gtk_widget_class_set_template() and gtk_widget_init_template()
Comment 16 Bastien Nocera 2014-09-26 15:54:04 UTC
The bug mentioned in comment 14 is caused by "Implement GTypeClass and GTypeInterface arguments" but fixed by "Stop exposing GType structures as such". I've made a note of that in the patches' commit messages that I'm about to upload.

I've also fixed the old API used in "add override to handle GtkWidget templates", and *especially* the -1 value for the struct offset which caused a crash, only took me 3h to find :(
Comment 17 Bastien Nocera 2014-09-26 15:54:52 UTC
Created attachment 287185 [details] [review]
Implement GTypeClass and GTypeInterface arguments

It is common in the GObject world to have non trivial functionality
exposed as GObjectClass/GTypeClass methods, to be used from
class_init. One example is the GtkWidget template system.
This commit implements marshalling of GType structures as
JS classes (constructors), going through the associated GType.

Note that this commit breaks the testGObjectClass.js test
Comment 18 Bastien Nocera 2014-09-26 15:55:20 UTC
Created attachment 287189 [details] [review]
Stop exposing GType structures as such

Having methods exposed in GType structures like they were boxeds
forces one to use a horrible hack through prototype to call them.
Instead, have those methods installed on the JS classes like
static methods. A hack is still required to call them on subclasses,
but it's smaller.

Note that this commit fixes the testGObjectClass.js test broken in the
previous commit.
Comment 19 Bastien Nocera 2014-09-26 15:55:37 UTC
Created attachment 287190 [details] [review]
object: add a custom hook to run at instance_init

This allows to run code that requires to be inside GTypeInstance
init(), such as gtk_widget_init_template()
Comment 20 Bastien Nocera 2014-09-26 15:56:09 UTC
Created attachment 287192 [details] [review]
Gtk: add override to handle GtkWidget templates

Add a new override module for Gtk, which provides a custom metaclass
to all GtkWidget derived classes.
This metaclass accepts an additional parameter, Template, which is either
a resource:/// URI or a bare XML string, and takes care of calling
gtk_widget_class_set_template() and gtk_widget_init_template()
Comment 21 Giovanni Campagna 2014-09-26 18:50:15 UTC
Review of attachment 287192 [details] [review]:

::: modules/overrides/Gtk.js
@@ +31,3 @@
+        return true;
+    return false;
+}

I don't remember why I keep handcoding this, but string.startsWith exists and works.

::: test/interactive/testGtk.js
@@ +6,3 @@
+const Lang = imports.lang;
+
+// This is ugly here, but usually it would be in a subclass

I think I originally meant a resource here...
Comment 22 Giovanni Campagna 2014-09-26 18:51:32 UTC
I'm not sure I'm the best person to review the code, given the involvement in the original version.
In any case, if two people went through it, and it passes the test suite, I believe it is ok to land it (especially this early in the cycle and especially given that gjs has no maintainers that will review the feature anyway)
Comment 23 Bastien Nocera 2014-09-26 19:33:55 UTC
(In reply to comment #22)
> I'm not sure I'm the best person to review the code, given the involvement in
> the original version.
> In any case, if two people went through it, and it passes the test suite, I
> believe it is ok to land it (especially this early in the cycle and especially
> given that gjs has no maintainers that will review the feature anyway)

I'll still get to ask you whether I should squash the first 2 patches together so that the test suite doesn't break :)

Other than that, the code seems to work. I'm currently doing changes on the gom side to get it working with gjs.

I'll update the patches after fixing your review comments.
Comment 24 Matthias Clasen 2014-09-26 19:46:11 UTC
please branch and do a 1.42.0 release before landing new feature stuff - we don't have a stable release for 3.14 yet.
Comment 25 Bastien Nocera 2014-09-29 11:02:07 UTC
Created attachment 287334 [details] [review]
Gtk: add override to handle GtkWidget templates

Add a new override module for Gtk, which provides a custom metaclass
to all GtkWidget derived classes.
This metaclass accepts an additional parameter, Template, which is either
a resource:/// URI or a bare XML string, and takes care of calling
gtk_widget_class_set_template() and gtk_widget_init_template()
Comment 26 Bastien Nocera 2014-09-29 11:04:47 UTC
Attachment 287185 [details] pushed as f3c9b6e - Implement GTypeClass and GTypeInterface arguments
Attachment 287189 [details] pushed as 18e82fd - Stop exposing GType structures as such
Attachment 287190 [details] pushed as f90468b - object: add a custom hook to run at instance_init
Attachment 287334 [details] pushed as 3e28510 - Gtk: add override to handle GtkWidget templates
Comment 27 Bastien Nocera 2014-09-29 11:19:14 UTC
(In reply to comment #24)
> please branch and do a 1.42.0 release before landing new feature stuff - we
> don't have a stable release for 3.14 yet.

I've done that before pushing.