[RFC] gtk: GObject Introspection for notify signal

172 views
Skip to first unread message

kugel

unread,
Apr 15, 2016, 6:07:14 PM4/15/16
to scintilla-interest
The notify signal of the ScintillaWidget is integral. It's currently supported by the gobject introsection support I added a while back.

There are two reasons it's not working currently:since SCNotification is allocated on stack, the copy and free functions must not do anything
1) It's passed a SCNotification pointer. This type doesn't have a corresonding GType. Instead, the g_signal_new() call is passed the generic G_TYPE_POINTER. The generic marshaller used in e.g. pygobject can't work with this. When trying to use the signal from python the paramter is unusable (void type in python without any fields)
2) g-ir-scanner doesn't properly recognize SCNotification because of the "struct Sci_NotifyHeader" member. When trying to use the notify signal (after fixing 1 locally), the header part can't be used from python. In fact, the struct word is the problem, as introducing a typedef for Sci_NotifyHeader and using that in SCNotification fixes g-ir-scanner.

Therefore, I propose two changes to Scintilla to enable using the notify signal through gobject-introspection
1) Wrapping SCNotifiaction in a GBoxed, which allows to get an appropiate GType. This GType can then be passed to the g_signal_new() call. NOTE: Since SCNotification is allocated on stack, the copy and free functions must not do anything
2) introducing "typedef struct Sci_Notifyheader Sci_NotifyHeader" and using that within SCNotification.

When that, I'm able to connect to the notify signal and access the scn param.

I'm attaching a proof of concept patch that shows my intended changes. Are you OK with these?

Best regards
sci-notify.patch

Matthew Brush

unread,
Apr 15, 2016, 8:06:14 PM4/15/16
to scintilla...@googlegroups.com
Hi,

I'm not sure what the oldest version of GLib/GObject Scintilla must
support but the generic marshaller came about in 2.30[0]. If it can be
used, the `SIG_MARSHAL`, `MARSHAL_ARGUMENTS` defines as well as the
`scintilla-marshal.c` and `scintilla-marshal.h` files could also be removed.

Also, it might be possible to use `G_DEFINE_POINTER_TYPE()` rather than
a fake boxed type, to avoid the stub copy/free functions.

Cheers,
Matthew Brush

[0]:
https://developer.gnome.org/gobject/unstable/gobject-Closures.html#g-cclosure-marshal-generic

kugel

unread,
Apr 16, 2016, 11:10:05 AM4/16/16
to scintilla-interest

I checked pointer types, but it turns out G_DEFINE_POINTER_TYPE-derived types aren't supported by g-ir-scanner at all. it will not emit proper gir for those no matter what you try (see https://bugzilla.gnome.org/show_bug.cgi?id=764067)

I already use the generic marshaller in my patch but I didn't remove the marshaller to make the point of the patch clearer. I agree that can be done finally.

Matthew Brush

unread,
Apr 16, 2016, 3:26:21 PM4/16/16
to scintilla...@googlegroups.com
On 2016-04-16 08:10 AM, kugel wrote:
>
>
> Am Samstag, 16. April 2016 02:06:14 UTC+2 schrieb Matthew Brush:
>> [...]
> I checked pointer types, but it turns out G_DEFINE_POINTER_TYPE-derived
> types aren't supported by g-ir-scanner at all. it will not emit proper gir
> for those no matter what you try (see
> https://bugzilla.gnome.org/show_bug.cgi?id=764067)
>

That's a shame. The big problem with the hack using a boxed type is that
it provides an API to be copied and freed, which a user could rightly
expect to work. If you just stub out the copy function, the user who
called `g_boxed_copy()` could be left with a dangling pointer where they
should have a "copy" (own a reference, or a real copy) when Scintilla
frees or re-uses the original.

> I already use the generic marshaller in my patch but I didn't remove the
> marshaller to make the point of the patch clearer. I agree that can be done
> finally.
>

Yep, I was just noting that your patch could remove more obsolete code
than it does.

Cheers,
Matthew Brush

kugel

unread,
Apr 24, 2016, 2:53:07 PM4/24/16
to scintilla-interest
Neil, can you please comment on this? Probably too late for 3.6.5 but can you say whether this is acceptable at all?

Neil Hodgson

unread,
Apr 24, 2016, 9:07:36 PM4/24/16
to scintilla...@googlegroups.com
kugel:

> Neil, can you please comment on this? Probably too late for 3.6.5 but can you say whether this is acceptable at all?

The patch looked difficult so went into the look-at-later pile.

The patch is made for Geany (with an additional directory level) so doesn’t apply on Scintilla until its been edited.

Then it uses “GEANY_API_SYMBOL” which isn’t defined for Scintilla so breaks compilation. Deleting that allows compilation but maybe its needed. There’s an extraneous ‘;’ warning on the G_DEFINE_BOXED_TYPE line.

Build some more and:

include/ScintillaWidget.h:20: Warning: Scintilla: symbol='scnotification_get_type': Unknown namespace for symbol ‘scnotification_get_type’

likely indicating that the GEANY_API_SYMBOL should have been set to something.

And,

<unknown>:: Warning: Scintilla: (Signal)sci-notify: context=Signal('sci-notify') argument p0: Unresolved type: ‘SCNotification'

make test fails and the resulting .gir doesn’t look as good:

92c92
< <glib:signal name="sci-notify" when="last" action="1">
---
> <glib:signal name="sci-notify" when="last" action="1" introspectable="0”>

So, no longer introspectable and,

101c101
< <type name="gpointer" c:type="gpointer"/>
---
> <type/>

Looks like it has dropped information.

When exporting code from Geany, it should be made independent of Geany and should not require any GEANY* symbols. It appears that the scintilla/test/gi/makefile wan’t used to “make test” as this should have exposed some of the above issues.

Neil

kugel

unread,
Apr 25, 2016, 11:54:54 AM4/25/16
to scintilla-interest
I posted this under RFC and did not actually expect you to apply and try the patch. I should have been more clear about this. Since you seem to be willing to consider it I'll update this thread with a real patch

kugel

unread,
Apr 26, 2016, 11:15:18 AM4/26/16
to scintilla-interest
Here's an updated patch. Fully tested with test/gi/gi-test.py
1-5876-Enable_g_ir_scanner_to_scan_ScintillaObject_signals.patch

Matthew Brush

unread,
Apr 26, 2016, 8:03:24 PM4/26/16
to scintilla...@googlegroups.com
On 2016-04-26 08:15 AM, kugel wrote:
> Here's an updated patch. Fully tested with test/gi/gi-test.py
>

I wonder if it would be prudent in the gi-test.py file to have a global
variable and inside the notify callback, re-bind it with the `scn`
parameter's value to ensure the GBoxed hack isn't going to cause
ownership bugs. Something like:

notification = None
def on_notify(sci, id, scn):
global notification
notification = scn
... rest of callback handler ...

... at end of program ...
del sci
print(notification.nmhdr.code)

I'm not very familiar with PyGI internals but my concern is that if a
reference escapes the callback handler, and PyGI calls g_boxed_copy()
under the covers but doesn't actually get a copy, that when the
reference is used later it will be pointing at garbage stack memory.

Cheers,
Matthew Brush

Matthew Brush

unread,
Apr 26, 2016, 8:56:30 PM4/26/16
to scintilla...@googlegroups.com
On the further thought, what if the dummy g_boxed_copy() implementation
did this instead:

static void *copy_(void*) { return 0; }

At least that way it would instantly crash any programs using it as a
normal GBoxed, and be easier for them to debug than stack corruption.

Just a though, I haven't tested it.

Cheers,
Matthew Brush

kugel

unread,
Apr 27, 2016, 8:42:15 AM4/27/16
to scintilla-interest

I wonder if it would be prudent in the gi-test.py file to have a global
variable and inside the notify callback, re-bind it with the `scn`
parameter's value to ensure the GBoxed hack isn't going to cause
ownership bugs. Something like:

     notification = None
     def on_notify(sci, id, scn):
         global notification
         notification = scn
         ... rest of callback handler ...

     ... at end of program ...
     del sci
     print(notification.nmhdr.
code)

I'm not very familiar with PyGI internals but my concern is that if a
reference escapes the callback handler, and PyGI calls g_boxed_copy()
under the covers but doesn't actually get a copy, that when the
reference is used later it will be pointing at garbage stack memory.




Sure. The notification is only valid within the handler. The same is true for handlers written in C (where you can also trivially copy the pointer to some global storage). So the same restriction applies for python.
 
 
Am Mittwoch, 27. April 2016 02:56:30 UTC+2 schrieb Matthew Brush:
On the further thought, what if the dummy g_boxed_copy() implementation
did this instead:

     static void *copy_(void*) { return 0; }

At least that way it would instantly crash any programs using it as a
normal GBoxed, and be easier for them to debug than stack corruption.

Just a though, I haven't tested it.



I fear (haven't check to be sure) that pygobject would just call _copy() for its own reasons without the programmer doing anything special. Then things would probably break with returning NULL.

If we want to be 100% safe we would need to actually dynamically allocate SCNotification and create real copy/free functions. I don't think that's justified here though.

Matthew Brush

unread,
Apr 27, 2016, 10:37:42 AM4/27/16
to scintilla...@googlegroups.com
On 2016-04-27 05:42 AM, kugel wrote:
>
>>
>>
>> I wonder if it would be prudent in the gi-test.py file to have a global
>> variable and inside the notify callback, re-bind it with the `scn`
>> parameter's value to ensure the GBoxed hack isn't going to cause
>> ownership bugs. Something like:
>>
>> notification = None
>> def on_notify(sci, id, scn):
>> global notification
>> notification = scn
>> ... rest of callback handler ...
>>
>> ... at end of program ...
>> del sci
>> print(notification.nmhdr.
>> code)
>>
>> I'm not very familiar with PyGI internals but my concern is that if a
>> reference escapes the callback handler, and PyGI calls g_boxed_copy()
>> under the covers but doesn't actually get a copy, that when the
>> reference is used later it will be pointing at garbage stack memory.
>>
>>
>>
>
> Sure. The notification is only valid within the handler. The same is true
> for handlers written in C (where you can also trivially copy the pointer to
> some global storage). So the same restriction applies for python.
>

If it's a non-const pointer supporting GBoxed semantics, I'd expect to
be able to be able to call g_boxed_copy() even from C and have my copy
outlive the callback. It's even more plausible for languages like Python
though, because you could close over the notification with another
function or save it into an instance variable, and it could escape that
way, just using normal everyday Python code.

>
> Am Mittwoch, 27. April 2016 02:56:30 UTC+2 schrieb Matthew Brush:
>>
>> On the further thought, what if the dummy g_boxed_copy() implementation
>> did this instead:
>>
>> static void *copy_(void*) { return 0; }
>>
>> At least that way it would instantly crash any programs using it as a
>> normal GBoxed, and be easier for them to debug than stack corruption.
>>
>> Just a though, I haven't tested it.
>>
>>
>
> I fear (haven't check to be sure) that pygobject would just call _copy()
> for its own reasons without the programmer doing anything special. Then
> things would probably break with returning NULL.
>

It would be a good way to check if such a bug is being introduced. Even
better might be to (also) put an `assert(false);` in the copy_()
function to ensure it's not called. If it is called, then there's a bug.

> If we want to be 100% safe we would need to actually dynamically allocate
> SCNotification and create real copy/free functions. I don't think that's
> justified here though.
>

This would make it a normal GBoxed and as such wouldn't violate the
semantics of GBoxed just to get a GType. The only problem is you'd need
to copy/ref any internal pointers like the handle as well.

Cheers,
Matthew Brush

kugel

unread,
Apr 27, 2016, 10:52:37 AM4/27/16
to scintilla-interest


Am Mittwoch, 27. April 2016 16:37:42 UTC+2 schrieb Matthew Brush:
On 2016-04-27 05:42 AM, kugel wrote:
>
>>
If it's a non-const pointer supporting GBoxed semantics, I'd expect to
be able to be able to call g_boxed_copy() even from C and have my copy
outlive the callback. It's even more plausible for languages like Python
though, because you could close over the notification with another
function or save it into an instance variable, and it could escape that
way, just using normal everyday Python code.

It's only GBoxed because g-ir-scanner doesn't work with G_TYPE_POINTER derived stuff. It's really a band-aid, don't expect proper GBoxed semantics.
 

> I fear (haven't check to be sure) that pygobject would just call _copy()
> for its own reasons without the programmer doing anything special. Then
> things would probably break with returning NULL.
>

It would be a good way to check if such a bug is being introduced. Even
better might be to (also) put an `assert(false);` in the copy_()
function to ensure it's not called. If it is called, then there's a bug.


Just checked. Such an assert immediately triggers in the test python program. I don't see how the test program is directly responsible for this so I'm assuming pygobject uses it internally. No matter what, asserting or returning NULL is not an option.
 

> If we want to be 100% safe we would need to actually dynamically allocate
> SCNotification and create real copy/free functions. I don't think that's
> justified here though.
>

This would make it a normal GBoxed and as such wouldn't violate the
semantics of GBoxed just to get a GType. The only problem is you'd need
to copy/ref any internal pointers like the handle as well.
 

I'll leave this decision to Neil. The band-aid works as-is so I'm not hugely motivated to change more of Scintilla for this. But I'll do so if required. I'm thinking changing ScintillaGTK::NotifyParent() should be sufficient, so to copy the SCNotification given to it into a dynamically allocated clone. Changing every caller instead would be a tough task and not limited to the gtk backend anymore.

(BTW the same band-aid is required to some structures in Geany as well)

Neil Hodgson

unread,
May 1, 2016, 9:31:19 AM5/1/16
to scintilla...@googlegroups.com
kugel:

> Here's an updated patch. Fully tested with test/gi/gi-test.py

To maintain project history, scintilla-marshal.c and related code which is being deleted by the patch were added by change sets 1856 and 1857 from Johannes Schmid from the Anjuta project and were discussed on the Anjuta-devel list between 8/1/2004 and 5/4/2004. It seems OK to remove this.
https://sourceforge.net/p/scintilla/code/ci/1a1bf677878d676bc691fc5562dc1168d5f93bb0/
https://sourceforge.net/p/scintilla/code/ci/c8b954b650351b5f9e6cc2e1bccfc64908fc12bf/

It seems rather strange that a C-oriented tool requires typedefs and can’t handle explicit structs. Is there documentation or a bug report for this?

Being able to perform shallow copies of SCNotiification objects could be useful but a deep copy that includes the text field may be more complex. The valid lifetime of the text field is limited so there’d have to be a secondary allocation and code to free this. Heap allocation of original SCNotification objects by Scintilla needlessly increases costs.

The original rationale for the .gir.good file was for checking for correctness by checking for equality. By including every constant in the .gir file, this will break every time a new API or enumerated parameter is changed so will churn far too much. Maybe remove all constants from the gir.good and compare against the .gir with its constants removed. Since its XML, it may be possible to use XSLT to perform this transform.

Neil

kugel

unread,
May 10, 2016, 5:27:56 PM5/10/16
to scintilla-interest, nyama...@me.com


Am Sonntag, 1. Mai 2016 15:31:19 UTC+2 schrieb Neil Hodgson:
kugel:

   It seems rather strange that a C-oriented tool requires typedefs and can’t handle explicit structs. Is there documentation or a bug report for this?


I'm not aware of documentation or bug reports about it. There is [1] but it describes explicit annotations recognized by g-ir-scanner, not how it works with unannotated code and implicit scanning.

[1]: https://wiki.gnome.org/Projects/GObjectIntrospection/Annotations
 
   Being able to perform shallow copies of SCNotiification objects could be useful but a deep copy that includes the text field may be more complex. The valid lifetime of the text field is limited so there’d have to be a secondary allocation and code to free this. Heap allocation of original SCNotification objects by Scintilla needlessly increases costs.

Okay. I deduce that the current implementation of dummy copy/free function is OK and will not change it.
 

   The original rationale for the .gir.good file was for checking for correctness by checking for equality. By including every constant in the .gir file, this will break every time a new API or enumerated parameter is changed so will churn far too much. Maybe remove all constants from the gir.good and compare against the .gir with its constants removed. Since its XML, it may be possible to use XSLT to perform this transform.



Here is an updated patch. I solved this issue by splitting the autogenerated constants into a separate header file. It is included by Scintilla.h so client code should not be concerned. This way picking up the generated constants is a matter of passing ScintillaGenerated.h to g-ir-scanner (which I would recommend except for the purpose of the test script here).

Sorry that it took me so long. I've got some real trouble working with mercurial (I'm only fluent in svn and git).

Best regards
 
1-5896-Enable_g_ir_scanner_to_scan_ScintillaObject_signals.patch

Neil Hodgson

unread,
May 13, 2016, 7:11:36 AM5/13/16
to scintilla-interest
kugel:

> I'm not aware of documentation or bug reports about it. There is [1] but it describes explicit annotations recognized by g-ir-scanner, not how it works with unannotated code and implicit scanning.

Then it should be reported as a bug on g-ir-scanner. It won’t be fixed until it is reported.

> Okay. I deduce that the current implementation of dummy copy/free function is OK and will not change it.

OK.

> The original rationale for the .gir.good file was for checking for correctness by checking for equality. By including every constant in the .gir file, this will break every time a new API or enumerated parameter is changed so will churn far too much. Maybe remove all constants from the gir.good and compare against the .gir with its constants removed. Since its XML, it may be possible to use XSLT to perform this transform.
>
> Here is an updated patch. I solved this issue by splitting the autogenerated constants into a separate header file.

The patch is 94K. Please try to minimise changes and isolate them to the gtk directory as much as possible. Every change made to shared headers has the potential to break other platforms or make things inconvenient (t). To me GObject introspection is an add-on, similar to a language binding or encapsulation in an object model like COM, that should not need any changes to platform-independent files. I’d really prefer not to add typedefs in Scintilla.h but will OK that if it is the only change visible on other platforms and really is required for g-ir-scanner.

The next release should be on May 23 and I’ll be on a lengthy vacation a few days later so I do not want to include anything destabilising.

Constant definitions should remain in Scintilla.h as other workflows may depend on its current format. If you need extra headers for the benefit of g-ir-scanner then create them as temporaries similar to the way that the python constants file and the autogenerated sections ScintillaEdit.h/.cpp are created by qt/ScintillaEdit/WidgetGen.py for those using Qt or PySide.

Neil

kugel

unread,
May 14, 2016, 9:53:25 AM5/14/16
to scintilla-interest, nyama...@me.com


Am Freitag, 13. Mai 2016 13:11:36 UTC+2 schrieb Neil Hodgson:
   The patch is 94K. Please try to minimise changes and isolate them to the gtk directory as much as possible. Every change made to shared headers has the potential to break other platforms or make things inconvenient (t). To me GObject introspection is an add-on, similar to a language binding or encapsulation in an object model like COM, that should not need any changes to platform-independent files. I’d really prefer not to add typedefs in Scintilla.h but will OK that if it is the only change visible on other platforms and really is required for g-ir-scanner.

   The next release should be on May 23 and I’ll be on a lengthy vacation a few days later so I do not want to include anything destabilising.

   Constant definitions should remain in Scintilla.h as other workflows may depend on its current format. If you need extra headers for the benefit of g-ir-scanner then create them as temporaries similar to the way that the python constants file and the autogenerated sections ScintillaEdit.h/.cpp are created by qt/ScintillaEdit/WidgetGen.py for those using Qt or PySide.



The new patch should cover your concerns. It now creates a temporary scintilla header (without autogenerated contents) which is used for .gir generation. Now the only change to non-GTK specific parts are the new typedefs. The patch seems minimally intrusive now.

I hope we can get this in for the new release so it'll be readily available in Geany.

kugel

unread,
May 14, 2016, 9:54:40 AM5/14/16
to scintilla-interest, nyama...@me.com
Here's the patch file. It mostly consists of the new contents for Scintilla-0.1.gir.good.
1-5896-Enable_g_ir_scanner_to_scan_ScintillaObject_signals.patch

Neil Hodgson

unread,
May 15, 2016, 6:24:10 AM5/15/16
to scintilla...@googlegroups.com
kugel:

> Here's the patch file. It mostly consists of the new contents for Scintilla-0.1.gir.good.

Shiboken, the binding generator for PySide hangs with the changes to Scintilla.h. Its possible that removing or modifying some of the changes or moving them from Scintilla.h to ScintillaWidget.h will make both targets work.

To run Shiboken, go to scintilla/qt/ScintillaEditPy and run “python sepbuild.py”. On Ubuntu this requires some packages installed and these should be enough:
apt-get install qtcreator python-dev libshiboken-dev shiboken libpyside-dev python-pyside

The filter on Scintilla.h used to strip out the APIs allows through the deprecated APIs like ’SCI_SETUSERPALETTE’. You really don’t want these.

Neil

kugel

unread,
May 15, 2016, 8:48:15 AM5/15/16
to scintilla-interest, nyama...@me.com


Am Sonntag, 15. Mai 2016 12:24:10 UTC+2 schrieb Neil Hodgson:
kugel:

> Here's the patch file. It mostly consists of the new contents for Scintilla-0.1.gir.good.

   Shiboken, the binding generator for PySide hangs with the changes to Scintilla.h. Its possible that removing or modifying some of the changes or moving them from Scintilla.h to ScintillaWidget.h will make both targets work.

   To run Shiboken, go to scintilla/qt/ScintillaEditPy and run “python sepbuild.py”. On Ubuntu this requires some packages installed and these should be enough:
apt-get install qtcreator python-dev libshiboken-dev shiboken libpyside-dev python-pyside


I found that the new typedefs can be restricted to non-C++ code which fixes the shiboken run. Since c++ implicitly adds such typedefs (you can use SCNotification without struct in C++ already) this is no change in practice.

So I guarded the typedefs into #ifndef __cplusplus. "python sepbuild.py" runs successfully now, and the GI test is good as well.

 

   The filter on Scintilla.h used to strip out the APIs allows through the deprecated APIs like ’SCI_SETUSERPALETTE’. You really don’t want these.


Since this is only for the GI test the deprecated symbols don't really matter. But I guess it is beneficial if removing the deprecated symbols wouldn't affect the GI test. So the latest patch filters out these symbols as well.
1-5896-Enable_g_ir_scanner_to_scan_ScintillaObject_signals.patch

Neil Hodgson

unread,
May 16, 2016, 2:50:53 AM5/16/16
to scintilla...@googlegroups.com
kugel:

> So I guarded the typedefs into #ifndef __cplusplus. "python sepbuild.py" runs successfully now, and the GI test is good as well.

OK. That works on current GTK+.

However, there are problems with older versions of GTK+ where G_DEFINE_BOXED_TYPE is not available before glib 2.26. Wrapping this in a version check leads to an error about a missing scnotification_get_type function. Scintilla current minimum glib version is 2.22 and GTK+ version is 2.18
https://developer.gnome.org/gobject/unstable/gobject-Type-Information.html#G-DEFINE-BOXED-TYPE:CAPS

Neil

kugel

unread,
May 16, 2016, 3:38:48 AM5/16/16
to scintilla-interest, nyama...@me.com

I wasn't aware of the glib minimum. Is this mentioned anywhere?

Anyway the minimum also means that the generic marshaller (through passing NULL to g_signal_new()) cannot be used (only 2.30+). Therefore I brought scintilla-marshall.c back and changed it accordingly - now two marshaller functions with specific types. For G_DEFINE_BOXED_TYPE, I just inlined the scnotification_get_type() implementation, similarly to scintilla_object_get_type(). So the latest patch ought to work with glib 2.22.

Hope it's acceptable now.
1-5896-Enable_g_ir_scanner_to_scan_ScintillaObject_signals.patch

Neil Hodgson

unread,
May 16, 2016, 3:58:14 AM5/16/16
to scintilla...@googlegroups.com
kugel:

> I wasn't aware of the glib minimum. Is this mentioned anywhere?

Its in the change log at http://www.scintilla.org/ScintillaHistory.html.

> Anyway the minimum also means that the generic marshaller (through passing NULL to g_signal_new()) cannot be used (only 2.30+). Therefore I brought scintilla-marshall.c back and changed it accordingly - now two marshaller functions with specific types. For G_DEFINE_BOXED_TYPE, I just inlined the scnotification_get_type() implementation, similarly to scintilla_object_get_type(). So the latest patch ought to work with glib 2.22.

That looks identical to the previous patch.

Neil

kugel

unread,
May 16, 2016, 4:05:26 AM5/16/16
to scintilla-interest, nyama...@me.com


Am Montag, 16. Mai 2016 09:58:14 UTC+2 schrieb Neil Hodgson:

   That looks identical to the previous patch.


Oops. Here's the right file 
1-5896-Enable_g_ir_scanner_to_scan_ScintillaObject_signals.patch

Matthew Brush

unread,
May 16, 2016, 6:39:40 AM5/16/16
to scintilla...@googlegroups.com
I think it's missing a `g_once_init_leave()` to match the
`g_once_init_enter()` in `scnotification_get_type()` function. Also
using `g_` prefix in that function is needlessly opening up possibility
of name clashes with G* stuff. Additionally, I don't think there's any
need to use volatile, and it's using the wrong `gsize` type instead of
`GType` for the `g_define_type_id` variable and then casting back to
`GType` for some reason.

Cheers,
Matthew Brush

kugel

unread,
May 16, 2016, 11:14:09 AM5/16/16
to scintilla-interest
Thanks for spotting the missing g_once_init_leave(), fixed that and also changed the variable names.
1-5896-Enable_g_ir_scanner_to_scan_ScintillaObject_signals.patch

Colomban Wendling

unread,
May 16, 2016, 6:40:51 PM5/16/16
to scintilla...@googlegroups.com
Shouldn't the marshallers rather be generated from
scintilla-marshal.list using glib-genmarshal? In Geany we even patch
Scintilla's marshaler, I think because it doesn't match glib-genmarshal
output (although it shouldn't really matter in practice as it only
affects flags and enum retrieval).

IMO, a better way would be something like that in the Makefile, so it's
easy to update:

> diff --git a/gtk/makefile b/gtk/makefile
> --- a/gtk/makefile
> +++ b/gtk/makefile
> @@ -6,7 +6,7 @@
> # Builds for GTK+ 2 and no longer supports GTK+ 1.
> # Also works with ming32-make on Windows.
>
> -.SUFFIXES: .cxx .c .o .h .a
> +.SUFFIXES: .cxx .c .o .h .a .list
> ifdef CLANG
> CXX = clang++ -Wno-deprecated-register
> CC = clang
> @@ -85,6 +85,14 @@
> .c.o:
> $(CC) $(CONFIGFLAGS) $(CFLAGS) -w -c $<
>
> +GLIB_GENMARSHAL = glib-genmarshal
> +GLIB_GENMARSHAL_FLAGS = --prefix=scintilla_marshal
> +
> +.list.h:
> + $(GLIB_GENMARSHAL) --header $(GLIB_GENMARSHAL_FLAGS) $< > $@
> +.list.c:
> + $(GLIB_GENMARSHAL) --body $(GLIB_GENMARSHAL_FLAGS) $< > $@
> +
> LEXOBJS:=$(addsuffix .o,$(basename $(notdir $(wildcard ../lexers/Lex*.cxx))))
>
> all: $(COMPLIB)

and then simply update the list to match what's needed now.


Do you have any reason not to totally auto-generate the marshallers?

Regards,
Colomban

Colomban Wendling

unread,
May 16, 2016, 6:59:29 PM5/16/16
to scintilla...@googlegroups.com
Le 17/05/2016 à 00:40, Colomban Wendling a écrit :
> […]
>
> IMO, a better way would be something like that in the Makefile, so it's
> easy to update:
>
>> diff --git a/gtk/makefile b/gtk/makefile
>> […]

Attached is a real patch implementing this.

Regards,
Colomban
1-GTK__Add_makefile_rules_to_auto_generate_marshallers.patch

Matthew Brush

unread,
May 16, 2016, 8:25:43 PM5/16/16
to scintilla...@googlegroups.com
On 2016-05-16 08:14 AM, kugel wrote:
> Thanks for spotting the missing g_once_init_leave(), fixed that and also
> changed the variable names.
>

NP. It still has a few uneeded casts. The g_once_init_enter()/leave()
functions don't require a gsize, you should be ok to use address of a
GType directly. Also since the copy_/free_ prototypes match the boxed
copy/free functions, they shouldn't need to be casted.

Cheers,
Matthew Brush

Neil Hodgson

unread,
May 17, 2016, 1:51:55 AM5/17/16
to scintilla...@googlegroups.com
Colomban Wendling:

> Shouldn't the marshallers rather be generated from
> scintilla-marshal.list using glib-genmarshal? In Geany we even patch
> Scintilla's marshaler, I think because it doesn't match glib-genmarshal
> output (although it shouldn't really matter in practice as it only
> affects flags and enum retrieval).

I expect that originally (2004) it was thought that glib-genmarshal may not be installed so it was an extra dependency. However, it appears to be OK as its available on my oldest Linux VM as well as an old Windows install.

Neil

kugel

unread,
May 17, 2016, 5:43:57 AM5/17/16
to scintilla-interest
Neil, will you commit colomban's patch first so that I have to update mine? Or is there anything else I should change? If not I'd like to ask you to accept my patch.

Neil Hodgson

unread,
May 17, 2016, 8:45:39 AM5/17/16
to scintilla...@googlegroups.com
kugel:

> Neil, will you commit colomban's patch first so that I have to update mine?

OK, Colomban’s patch is now committed.
https://sourceforge.net/p/scintilla/code/ci/e97d76e22d846667855ba1461104076f583b43cc/

Neil

kugel

unread,
May 17, 2016, 3:08:40 PM5/17/16
to scintilla-interest, nyama...@me.com
I've found you merged my patch. Thank you a lot!

kugel

unread,
May 17, 2016, 4:23:20 PM5/17/16
to scintilla-interest, nyama...@me.com


Am Dienstag, 17. Mai 2016 21:08:40 UTC+2 schrieb kugel:
I've found you merged my patch. Thank you a lot!



Alright, now I saw you reverted / backed out my change after merging, so that Colomban's patch applies cleanly. Here's my updated patch. The only change to my last patch is that the marshall functions are now auto-generated.
1-5908-Enable_g_ir_scanner_to_scan_ScintillaObject_signals.patch

Neil Hodgson

unread,
May 17, 2016, 8:34:46 PM5/17/16
to scintilla...@googlegroups.com
kugel:

> Alright, now I saw you reverted / backed out my change after merging, so that Colomban's patch applies cleanly. Here's my updated patch. The only change to my last patch is that the marshall functions are now auto-generated.

OK, committed.
https://sourceforge.net/p/scintilla/code/ci/64d36f2eac4de4fa1a4ef478fe784bd9b522fa3e/

Also normalized formatting with
https://sourceforge.net/p/scintilla/code/ci/4ab6041f41d60db8f0829c2fa080220412a496ec/

Neil

kugel

unread,
May 22, 2016, 4:45:00 PM5/22/16
to scintilla-interest, nyama...@me.com


Am Mittwoch, 18. Mai 2016 02:34:46 UTC+2 schrieb Neil Hodgson:
kugel:

> Alright, now I saw you reverted / backed out my change after merging, so that Colomban's patch applies cleanly. Here's my updated patch. The only change to my last patch is that the marshall functions are now auto-generated.

   OK, committed.
https://sourceforge.net/p/scintilla/code/ci/64d36f2eac4de4fa1a4ef478fe784bd9b522fa3e/
 

Thanks a lot!!
Reply all
Reply to author
Forward
0 new messages