GNOME Bugzilla – Bug 794345
forward_display_line doesn't pass the argument by reference
Last modified: 2018-03-22 17:26:15 UTC
Gtk.TextView Vala binding should pass arguments of `forward_display_line`, `forward_display_line_end`, and possibly of some other functions as references. Currently, they are passed by copy, and so the method side-effect is lost, unlike when working with GTK and C directly, where the argument is passed by pointer (and thus the function works as expected). When I edited the VAPI to include the `ref` keyword, it worked as expected. I am willing to find all the bugs of this type in the TextView bindings, and submit a commit, when this bug gets confirmed. Also, since this would be my first contribution, a little help on getting started with committing would be very appreciated. A minimal example and some discussion on this bug can be found here: https://gitlab.gnome.org/GNOME/gtk/issues/78
Hi, Thanks for your interest in contributing back to Vala. Overview of How the GTK+3 Bindings are Generated ------------------------------------------------ Since Vala 0.40+ the Vala bindings are generated from GObject Introspection. The process is: 1. The GTK+ source code has GObject annotations. For an example, see the comments above gtk_text_view_forward_display_line in https://gitlab.gnome.org/GNOME/gtk/blob/master/gtk/gtktextview.c For reference the documentation for the annotations is: https://wiki.gnome.org/Projects/GObjectIntrospection/Annotations/ 2. The GTK+ build system runs the g-ir-scanner program to extract the annotations and build a GObject Introspection Repository (GIR) XML file. The source GIR for Vala bindings is frequently updated and is here: https://github.com/nemequ/vala-girs/blob/master/gir-1.0/Gtk-3.0.gir 3. The Vala build system has a target to make the VAPI. The build file is: https://git.gnome.org/browse/vala/tree/vapi/Makefile.am Look for the gtk+-3.0 target. It uses the vapigen program with the options: vapigen --library $(srcdir)/gtk+-3.0 --pkg atk --pkg gdk-3.0 --pkg gdk-pixbuf-2.0 --pkg gio-2.0 --pkg pango --pkg cairo --metadatadir $(METADATADIR) $(METADATADIR)/Gtk-3.0-custom.vala $(GIRDIR)/Gtk-3.0.gir Best Solution for Fixing the TextView Bindings ---------------------------------------------- This is simply to add (inout) to the C source code for gtk_text_view_forward_display_line. So in https://gitlab.gnome.org/GNOME/gtk/blob/master/gtk/gtktextview.c the annotation: * @iter: A #GtkTreeIter. is changed to: * @iter: (inout): A #GtkTreeIter. For a working example see gdk_keymap_add_virtual_modifiers () in: https://gitlab.gnome.org/GNOME/gtk/blob/master/gdk/gdkkeys.c and how that produces the ref modifier in the VAPI: https://git.gnome.org/browse/vala/tree/vapi/gdk-3.0.vapi Modifying the C source annotations is the best solution because it will filter through to all bindings, not just Vala. The disadvantage is that is takes time for it to filter back to Vala. I think the change will probably be backported to earlier Vala release series, like 0.38, though. The difficulty is testing this. You have to generate the GIR and then use vapigen to generate the VAPI. -- I hope that is enough information to get you on the way. If there are any problems let me know, Al
(In reply to Al Thomas from comment #1) Hope you have time, if you've done already, add a Wiki page to explain this process to all Vala GIR based bindings.
(In reply to Daniel Espinosa from comment #2) > Hope you have time, if you've done already, add a Wiki page to explain this > process to all Vala GIR based bindings. The page is: https://wiki.gnome.org/Projects/Vala/Bindings and linked to from: https://wiki.gnome.org/Projects/Vala/DeveloperDocumentation They do need some updating though.
Thanks for the info! I've reopened the GTK+ bug on GitLab, now that we know for sure it's on that side and what to do. Aleksandar or I can cook up the patch. While this should definitely be fixed at the source, i.e. GTK+, shall we keep it open for the possibility of backporting to older Vala APIs?
(In reply to Daniel Boles from comment #4) > While this should definitely be fixed at the source, i.e. GTK+, shall we > keep it open for the possibility of backporting to older Vala APIs? Yes, keep it open. Once we know how many functions are changed upstream then we can modify the older bindings. Before Vala 0.40 the bindings were generated using a different method so I'll have a go at a patch if nobody else gets there first ;) For the sake of completeness any function marked as ref in this file: https://git.gnome.org/browse/vala/tree/vapi/metadata/Gtk-3.0.metadata should be changed to (inout) in the C source annotations. An example is: TextBuffer .delete.start ref .delete.end ref .delete_interactive.start_iter ref .delete_interactive.end_iter ref .insert*.iter ref .insert_text.pos ref .insert_text#signal.location ref So the first one is gtk_text_buffer_delete and its start parameter. I'm not sure how this affects other bindings, but (inout) is documented so they should cope with it. I had a quick look at Python: https://lazka.github.io/pgi-docs/index.html#Gdk-3.0/classes/Keymap.html#Gdk.Keymap.add_virtual_modifiers That sets the return value as the argument passed in, although in C it returns a boolean. I know Python can return tuples, but I'm not sure why it isn't a tuple of the boolean and state. TextBuffer.delete doesn't return anything at present in the Python bindings: https://lazka.github.io/pgi-docs/Gtk-3.0/classes/TextBuffer.html#Gtk.TextBuffer.delete
Backporting "ref" parameter changes is quite problematic while it is a hard break of the vapi. This specific TreeIter parameter change already caused some issues in the past which is the reason I avoided backporting them. Please get this fixed with g-i annotations and confirmed upstream.
(In reply to Rico Tzschichholz from comment #6) > Backporting "ref" parameter changes is quite problematic while it is a hard > break of the vapi. This specific TreeIter parameter change already caused > some issues in the past which is the reason I avoided backporting them. No idea how I managed to copy and paste #GtkTreeIter instead of #GtkTextIter, but we are talking about GtkTextIter. Of course that will still break older VAPIs. My plan of action is: 1. Wait for the good folks to fix the annotations upstream. A bonus if any additional (inout) annotations are added 2. Patch the current Gtk-3.0.metadata if any bonus (inout) parameters upstream get added, basically remove the ref lines 3. Patch the old metadata for `forward_display_line` and `forward_display_line_end`. Given these don't seem to work at present a breaking change shouldn't be too hard. Any serious user would have to set a minimum version of Vala in their build system or make the feature conditional in their code. This is a trade off against not having the feature until 0.40.1 gets released for that distribution. Some distributions are some way behind. What is the consensus on breaking this? For reference the last TextIter breaking change seems to be in 2014: https://git.gnome.org/browse/vala/commit/vapi/packages/gtk+-3.0/gtk+-3.0.metadata?h=0.38&id=a9a11d23aab730aa67a8be7d0fbc92e4d94beab9 I think this is probably the commit Rico is referring to from Feb 2017 that relates to GtkTreeIter: https://git.gnome.org/browse/vala/commit/vapi/packages/gtk+-3.0/gtk+-3.0.metadata?h=0.38&id=ddca99ea2f25b9a734831589ec209df5322520a4
Discussion on the GTK+ side has revealed a can of worms about adding (inout) there. In short, while it'll work for Vala, it seems to crash PyGObject and isn't anticipated to work well for GJs. There may be others. It seems all to stem from no one really being sure of the meaning of (inout) or an agreed way to handle it. See https://gitlab.gnome.org/GNOME/gobject-introspection/issues/192 for that. So, it might well be that the quicker fix to make these usable for people in Vala is to patch the VAPI, if you decide you're willing to do that. fwiw, here are TextView and TextLayout symbols I noticed don't get ref'd as they should, making them useless in Vala. I didn't go out and look for any others, but they may well be out there... https://gitlab.gnome.org/GNOME/gtk/merge_requests/63/diffs?commit_id=2786f1ffdad8fd9fd799ec28b0cef4041bab7ff9#29c32661da885d6d4476fa28c5d6f28584a30345_3209_3209
The latest bit of the discussion, including problems with PyGI/gjs - and a suggestion to make Vala put ref on boxed types like these by default(?) - is on the original GTK+ issue: https://gitlab.gnome.org/GNOME/gtk/issues/78#note_82073
Created attachment 370016 [details] [review] gtk+-*.0: Fix iter parameter of TextView.backward*/forward*/move_visually() Those methods are changing the passed iter structure and therefore it needs be marked as ref for vala taking this into account.
Ok, since changing the annotation would have a bad effect on other languages, we will need to fix this with metadata in vala. I am hoping this covers the methods changing its TextIter parameter.
I noticed a bunch in TextLayout too: https://gitlab.gnome.org/GNOME/gtk/merge_requests/63/diffs?commit_id=2786f1ffdad8fd9fd799ec28b0cef4041bab7ff9#29c32661da885d6d4476fa28c5d6f28584a30345 It'd make sense to change these at the same time if possible as they are so closely related.
Those are not actual public API and not part of the vapis.
Attachment 370016 [details] pushed as 0050ca7 - gtk+-*.0: Fix iter parameter of TextView.backward*/forward*/move_visually()