GNOME Bugzilla – Bug 572408
Have interface X static methods as methods of the XIface struct
Last modified: 2015-02-07 16:55:13 UTC
For instance in Gio-2.0.gir, the File interface static methods (file_*) are declared as functions in the Gio namespace. Also there is no additionnal information about the fact they relate to the File interface. These should be provided as methods in the FileIface struct. Other information:
They are generated as methods on the File interface here. Are you using trunk of gobject-introspection? <interface name="File" c:type="GFile" doc="A handle to an object implementing the #GFileIface interface. Generally stores a location within the file system. Handles do not necessarily represent files or directories that currently exist." glib:type-name="GFile" glib:get-type="g_file_get_type"> <method name="dup" c:identifier="g_file_dup"> <return-value transfer-ownership="full"> <type name="File" c:type="GFile*"/> </return-value> </method> ... </interface>
I do use trunk. Do the following grep : grep "function name=\"file_" gir/Gio-2.0.gir and you'll have : <function name="file_new_for_path" c:identifier="g_file_new_for_path"> <function name="file_new_for_uri" c:identifier="g_file_new_for_uri"> <function name="file_new_for_commandline_arg" <function name="file_parse_name" c:identifier="g_file_parse_name"> <function name="file_hash" c:identifier="g_file_hash"> which are all static methods of the File interface.
I don't agree that they should be on the interface, instead I see them as factory functions. Which in my opinion would be the sanest way to wrap in a language binding.
The idea of the .gir is to describe the C libraries on a high level to allow any language to bind the library in a way that is appropriate for the specific language. g_file_new_for_path and the other g_file_* functions clearly belong to the GFile interface and this information should be kept in the .gir file in one way or another. This is completely unrelated to whether language binding X supports static methods on interfaces. Ignoring the association with the interface is always possible, however, language bindings should not need to infer the association, e.g., using C naming heuristics. The .gir file (and typelib) should make it easy to provide any kind of binding.
Created attachment 129097 [details] [review] A patch to push interface static methods as 'function' nodes in 'interface' nodes in the gir file This would still require a typelib patch.
It's really arguable if anything returning a GFile* should be on the interface. The actual object returned by the functions mentioned above are returning objects implementing GFiles. A similar situation is gst_element_factory_make() which return GStreamer element subclasses instead of elements. There are other examples in Gtk+, for instance gtk_window_list_toplevels() shouldn't necessary be a static method on GtkWindow. It's merely function returning all global windows. Guess we can argue about this forever...
The questions to me with stuff like this in general are: 1) How do we want existing things to appear in bindings? (This is a binding-specific question) 2) Is this something we expect C authors to do in the future? Or are we just being compatible with older code (this would imply a warning when we add warning support) Trying to answer this: 1) For these "interface static methods", I can say for JGIR it's not a big deal, we'll punt them to the "FooGlobals" class we use for all global methods. Possibly there could be an inner class like "GFile.Methods" or "GFile.Statics", but I don't have a strong opinion. How well does this work in JavaScript? Can it just be Gtk.Window.list_toplevels()? Is that how we want it to be bound or not? 2) I'd like to discourage this, and warn on it in fact, but that's open to debate.
Another way to take on it - do we know of cases where this patch would do something that we rough-consensus agree is wrong? One data point we could also consider is whether the function appears in the same C header file where we found the _get_type function. If not, that to my mind makes it more likely the function should just be a global. Finally, we need to either get this patch in before declaring introspection formats stable, or reject it (and consider adding annotations so that C authors can make this association if desired). I lean towards getting it in, but we should review the diffs to gir-repository.
(In reply to comment #8) > One data point we could also consider is whether the function appears in the > same C header file where we found the _get_type function. If not, that to my > mind makes it more likely the function should just be a global. I would say, if one makes a binding generator for an aspect-oriented language, he may want to have that information, even if the _get_type function is not here. This is called "inner-type declaration" in aspect-oriented languages. A similar mechanism exists in C# and is called "extension methods". As to complete this comment, there is great chance that Vala supports inner-type declaration.
I'd like to revive this as I also think that class-static methods should be put in their proper namespace. Yes, things like g_file_new_for_uri are probably best thought of as factory functions, but they should still be namespaced as Gio.File.new_for_uri instead of Gio.file_new_for_uri even if just for orthographic reasons: "file_new_for_uri" is not a proper method name. I'll add an updated patch and the requested differences between the generated .gir files.
Created attachment 150606 [details] [review] Updated patch to let interfaces have static methods
Created attachment 150607 [details] [review] Differences between generated .gir files Apparently, only two files were affected by this change: Gio-2.0.gir and Atk-1.0.gir. The changes in Gio-2.0.gir all seem to make sense to me. But the changes in Atk-1.0.gir are not intentional: AtkTextAttribute functions are interpreted as static methods for AtkText. I think this happens because we don't allow enums to have static methods, and so the fallback loop in GLibTransformer._parse_static_method associates functions like atk_text_attribute_register with AtkText. This problem would go away if we allowed enums and the like to have static methods, which I think we should.
One alternative to letting the scanner assume that all methods returning a specific class/interface belongs to the corresponding class/interface is to use an annotation in the cases which that is true. In my opinion the GFile examples are cases which should belong to the interface, at least new_for_path/new_for_uri/new_for_command_line arg. I'm unsure about hash and parse_name. I propose that an annotation is added called (static obj), for instance: /** * g_file_new_for_uri: * * Attributes: (static-method-of GFile) */ Not sure if we should re-use attributes or create a new tag.
As far as I know, this is not about the type a function returns. What I would want, and what the patch tries to do, is to have functions with a g_file_ prefix associated with the Gio.File namespace. Irrespective of the return type. This is what's already happening for class-static functions on objects. For example, gdk_display_get_default is mapped to Gdk.Display.get_default and gtk_window_list_toplevels is mapped to Gtk.Window.list_toplevels. This bug is only about interfaces receiving the same treatment.
Comment on attachment 150606 [details] [review] Updated patch to let interfaces have static methods This patch doesn't modify the test suite and thus can't be complete.
So a high level concern with this at this point is backwards compatibility; if we move the methods to Gio.File that will break existing code. Now I'm broadly in favor of the concept but we need to figure out how to do this in a way that will be the least disruptive. My thought on that is that we need to coalesce API/ABI breaking changes as much as possible.
Created attachment 152174 [details] [review] Updated patch, now including tests Here's an updated patch that now also includes tests. I'll attach another patch and then the list of changes in the installed *.gir files.
Created attachment 152175 [details] [review] Don't incorrectly associate certain functions with the wrong namespace Here's a fix to association heuristic to avoid mapping, say, atk_text_attribute_register to Atk.Text.attribute_register just because Atk.Text.Attribute is an enum.
Created attachment 152177 [details] [review] Differences between generated .gir files And here's an updated set of differences between the generated installed *.gir files. In summary: * atk_relation_type_register & co. are mapped to Atk.relation_type_register instead of Atk.Relation.type_register * atk_text_free_ranges is mapped to Atk.Text.free_ranges instead of Atk.text_free_ranges (I actually don't understand why this didn't happen before, as well) * g_app_info_create_from_commandline & co. are mapped to Gio.AppInfo.create_from_commandline instead of Gio.app_info_create_from_commandline * similarly for g_async_initable_new_async, g_file_new_for_path, g_icon_hash, g_initable_new and their respective friends * gst_child_proxy_lookup & co. are mapped to Gst.ChildProxy.lookup instead of Gst.child_proxy_child_added * similarly for gst_implements_interface_cast and friend * gst_mixer_message_get_type & co. are mapped to GstInterfaces.Mixer.message_get_type instead of GstInterfaces.gst_mixer_message_get_type * similarly for gst_navigation_query_get_type and friends * gtk_container_class_find_child_property & co. are mapped to Gtk.container_class_find_child_property instead of Gtk.Container.class_find_child_property (since GtkContainerClass is registered as a record, and thus cannot currently have class-static methods) * gtk_tooltips_data_get is mapped to Gtk.tooltips_data_get instead of Gtk.Tooltips.data_get (since GtkTooltipsData is registered as a record) * pango_font_description_from_string is mapped to Pango.font_description_from_string instead of Pango.Font.description_from_string (both variants are actually incorrect, but this seems unrelated to this patch set, see bug #595889) * pango_cairo_font_map_new & co. are mapped to PangoCairo.FontMap.new instead of PangoCairo.font_map_new All of these seem to me like they improve the correctness of the API representations. This could still be further improved by allowing enums, bitfields and records to have class-static methods. Then, for example, atk_relation_type_register would be mapped to Atk.RelationType.register -- which seems perfect to me.
So, what's the status of this? I think it should be decided soon, to avoid annoying to many people with an "API" change. And I'm still very much convinced that allowing interfaces to have class-static methods is the correct thing to do.
The more I think about it, I would really like to do this. But it will be very disruptive for consumers now. So, options as I see it are: * Bite the bullet, ask people to port * Create a new typelib/gir version (and we'd need to figure out how bindings opt-in to this...painful) * Middle ground, have *both* versions of the functions in the .gir and the .typelib, but the old one is marked as "legacy", and ideally bindings generate a warning on the first use of one of these functions.
Ok, let's do this before 3.0. We will need to bump the typelib version at least.
A patch which added this was committed to HEAD, I reverted it since it needs further discussion about migration plans etc. 1) Changes the API without discussing 2) Removes g_file_new_* from API completely. They should be present somewhere in the api, preferable on the GFile interface.
After some discussion we decided to do static methods on interfaces as a separate change. I do want to do this before 1.0 though.
Created attachment 169692 [details] [review] Handle static methods on all types This patch should be right; but it needs to have test cases added before it is landed. Note that it is short because dead-code to write static methods was left in girwriter.py when we went back to the GLIbObject only behavior.
Review of attachment 169692 [details] [review]: I think we need to write out both, and flag the old ones as "compatibility" or something like that.
see also https://bugzilla.gnome.org/show_bug.cgi?id=646742, which is related and probably conflicting
Created attachment 193642 [details] [review] Handle static methods on all types Instead of just handling static methods for GLibObject subclasses, handle them for: - All classes - Records and boxed - Unions - Interfaces Based on a patch by Owen Taylor.
Here's a new attempt at this. Instead of moving the functions to their correct place (and thus break API), we copy it there instead. Also, if the resulting method would have an empty name, we don't do anything. The changes to the generated *.gir files look sensible. For example, in Gtk-3.0.gir, the following changes occur: * gtk_binding_entry_add_signal becomes Gtk.BindingEntry.add_signal, similarly for gtk_binding_entry_remove * gtk_paper_size_get_default becomes Gtk.PaperSize.get_default, similarly for gtk_paper_size_get_paper_sizes * gtk_rc_property_parse_* becomes Gtk.RcProperty.parse_* * gtk_tree_row_reference_deleted becomes Gtk.TreeRowReference.deleted, similarly for the other proxy things in there
Review of attachment 193642 [details] [review]: It might be nice to have an XML attribute pointing to the non-deprecated function. In the generated DocBook I think we want to point users to the new one. ::: giscanner/maintransformer.py @@ +997,3 @@ return False (node, funcname) = split + if (funcname == ''): Python doesn't need the parens. @@ +1011,3 @@ + new_func = copy.copy(func) + new_func.name = funcname + node.static_methods.append(new_func) We need to return True actually in both these cases. It was a latent bug from before. And probably return False if neither case is true.
Created attachment 193750 [details] [review] Updated patch Here's an updated patch incorporating Colin's suggestions, and also fixing another bug: we need to copy the parameters list explicitly so that any change to one of the functions does not propagate to the other.
Created attachment 193753 [details] [review] Add a moved_to property scanner: add a moved_to property to backcompat functions Use it to remove backcompat copies of functions that non introspectable anyway.
Review of attachment 193753 [details] [review]: Looks good; there are no tests affected by this change? Might be nice to add one. ::: giscanner/introspectablepass.py @@ +229,3 @@ + if isinstance(obj, ast.Function): + if not obj.introspectable: + if obj.moved_to is not None: Could compress this to: if (isinstance(obj, ast.Function) and not obj.introspectable and obj.moved_to is not None): parens in Python are necessary for multi-line if statements. @@ +230,3 @@ + if not obj.introspectable: + if obj.moved_to is not None: + print "*** removing %s (moved to %s)" % (obj.name, obj.moved_to) Don't forge to remove this debugging print.
Review of attachment 193750 [details] [review]: This looks good.
Both patches committed with the suggestions incorporated.
[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]