GNOME Bugzilla – Bug 700347
Add support for Gtk widget templates
Last modified: 2014-09-29 11:19:14 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.
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.
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.
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()
Created attachment 250163 [details] [review] [PATCH] object: add a custom hook to run at instance_init
*** Bug 728301 has been marked as a duplicate of this bug. ***
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.
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 on attachment 244259 [details] [review] Gtk: add override to handle GtkWidget templates Doesn't apply anymore.
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?
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.
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.
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()
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()
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:
+ Trace 234098
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.
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()
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 :(
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
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.
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()
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()
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...
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)
(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.
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.
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()
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
(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.