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 572408 - Have interface X static methods as methods of the XIface struct
Have interface X static methods as methods of the XIface struct
Status: RESOLVED FIXED
Product: gobject-introspection
Classification: Platform
Component: general
2.18.x
Other All
: Normal minor
: ---
Assigned To: gobject-introspection Maintainer(s)
gobject-introspection Maintainer(s)
Depends on:
Blocks: 559704
 
 
Reported: 2009-02-19 13:34 UTC by Didier "Ptitjes"
Modified: 2015-02-07 16:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
A patch to push interface static methods as 'function' nodes in 'interface' nodes in the gir file (2.54 KB, patch)
2009-02-19 21:01 UTC, Didier "Ptitjes"
none Details | Review
Updated patch to let interfaces have static methods (4.36 KB, patch)
2009-12-31 12:10 UTC, Torsten Schoenfeld
needs-work Details | Review
Differences between generated .gir files (28.76 KB, patch)
2009-12-31 12:16 UTC, Torsten Schoenfeld
none Details | Review
Updated patch, now including tests (7.66 KB, patch)
2010-01-24 20:26 UTC, Torsten Schoenfeld
none Details | Review
Don't incorrectly associate certain functions with the wrong namespace (2.09 KB, patch)
2010-01-24 20:28 UTC, Torsten Schoenfeld
none Details | Review
Differences between generated .gir files (82.73 KB, patch)
2010-01-24 20:51 UTC, Torsten Schoenfeld
none Details | Review
Handle static methods on all types (1.43 KB, patch)
2010-09-07 17:20 UTC, Owen Taylor
reviewed Details | Review
Handle static methods on all types (4.34 KB, patch)
2011-08-11 14:35 UTC, Torsten Schoenfeld
reviewed Details | Review
Updated patch (5.73 KB, patch)
2011-08-13 10:26 UTC, Torsten Schoenfeld
committed Details | Review
Add a moved_to property (4.01 KB, patch)
2011-08-13 10:46 UTC, Torsten Schoenfeld
committed Details | Review

Description Didier "Ptitjes" 2009-02-19 13:34:26 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:
Comment 1 Johan (not receiving bugmail) Dahlin 2009-02-19 15:46:40 UTC
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>
Comment 2 Didier "Ptitjes" 2009-02-19 15:57:30 UTC
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.
Comment 3 Johan (not receiving bugmail) Dahlin 2009-02-19 17:09:31 UTC
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.
Comment 4 Jürg Billeter 2009-02-19 20:49:02 UTC
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.
Comment 5 Didier "Ptitjes" 2009-02-19 21:01:40 UTC
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.
Comment 6 Johan (not receiving bugmail) Dahlin 2009-02-19 21:27:41 UTC
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...
Comment 7 Colin Walters 2009-02-23 20:41:15 UTC
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.





Comment 8 Colin Walters 2009-03-13 16:27:37 UTC
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.
Comment 9 Didier "Ptitjes" 2009-03-14 18:38:42 UTC
(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.
Comment 10 Torsten Schoenfeld 2009-12-31 12:02:59 UTC
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.
Comment 11 Torsten Schoenfeld 2009-12-31 12:10:22 UTC
Created attachment 150606 [details] [review]
Updated patch to let interfaces have static methods
Comment 12 Torsten Schoenfeld 2009-12-31 12:16:46 UTC
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.
Comment 13 Johan (not receiving bugmail) Dahlin 2010-01-23 14:25:05 UTC
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.
Comment 14 Torsten Schoenfeld 2010-01-23 18:21:10 UTC
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 15 Colin Walters 2010-01-23 19:54:12 UTC
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.
Comment 16 Colin Walters 2010-01-23 19:59:17 UTC
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.
Comment 17 Torsten Schoenfeld 2010-01-24 20:26:20 UTC
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.
Comment 18 Torsten Schoenfeld 2010-01-24 20:28:55 UTC
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.
Comment 19 Torsten Schoenfeld 2010-01-24 20:51:43 UTC
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.
Comment 20 Torsten Schoenfeld 2010-04-24 15:14:49 UTC
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.
Comment 21 Colin Walters 2010-04-24 17:24:54 UTC
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.
Comment 22 Colin Walters 2010-06-02 14:53:54 UTC
Ok, let's do this before 3.0.

We will need to bump the typelib version at least.
Comment 23 Johan (not receiving bugmail) Dahlin 2010-09-06 20:10:41 UTC
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.
Comment 24 Colin Walters 2010-09-07 16:21:52 UTC
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.
Comment 25 Owen Taylor 2010-09-07 17:20:37 UTC
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.
Comment 26 Colin Walters 2011-05-25 15:26:37 UTC
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.
Comment 27 Dan Winship 2011-05-25 15:37:21 UTC
see also https://bugzilla.gnome.org/show_bug.cgi?id=646742, which is related and probably conflicting
Comment 28 Torsten Schoenfeld 2011-08-11 14:35:36 UTC
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.
Comment 29 Torsten Schoenfeld 2011-08-11 14:52:28 UTC
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
Comment 30 Colin Walters 2011-08-11 15:58:13 UTC
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.
Comment 31 Torsten Schoenfeld 2011-08-13 10:26:53 UTC
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.
Comment 32 Torsten Schoenfeld 2011-08-13 10:46:51 UTC
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.
Comment 33 Colin Walters 2011-08-13 12:16:45 UTC
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.
Comment 34 Colin Walters 2011-08-13 12:18:18 UTC
Review of attachment 193750 [details] [review]:

This looks good.
Comment 35 Torsten Schoenfeld 2011-08-13 13:05:42 UTC
Both patches committed with the suggestions incorporated.
Comment 36 André Klapper 2015-02-07 16:55:13 UTC
[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]