Gi signals #1038

kugel- · 2016-05-16T22:24:13Z

This are the changes to geany that allow plugin signals to be used by GI-based plugins (e.g. through peasy).

signal params must be boxed or gobject (I used gboxed)
the GType must be passed to g_signal_new()
since glib 2.30 there is a generic marshaller which can be used. otherwise we'd need a lot of duplicated, new marshaller code. I chosed to use the generic marshaller (by passing NULL to g_signal_new's marshall func)
geanyobject must be exposed to connect to. I chosed to implement a get_instance() function (singleton pattern)

Please review and comment

codebrainz · 2016-05-17T00:40:23Z

IMO, we shouldn't abuse GBoxed like that. Would be better to fix upstream to work properly than spread bug-generating hacks all over. Isn't there also some kind of annotation comment that can be used to hint gir-scanner that the parameter is just a raw pointer with a G_TYPE_POINTER?

I'm not super fond of exposing the GeanyObject either. IMO, if it must be exported to make this work, we shouldn't add Doxygen comments so it remains "private" and we can drop at will whenever we get around to moving those signals to the correct objects.

I think this PR also depends on changes that are being worked on upstream, so the Travis builds fail.

kugel- · 2016-05-17T11:24:21Z

Am 17.05.2016 um 02:40 schrieb Matthew Brush:

IMO, we shouldn't abuse GBoxed like that. Would be better to fix
upstream to work properly than spread bug-generating hacks all over.
Isn't there also some kind of annotation comment that can be used to
hint gir-scanner that the parameter is just a raw pointer with a
|G_TYPE_POINTER|?

Using G_TYPE_POINTER subtypes has absolutely no benefit, apart from
being slightly "prettier" from a design POV.

However, you know upstream bug report[1] which is largely ignored. I've
been told that if I want this fixed I've got to do it myself. GI* stuff
is largely in maintainace mode with little development.

I've looked into the code and I won't be able to implement this support
any time soon. It's really a lot of code to change.

And then it would have to be reviewed and accepted which would take
months, and then years before the fixed program versions reach users
through updated packages.

Since there is no technical benefit I don't see the point of investing
so much of my time into that and delay deployment of my peasy plugin for
some years.

I'm not super fond of exposing the GeanyObject either. IMO, if it /must/
be exported to make this work, we shouldn't add Doxygen comments so it
remains "private" and we can drop at will whenever we get around to
moving those signals to the correct objects.

Why that? My peasy plugin is a normal plugin and wants to use standard
Geany APIs. I thought if plugins require new APIs we consider adding them?

Are there specific plans to "moving those signals to the correct
objects"? These object don't even exist. Unless there are plans to
change the status quo in the near future I think we should just expose
what we've got currently.

I think this PR also depends on changes that are being worked on
upstream, so the Travis builds fail.

That was a mistake and is corrected now. The travis build still fails,
I'll check that later.

codebrainz · 2016-05-17T14:27:06Z

On 2016-05-17 04:24 AM, Thomas Martitz wrote:

Am 17.05.2016 um 02:40 schrieb Matthew Brush:

IMO, we shouldn't abuse GBoxed like that. Would be better to fix
upstream to work properly than spread bug-generating hacks all over.
Isn't there also some kind of annotation comment that can be used to
hint gir-scanner that the parameter is just a raw pointer with a
|G_TYPE_POINTER|?

Using G_TYPE_POINTER subtypes has absolutely no benefit, apart from
being slightly "prettier" from a design POV.

However, you know upstream bug report[1] which is largely ignored. I've
been told that if I want this fixed I've got to do it myself. GI* stuff
is largely in maintainace mode with little development.

I've looked into the code and I won't be able to implement this support
any time soon. It's really a lot of code to change.

And then it would have to be reviewed and accepted which would take
months, and then years before the fixed program versions reach users
through updated packages.

Since there is no technical benefit I don't see the point of investing
so much of my time into that and delay deployment of my peasy plugin for
some years.

There is a technical benefit, you just don't see it yet because there's
no body of plugins to actually show the problem in real life. The very
fact that you can't return NULL from the boxed copy function shows
that PyGI is snarfing away what it thinks are copies of these objects
when in reality it's only getting weak/unowned pointers to them.

For example, what happens when a plugin stores a GeanyDocument in a
list/dict/variable for longer than the document is open?

Is there not some annotation comment or override you can use to guide GI
to the right conclusions? I thought it had some mechanism for this, and
it would be a lot better to add hacks and workarounds in the form of
comments/meta-files than to the source code.

I'm not super fond of exposing the GeanyObject either. IMO, if it /must/
be exported to make this work, we shouldn't add Doxygen comments so it
remains "private" and we can drop at will whenever we get around to
moving those signals to the correct objects.

Why that? My peasy plugin is a normal plugin and wants to use standard
Geany APIs. I thought if plugins require new APIs we consider adding them?

But like the GBoxed hack, the only reason you need to expose this object
in the plugin API is because of a limitation in some code generator
tool. The signals on GeanyObject are already exposed to plugins.

Are there specific plans to "moving those signals to the correct
objects"? These object don't even exist. Unless there are plans to
change the status quo in the near future I think we should just expose
what we've got currently.

Eventually it would be nice to fix this up (see all the threads about
GObjectification of parts of the plugin API). If we expose it publicly
now, we can never do that later.

codebrainz · 2016-05-19T23:37:41Z

Looking at the GI docs, it appears both the type and foreign annotations are designed for this exact use-case, without causing any weird ownership issues in garbage collected languages like the GBoxed hack will do.

codebrainz · 2016-05-19T23:52:48Z

For GeanyObject, it seems like one could use signal doc-comments and annotations to guide GIR scanner, though I have no idea if it actually would.

codebrainz · 2016-05-20T01:38:19Z

I made a simple test repo, GI seems to have no problem dealing with G_TYPE_POINTER signal parameters, and there seems to be no need to expose the actual object structures or special doc-comments or anything. I know it's super trivial, but what are the problems with Geany that requires faking boxed types and exposing the GeanyObject that GI can't cope with?

codebrainz · 2016-05-20T01:45:07Z

src/document.c

@@ -825,7 +825,8 @@ GeanyDocument *document_new_file_if_non_open(void)
 }


-/**
+/** @girskip


Couldn't we just put @girskip on the original document_new_file function comments to have it ignored rather than upsetting the API to work around GI tool quirks?

I want to have the functionality of document_new_file() available for GI-based plugins, obviously. So the idea was to hide document_new_file() from GI and make it available through a new name.

BTW, what is the real issue with document_new_file()? From the API POV it IS a constructor, and in practice it mostly actually is. The only difference is that it doesn't give ownership to the caller, but anyway the only way to destruct a document is to close it, so I don't really see the problem.
Admittedly I didn't test that with GI, but I fail to see the issue, and the current name makes more sense to me, so I'd rather not change it without being forced and convinced.

With more and more and more stuff changing and added to please g-ir-scanner, I'm starting to think maybe a maintained compatibility layer specifically for it would make more sense, leaving current API alone. Sure, some changes for it were real nice for everyone, but some look arbitrary and are just a pain to port to with no benefit and no justification for the non-GI user.

The ownership is one essential difference, yes. I'd rather not have the bindings think they received a reference they own (although the unref/free function does nothing anyway).

The other significant difference is that, depending on the language, the syntax to invoke a constructor is different (not for python, but think of Vala new XXX.file() vs XXX.new_file()).

Maybe other languages put other constraints on constructors. I really wouldn't want GI think it's a constructor when it isn't (in GI's sense).

document_open_file() isn't considered a constructor. Alternatively we can make it one (using the constructor annotation) to enforce correspondence with document_new_file(). But since they aren't true constructors I prefer my proposed change. Unfortunately, there is no annotation to enforce non-constructor behavior.

Mmmkay I see. No new opinion just yet, but I was thinking: new proposed name document_create_file() suggests it creates a file, literally; which ain't the case, it just opens a new buffer not baked to a file (so, quite the contrary). new had less attached meaning to it.

codebrainz · 2016-05-20T01:52:22Z

I think c9f291a will require to bump min. GLib version here and wherever else it's referenced.

codebrainz · 2016-05-20T02:58:00Z

For the sake of comparison I made a branch to show what the minimal changes would look like to make GeanyDocument a real GObject instead of GBoxed: codebrainz@60feae4

It's within a few lines +/- of 07a84b1 and also probably still has lifetime issues (still abusing G* ownership semantics a bit).

codebrainz · 2016-05-20T04:26:22Z

Here's what it would look like to proxy the GeanyObject signals to the GeanyDocument objects in a straightforward manner. The lines of code could be reduced greatly with a few well-placed C preprocessor macros.

kugel- · 2016-05-23T06:26:50Z

So which object would you connect to for getting document-new and document-open? The corresponding GeanyDocument doesn't exist beforehand.

There are also lots of other signals that do not fit simply with new GObjects so I'm not seeing the point yet.

b4n · 2016-05-23T14:12:27Z

src/geanyobject.c

 		G_TYPE_NONE, 1,
-		G_TYPE_POINTER);
+		G_TYPE_KEY_FILE);


Requires GLib >= 2.31 GNOME/glib@9681774

b4n · 2016-05-23T14:36:08Z

For most use case having our signals on an "application" object makes sense, as they are meant as global events. We could have object-specific ones, but some are truly useful as is, not only new and open can't be on a Document object as @kugel- pointed out, but many others are really handy to have globally instead of one handler per document: many plugin don't really care to attach to each document, and rather want the event in any one. This avoids doing stuff like foreach (geany_documents as doc) { doc.connect(...); }, which would also be less optimized, and would require to also geany_app.connect("document-new", (app, doc) => { doc.connect(...) }) and same for new.
My point being that some signals are great for global events and make sense to be on the application/window rather than on each specific document (or alike).

b4n · 2016-06-05T09:59:04Z

src/geanyobject.c

 static void create_signals(GObjectClass *g_object_class)
 {
 	/* Document signals */
 	geany_object_signals[GCB_DOCUMENT_NEW] = g_signal_new (
 		"document-new",
 		G_OBJECT_CLASS_TYPE (g_object_class),
 		G_SIGNAL_RUN_FIRST,
-		G_STRUCT_OFFSET (GeanyObjectClass, document_new),


this indeed is probably useless, but you should probably document why you removed it so future gazers don't wonder.

also, if you really ant to remove this, drop the GeanyObjectClass prototypes that are now not only useless but also unused.

b4n · 2016-06-05T10:21:21Z

I would also like to see how a GI plugin will deal with disconnection of signals at plugin unload. It wouldn't be acceptable to get worse security from a GI plugin than a C one (it calling unloaded code, or even "simply" deinitialized code).

kugel- · 2016-06-05T13:09:24Z

Normally a plugin would connect using its own plugin object, this is what I expect for peasy based ones at least:

def on_document_new(self, doc):
  pass
def do_enable(self):
 self.geany_plugin.geany_data.object.connect("document-new", self.on_document_new)

In this case the signal connection is automatically cleaned up when self is destructed, i.e. when geany (via peasy) unloads the plugin.

I'm not sure we can enforce such behavior for all GI based plugins but I expect the most common cases are handled like above.

Also, we can't enforce it for plain C plugins either (for non-geany_object signals) since those can too always use plain g_object_connect().

b4n · 2016-06-05T13:19:42Z

@kugel- ok, if "something" takes care of disconnecting it all if it references a callback implemented in that language, should be fairly fine. Did you actually try this in a plugin to see how it behaves? Connect stuff to Geany's signals, and more importantly, Scintilla's ones and stress-test that it does behave fine in all cases (when the Scintilla object is destroyed before the plugin, and when the plugin is destroyed before the object).

kugel- · 2016-06-05T13:43:39Z

I tried the above use case without explicitly disconnecting. The scintilla case works differently, since the emitting object is destroyed. Here all signal connections are automatically unlinked, regardless of how/what you connect

codebrainz · 2016-06-06T01:26:17Z

src/document.c

+static void        *copy_(void *src) { return src; }
+static void         free_(void *doc) { }
+
+/** Gets the GBOxed-derived type of GeanyDocument


Should make this and the others @gironly or not doxygen-commented.

b4n · 2016-06-10T22:06:16Z

TODO: fix Travis build.

Probably using the same custom bundle we use to create the Windows installers.

b4n · 2016-06-18T17:23:47Z

@kugel- you can use b4n/geany@049a4d7 (from the windows-cross-newer-glib branch on my fork)

kugel- · 2016-06-19T21:37:49Z

@b4n like this?

b4n · 2016-06-19T21:44:05Z

Yeah, and apparently Travis is happy :)
6678075 at least might better be off squashed into the relevant commits.

b4n · 2016-06-20T17:13:07Z

src/document.c

+
+/** Gets the GType of GeanyDocument
+ *
+ * @return the GeanyDocument type */


hum… shouldn't those be @gironly too just yet?
they aren't really useful types as they don't to any instance management, so they are pointless in C (on advantage to use their type over G_TYPE_POINTER), so what about keeping that off API for the moment?

That would also limit the exposed surface so possibly limiting some future compatibility breakage.

What makes you all think my plugin doesn't want a stable gir interface? It seems to me that there's a notion "let's make it @gironly so we can change it any time", just because these symbols aren't immediately useful for c plugins.
This is not what I wanted. I wanted the gir exposed api to be just a different representation of the original plugin api (baring bugs), with the same stability consideration.

My peasy plugin is depending on the gir in the same way as C API. But you seem to treat it differently, as something that can be broken without consideration.

But if that's how it's acceptable for you, so be it. Can add @gironly to this too.

If you just want your peasy to use public API, make it so. But you need stuff exposed that is only useful for GIR (and even, just because GIR tools are kind of broken). So you add it, makes sense.

And no, I'm not saying the GIRonly part should be unstable and we can break it everytime we get bored. But you yourself admitted we probably want to change these GTypes later on, so as there is zero benefit for anything but GIR, why shoot ourselves in the foot and give it more exposure than it requires? If we change from what the type is derived, it'll basically be both and ABI and API break, because yes, it being boxed matters at least for signal marshallers and other GValue packers, so we can't simply pretend nothing changed. Well, we can, and it'd be fine after a rebuild in 99% of the probable use cases, but it still wouldn't be technically correct. So yes, I propose to lower the annoyance for everyone by making it only affect GIR, in the hope that less code will depend on it, and also that GIR being automated it'd adapt more easily. We'll still have to break API and ABI if we change it, but hopefully it'll only actually require a rebuild and no actual code changes.
Exposing it to people that don't benefit from it only makes it more likely they will use it thinking it's useful (i.e. as to put one of those in a GtkTreeModel?) and suffer later. And as the type is somewhat a lie anyway (it doesn't follow normal GBoxed semantic, so it's no use to properly copy/free it), it at best is useless, and at worse make people think they can hold a copy/ref of their own.

And well. Let's be honest, your requirements for GIR change very often as you progress towards an usable thing. I get it, but it's also true that it can make us wonder whether this won't change in a few months because another issue arose and this one will be outdated and irrelevant. So yeah, compartimenting also makes sense.

And now I think about it yes, until the GIR gets actually used by something that does work (i.e. Peasy), I'm fine considering experimental and unstable. It actually does change very often with your patches, which is good here, but doesn't provide so much stability (although recently I believe it only got extended).
An example going against your rant about it staying "stable" was the change you wanted to introduce renaming document_new(): it would have changed GIR in an incompatible manner, yet you didn't make a big deal out of it.

So. Yes please, unless you convince me there's a valid use case for non-GIR to have those, please mark them GIR only.

b4n · 2016-06-20T17:13:44Z

d80ecf2 should be squashed in b53f38d

GeanyObject signals require GTypes to be gobject-introspection compatible.

geanyobject can be used by plugins to connect to plugin signals directly (required for GI-based plugins). Access through GeanyData::object. The related doxygen comments are @gironly for now, since plugin_signal_connect() is still preferred. Finally, the useless function pointer prototypes are removed from the GeanyObjectClass structure as they became useless (they have been unused and generally wrong since ever).

Updated scintilla_changes.patch accordingly

codebrainz · 2016-06-23T00:29:39Z

scintilla/gtk/ScintillaGTK.cxx

@@ -3250,6 +3250,7 @@ void scintilla_release_resources(void) {
 static void *copy_(void *src) { return src; }
 static void free_(void *doc) { }

+GEANY_API_SYMBOL
 GType scnotification_get_type(void) {
 	static gsize type_id = 0;


I think mentioned this before (maybe on scintilla-interest?), but I don't think there's any point using gsize here and keeping casting from GType to gsize. I think you can just use GType everywhere here (or use gsize without the casts). Not a big deal, but casts are kind of gross, and could cause subtle bugs if/when GType is ever not the same as gsize. No biggie though, it's not our code :)

FWIW, it's (almost) what GObject does: https://git.gnome.org/browse/glib/tree/gobject/gtype.h#n1957

Yeah, that's what I meant by "or use gsize without the casts", though I think it's kind of weird to intentionally use the (semantically) wrong type name for no reason.

It might be because g_once_init_leave() expects a gsize value, and if that API expects a pointer to a gsize-sized value, it is a lot safer to use a real gsize as the pointer and have the cast happen by value, instead of dereferencing a pointer to a GType-sized value as if it was a gsize-sized one.

It's kinda odd g_once_init_*() API claims a void* pointer though, so I don't know.

Am 23. Juni 2016 05:07:10 MESZ, schrieb Matthew Brush notifications@github.com:

@@ -3250,6 +3250,7 @@ void scintilla_release_resources(void) {
static void *copy_(void *src) { return src; }
static void free_(void *doc) { }

+GEANY_API_SYMBOL
GType scnotification_get_type(void) {
static gsize type_id = 0;

Yeah, that's what I meant by "or use gsize without the casts", though I
think it's kind of weird to intentionally use the (semantically) wrong
type name for no reason.

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/1038/files/6b5a47d05f575b321ff158044420864ee9e59889#r68169864

g_once_init_leave() takes a gsize which is stored at the value location. That suggested that the value location should point to an gsize compatible location. This guided me.

Anyway the discussion out of scope for this PR and really minor anyway.

It's kinda odd g_once_init__() API claims a void_ pointer though, so I don't know.

Yeah, and I think even gsize isn't the correct type, it's probably meant to be guintptr, but oh well.

Anyway the discussion out of scope for this PR

Code review is out of scope for this PR? At the least it could do like the code @b4n linked to, and avoid ugly (and useless) casts.

and really minor anyway

This is true

Anyway the discussion out of scope for this PR

Code review is out of scope for this PR? At the least it could do like
the code @b4n https://github.com/b4n linked to, and avoid ugly (and
useless) casts.

Yes, reviewing this code is out of scope for this PR. The code you
question is not changed in this PR and is already part Geany.

This PR just adds the GEANY_API_SYMBOL decoration and the implementation
is irrelevant to the PR.

Well you completely ignored me when I mentioned it on scintilla-interest and I didn't see any PR where it was introduced to Geany, so I mentioned it again here.

If possible, register signals with the proper argument types (boxed or gobject). This is required for successful introspection of the signals and important for GI-based plugins. As for the marshallers, if available use a predefined one from glib. Otherwise use the generic marshaller available since 2.30 (in theory all signals could use that one but it has a bit of overhead). This builds on the gboxed conversions of earlier commits. This also bumps the minimum glib requirement. - g_cclosure_marshal_generic requires 2.30 (if NULL is passed as marshaller to g_signal_new()) - G_TYPE_KEYFILE requires 2.32

Combine the libraries from the GTK3 bundle with GTK from the GTK2 one to get newer GLib & co for GTK2 builds.

Fix signals for GI

kugel- force-pushed the gi-signals branch from 4c766e2 to c9f291a Compare May 17, 2016 11:06

codebrainz reviewed May 20, 2016
View reviewed changes

b4n reviewed May 23, 2016
View reviewed changes

src/geanyobject.c

G_TYPE_NONE, 1,

G_TYPE_POINTER);

G_TYPE_KEY_FILE);

Copy link

Member

b4n May 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requires GLib >= 2.31 GNOME/glib@9681774

kugel- force-pushed the gi-signals branch from c9f291a to a842d18 Compare June 1, 2016 06:08

b4n reviewed Jun 5, 2016
View reviewed changes

codebrainz reviewed Jun 6, 2016
View reviewed changes

b4n added this to the 1.28 milestone Jun 10, 2016

kugel- force-pushed the gi-signals branch from d586ca2 to eb3a1a6 Compare June 19, 2016 22:01

b4n reviewed Jun 20, 2016
View reviewed changes

kugel- added 4 commits June 22, 2016 16:09

plugin api: convert GeanyDocument to GBoxed internally

60fb01d

GeanyObject signals require GTypes to be gobject-introspection compatible.

plugin api: convert GeanyFiletype to GBoxed internally

7702a46

GeanyObject signals require GTypes to be gobject-introspection compatible.

plugin api: convert GeanyEditor to GBoxed internally

be64b17

GeanyObject signals require GTypes to be gobject-introspection compatible.

kugel- force-pushed the gi-signals branch from eb3a1a6 to f88c69e Compare June 22, 2016 14:17

plugin api: export scnotification_get_type

c6f1fad

Updated scintilla_changes.patch accordingly

kugel- force-pushed the gi-signals branch from f88c69e to 6b5a47d Compare June 22, 2016 14:19

codebrainz reviewed Jun 23, 2016
View reviewed changes

kugel- and others added 2 commits June 23, 2016 22:33

scripts/cross-build-mingw.sh: Use newer support libraries with GTK2

ec15b6f

Combine the libraries from the GTK3 bundle with GTK from the GTK2 one to get newer GLib & co for GTK2 builds.

kugel- force-pushed the gi-signals branch from 6b5a47d to ec15b6f Compare June 23, 2016 20:33

b4n merged commit ec15b6f into geany:master Jun 23, 2016

b4n added a commit that referenced this pull request Jun 23, 2016

Merge pull request #1038 from kugel-/gi-signals

5ad6316

Fix signals for GI

Gi signals #1038

Gi signals #1038

Conversation

kugel- commented May 16, 2016

codebrainz commented May 17, 2016

kugel- commented May 17, 2016

codebrainz commented May 17, 2016

codebrainz commented May 19, 2016

codebrainz commented May 19, 2016

codebrainz commented May 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codebrainz commented May 20, 2016

codebrainz commented May 20, 2016

codebrainz commented May 20, 2016 • edited

kugel- commented May 23, 2016

Choose a reason for hiding this comment

b4n commented May 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b4n commented Jun 5, 2016

kugel- commented Jun 5, 2016

b4n commented Jun 5, 2016

kugel- commented Jun 5, 2016 via email

Choose a reason for hiding this comment

b4n commented Jun 10, 2016

b4n commented Jun 18, 2016

kugel- commented Jun 19, 2016

b4n commented Jun 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b4n commented Jun 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codebrainz commented May 20, 2016 •

edited