[geany/geany] Gi signals (#1038)

List overview All Threads

newer

older

[geany/geany] tagmanager: Fix UB...

[geany-plugins] scope: Fix...

Thomas Martitz

17 May 2016 17 May '16

12:24 a.m.

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 You can view, comment on, or merge this pull request online at:

https://github.com/geany/geany/pull/1038

-- Commit Summary --

* plugin api: convert GeanyDocument to GBoxed internally * plugin api: convert GeanyFiletype to GBoxed internally * plugin api: convert GeanyEditor to GBoxed internally * plugin api: rename document_new_file to _create_file * plugin api: export geanyobject

-- File Changes --

M plugins/geanyplugin.h (1) M src/Makefile.am (1) M src/document.c (32) M src/document.h (4) M src/editor.c (11) M src/editor.h (4) M src/filetypes.c (11) M src/filetypes.h (3) M src/geanyobject.c (231) M src/geanyobject.h (3)

-- Patch Links --

https://github.com/geany/geany/pull/1038.patch https://github.com/geany/geany/pull/1038.diff

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1038

Attachments:

attachment.htm (text/html — 3.5 KB)

Show replies by date

Matthew Brush

17 May 17 May

2:40 a.m.

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.

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1038#issuecomment-219589344

Thomas Martitz

1:24 p.m.

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.

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1038#issuecomment-219690234

Matthew Brush

4:27 p.m.

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.

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1038#issuecomment-219734668

Matthew Brush

20 May 20 May

1:50 a.m.

Looking at the [GI docs](https://wiki.gnome.org/action/show/Projects/GObjectIntrospection/Annotations...), 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.

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1038#issuecomment-220480921

Matthew Brush

2 a.m.

For `GeanyObject`, it seems like one could use [signal doc-comments](https://wiki.gnome.org/Projects/GTK%2B/DocumentationSyntax#Signals) and annotations to guide GIR scanner, though I have no idea if it actually would.

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1038#issuecomment-220483000

Matthew Brush

3:38 a.m.

I made a [simple test repo](https://github.com/codebrainz/gitest), 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?

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1038#issuecomment-220496575

Matthew Brush

3:45 a.m.

...

@@ -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?

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1038/files/07a84b11982327f7aa1dbbd3129bf...

Matthew Brush

3:52 a.m.

I think c9f291ae4015a3bb8d6ea142b1beb6c9fcc43129 will require to bump min. GLib version [here](https://github.com/geany/geany/blob/master/configure.ac#L77) and wherever else it's referenced.

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1038#issuecomment-220498231

Matthew Brush

4:58 a.m.

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: https://github.com/codebrainz/geany/commit/60feae4be48835e5c74f7c5c1adbf0070...

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

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1038#issuecomment-220505695

Matthew Brush

6:26 a.m.

[Here's what it would look like](https://github.com/codebrainz/geany/commit/2a594aa7f0a5f139e10ee22daa48fc5c5...) 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.

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1038#issuecomment-220514425

Thomas Martitz

23 May 23 May

8:12 a.m.

...

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

-/** +/** @girskip

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.

Thomas Martitz

8:26 a.m.

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.

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1038#issuecomment-220896822

Colomban Wendling

3:32 p.m.

...

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

-/** +/** @girskip

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.

Thomas Martitz

4:02 p.m.

...

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

-/** +/** @girskip

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).

Thomas Martitz

4:09 p.m.

...

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

-/** +/** @girskip

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.

Colomban Wendling

4:12 p.m.

...

G_TYPE_NONE, 1,

```
G_TYPE_POINTER);
```

```
G_TYPE_KEY_FILE);
```

Requires GLib >= 2.31 https://github.com/GNOME/glib/commit/96817746d999ce34b4fd3471b3c58d3a6be2dcb...

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1038/files/bdd0720128df6afda95851dd529ba...

Colomban Wendling

4:18 p.m.

...

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

-/** +/** @girskip

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.

Colomban Wendling

4:23 p.m.

...

geany_object_signals[GCB_DOCUMENT_FILETYPE_SET] = g_signal_new ( "document-filetype-set", G_OBJECT_CLASS_TYPE (g_object_class), G_SIGNAL_RUN_FIRST,
G_STRUCT_OFFSET (GeanyObjectClass, document_filetype_set),
NULL, NULL,
geany_cclosure_marshal_VOID__POINTER_POINTER,
0, NULL, NULL, g_cclosure_marshal_generic,

shouldn't you simply pass `NULL` as the marshaller instead of explicitly passing the generic one? Clearer for me (as I'm not fooled to see there's a specific one selected), shorter, and there doesn't seem to be no use for passing it explicitly, is it?

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1038/files/c9f291ae4015a3bb8d6ea142b1beb...

Colomban Wendling

4:29 p.m.

...

@@ -380,3 +389,13 @@ GObject *geany_object_new(void) { return g_object_new(GEANY_OBJECT_TYPE, NULL); }

+/** Get the global GeanyObject instance

@return @transfer{none} The instance

*/

+GEANY_API_SYMBOL +GObject *geany_object_get_instance(void)

meh. If *really* you can't use a custom `signal_connect()` (why, as anyway you need to fake the object?), I'd really rather add the object to `GeanyData` or something like that than a `get_instance()` method (like main widgets, etc.).

It's even nicer API-wise IMO to do `self.geany_data.object.connect(...)` than `Geany.Object.get_instance().connect(...)`

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1038/files/df4e17e853e84a23f4fe27ba9bca0...

Thomas Martitz

4:32 p.m.

...

geany_object_signals[GCB_DOCUMENT_FILETYPE_SET] = g_signal_new ( "document-filetype-set", G_OBJECT_CLASS_TYPE (g_object_class), G_SIGNAL_RUN_FIRST,
G_STRUCT_OFFSET (GeanyObjectClass, document_filetype_set),
NULL, NULL,
geany_cclosure_marshal_VOID__POINTER_POINTER,
0, NULL, NULL, g_cclosure_marshal_generic,

Yea, thought it would be more consistent since we pass an explicit marshaller for some signals. Can change to NULL if you prefer.

Colomban Wendling

4:36 p.m.

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).

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1038#issuecomment-220998378

Thomas Martitz

4:36 p.m.

...

@@ -380,3 +389,13 @@ GObject *geany_object_new(void) { return g_object_new(GEANY_OBJECT_TYPE, NULL); }

+/** Get the global GeanyObject instance

@return @transfer{none} The instance

*/

+GEANY_API_SYMBOL +GObject *geany_object_get_instance(void)

pygobject only provides the facility to use python functions for signal callbacks for g_signal_connect (it acually maps GObject.connect() internally to g_signal_connect_closure() with a custom closure that creates PyGObject instances from whatever is passed to the signal handlers. this is also one reason that signal callback parameters must have a associated GType).

Colomban Wendling

24 May 24 May

1:33 p.m.

...

@@ -380,3 +389,13 @@ GObject *geany_object_new(void) { return g_object_new(GEANY_OBJECT_TYPE, NULL); }

+/** Get the global GeanyObject instance

@return @transfer{none} The instance

*/

+GEANY_API_SYMBOL +GObject *geany_object_get_instance(void)

Okay, that part explains why you can't use a custom function. Though, note that this means that pygobject needs to manage the signal connection lifetime itself well enough not to cause the issues our `plugin_signal_connect()` addresses (i.e. disconnect every signal handlers it still has registered when the plugin is unloaded).

Colomban Wendling

1:34 p.m.

...

@@ -380,3 +389,13 @@ GObject *geany_object_new(void) { return g_object_new(GEANY_OBJECT_TYPE, NULL); }

+/** Get the global GeanyObject instance

@return @transfer{none} The instance

*/

+GEANY_API_SYMBOL +GObject *geany_object_get_instance(void)

BTW, couldn't we simply provide a `full` version of our custom function with the appropriate args? Maybe not, but better be sure.

Thomas Martitz

3:14 p.m.

...

@@ -380,3 +389,13 @@ GObject *geany_object_new(void) { return g_object_new(GEANY_OBJECT_TYPE, NULL); }

+/** Get the global GeanyObject instance

@return @transfer{none} The instance

*/

+GEANY_API_SYMBOL +GObject *geany_object_get_instance(void)

Nope, I thought about this too. But pygobject's magic (that allows passing callbacks that fit to the signal's expected signature) only triggers for GObject.connect(). For all other functions the python function must match the signature of the C callback, but for signal handlers there is only GCallback regardless of the actual expected callback. `plugin_signal_connect_full` wouldn't help.

Colomban Wendling

5 Jun 5 Jun

11:59 a.m.

...

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.

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1038/files/f681a14158faddb1ff7f425f3da35...

Colomban Wendling

11:59 a.m.

...

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

+GEANY_API_SYMBOL

requires altering the patch

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1038/files/a52f37027e6ddccad7bc2ccf5f424...

Colomban Wendling

12:03 p.m.

...

@@ -183,6 +183,11 @@ typedef struct GeanyData struct GeanyTemplatePrefs *template_prefs; /**< Template settings */ gpointer *_compat; /* Remove field on next ABI break (abi-todo) */ GSList *filetypes_by_title; /**< See filetypes.h#filetypes_by_title. */
/** Global object signalling events via signals
*
* This is mostly for language bindings. Otherwise prefer to use
* plugin_signal_connect() instead this which adds automatic cleanup. */
GeanyObject * const object;

`const` is cute and all but mostly useless (if anything changes in that structure I prefer not imagine which hells break loose), and is both different from the rest and disallows you to fill the structure. Meh.

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1038/files/058d09f360a420cc639495dbf1567...

Colomban Wendling

12:05 p.m.

...

@@ -183,6 +183,11 @@ typedef struct GeanyData struct GeanyTemplatePrefs *template_prefs; /**< Template settings */ gpointer *_compat; /* Remove field on next ABI break (abi-todo) */ GSList *filetypes_by_title; /**< See filetypes.h#filetypes_by_title. */
/** Global object signalling events via signals
*
* This is mostly for language bindings. Otherwise prefer to use
* plugin_signal_connect() instead this which adds automatic cleanup. */
GeanyObject * const object;

Also, it should probably be `@gironly` as I can't see a real use case in plain C that wouldn't be better served by the `plugin_signal_connect()` API.

Colomban Wendling

12:05 p.m.

...

};

geany_data = gd;

memcpy(&geany_data, &gd, sizeof(GeanyData));

just remove the const and be happy

Colomban Wendling

12:06 p.m.

...

@@ -120,10 +120,11 @@ geany_data_init(void) &tool_prefs, &template_prefs, NULL, /* Remove field on next ABI break (abi-todo) */
filetypes_by_title
filetypes_by_title,
(GeanyObject *) geany_object,

Why a cast? either make the field a `GObject` or the variable a GeanyObject.

Colomban Wendling

12:06 p.m.

...

@@ -110,6 +112,7 @@ struct _GeanyObjectClass

GType geany_object_get_type (void); GObject* geany_object_new (void); +GObject * geany_object_get_instance(void);

leftover

Colomban Wendling

12:08 p.m.

...

@@ -52,6 +56,11 @@ struct _GeanyObjectPrivate gchar dummy; };

+/** Get the GObject-derived GType for GeanyObject

@return GeanyObject type */

+GEANY_API_SYMBOL +GType geany_object_get_type(void);

that too, it's totally useless but for girscanner, so make it @gironly at least (and better, it should be private)

Colomban Wendling

12:09 p.m.

...

@@ -52,6 +56,11 @@ struct _GeanyObjectPrivate gchar dummy; };

+/** Get the GObject-derived GType for GeanyObject

@return GeanyObject type */

+GEANY_API_SYMBOL +GType geany_object_get_type(void);

Actually all the doc for geanyobject should probably be `@gironly`

Colomban Wendling

12:10 p.m.

...

@@ -43,6 +43,7 @@ geany_include_HEADERS = \ encodings.h \ filetypes.h \ geany.h \

geanyobject.h \

do we really need to install that file?

Colomban Wendling

12:10 p.m.

...

@@ -42,6 +42,7 @@ #include "filetypes.h" #include "geany.h" #include "geanyfunctions.h" +#include "geanyobject.h"

do we really need this?

Colomban Wendling

12:13 p.m.

...

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),

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

Colomban Wendling

12:21 p.m.

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).

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1038#issuecomment-223804129

elextr

12:51 p.m.

...

};

geany_data = gd;

memcpy(&geany_data, &gd, sizeof(GeanyData));

And don't violate anti-aliasing rules.

Thomas Martitz

3:09 p.m.

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().

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1038#issuecomment-223812224

Colomban Wendling

3:19 p.m.

@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).

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1038#issuecomment-223812705

Thomas Martitz

3:43 p.m.

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

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1038#issuecomment-223813798

Matthew Brush

6 Jun 6 Jun

3:26 a.m.

...

@@ -3807,3 +3807,14 @@ void document_grab_focus(GeanyDocument *doc)

gtk_widget_grab_focus(GTK_WIDGET(doc->editor->sci)); }

+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.

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1038/files/112ef938ff8c100b62c3ca12a608e...

Matthew Brush

3:26 a.m.

...

@@ -70,6 +70,9 @@ typedef struct GeanyFilePrefs GeanyFilePrefs;

+#define GEANY_TYPE_DOCUMENT (document_get_type()) +GType document_get_type (void);

Do these need to go in the headers if only used by g-ir-scanner?

Matthew Brush

3:27 a.m.

...

@@ -32,6 +32,10 @@

Emitted just after loading main keyfile settings.

*/

+/**

@file geanyobject.h

Definition of the global GeanyObject */

No point in adding this if the stuff in this file is @gironly.

Matthew Brush

3:30 a.m.

...

@@ -74,14 +74,11 @@ AS_IF([test "x$enable_gtk3" = xyes], AM_CONDITIONAL([GTK3], [test "x$gtk_package" = "xgtk+-3.0"])

# GTK/GLib/GIO checks -gtk_modules="$gtk_package >= $gtk_min_version glib-2.0 >= 2.28" -gtk_modules_private="gio-2.0 >= 2.28 gmodule-no-export-2.0" +gtk_modules="$gtk_package >= $gtk_min_version glib-2.0 >= 2.32" +gtk_modules_private="gio-2.0 >= 2.32 gmodule-no-export-2.0"

Breaks Travis build, @b4n is it possible to get a later GLib in Travis?

elextr

4:31 a.m.

...

@@ -74,14 +74,11 @@ AS_IF([test "x$enable_gtk3" = xyes], AM_CONDITIONAL([GTK3], [test "x$gtk_package" = "xgtk+-3.0"])

# GTK/GLib/GIO checks -gtk_modules="$gtk_package >= $gtk_min_version glib-2.0 >= 2.28" -gtk_modules_private="gio-2.0 >= 2.28 gmodule-no-export-2.0" +gtk_modules="$gtk_package >= $gtk_min_version glib-2.0 >= 2.32" +gtk_modules_private="gio-2.0 >= 2.32 gmodule-no-export-2.0"

Travis should test with the oldest Geany uses, which is currently 2.28, so that will have to be upgraded as well.

Thomas Martitz

7 Jun 7 Jun

4:55 p.m.

...

@@ -32,6 +32,10 @@

Emitted just after loading main keyfile settings.

*/

+/**

@file geanyobject.h

Definition of the global GeanyObject */

Am 06.06.2016 um 03:27 schrieb Matthew Brush:

...

In src/geanyobject.c https://github.com/geany/geany/pull/1038#discussion_r65829764:

...
@@ -32,6 +32,10 @@

Emitted just after loading main keyfile settings.

*/

+/**

@file geanyobject.h

Definition of the global GeanyObject */

No point in adding this if the stuff in this file is @gironly.

Corrected that.

...

...
+#define GEANY_TYPE_DOCUMENT (document_get_type()) +GType document_get_type (void);

Do these need to go in the headers if only used by g-ir-scanner?

Is also used inside Geany for g_signal_new() calls.

...

...

+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.

Why? I don't want GI-scanned API and actual API to diverge. The _get_type() functions can be exported without problems. _get_type() itself doesn't imply GBoxed or GObject or anything, nor does it imply any memory management. It just says there's a an associated GType to that type name.

I did change the comment that the GType is derived from GBoxed so that we can freely change these things to GObject later on.

Thomas Martitz

4:56 p.m.

...

@@ -74,14 +74,11 @@ AS_IF([test "x$enable_gtk3" = xyes], AM_CONDITIONAL([GTK3], [test "x$gtk_package" = "xgtk+-3.0"])

# GTK/GLib/GIO checks -gtk_modules="$gtk_package >= $gtk_min_version glib-2.0 >= 2.28" -gtk_modules_private="gio-2.0 >= 2.28 gmodule-no-export-2.0" +gtk_modules="$gtk_package >= $gtk_min_version glib-2.0 >= 2.32" +gtk_modules_private="gio-2.0 >= 2.32 gmodule-no-export-2.0"

Travis is based on Ubuntu 12.04 which generally has glib 2.32. I don't know what's up with that single test.

Matthew Brush

8 Jun 8 Jun

4:28 a.m.

...

@@ -32,6 +32,10 @@

Emitted just after loading main keyfile settings.

*/

+/**

@file geanyobject.h

Definition of the global GeanyObject */

...

Why? ...

Because it's not useful to normal plugins and only used internally by the bindings generated by a core script, so there's no point in having it clutter up the normal API documentation. I thought that's what `@gironly` is for. No big deal though.

Matthew Brush

5:18 a.m.

...

@@ -74,14 +74,11 @@ AS_IF([test "x$enable_gtk3" = xyes], AM_CONDITIONAL([GTK3], [test "x$gtk_package" = "xgtk+-3.0"])

# GTK/GLib/GIO checks -gtk_modules="$gtk_package >= $gtk_min_version glib-2.0 >= 2.28" -gtk_modules_private="gio-2.0 >= 2.28 gmodule-no-export-2.0" +gtk_modules="$gtk_package >= $gtk_min_version glib-2.0 >= 2.32" +gtk_modules_private="gio-2.0 >= 2.32 gmodule-no-export-2.0"

Ah, it's the MinGW GTK2 build [that downloads the 2.24 bundle](https://travis-ci.org/geany/geany/jobs/135397307#L551) which presumably doesn't have a new enough GLib version in it.

This might genuinely be a problem on Windows if we're still supporting pre-built GTK2 bundle (ex. makefile.win32 files or something). It would be easy to fix it though by just replacing `G_TYPE_KEYFILE` with `G_TYPE_POINTER` or whatever, since it's an opaque type, it shouldn't matter ABI-wise or anything. If `g-ir-scanner` has troubles, maybe it could be `#ifdef`'d back to `G_TYPE_KEYFILE` for when being scanned by that tool.

Colomban Wendling

11:23 a.m.

...

@@ -74,14 +74,11 @@ AS_IF([test "x$enable_gtk3" = xyes], AM_CONDITIONAL([GTK3], [test "x$gtk_package" = "xgtk+-3.0"])

# GTK/GLib/GIO checks -gtk_modules="$gtk_package >= $gtk_min_version glib-2.0 >= 2.28" -gtk_modules_private="gio-2.0 >= 2.28 gmodule-no-export-2.0" +gtk_modules="$gtk_package >= $gtk_min_version glib-2.0 >= 2.32" +gtk_modules_private="gio-2.0 >= 2.32 gmodule-no-export-2.0"

It's more subtle than that because this commit set also uses the generic marshaller new in 2.30. We could generate our own though.

But anyway, we'll probably want upgrading GLib at some point for other reasons, so it might make sense to find a way to have it on Windows GTK2. It's a fair bit annoying though, as the GTK2 bundle used to be basically all we really needed, and that wouldn't be the same anymore.

Though, it might be fairly easy to pick up GLib only from http://win32builder.gnome.org/ and use that one (2.34.3), if it doesn't requires fetching too much extra. Or merge the GTK2 and GTK3 bundles and rely on GTK versions not having any name clashes.

Colomban Wendling

12:24 p.m.

...

@@ -74,14 +74,11 @@ AS_IF([test "x$enable_gtk3" = xyes], AM_CONDITIONAL([GTK3], [test "x$gtk_package" = "xgtk+-3.0"])

# GTK/GLib/GIO checks -gtk_modules="$gtk_package >= $gtk_min_version glib-2.0 >= 2.28" -gtk_modules_private="gio-2.0 >= 2.28 gmodule-no-export-2.0" +gtk_modules="$gtk_package >= $gtk_min_version glib-2.0 >= 2.32" +gtk_modules_private="gio-2.0 >= 2.32 gmodule-no-export-2.0"

This *might* work (overwriting with newer GLib): ```diff diff --git a/scripts/cross-build-mingw.sh b/scripts/cross-build-mingw.sh index f3cf9b5..6aab88a 100755 --- a/scripts/cross-build-mingw.sh +++ b/scripts/cross-build-mingw.sh @@ -52,7 +52,7 @@ fetch_and_unzip() { local basename=${1##*/} curl -L -# "$1" > "$basename" - unzip -q "$basename" -d "$2" + unzip -qo "$basename" -d "./$2" rm -f "$basename" }

@@ -86,6 +86,11 @@ cd "$BUILDDIR"

mkdir _deps fetch_and_unzip "$BUNDLE_ZIP" _deps +if [ "$GTK3" = no ]; then + # get newer GLib + fetch_and_unzip "http://win32builder.gnome.org/packages/3.6/glib-dev_2.34.3-1_win32.zip" _deps + fetch_and_unzip "http://win32builder.gnome.org/packages/3.6/glib_2.34.3-1_win32.zip" _deps +fi # fixup the prefix= in the pkg-config files sed -i "s%^(prefix=).*$%\1$PWD/_deps%" _deps/lib/pkgconfig/*.pc

```

Alternatively, this is trickier but might also work (add files from the GTK2 bundle onto the GTK3 one, not overwriting): ```diff diff --git a/scripts/cross-build-mingw.sh b/scripts/cross-build-mingw.sh index f3cf9b5..6f4b2c7 100755 --- a/scripts/cross-build-mingw.sh +++ b/scripts/cross-build-mingw.sh @@ -52,7 +52,7 @@ fetch_and_unzip() { local basename=${1##*/} curl -L -# "$1" > "$basename" - unzip -q "$basename" -d "$2" + unzip -qn "$basename" -d "./$2" rm -f "$basename" }

@@ -85,7 +85,8 @@ mkdir "$BUILDDIR" cd "$BUILDDIR"

mkdir _deps -fetch_and_unzip "$BUNDLE_ZIP" _deps +fetch_and_unzip "$GTK3_BUNDLE_ZIP" _deps +[ "$GTK3" = yes ] || fetch_and_unzip "$BUNDLE_ZIP" _deps # fixup the prefix= in the pkg-config files sed -i "s%^(prefix=).*$%\1$PWD/_deps%" _deps/lib/pkgconfig/*.pc

```

Colomban Wendling

12:26 p.m.

...

@@ -74,14 +74,11 @@ AS_IF([test "x$enable_gtk3" = xyes], AM_CONDITIONAL([GTK3], [test "x$gtk_package" = "xgtk+-3.0"])

# GTK/GLib/GIO checks -gtk_modules="$gtk_package >= $gtk_min_version glib-2.0 >= 2.28" -gtk_modules_private="gio-2.0 >= 2.28 gmodule-no-export-2.0" +gtk_modules="$gtk_package >= $gtk_min_version glib-2.0 >= 2.32" +gtk_modules_private="gio-2.0 >= 2.32 gmodule-no-export-2.0"

(note: both do seem to result in a successful build, but it doesn't necessarily mean it runs cleanly)

Thomas Martitz

4:23 p.m.

...

@@ -74,14 +74,11 @@ AS_IF([test "x$enable_gtk3" = xyes], AM_CONDITIONAL([GTK3], [test "x$gtk_package" = "xgtk+-3.0"])

# GTK/GLib/GIO checks -gtk_modules="$gtk_package >= $gtk_min_version glib-2.0 >= 2.28" -gtk_modules_private="gio-2.0 >= 2.28 gmodule-no-export-2.0" +gtk_modules="$gtk_package >= $gtk_min_version glib-2.0 >= 2.32" +gtk_modules_private="gio-2.0 >= 2.32 gmodule-no-export-2.0"

Cool. Might mention that for win32 release we build our own bundle (using scripts/gtk-bundle-from-msys2.sh). Might use that to host our own bundle for travis?

Colomban Wendling

5:55 p.m.

...

@@ -74,14 +74,11 @@ AS_IF([test "x$enable_gtk3" = xyes], AM_CONDITIONAL([GTK3], [test "x$gtk_package" = "xgtk+-3.0"])

# GTK/GLib/GIO checks -gtk_modules="$gtk_package >= $gtk_min_version glib-2.0 >= 2.28" -gtk_modules_private="gio-2.0 >= 2.28 gmodule-no-export-2.0" +gtk_modules="$gtk_package >= $gtk_min_version glib-2.0 >= 2.32" +gtk_modules_private="gio-2.0 >= 2.32 gmodule-no-export-2.0"

that would make sense indeed, if it's what we use to make our installer builds

Colomban Wendling

11 Jun 11 Jun

12:06 a.m.

TODO: fix Travis build.

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

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1038#issuecomment-225307576

Colomban Wendling

18 Jun 18 Jun

7:23 p.m.

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

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1038#issuecomment-226954198

Thomas Martitz

19 Jun 19 Jun

11:29 p.m.

@kugel- pushed 3 commits.

11c32f6 api: remove @file command from geanyobject 61fd88d api: don't set GBoxed in stone via API doc d586ca2 scripts/cross-build-mingw.sh: Use newer support libraries with GTK2

--- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/geany/geany/pull/1038/files/112ef938ff8c100b62c3ca12a608e...

Thomas Martitz

11:37 p.m.

@b4n like this?

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1038#issuecomment-227021831

Colomban Wendling

11:42 p.m.

...

@@ -70,6 +70,9 @@ typedef struct GeanyFilePrefs GeanyFilePrefs;

+#define GEANY_TYPE_DOCUMENT (document_get_type()) +GType document_get_type (void);

interesting question. But it probably doesn't matter so much as it's not in docs.

Colomban Wendling

11:44 p.m.

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

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1038#issuecomment-227022115

Colomban Wendling

20 Jun 20 Jun

7:13 p.m.

...

@@ -3813,3 +3813,14 @@ void document_grab_focus(GeanyDocument *doc)

gtk_widget_grab_focus(GTK_WIDGET(doc->editor->sci)); }

+static void *copy_(void *src) { return src; } +static void free_(void *doc) { }

+/** 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.

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1038/files/eb3a1a6edf796e71bcd688a1f0e68...

Colomban Wendling

7:13 p.m.

d80ecf2 should be squashed in b53f38d

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1038#issuecomment-227207101

Thomas Martitz

10:32 p.m.

...

@@ -3813,3 +3813,14 @@ void document_grab_focus(GeanyDocument *doc)

gtk_widget_grab_focus(GTK_WIDGET(doc->editor->sci)); }

+static void *copy_(void *src) { return src; } +static void free_(void *doc) { }

+/** Gets the GType of GeanyDocument

@return the GeanyDocument type */

<rant> 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. </rant>

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

Colomban Wendling

11:30 p.m.

...

@@ -3813,3 +3813,14 @@ void document_grab_focus(GeanyDocument *doc)

gtk_widget_grab_focus(GTK_WIDGET(doc->editor->sci)); }

+static void *copy_(void *src) { return src; } +static void free_(void *doc) { }

+/** Gets the GType of GeanyDocument

@return the GeanyDocument type */

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.

Colomban Wendling

22 Jun 22 Jun

5:31 p.m.

...

@@ -70,6 +70,9 @@ typedef struct GeanyFilePrefs GeanyFilePrefs;

+#define GEANY_TYPE_DOCUMENT (document_get_type()) +GType document_get_type (void);

@kugel- something to say on this?

Thomas Martitz

5:45 p.m.

...

@@ -70,6 +70,9 @@ typedef struct GeanyFilePrefs GeanyFilePrefs;

+#define GEANY_TYPE_DOCUMENT (document_get_type()) +GType document_get_type (void);

Am 22.06.2016 um 17:31 schrieb Colomban Wendling:

...

In src/document.h https://github.com/geany/geany/pull/1038#discussion_r68075614:

...
@@ -70,6 +70,9 @@ typedef struct GeanyFilePrefs GeanyFilePrefs;

+#define GEANY_TYPE_DOCUMENT (document_get_type()) +GType document_get_type (void);

@kugel- https://github.com/kugel- something to say on this?

They are used in the g_signal_new() calls in geanyobject.c. What's wrong with them?

Colomban Wendling

6:19 p.m.

...

@@ -70,6 +70,9 @@ typedef struct GeanyFilePrefs GeanyFilePrefs;

+#define GEANY_TYPE_DOCUMENT (document_get_type()) +GType document_get_type (void);

Not much, but if they aren't needed by GIR and are only used internally, they probably be better off in the `GEANY_PRIVATE` section.

Thomas Martitz

11:27 p.m.

...

@@ -70,6 +70,9 @@ typedef struct GeanyFilePrefs GeanyFilePrefs;

+#define GEANY_TYPE_DOCUMENT (document_get_type()) +GType document_get_type (void);

Well, the functions are exported, even if just for the gir. So putting them under GEANY_PRIVATE just isn't right.

Besides, I do generate a (mostly) working .vapi from the .gir. Vala plugins written against that use the C header files so they need to be in the public headers (peasy is partly written in vala).

Colomban Wendling

11:41 p.m.

...

@@ -70,6 +70,9 @@ typedef struct GeanyFilePrefs GeanyFilePrefs;

+#define GEANY_TYPE_DOCUMENT (document_get_type()) +GType document_get_type (void);

...

Besides, I do generate a (mostly) working .vapi from the .gir. Vala plugins written against that use the C header files so they need to be in the public headers (peasy is partly written in vala).

Okay then.

Matthew Brush

23 Jun 23 Jun

2:29 a.m.

...

@@ -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 :)

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1038/files/6b5a47d05f575b321ff1580444208...

Colomban Wendling

3:03 a.m.

...

@@ -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;

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

Matthew Brush

5:07 a.m.

...

@@ -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.

Colomban Wendling

2:43 p.m.

...

@@ -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;

It might be because [`g_once_init_leave()`](https://developer.gnome.org/glib/unstable/glib-Threads.html#g-once-init-leav...) 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.

Thomas Martitz

3:08 p.m.

...

@@ -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;

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/6b5a47d05f575b321ff1580444208...

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.

Matthew Brush

4:29 p.m.

...

@@ -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;

...

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

Colomban Wendling

8:23 p.m.

...

G_TYPE_BOOLEAN, 2,

```
G_TYPE_POINTER, G_TYPE_POINTER);
```

```
GEANY_TYPE_EDITOR, G_TYPE_POINTER);
```

shouldn't the second type be `SCINTILLA_TYPE_NOTIFICATION`?

Thomas Martitz

10:17 p.m.

...

@@ -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;

...

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.

Thomas Martitz

10:36 p.m.

...

G_TYPE_BOOLEAN, 2,

```
G_TYPE_POINTER, G_TYPE_POINTER);
```

```
GEANY_TYPE_EDITOR, G_TYPE_POINTER);
```

Yes, absolutely. Fixed that. Good catch.

Colomban Wendling

11:23 p.m.

Merged #1038.

--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1038#event-702497468

Matthew Brush

24 Jun 24 Jun

2 a.m.

...

@@ -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;

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.

3103

Age (days ago)

3142

Last active (days ago)

github-comments@lists.geany.org

82 comments

4 participants

tags (0)

participants (4)

Colomban Wendling
elextr
Matthew Brush
Thomas Martitz