Enable g-ir-scanner for ScintillaWidget.h

[Thomas Martitz]

Hello folks,

the following patch enables g-ir-scanner to correctly parse
ScintillaWidget.h, i.e. so that it creates a .gir file (to be used with
gobject-introspection for automatic language bindings) for scintilla.

This patch changes ScintillaObject to a more GObject-friendly
ScintillaWidget. I provided a compatibility header so that scintilla
clients continue to compile and run fine when they upgrade their copy.
The header should be removed after some deprecation period.

If you want I can also provide the stub program that I created to make
g-ir-scanner do it's job.

I need this to enable python bindings to scintilla automatically via
introspection for the Geany IDE project.

Please comment and consider this for merging.

Best regards

[Neil Hodgson]

Thomas Martitz:

> This patch changes ScintillaObject to a more GObject-friendly ScintillaWidget.

This seems to be quite a heavy set of changes which only really modifies a couple of aspects: there are 6 new preprocessor macros that follow a naming scheme and the name is changed from “Scintilla” to “ScintillaWidget” with corresponding name changes flowing on.

To prevent client code churn, I’d prefer a smaller set of changes that maintains the current names and simply adds alternates that follow the naming convention.

What is the absolute minimum set of changes that allow use of g-ir-scanner?

Neil

[Thomas Martitz]

Hello folks,

the following patch enables g-ir-scanner to correctly parse
ScintillaWidget.h, i.e. so that it creates a .gir file (to be used with
gobject-introspection for automatic language bindings) for scintilla.

This patch changes ScintillaObject to a more GObject-friendly

[Thomas Martitz]

Client code churn is prevented by the compatibility header. It will continue to compile and run without any changes, so downstream can decide themselv when to adopt the new naming scheme.

Is that still not acceptable?
 
(PS: ignore my other mail, i didn't receive your response as mail so I assumed I had to resend my mail after my membership to this group is confirmed)

[Neil Hodgson]

Thomas Martitz:

> Client code churn is prevented by the compatibility header.

You have deprecated the current interface, thus requiring clients to update. It is better to retain the current interface allowing clients to ignore this change.

> Is that still not acceptable?

Not without stronger justification.

Neil

[Thomas Martitz]

Am Donnerstag, 24. Juli 2014 01:01:53 UTC+2 schrieb Neil Hodgson:

Thomas Martitz:

> Client code churn is prevented by the compatibility header.

   You have deprecated the current interface, thus requiring clients to update. It is better to retain the current interface allowing clients to ignore this change.

In my understanding: the current interface is still working and supported with my patch, clients need _not_ to update now and can ignore the change for a while now. Perhaps you have missed it but that's because the compatibility header is implicitly included by ScintillaWidget.h. Clients need to update only once that implicit inclusion is removed, I don't care if/when that's going to happen.

 

> Is that still not acceptable?

   Not without stronger justification.

That's unfortunate.

The major issue with the current interface is that the type is registered as "Scintilla" and the methods begin with scintilla_ while the actual structure is called ScintillaObject. g-ir-scanner expects to find a registered type ScintillaXXX and scintilla_xxx_ (first and foremost scintilla_xxx_get_type()). Another problem is that the class structure is called ScintillaClass and not ScintillaXXXClass.

Naming the type only Scintilla is not appropriate (for g-ir-scanner) because objects should be named NamespaceTypename for introspection to pick it up. You can see how it used to construct the type macros, but more importantly the generated .gir file must have a <namespace name="Namespace"> attribute, and one or more <class name="Typename"> attribtues under it.

Therefore I decided to name the object ScintillaWidget all over the place, with the old name ScintillaObject still being available by default and without any changes to client code.

Best regards

[Thomas Martitz]

Am Donnerstag, 24. Juli 2014 08:43:01 UTC+2 schrieb Thomas Martitz:

Best regards

To give a little bit more background. I'm doing this to make the following python snipped work (as an example) without creating any bindings manually

"""
from gi.repository import Scintilla

if __name__ == "__main__":
    sci = Scintilla.Widget()
    sci.send_message(Scintilla.SCI_SETREADONLY, True, 0)
    sci.connect("sci-notify", lambda x,y : print(str(x)))
"""

This can be achieved using gobject introspection. And since Scintilla is already a gobject we're mostly set. We just need to make the source code more friendly towards the introspection tools.

As a bonus, the python code will probably be the same or at least very similar across scintilla clients (geany and others).

[Neil Hodgson]

Thomas Martitz:

> In my understanding: the current interface is still working and supported with my patch, clients need _not_ to update now and can ignore the change for a while now. Perhaps you have missed it but that's because the compatibility header is implicitly included by ScintillaWidget.h. Clients need to update only once that implicit inclusion is removed, I don't care if/when that's going to happen.

You have changed the canonical names of several features then covered that up with a compatibility header. Clients are expected to use the new names and will eventually break if they don’t. Therefore there is a burden on clients to change names at some point. That burden should be avoided/minimised if at all possible.

A different approach would be to retain all the current names as canonical and introduce g-ir-scanner friendly names in a new header that is maintained for use with introspection. Clients may choose either header depending on whether they care about introspection. Nothing would be deprecated.

> The major issue with the current interface is that the type is registered as "Scintilla" and the methods begin with scintilla_ while the actual structure is called ScintillaObject. g-ir-scanner expects to find a registered type ScintillaXXX and scintilla_xxx_ (first and foremost scintilla_xxx_get_type()). Another problem is that the class structure is called ScintillaClass and not ScintillaXXXClass.

Why wasn’t this information in the original post? Patch dumps are not fun for maintainers. Not including the reasons for changes makes them difficult to review.

g-ir-scanner seems to allow omitting namespaces with —accept-unprefixed.

As I said in an earlier mail, “What is the absolute minimum set of changes that allow use of g-ir-scanner?"

Neil

[Thomas Martitz]

Am Freitag, 25. Juli 2014 03:22:14 UTC+2 schrieb Neil Hodgson:

Thomas Martitz:

> In my understanding: the current interface is still working and supported with my patch, clients need _not_ to update now and can ignore the change for a while now. Perhaps you have missed it but that's because the compatibility header is implicitly included by ScintillaWidget.h. Clients need to update only once that implicit inclusion is removed, I don't care if/when that's going to happen.

   You have changed the canonical names of several features then covered that up with a compatibility header. Clients are expected to use the new names and will eventually break if they don’t. Therefore there is a burden on clients to change names at some point. That burden should be avoided/minimised if at all possible.

Right, and I understand that and I fully agree with your last point. Client compatibility is important for me too. And I hoped I successfully minimized the burden with the compat-header. Do yo disagree with that?. The compat header might very well be never dropped to keep the current names working forever.

On the other hand I also do not want to unnecessarily increase the maintenance burden for the scintilla project by creating new headers with duplicated definitions (see below).

 

   A different approach would be to retain all the current names as canonical and introduce g-ir-scanner friendly names in a new header that is maintained for use with introspection. Clients may choose either header depending on whether they care about introspection. Nothing would be deprecated.

For g-ir-scanner the struct name and the runtime-registered typename must match. This isn't the case currently: the typename is registered as Scintilla and the struct is ScintillaObject. Plus, plus naming the struct just Scintilla makes g-ir-scanner unable to infer the namespace which is important for the generated .gir and .typelib as well as the thing which is imported by scripts (e.g. "from gi.repository import <namespace>") (--accept-unprefixed is not an option because it pulls in all other unrelated symbols too). The namespace is needed for inferring the _get_type() function for each gobject as well.

IIUC a g-ir-scanner-friendly header doesnt suffice. You still need to match the struct name and typename. And you'd have to duplicate the struct defintion in the new header, as g-ir-scanner doesn't pull in the struct defintion when it sees a typedef. Perhaps I'm missing something here?

 

> The major issue with the current interface is that the type is registered as "Scintilla" and the methods begin with scintilla_ while the actual structure is called ScintillaObject. g-ir-scanner expects to find a registered type ScintillaXXX and scintilla_xxx_ (first and foremost scintilla_xxx_get_type()). Another problem is that the class structure is called ScintillaClass and not ScintillaXXXClass.

   Why wasn’t this information in the original post? Patch dumps are not fun for maintainers. Not including the reasons for changes makes them difficult to review.

   g-ir-scanner seems to allow omitting namespaces with —accept-unprefixed.

I'm sorry, you are right, I should have added that to the initial post. It was not nice of me to provide insufficient information. It was an oversight on my side. I hope to have cleared that up now. If you have other questions feel free to ask them, and I hope to be able to answer them.

But, on the other hand, I'm just a user of GIR and am relatively new to the related tools as well so there might be information that is not known to me either.


 

   As I said in an earlier mail, “What is the absolute minimum set of changes that allow use of g-ir-scanner?"

a) The struct name and the runtime-registered typename must match
b) Methods must follow the <namespace>_<type>_xxx convention, and at least <namespace>_<type>_get_type() must be present.

I'm pretty confident that I implemented the minimum set of changes while keeping compatibility for clients. What we could do is to undo the ScintillaObject -> ScintillaWidget part of the change, if you like to make the diff a bit smaller. Though I find ScintillaWidget is more appropriate because GtkWidget is earlier in the class hierarchy and scintilla_new/scintilla_{object,widget}_new() returns a GtkWidget.

[Thomas Martitz]

Moving us back to scintilla-interest, assuming you mailed privately by
accident.

Am 27.07.2014 01:44, schrieb Neil Hodgson:
> Thomas Martitz:
>

>> And I hoped I successfully minimized the burden with the compat-header. Do yo disagree with that?. The compat header might very well be never dropped to keep the current names working forever.
>

> The existence of the use of the compatibility header may cause confusion and support issues. If possible I’d like to avoid it.

>
>> For g-ir-scanner the struct name and the runtime-registered typename must match. This isn't the case currently: the typename is registered as Scintilla and the struct is ScintillaObject.
>

> I’m still not seeing why ScintillaObject is changed to ScintillaWidget. Does there have to be a correspondence between the name of the header file and the struct name?

>
>> Plus, plus naming the struct just Scintilla makes g-ir-scanner unable to infer the namespace which is important for the generated .gir and .typelib as well as the thing which is imported by scripts (e.g. "from gi.repository import <namespace>") (--accept-unprefixed is not an option because it pulls in all other unrelated symbols too).
>

> Python’s import statement can pick individual classes.

>
>> I'm pretty confident that I implemented the minimum set of changes while keeping compatibility for clients. What we could do is to undo the ScintillaObject -> ScintillaWidget part of the change, if you like to make the diff a bit smaller. Though I find ScintillaWidget is more appropriate because GtkWidget is earlier in the class hierarchy and scintilla_new/scintilla_{object,widget}_new() returns a GtkWidget.
>

> That would be a start. Part of my problem is that this is a 300 line patch for something that doesn’t appear that big.
>
> Neil
>


Okay, here is a patch that keeps ScintillaObject. The compatiblity
header is still required as an extra file because g-ir-scanner otherwise
chokes on the extra symbols.

Please tell me what you think.

Best regards

[Neil Hodgson]

Thomas Martitz:

> Moving us back to scintilla-interest, assuming you mailed privately by accident.

Yes, accident.

> Okay, here is a patch that keeps ScintillaObject.

So why did [_]ScintillaClass change to [_]ScintillaObjectClass?

> The compatiblity header is still required as an extra file because g-ir-scanner otherwise chokes on the extra symbols.

Isn’t there an #if it understands?

Neil

[Matthew Brush]

On 14-07-28 03:25 PM, Neil Hodgson wrote:
> Thomas Martitz:
>
>> Moving us back to scintilla-interest, assuming you mailed privately by accident.
>
> Yes, accident.
>
>> Okay, here is a patch that keeps ScintillaObject.
>
> So why did [_]ScintillaClass change to [_]ScintillaObjectClass?
>

FWIW, that typo also makes it hard to use Scintilla with Vala because
like g-ir-scanner, it also assumes the proper naming conventions are
followed (same goes for most of the other stuff Thomas fixed).

>> The compatiblity header is still required as an extra file because g-ir-scanner otherwise chokes on the extra symbols.
>
> Isn't there an #if it understands?
>

You can -DSOMETHING for the g-ir-scanner and then #ifdef guard certain
code when scanning, but then the improvements wouldn't be available
other places these fixes are useful (ex. subclassing, using from
Vala/Genie, likely GTKmm, etc.), unless of course they also define the
same macro before hand.

Cheers,
Matthew Brush

[Neil Hodgson]

Matthew Brush:

> FWIW, that typo also makes it hard to use Scintilla with Vala because like g-ir-scanner, it also assumes the proper naming conventions are followed (same goes for most of the other stuff Thomas fixed).

Was this naming convention documented somewhere?

> You can -DSOMETHING for the g-ir-scanner and then #ifdef guard certain code when scanning, but then the improvements wouldn't be available other places these fixes are useful (ex. subclassing, using from Vala/Genie, likely GTKmm, etc.), unless of course they also define the same macro before hand.

The #if would protect the definitions that cause g-ir-scanner to choke. Does Vala also choke on these definitions?

Neil

[Matthew Brush]

On 14-07-28 04:34 PM, Neil Hodgson wrote:
> Matthew Brush:
>
>> FWIW, that typo also makes it hard to use Scintilla with Vala because like g-ir-scanner, it also assumes the proper naming conventions are followed (same goes for most of the other stuff Thomas fixed).
>
> Was this naming convention documented somewhere?
>

https://developer.gnome.org/gobject/stable/gtype-conventions.html

>> You can -DSOMETHING for the g-ir-scanner and then #ifdef guard certain code when scanning, but then the improvements wouldn't be available other places these fixes are useful (ex. subclassing, using from Vala/Genie, likely GTKmm, etc.), unless of course they also define the same macro before hand.
>
> The #if would protect the definitions that cause g-ir-scanner to choke. Does Vala also choke on these definitions?
>

It's a bit different for Vala, instead when you write a class, like say:

class MyScintilla : Scintilla.Object {
// ...
}

It generates GObject C code that gets compiled like normal C code, and
it will output something like this for the structures:

struct _MyScintilla {
ScintillaObject parent;
}

struct _MyScintillaClass {
ScintillaObjectClass parent;
}

It may be possible to control how it outputs this using code generation
attributes in the API binding or Vala code, but by default it doesn't work.

Cheers,
Matthew Brush

[Neil Hodgson]

Matthew Brush:

> It's a bit different for Vala, instead when you write a class, like say:

>
> class MyScintilla : Scintilla.Object {
> // ...
> }
>
> It generates GObject C code that gets compiled like normal C code, and it will output something like this for the structures:
>
> struct _MyScintilla {
> ScintillaObject parent;
> }
>
> struct _MyScintillaClass {
> ScintillaObjectClass parent;
> }

ScintillaClass is a typedef of _ScintillaClass (which is not defined by the naming convention) so can’t the convention-following name be added with
typedef struct _ScintillaClass ScintillaObjectClass;

scintilla_init and scintilla_class_init are static so there is no need to change them to scintilla_object_init and scintilla_object_class_init.

Since the patch only registers (g_type_register_static) “ScintillaObject” and not “Scintilla” won’t it break any applications that are calling something like g_type_from_name("Scintilla")?

Neil

[Matthew Brush]

On 14-07-28 05:29 PM, Neil Hodgson wrote:
> Matthew Brush:
>
>> It's a bit different for Vala, instead when you write a class, like say:
>
>>
>> class MyScintilla : Scintilla.Object {
>> // ...
>> }
>>
>> It generates GObject C code that gets compiled like normal C code, and it will output something like this for the structures:
>>
>> struct _MyScintilla {
>> ScintillaObject parent;
>> }
>>
>> struct _MyScintillaClass {
>> ScintillaObjectClass parent;
>> }
>
> ScintillaClass is a typedef of _ScintillaClass (which is not defined by the naming convention) so can't the convention-following name be added with
> typedef struct _ScintillaClass ScintillaObjectClass;
>

Yes.

> scintilla_init and scintilla_class_init are static so there is no need to change them to scintilla_object_init and scintilla_object_class_init.
>

Yeah, I think since ScintillaGTK.cxx implements its own _get_type()
function instead of using the G_DEFINE_TYPE macro, which assumes certain
naming conventions.

> Since the patch only registers (g_type_register_static) "ScintillaObject" and not "Scintilla" won't it break any applications that are calling something like g_type_from_name("Scintilla")?
>

Yes, I think it would.

For the purposes of Vala and possibly g-ir-scanner, you could probably
make it work with a simple wrapper header like this:

// ScintillaWrapper.h
#ifndef SCINTILLA_WRAPPER_H
#define SCINTILLA_WRAPPER_H
#define GTK // ScintillaWidget.h needs this
#include "Scintilla.h" // ScintillaWidget.h needs this
#include <gtk/gtk.h> // ScintillaWidget.h needs this
#include <ScintillaWidget.h>
typedef struct _ScintillaClass ScintillaObjectClass;
#define SCINTILLA_TYPE_OBJECT (scintilla_object_get_type())
#define scintilla_object_get_type scintilla_get_type
#define SCINTILLA_IS_OBJECT(obj) IS_SCINTILLA(obj)
#define SCINTILLA_OBJECT(obj) SCINTILLA(obj)
#define scintilla_object_new scintilla_new
#define scintilla_object_send_message scintilla_send_message
#endif

Even less probably, but that handles the most common uses.

Cheers,
Matthew Brush

[Matthew Brush]

On 14-07-28 05:44 PM, Matthew Brush wrote:
> [snip]

> For the purposes of Vala and possibly g-ir-scanner, you could probably
> make it work with a simple wrapper header like this:
>
> // ScintillaWrapper.h

> [snip]

Actually a header like that probably wouldn't work with
GObject-Introspection since I guess it does code parsing and runtime
type system introspection and stuff, after the macros are expanded. For
Vala's code-generator, something like that suffices though.

Cheers,
Matthew Brush

[Neil Hodgson]

Matthew Brush:

   Since the patch only registers (g_type_register_static) "ScintillaObject" and not "Scintilla" won't it break any applications that are calling something like g_type_from_name("Scintilla")?

Yes, I think it would.

   I suppose adding an second name for the type like this would lead to failures.

g_type_register_static(GTK_TYPE_CONTAINER, “Scintilla”, …

g_type_register_static(GTK_TYPE_CONTAINER, “ScintillaObject”, …

   While asking on the mailing list won’t find everyone, its best to try:

   Is anyone accessing the Scintilla type by string name “Scintilla” on GTK+?

   Neil

[Matthew Brush]

On 14-07-28 06:01 PM, Neil Hodgson wrote:
> Matthew Brush:
>
>>> Since the patch only registers (g_type_register_static) "ScintillaObject" and not "Scintilla" won't it break any applications that are calling something like g_type_from_name("Scintilla")?
>>>
>>
>> Yes, I think it would.
>
> I suppose adding an second name for the type like this would lead to failures.

> g_type_register_static(GTK_TYPE_CONTAINER, "Scintilla", ...
> g_type_register_static(GTK_TYPE_CONTAINER, "ScintillaObject", ...
>

You would probably have to fully register a separate GType, like make
another _get_type() function that registers/returns a different static
GType id with the different names and such.

>
> While asking on the mailing list won't find everyone, its best to try:
>
> Is anyone accessing the Scintilla type by string name "Scintilla" on GTK+?
>

FWIW, I don't think Geany does (after a quick search).

Cheers,
Matthew Brush

[Matthew Brush]

On 14-07-28 06:14 PM, Matthew Brush wrote:
> On 14-07-28 06:01 PM, Neil Hodgson wrote:
>> Matthew Brush:
>>
>>>> Since the patch only registers (g_type_register_static)
>>>> "ScintillaObject" and not "Scintilla" won't it break any
>>>> applications that are calling something like
>>>> g_type_from_name("Scintilla")?
>>>>
>>>
>>> Yes, I think it would.
>>
>> I suppose adding an second name for the type like this would lead
>> to failures.
>> g_type_register_static(GTK_TYPE_CONTAINER, "Scintilla", ...
>> g_type_register_static(GTK_TYPE_CONTAINER, "ScintillaObject", ...
>>
>
> You would probably have to fully register a separate GType, like make
> another _get_type() function that registers/returns a different static
> GType id with the different names and such.
>

...and also take care of static members of ScintillaGTK class and
multiple calls to ScintillaGTK::ClassInit().

Cheers,
Matthew Brush

[kugel]

Am Dienstag, 29. Juli 2014 02:29:53 UTC+2 schrieb Neil Hodgson:

Matthew Brush:

> It's a bit different for Vala, instead when you write a class, like say:

>
> class MyScintilla : Scintilla.Object {
>  // ...
> }
>
> It generates GObject C code that gets compiled like normal C code, and it will output something like this for the structures:
>
> struct _MyScintilla {
>  ScintillaObject parent;
> }
>
> struct _MyScintillaClass {
>  ScintillaObjectClass parent;
> }

    ScintillaClass is a typedef of _ScintillaClass (which is not defined by the naming convention) so can’t the convention-following name be added with
typedef struct _ScintillaClass     ScintillaObjectClass;

I think that could work, though it's pretty equivalent to the "typedef struct _ScintillaObjectClass     ScintillaClass;" that my patch contains. In the end both ScintillaClass and ScintillaObjectClass are defined to the same type.

 

   scintilla_init and scintilla_class_init are static so there is no need to change them to scintilla_object_init and scintilla_object_class_init.

Ok, but since it's static client code is not affected and therefore it's a good opportunity to adapt gobject naming style. It would be going to be necessary if we wanted to change to one of the G_DEFINE_TYPE() macros.

 

   Since the patch only registers (g_type_register_static) “ScintillaObject” and not “Scintilla” won’t it break any applications that are calling something like g_type_from_name("Scintilla")?

 Yes I think so. At least such breakage would be immediately obvious by an application crash if clients do not update their code (once they use the NULL return value). But as noted at the end of the mail I don't think anyone is calling g_type_from_name("Scintilla").

> The compatiblity header is still required as an extra file because g-ir-scanner otherwise chokes on the extra symbols.

   Isn’t there an #if it understands?

There is not a predefined macro, but as Matthew said you can have g-ir-scanner define one (and consequently skip sections). IMO a separate file would be cleaner because we don't have to make up a define that all clients have to follow. Is there a specific reason to avoid the extra header (it doesn't affect client code)?
 

   I suppose adding an second name for the type like this would lead to failures.

g_type_register_static(GTK_TYPE_CONTAINER, “Scintilla”, …

g_type_register_static(GTK_TYPE_CONTAINER, “ScintillaObject”, …

I never tried, but in the best case you'll get to two distinct GTypes which are not compatible for glib, so it's not an option.
 

   While asking on the mailing list won’t find everyone, its best to try:

   Is anyone accessing the Scintilla type by string name “Scintilla” on GTK+?

Seems unlikely given how scintilla is distributed: clients have immediate access to all gtype-related macros and scintilla_get_type(). In fact I haven't seen g_type_from_name() in any real code even outside scintilla.

Do you want another patch round or could we convince you? :)

PS: I would like to emphasize on what Matthew brought up; not only g-ir-scanner friendly-ness is required. Being able to interface with vala code, such as creating subclasses, is also important.

Best regards

[Neil Hodgson]

kugel:

Ok, but since it's static client code is not affected and therefore it's a good opportunity to adapt gobject naming style.

   I want to review a *minimal* patch. Bugs hide in unreviewed code and the more lines of change the less each is examined.

 Yes I think so. At least such breakage would be immediately obvious by an application crash if clients do not update their code (once they use the NULL return value). But as noted at the end of the mail I don't think anyone is calling g_type_from_name("Scintilla”).

   Its very dangerous to assume anything about how client code is using legitimate calls. Crashing applications leads to unhappiness.

There is not a predefined macro, but as Matthew said you can have g-ir-scanner define one (and consequently skip sections).

   How? For that matter, what command are you using to invoke g-ir-scanner?

IMO a separate file would be cleaner because we don't have to make up a define that all clients have to follow. Is there a specific reason to avoid the extra header (it doesn't affect client code)?

   Avoiding additional files avoids complexity and various forms of failure. Should make dependencies be updated for this change? Never multiply entities without need. 

   I suppose adding an second name for the type like this would lead to failures.

g_type_register_static(GTK_TYPE_CONTAINER, “Scintilla”, …

g_type_register_static(GTK_TYPE_CONTAINER, “ScintillaObject”, …

I never tried, but in the best case you'll get to two distinct GTypes which are not compatible for glib, so it's not an option.

   Why isn’t it an option? It looks to me like exporting two distinct types would support all current users as well as users of introspection.

Seems unlikely given how scintilla is distributed: clients have immediate access to all gtype-related macros and scintilla_get_type(). In fact I haven't seen g_type_from_name() in any real code even outside scintilla.

   I’d expect it to be used when applications want to be able to instantiate widgets that aren’t known at compile time. Perhaps in form builders or testing applications. Ohloh showed 56 million hits for “g_type_from_name” and while I’m sure lots of those are checking before registration, some are interesting.

Do you want another patch round or could we convince you? :)

   I want to see a minimal change set that I can easily review. It took too much work to uncover the fact that the current changes are incompatible with client code that looks up Scintilla by name. It would have been even better if the original submission had included this information.

   Neil

[kugel]

Am Mittwoch, 30. Juli 2014 07:26:51 UTC+2 schrieb Neil Hodgson:

kugel:

Ok, but since it's static client code is not affected and therefore it's a good opportunity to adapt gobject naming style.

   I want to review a *minimal* patch. Bugs hide in unreviewed code and the more lines of change the less each is examined.

Ok, I understand that.

 

 Yes I think so. At least such breakage would be immediately obvious by an application crash if clients do not update their code (once they use the NULL return value). But as noted at the end of the mail I don't think anyone is calling g_type_from_name("Scintilla”).

   Its very dangerous to assume anything about how client code is using legitimate calls. Crashing applications leads to unhappiness.

You are right, but avoiding unhappiness sometimes also comes at a (serious) cost. I'm still questioning that any clients even makes this call. I conducted source codes of Geany (doesn't call g_type_from_name()), Code::Blocks (doesn't call g_type_from_name()), Scite (doesn't call g_type_from_name()) and Anjuta (the main application does call it, but the scintilla editor plugin doesn't. the scintilla editor is not used by default and is not even part of the anjuta source code distribution, nor part of the GNOME git repositories. the scintilla editor plugin is in a out-of-tree github repository and not actively maintained (updated last year, scintilla copy is still at 3.3.4)). Is there any known ScintillaGTK user that I missed?
 

There is not a predefined macro, but as Matthew said you can have g-ir-scanner define one (and consequently skip sections).

   How? For that matter, what command are you using to invoke g-ir-scanner?

g-ir-scanner --program ./girhelperscintilla -DGTK -Duptr_t=gulong -Dsptr_t=glong $(pkg-config --cflags gtk+-2.0) -Iscintilla/include --cflags-begin -include gtk/gtk.h --cflags-end -n Scintilla -i Gtk-2.0 -i GObject-2.0 --warn-all scintilla/inlcude/ScintillaWidget.h -o Scintilla.gir

girhelperscintilla is a helper program which statically links scintilla and exports (via --export-dynamic) the scintilla_object_* symbols for gobject-introspection.

g-ir-scanner does not define any CPP macro on its own, but as you can see you can define some with -D. You could define something like G_IR_SCANNER to exclude header/code section just for g-ir-scanner

I still would like to avoid enforcing a define on clients. They might already use it for other purposes (unlikely though), but more importantly it potentially must be used for their non-scintilla code because you normally want to collect GIR information in a single g-ir-scanner invokation. This is because you compile 1 .gir file into 1 .typelib (g-ir-compiler cannot compile a .typelib out of multiple .gir files). So if the client wants scintilla in its global typelib it has to be in the same gir.

Note though that this is not what I'm _currently_ doing for geany, there I have an extra scintilla .gir/.typelib for now (though it includes a bit more than just ScintillaObject)
 

IMO a separate file would be cleaner because we don't have to make up a define that all clients have to follow. Is there a specific reason to avoid the extra header (it doesn't affect client code)?

   Avoiding additional files avoids complexity and various forms of failure. Should make dependencies be updated for this change? Never multiply entities without need. 

What does the make dependency have to do with this?

 

   I suppose adding an second name for the type like this would lead to failures.

g_type_register_static(GTK_TYPE_CONTAINER, “Scintilla”, …

g_type_register_static(GTK_TYPE_CONTAINER, “ScintillaObject”, …

I never tried, but in the best case you'll get to two distinct GTypes which are not compatible for glib, so it's not an option.

   Why isn’t it an option? It looks to me like exporting two distinct types would support all current users as well as users of introspection.

Then IS_SCINTILLA(x) != SCINTILLA_IS_OBJECT(x), that would certainly be unexpected and may not be easy worked around by client code. You could not interchange Scintilla and ScintillaObject instances. (imagine a python script which creates ScintillaObject instances (via GIR) and passes it to the main application which expects Scintilla objects).

And my patch does #define {TYPE,IS}_SCINTILLA SCINTILLA_{TYPE,IS}_OBJECT (and friends) therefore scintilla_new()-constructed objects are not compatible with g_type_by_name("Scintilla"). Do you want these problems just to avoid clients having to update g_type_by_name() calls (which I still doubt they exist)? That would be unacceptable to me.

 

Seems unlikely given how scintilla is distributed: clients have immediate access to all gtype-related macros and scintilla_get_type(). In fact I haven't seen g_type_from_name() in any real code even outside scintilla.

   I’d expect it to be used when applications want to be able to instantiate widgets that aren’t known at compile time. Perhaps in form builders or testing applications. Ohloh showed 56 million hits for “g_type_from_name” and while I’m sure lots of those are checking before registration, some are interesting.

Do you want another patch round or could we convince you? :)

   I want to see a minimal change set that I can easily review. It took too much work to uncover the fact that the current changes are incompatible with client code that looks up Scintilla by name. It would have been even better if the original submission had included this information.

I'm sorry about this. I didn't (and still don't) think it would have any impact. But registering the type as "ScintillaObject" is really a requirement for GIR to work.

[Colomban Wendling]

Le 29/07/2014 22:49, kugel a écrit :
> [...]

> I suppose adding an second name for the type like this would lead
> to failures.
> g_type_register_static(GTK_TYPE_CONTAINER, “Scintilla”, …
> g_type_register_static(GTK_TYPE_CONTAINER, “ScintillaObject”, …
>
>
> I never tried, but in the best case you'll get to two distinct GTypes
> which are not compatible for glib, so it's not an option.

What about having ScintillaObject subclassing Scintilla? This way a
ScintillaObject instance would be a valid Scintilla one -- and bypassing
the type system, the other way around would also be true if the subclass
adds absolutely nothing.
It's probably not the best thing performance-wise as it probably adds a
little creation-time overhead, but I doubt it's much if it adds nothing
to the parent.

Just a thought.

Regards,
Colomban

[Neil Hodgson]

Colomban Wendling:

> What about having ScintillaObject subclassing Scintilla? This way a
> ScintillaObject instance would be a valid Scintilla one -- and bypassing
> the type system, the other way around would also be true if the subclass
> adds absolutely nothing.

That sounds like it would result in complete compatibility with the current release. In C++ subclassing a type requires little code but I’m unsure just what to do in GTK+ and expect it will take a bit more code.

Neil

[Matthew Brush]

GtkScintilla is this, plus actual methods, signals, properties, etc like
the Qt widgets do, rather than the current Win32-API-style interface. I
stopped working on GtkScintilla as it's pointless if it's not part of
Scintilla proper (like the similar Qt backend is), but if you ever want
to add a good GTK+ API/widget to core, I'd be willing to revive/finish
it. I also have some improvements on it locally somewhere too that make
it more automated from the Sctinilla.iface, I just never bothered
merging them once it was rejected from core last time.

Cheers,
Matthew Brush

[Colomban Wendling]

Le 31/07/2014 00:37, Matthew Brush a écrit :
> On 14-07-30 02:48 PM, Neil Hodgson wrote:
>> Colomban Wendling:
>>
>>> What about having ScintillaObject subclassing Scintilla? This way a
>>> ScintillaObject instance would be a valid Scintilla one -- and bypassing
>>> the type system, the other way around would also be true if the subclass
>>> adds absolutely nothing.
>>
>> That sounds like it would result in complete compatibility with
>> the current release. In C++ subclassing a type requires little code
>> but I'm unsure just what to do in GTK+ and expect it will take a bit
>> more code.

It would require more code than in C++ but would still be very
manageable. Something like that:

/* in header */
#define SCINTILLA_TYPE_WIDGET (scintilla_widget_get_type())
#define SCINTILLA_WIDGET(o) (G_TYPE_CHECK_INSTANCE_CAST((o),
SCINTILLA_TYPE_WIDGET, ScintillaWidget))
#define SCINTILLA_WIDGET_CLASS(k) (G_TYPE_CHECK_CLASS_CAST((k),
SCINTILLA_TYPE_WIDGET, ScintillaWidgetClass))
#define SCINTILLA_IS_WIDGET(o) (G_TYPE_CHECK_INSTANCE_TYPE((o),
SCINTILLA_TYPE_WIDGET))
#define SCINTILLA_IS_WIDGET_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE((k),
SCINTILLA_TYPE_WIDGET))
typedef struct _ScintillaWidget { ScintillaObject parent; } ScintillaWidget;
typedef struct _ScintillaWidgetClass { ScintillaClass parent; }
ScintillaWidgetClass;
GType scintilla_widget_get_type(void) G_GNUC_CONST;
GtkWidget *scintilla_widget_new(void); /* optional */

/* in source */
G_DEFINE_TYPE(ScintillaWidget, scintilla_widget, SCINTILLA_TYPE)
static void scintilla_widget_class_init(gpointer klass) {}
static void scintilla_widget_init(gpointer instance) {}
GtkWidget *scintilla_widget_new(void) {
return g_object_new(SCINTILLA_TYPE_WIDGET, NULL);

}


> GtkScintilla is this, plus actual methods, signals, properties, etc like

> the Qt widgets do, rather than the current Win32-API-style interface. [...]

For what is worth, I would love to have that :)

Regards,
Colomban

[Neil Hodgson]

Matthew Brush:

> I stopped working on GtkScintilla as it's pointless if it's not part of Scintilla proper (like the similar Qt backend is),…

This is a bit strange to me as I don’t really see inclusion in core as that important. My impression (without hard data) is that RiverBank’s QScintilla is the second most widely used Scintilla platform layer after Win32 in the core.

What benefits are there to inclusion in core? This issue comes up with many open source projects. I’m most familiar with Python and much of the pressure for moving libraries into Python’s main distribution appears to come from a desire for ongoing maintenance.

Neil

[Matthew Brush]

On 14-07-30 03:55 PM, Neil Hodgson wrote:
> Matthew Brush:
>

>> I stopped working on GtkScintilla as it's pointless if it's not part of Scintilla proper (like the similar Qt backend is),...

>
> This is a bit strange to me as I don't really see inclusion in core as that important. My impression (without hard data) is that RiverBank's QScintilla is the second most widely used Scintilla platform layer after Win32 in the core.
>
> What benefits are there to inclusion in core? This issue comes up with many open source projects. I'm most familiar with Python and much of the pressure for moving libraries into Python's main distribution appears to come from a desire for ongoing maintenance.
>

Just a few points anyway:

1) Make the "default" GTK+ API better for everyone
2) Not have to maintain separate build systems, packaging, distribution
channels, etc.
3) Not be "yet another dependency" for apps, for example SciTE probably
wouldn't even use it since it's not in core, so probably nobody else would.
4) More active contributors than 1 (me) to use, test, debug and improve it.

And generally for all the reasons people contribute improvements to
upstream open source projects instead of forking and maintaining a
separate project that only a tiny subset of upstream users would find,
try, and use, rather than (or actually, in addition to) using upstream.

Cheers,
Matthew Brush

[Thomas Martitz]

Replying to several points at once

> I want to see a minimal change set that I can easily review. It took too much work to uncover the fact that the current changes are incompatible with client code that looks up Scintilla by name. It would have been even better if the original submission had included this information.

Here's a new patch that should be really minimal in terms of diff-size
that realizes my goal. However I don't believe it's the best from a
technical viewpoint (i.e. it doesn't have the extra header, I still
would prefer ScintillaWidget as the new name).

It still has the registered name to ScintillaObject since this is a hard
requirement for GIR (struct name and registered (runtime) name must match).

>
> And generally for all the reasons people contribute improvements to
> upstream open source projects instead of forking and maintaining a
> separate project that only a tiny subset of upstream users would find,
> try, and use, rather than (or actually, in addition to) using upstream.

I agree with Matthew that GtkScintilla would be really nice to have
upstream so that GTK applications can use a natural interface to
scintilla, and one which is immediately compatible to gobject
introspection. Having it upstream makes a lot of sense, for keeping it
in sync and exposing it automatically to all GTK clients. The old API
doesn't need to be removed (not now, anyway).

> What about having ScintillaObject subclassing Scintilla? This way a
> ScintillaObject instance would be a valid Scintilla one -- and bypassing
> the type system, the other way around would also be true if the subclass
> adds absolutely nothing.

I thought about subclassing too but I see a number of problems.

* ScintillaObject is already there, for the struct name. The subclass
would need a different struct ("struct _ScintillaXXX { struct
ScintillaObject parent; }". I don't think it could use the same one, not
without confusing g-ir-scannler anyway, can it?
* It should have a separate header which doesn't seem to be liked.
* Subclassing certainly requires more boilerplate code, and therefore a
larger diff, than my proposed patch. I think a minimal diff is a goal
for this?
* The subclass provides little value (it's just a hack to enable better
compat) for being an extra (though minor) maintenance burden
* The biggest problem: ScintillaObject would be a Scintilla, but not the
other way around. Objects created with scintilla_new() would fail the
SCINTILLA_IS_OBJECT(o) check. Therefore the two classes could not be
used interchangeably. And you couldn't just cast Scintilla to
ScintillaObject, as any runtime check in the pipeline will uncover it.

Note that I prefer to not have two classes anyway. It's just a hack for
backward compatibility for clients. IMO two
almost-but-not-quite-the-same classes would only confuse these and new
clients. And IMO the backwards compatibility concerns, which boil down
to only the runtime type name problem, are not worse the hassle of
subclassing and stuff.

I thought of another solution to the g_type_from_name() problem.
Considering that scintilla is to be linked statically we could easily
add an override for g_type_from_name(), catching when it's called for
"Scintilla". How's this one? (Yes, it's also a hack.)

That said, I still don't believe that any client is even doing that. I
looked at the major GTK clients, and would look into more if I knew
about them. In my book this should be a non-issue.

Neil, I'm wondering if you even appreciate this work and see
GIR-compatibility as a useful improvement to Scintilla? IMO it'd be a
huge win for Scintilla if clients could readily enable GIR for scintilla
and have python/java script plugins mess with the text component.

If you say a patch that implements the subclassing idea would be
acceptable and the one attached to this mail not, then I would do the
change so please comment on this.

Best regards

[Neil Hodgson]

kugel:

g-ir-scanner --program ./girhelperscintilla -DGTK -Duptr_t=gulong -Dsptr_t=glong $(pkg-config --cflags gtk+-2.0) -Iscintilla/include --cflags-begin -include gtk/gtk.h --cflags-end -n Scintilla -i Gtk-2.0 -i GObject-2.0 --warn-all scintilla/inlcude/ScintillaWidget.h -o Scintilla.gir

   This doesn’t appear to be the actual command you are using since “include” is misspelt. After fixing that I see these messages:

scintilla/include/ScintillaWidget.h:2: Warning: Scintilla: GTK-Doc comment block start token "/**" should not be followed by comment text:

/** @file ScintillaWidget.h

    ^

scintilla/include/ScintillaWidget.h:3: Error: Scintilla: identifier not found on the first line:

@file ScintillaWidget.h

^

Traceback (most recent call last):

  File "/usr/bin/g-ir-scanner", line 46, in <module>

    sys.exit(scanner_main(sys.argv))

  File "/usr/lib/gobject-introspection/giscanner/scannermain.py", line 488, in scanner_main

    shlibs = create_binary(transformer, options, args)

  File "/usr/lib/gobject-introspection/giscanner/scannermain.py", line 377, in create_binary

    gdump_parser.parse()

  File "/usr/lib/gobject-introspection/giscanner/gdumpparser.py", line 110, in parse

    tree = self._execute_binary_get_tree()

  File "/usr/lib/gobject-introspection/giscanner/gdumpparser.py", line 167, in _execute_binary_get_tree

    subprocess.check_call(args, stdout=sys.stdout, stderr=sys.stderr)

  File "/usr/lib/python2.7/subprocess.py", line 535, in check_call

    retcode = call(*popenargs, **kwargs)

  File "/usr/lib/python2.7/subprocess.py", line 522, in call

    return Popen(*popenargs, **kwargs).wait()

  File "/usr/lib/python2.7/subprocess.py", line 710, in __init__

    errread, errwrite)

  File "/usr/lib/python2.7/subprocess.py", line 1327, in _execute_child

    raise child_exception

OSError: [Errno 2] No such file or directory

>Exit code: 1

girhelperscintilla is a helper program which statically links scintilla and exports (via --export-dynamic) the scintilla_object_* symbols for gobject-introspection.

   Where is the source to this?

What does the make dependency have to do with this?

   Currently downstream projects have dependencies in their make files (or other build files) from ScintillaWidget.h to their executables and this would have to change to depending on both ScintillaWidget.h and ScintillaWidgetCompat.h.

Here's a new patch that should be really minimal in terms of diff-size that realizes my goal. However I don't believe it's the best from a technical viewpoint (i.e. it doesn't have the extra header, I still would prefer ScintillaWidget as the new name).

   This is still changing stuff that doesn’t appear to need changing like _ScintillaClass to _ScintillaObjectClass.

+ * All of this is deprecated and may be removed in the future, so you should

+ * transition to ScintillaObject or maintain a copy of the blow

+ */

   “blow”? Just drop the deprecation commentary.

+void scintilla_object_set_id (ScintillaObject *sci, uptr_t id);

   Since there is now SCI_SETIDENTIFIER, this no longer needs a function.

   Neil

[Neil Hodgson]

Matthew Brush:

[Reordering]

> 3) Not be "yet another dependency" for apps, for example SciTE probably wouldn't even use it since it's not in core, so probably nobody else would.

SciTE tries to access Scintilla in a platform-indpendent manner through its ScintillaWindow class. Using the GTK+-specific GtkScintilla code would get in the way of this platform-independence. I suppose you could try to generalise GtkScintilla to be independent of GTK+.

> 1) Make the "default" GTK+ API better for everyone

ScintillaGTK is a flat C-level API. Most will be using Scintilla from an object-oriented language like C++ or Python so would be better served by an object based API. Its unlikely they will want to use GObject.

> And generally for all the reasons people contribute improvements to upstream open source projects instead of forking and maintaining a separate project that only a tiny subset of upstream users would find, try, and use, rather than (or actually, in addition to) using upstream.

Its not a fork, its a wrapper and separately maintained wrappers are quite common in open source.

Neil

[Matthew Brush]

On 14-08-01 05:40 PM, Neil Hodgson wrote:
> Matthew Brush:
>
> [Reordering]
>
>> 3) Not be "yet another dependency" for apps, for example SciTE probably wouldn't even use it since it's not in core, so probably nobody else would.
>
> SciTE tries to access Scintilla in a platform-indpendent manner through its ScintillaWindow class. Using the GTK+-specific GtkScintilla code would get in the way of this platform-independence. I suppose you could try to generalise GtkScintilla to be independent of GTK+.
>

If you generalized wrappers of Scintilla in the core, you wouldn't need
wrappers at all :) Actually just making Scintilla's "internal" API
public would make the situation much better with respect to not being a
type-unsafe flat win32-like API that is more easily mappable to OOP
languages (including C+GObject and all the languages that makes nearly
automatic bindings for).

>> 1) Make the "default" GTK+ API better for everyone
>
> ScintillaGTK is a flat C-level API. Most will be using Scintilla from an object-oriented language like C++ or Python so would be better served by an object based API. Its unlikely they will want to use GObject.
>

The problem *is* the flat API. If it instead followed platform norms
(similar to what is in the qt/ or cocoa/ directories, but for GTK+)
instead of the flat API, it would be automagically bindable to any OOP
languages that GIR supports, plus fairly easily semi-manually bound to
non-GIR stuff (eg. C++ using GTKmm, Vala using vapigen, etc.).

Also, many (most?) GObject/GTK+ using C applications heavily use
GObject's OOP features, despite being in C (see most any GNOME
application's source code, for example). Nobody wants to use a flat API
directly, except maybe win32-API C applications (it's still crazy, but
at least it's "normal" there).

>> And generally for all the reasons people contribute improvements to upstream open source projects instead of forking and maintaining a separate project that only a tiny subset of upstream users would find, try, and use, rather than (or actually, in addition to) using upstream.
>
> Its not a fork, its a wrapper and separately maintained wrappers are quite common in open source.
>

It is a fork. I have to make a copy of your upstream code, put it in my
repo, modify it to make it better on my platform, and then force users
to choose to use mine or yours. The situation would be greatly improved
if Scintilla "shipped" as a shared library that GtkScintila could link
to and extend, but a good upstream GTK+ widget would be even better, IMO.

FWIW, the intro to the Scintilla documentation sums up the problem
pretty well:

> The Windows version of Scintilla is a Windows Control. As such, its primary programming interface is through Windows messages.[SNIP]
>
> The GTK+ version also uses messages in a similar way to the Windows version. This is different to normal GTK+ practice but made it easier to implement rapidly.
>
> Scintilla also builds with Cocoa on OS X and with Qt, and follows the conventions of those platforms.

Cheers,
Matthew Brush

[Neil Hodgson]

Matthew Brush:

> If you generalized wrappers of Scintilla in the core, you wouldn't need wrappers at all :)

Not really seeing what this would look like.

> Actually just making Scintilla's "internal" API public would make the situation much better with respect to not being a type-unsafe flat win32-like API that is more easily mappable to OOP languages (including C+GObject and all the languages that makes nearly automatic bindings for).

Exposing the internal arrangement of classes inside Scintilla would be brittle. Clients that worked with 3.4.4 would probably not work with 3.4.5 due to the class refactoring.

> The problem *is* the flat API. If it instead followed platform norms (similar to what is in the qt/ or cocoa/ directories,
> but for GTK+)

The current GTK+ and Cocoa implementations tie into the platform at approximately the same level, as a basic system widget that can be sent messages. There are some more internals exposed on Cocoa and there are a few extra methods but its not really all that different.

> It is a fork. I have to make a copy of your upstream code, put it in my repo, modify it

Why?

Neil

[Matthew Brush]

On 14-08-03 01:24 AM, Neil Hodgson wrote:
> Matthew Brush:
>
>> If you generalized wrappers of Scintilla in the core, you wouldn't need wrappers at all :)
>
> Not really seeing what this would look like.
>

I just meant that the "internal" API is roughly what I'd expect from a
decent editor widget (ie. awesome). If you could use that API directly
from an application, you wouldn't really need wrappers, although
obviously it's nice to have the platform-specific integration that is
found on non-GTK platforms currently.

>> Actually just making Scintilla's "internal" API public would make the situation much better with respect to not being a type-unsafe flat win32-like API that is more easily mappable to OOP languages (including C+GObject and all the languages that makes nearly automatic bindings for).
>
> Exposing the internal arrangement of classes inside Scintilla would be brittle. Clients that worked with 3.4.4 would probably not work with 3.4.5 due to the class refactoring.
>

GtkSourceView, for example, has kind of a similar API as Scintilla's
internal API, except it's public and they just don't break it. Obviously
it requires more work/planning but the result is a more robust API for
application code to use. For example, with Scintilla's current GTK+
platform, it's impossible to create a document/model independent of the
widget/view because of the flat API, all messages are sent to a view only.

>> The problem *is* the flat API. If it instead followed platform norms (similar to what is in the qt/ or cocoa/ directories,
>> but for GTK+)
>
> The current GTK+ and Cocoa implementations tie into the platform at approximately the same level, as a basic system widget that can be sent messages. There are some more internals exposed on Cocoa and there are a few extra methods but its not really all that different.
>
>> It is a fork. I have to make a copy of your upstream code, put it in my repo, modify it
>
> Why?
>

Because upstream Scintilla doesn't "ship" a shared library one can link
to, it requires any project using Scintilla to fork it. See, for example:

https://github.com/wxWidgets/wxWidgets/tree/master/src/stc/scintilla
https://github.com/geany/geany/tree/master/scintilla
http://svn.tuxfamily.org/viewvc.cgi/notepadplus_repository/trunk/scintilla/
https://git.gnome.org/browse/anjuta-extras/tree/plugins/scintilla/scintilla
https://github.com/codebrainz/GtkScintilla/tree/master/scintilla

Basically any project that uses Scintilla has to fork its source (in the
technical sense) and continually keep it synced up, potentially with
various patches that need to be applied, each release Scintilla makes,
or else let it drift out of sync with upstream. Any given distro has as
many duplicate copies of Scintilla as it has Scintilla-using applications.

And then to make it usable in GTK+, each project is has to write their
own wrapper to avoid using the Win32-like API, for example:

https://github.com/geany/geany/blob/master/src/sciwrappers.h
https://git.gnome.org/browse/anjuta-extras/tree/plugins/scintilla/text_editor.h

Cheers,
Matthew Brush

[Neil Hodgson]

Matthew Brush:

> I just meant that the "internal" API is roughly what I'd expect from a decent editor widget (ie. awesome).

Can you point to this '"internal” API’ in the code because I can’t see what you are talking about.

> For example, with Scintilla's current GTK+ platform, it's impossible to create a document/model independent of the widget/view because of the flat API, all messages are sent to a view only.

Its possible but requires using extra hidden view objects.

> Because upstream Scintilla doesn't "ship" a shared library one can link to,

If you want to ship a Scintilla shared library then do so. I do not think it is a good idea and do not want the work.

> it requires any project using Scintilla to fork it. See, for example:
>
> https://github.com/wxWidgets/wxWidgets/tree/master/src/stc/scintilla
> https://github.com/geany/geany/tree/master/scintilla
> http://svn.tuxfamily.org/viewvc.cgi/notepadplus_repository/trunk/scintilla/
> https://git.gnome.org/browse/anjuta-extras/tree/plugins/scintilla/scintilla
> https://github.com/codebrainz/GtkScintilla/tree/master/scintilla
>
> Basically any project that uses Scintilla has to fork its source (in the technical sense)

That is not a fork. When you said “fork”, I thought you meant a real fork. Not just taking a static library and wrapping it.

> and continually keep it synced up, potentially with various patches that need to be applied, each release Scintilla makes, or else let it drift out of sync with upstream. Any given distro has as many duplicate copies of Scintilla as it has Scintilla-using applications.

Which is great. Applications should ship with the version of Scintilla they are tested against. Not one distributed by the system.

Neil

[Matthew Brush]

On 14-08-03 02:58 PM, Neil Hodgson wrote:
> Matthew Brush:
>
>> I just meant that the "internal" API is roughly what I'd expect from a decent editor widget (ie. awesome).
>
> Can you point to this '"internal" API' in the code because I can't see what you are talking about.
>

I'm certainly not expert in Scintilla's code, but for a couple main
examples:

http://sourceforge.net/p/scintilla/code/ci/default/tree/src/Editor.h#l154

http://sourceforge.net/p/scintilla/code/ci/default/tree/src/Document.h#l193

And related classes. Like how you can use the doc watcher to get signals
on a document, rather than through a view, etc. I was pretty impressed
when I studied the Qt platforms wrapper compared to the one for GTK+.

>> For example, with Scintilla's current GTK+ platform, it's impossible to create a document/model independent of the widget/view because of the flat API, all messages are sent to a view only.
>
> Its possible but requires using extra hidden view objects.
>

But only because the GTK+ API lacks abilities, compared to the Qt
platform that exposes the Document properly:

http://sourceforge.net/p/scintilla/code/ci/default/tree/qt/ScintillaEdit/ScintillaDocument.h

>> Because upstream Scintilla doesn't "ship" a shared library one can link to,
>
> If you want to ship a Scintilla shared library then do so. I do not think it is a good idea and do not want the work.
>

It's how Linux libraries are shipped[0][1], for better or worse. For
more details, see:

https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries
https://www.debian.org/doc/debian-policy/ch-source.html#s-embeddedfiles

If no shared lib is provided, there is no way for any project to extend
Scintilla, rather each project is doomed to re-distribute its own "fork"
that is incompatible with upstream and duplicates N other programs that
use it.

>> it requires any project using Scintilla to fork it. See, for example:
>>
>> https://github.com/wxWidgets/wxWidgets/tree/master/src/stc/scintilla
>> https://github.com/geany/geany/tree/master/scintilla
>> http://svn.tuxfamily.org/viewvc.cgi/notepadplus_repository/trunk/scintilla/
>> https://git.gnome.org/browse/anjuta-extras/tree/plugins/scintilla/scintilla
>> https://github.com/codebrainz/GtkScintilla/tree/master/scintilla
>>
>> Basically any project that uses Scintilla has to fork its source (in the technical sense)
>
> That is not a fork. When you said "fork", I thought you meant a real fork. Not just taking a static library and wrapping it.
>

Sorry, I meant "fork" like "independent, incompatible copy of the source
code from upstream, potentially with patches applied, distributed
independently from upstream". It's not a fork in spirit.

>> and continually keep it synced up, potentially with various patches that need to be applied, each release Scintilla makes, or else let it drift out of sync with upstream. Any given distro has as many duplicate copies of Scintilla as it has Scintilla-using applications.
>
> Which is great. Applications should ship with the version of Scintilla they are tested against. Not one distributed by the system.
>

Looking at it the other way, applications could test against the
versions that various distros that they support provide, like they do
with nearly every single other library (ex. GTK+, Qt, etc).

Cheers,
Matthew Brush

[0]: http://sourceforge.net/p/scintilla/feature-requests/555/
[1]:
https://lists.fedoraproject.org/pipermail/packaging/2009-July/006206.html

[Neil Hodgson]

Matthew Brush:

> I'm certainly not expert in Scintilla's code, but for a couple main examples:
>
> http://sourceforge.net/p/scintilla/code/ci/default/tree/src/Editor.h#l154
>
> http://sourceforge.net/p/scintilla/code/ci/default/tree/src/Document.h#l193

Exposing all of those internal details as a supported API would lead to breaking compatibility with every release. It is also C++ so can not be consumed easily by other languages.

In general C++ is poorly supported as a source of APIs to be used from different languages. There are just too many possibilities in C++ to be easily converted to other languages. Boost.Python is about as good as it gets and it is still too complex for most - PySide (which is supported by Scintilla) gave up on it.

> And related classes. Like how you can use the doc watcher to get signals on a document, rather than through a view, etc. I was pretty impressed when I studied the Qt platforms wrapper compared to the one for GTK+.

This was driven by the WingWare guys, not me (maybe the things you like in Scintilla are the bits I didn’t do ;-) ) and doesn’t have the same API stability guarantees as the message API. ScintillaDocument has remained at its original state and hasn’t added new and preferred operations like GetRelativePosition/GetCharacterAndWidth. These are provided by the COM-like IDocument/IDocumentWithLineEnd which do have strong compatibility guarantees (which has a cost in explicit version checks) and which would be my preferred way of exposing more APIs.

> But only because the GTK+ API lacks abilities, compared to the Qt platform that exposes the Document properly:
>
> http://sourceforge.net/p/scintilla/code/ci/default/tree/qt/ScintillaEdit/ScintillaDocument.h

I don’t see this as ‘properly’. It may be preferred by some in some cases.

> It's how Linux libraries are shipped[0][1], for better or worse. For more details, see:
>
> https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries
> https://www.debian.org/doc/debian-policy/ch-source.html#s-embeddedfiles

Yes, I have read these and disagree strongly with that point of view. It adds an extra intermediary, the distribution, between vendors and their customers adding latency to distribution. With SciTE a new release can be used immediately from scintilla.org or within a few days via Apple’s app store but when you are using a Linux distribution you are using a release that is 3-9 months old or even older in some cases.

> Looking at it the other way, applications could test against the versions that various distros that they support provide, like they do with nearly every single other library (ex. GTK+, Qt, etc).

Unsure whether you are wanting testing of Scintilla against each release of GTK+/Qt which I think would help with a web page showing the results.

Neil

[Matthew Brush]

On 14-08-04 04:25 PM, Neil Hodgson wrote:
> Matthew Brush:
>
>> I'm certainly not expert in Scintilla's code, but for a couple main examples:
>>
>> http://sourceforge.net/p/scintilla/code/ci/default/tree/src/Editor.h#l154
>>
>> http://sourceforge.net/p/scintilla/code/ci/default/tree/src/Document.h#l193
>
> Exposing all of those internal details as a supported API would lead to breaking compatibility with every release. It is also C++ so can not be consumed easily by other languages.
>

Well not all of them, but more than than a single opaque pointer that
receives/sends all messages.

The API only breaks if it's broken, there are techniques to keep it
stable (even the ABI).

> In general C++ is poorly supported as a source of APIs to be used from different languages. There are just too many possibilities in C++ to be easily converted to other languages. Boost.Python is about as good as it gets and it is still too complex for most - PySide (which is supported by Scintilla) gave up on it.
>

Agreed, a C API is much easier to work with.

>> And related classes. Like how you can use the doc watcher to get signals on a document, rather than through a view, etc. I was pretty impressed when I studied the Qt platforms wrapper compared to the one for GTK+.
>
> This was driven by the WingWare guys, not me (maybe the things you like in Scintilla are the bits I didn't do ;-) ) and doesn't have the same API stability guarantees as the message API. ScintillaDocument has remained at its original state and hasn't added new and preferred operations like GetRelativePosition/GetCharacterAndWidth. These are provided by the COM-like IDocument/IDocumentWithLineEnd which do have strong compatibility guarantees (which has a cost in explicit version checks) and which would be my preferred way of exposing more APIs.
>

Yeah, the lack of stability guarantees would be a problem.

>> But only because the GTK+ API lacks abilities, compared to the Qt platform that exposes the Document properly:
>>
>> http://sourceforge.net/p/scintilla/code/ci/default/tree/qt/ScintillaEdit/ScintillaDocument.h
>
> I don't see this as 'properly'. It may be preferred by some in some cases.
>

Without, it makes application code weird and confused, unlike the
backing C++ code which makes a nice separation between the models and
the views. It's possible to hack around it with dummy views and/or
recycling views and stuff, but it's not as clean as the way the Qt
widget exposes, IMO.

>> It's how Linux libraries are shipped[0][1], for better or worse. For more details, see:
>>
>> https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries
>> https://www.debian.org/doc/debian-policy/ch-source.html#s-embeddedfiles
>
> Yes, I have read these and disagree strongly with that point of view. It adds an extra intermediary, the distribution, between vendors and their customers adding latency to distribution. With SciTE a new release can be used immediately from scintilla.org or within a few days via Apple's app store but when you are using a Linux distribution you are using a release that is 3-9 months old or even older in some cases.
>

It works pretty good in practice, for libraries that provide API/ABI
guarantees and properly increment the library versions and stuff. At
least it's a lot easier to compile against such a lib (ex. using
pkg-config) than to embed a copy of the whole lib in tree and make it
built with own build system and such.

>> Looking at it the other way, applications could test against the versions that various distros that they support provide, like they do with nearly every single other library (ex. GTK+, Qt, etc).
>
> Unsure whether you are wanting testing of Scintilla against each release of GTK+/Qt which I think would help with a web page showing the results.
>

No I just meant, rather than having each app keep in-sync directly with
upstream, they could instead use typical platform mechanisms to link to
appropriate library versions they are tested against and such. For
example if you wrote an app against GTK+ 2.4 and your build system
checked against the GTK+ version >= 2.4, all the way until the breaking
GTK+ v3, your app would happily link and work (possibly with some
deprecation messages).

Cheers,
Matthew Brush

[kugel]

Okay, so if you're done with hijacking this thread we can continue try to get my patch in I think.

Neil, _ScintillaClass -> ScintillaObjectClass is required to get g-ir-scanner pick it up as class structure that corresponds to ScintillaObject. I can fix the blow typo and drop the uneccesary scintilla_object_set_id function later.

The g-ir-scanner helper code is pasted below. It's only purpose is to create a ScintillaObject instance (register with the type system) and provide a command line interface that accepts the gobject-introspection options.


#undef GTK
#define GTK

#include <gtk/gtk.h>
#include <girepository.h>
#include <Scintilla.h>
#include <ScintillaWidget.h>

int main(int argc, char **argv)
{
GError *error;
GOptionContext *context;
GtkWidget *obj;

#if ! GLIB_CHECK_VERSION(2, 36, 0)
g_type_init();
#endif

context = g_option_context_new ("hello");
g_option_context_add_group (context, g_irepository_get_option_group ());

if (!g_option_context_parse (context, &argc, &argv, &error))
return 1;

gtk_init (&argc, &argv);
obj = scintilla_new();
g_object_unref(obj);

return 0;
}

[Neil Hodgson]

kugel:

> Neil, _ScintillaClass -> ScintillaObjectClass is required to get g-ir-scanner pick it up as class structure that corresponds to ScintillaObject.

It only appears necessary to typedef _scintillaClass to ScintillaObjectClass so there is no need to rename _ScintillaClass.

The attached patch adds the new functions and macros leaving as the old names and calls intact. There are some warnings for the comment and for scintilla_new but that could be avoided with an annotation.

Neil

[kugel]

I need to check again with the _ScintillaClass. Perhaps I something wrong when I tested initially.

Your patch leaves out the changes to ScintillaGTK.cxx (so no implementation for scintilla_object_new() etc), is that intentional ?

[Neil Hodgson]

kugel:

> Your patch leaves out the changes to ScintillaGTK.cxx (so no implementation for scintilla_object_new() etc), is that intentional ?

Just use the ScintillaGTK.cxx from your patch, except that the scintilla_object_set_id function can be removed as it isn’t exposed.

It appears that doc-comments are the control mechanism used in introspectable headers so should be used if you want to change or hide “(skip)” methods.
https://wiki.gnome.org/Projects/GObjectIntrospection/Annotations
It may also be possible to use "(rename-to …)” instead of adding separate scintilla_object_… functions.

Neil

[kugel]

I didn't think you'd want gtk-doc comments in your code base as that would be inconsistent (and perhaps confusing for newcomers) with the doxygen-style ones used throughout scintilla.

[Neil Hodgson]

kugel:

> I didn't think you'd want gtk-doc comments in your code base as that would be inconsistent (and perhaps confusing for newcomers) with the doxygen-style ones used throughout scintilla.

The doxygen-style comments aren’t very consistent and there is no intent to run Doxygen.

gtk-doc appears a bit long winded to me - 4 lines of comment for a simple constant, so its not something I would want to see a lot of. Its unclear whether these comment annotations are used just for introspection or whether bindings such as Vala can use them too.

Neil

[kugel]

Sorry for answering this late. I've got side tracked and lost focus on this patch. However I still would like to get this in.

You can auto-generate vala bindings (.vapi) from the introspection result. i.e. vapigen generates a .vapi from a .gir. So yes, gtk-doc comments can be used to generate Vala bindings as well. Lastly, you can generate API documentation (html, devhelp book format) from it too.

[kugel]

Coming back from the 4.0 thread

kugel:

> On that opporunity I would like to ask to include the changes I proposed in https://groups.google.com/d/topic/scintilla-interest/Z7lk3SkBkxs/discussion which you deemed too invasive.

   I don’t think I used the term “invasive”. That would be a bit too territorial.

Sorry, I didn’t mean to insult. But "too invasive" was the impression I got.

 

> Please tell me if you'll consider these then I'll pick up that effort again and rebase to the current development head.

   I tried to push this forward with the scigirmin.patch which was my attempt to simplify the header by removing any changes from the change set that didn’t appear necessary. The goal here is to enable Scintilla for g-ir-scanner in the simplest way possible, not to change the names of things or to add files.

   Neil

If 4.0 is a little incompatible anyway then I think we should be able to push this not in the most minimalistic way, but in a way that produces the best results. We can still omit the compat header if you want, I only added it because I thought you'd like it for maintaining compatibility.

[Neil Hodgson]

kugel:

> If 4.0 is a little incompatible anyway then I think we should be able to push this not in the most minimalistic way, but in a way that produces the best results. We can still omit the compat header if you want, I only added it because I thought you'd like it for maintaining compatibility.

Incompatibilities should be weighed up against the degree of benefit. I don’t understand what is gained here and I also don’t see why scigirmin.patch is not viable.

Neil

[kugel]

I'll try to describe the benefit: The change allows the g-ir-scanner program to evaluate scintilla and generate gobject-introspection (GI) data, in a .gir file. This can be compiled into a .typelib and then be loaded by interpreters for dynamic languages (python, ruby, lua). This makes language bindings immediately available without writing any line of binding code manually.

So the effect is that python, ruby, lua, etc. programs can work with scintilla object directly without the need for manually maintained bindings.

Additionally, for vala, the vapigen program can generate .vapi files so that scintilla can be readily used from within vala programs [please ask if you are not clear what vala or a .vapi is].

The primary purpose of GI is to provide language bindings of library X in a fully automated manner. You only need to generate the .gir, but that task is intended to be automated as well (by the g-ir-scanner program).

My personal benifit (or rather the benefit for users of Geany): I want to allow python plugins for Geany. To do that, I want to use GI for the reasons above. If the plugins want to be remotely useful they need access to scintilla bindings. This is what the change implements.

I'll check again if scigirmin.patch + my changes to ScintillaGTK.cxx are sufficient (scigirmin.patch alone is not sufficient as it doesn't provide the scintilla_object_* methods and does not rename the registered type to match the class name).

[kugel]

Am Montag, 10. August 2015 12:07:18 UTC+2 schrieb kugel:

I'll check again if scigirmin.patch + my changes to ScintillaGTK.cxx are sufficient (scigirmin.patch alone is not sufficient as it doesn't provide the scintilla_object_* methods and does not rename the registered type to match the class name).

scigirmin.patch+my changes to ScintillaGTK.cxx work mostly. I did have to make a small modification, so that g-ir-scanner does not see the old declarations (moved those withion #ifndef G_IR_SCANNING). I uploaded a new patch version to reflect that (patch is complete including ScintillaGTK.cxx changes).

[Neil Hodgson]

 kugel:

scigirmin.patch+my changes to ScintillaGTK.cxx work mostly. I did have to make a small modification, so that g-ir-scanner does not see the old declarations (moved those withion #ifndef G_IR_SCANNING). I uploaded a new patch version to reflect that (patch is complete including ScintillaGTK.cxx changes).

 

   This doesn't appear to make scintilla_object_new introspectible:

    <function name="new"
              c:identifier="scintilla_object_new"
              introspectable="0">
      <return-value>
        <type name="Gtk.Widget" c:type="GtkWidget*"/>
      </return-value>
    </function>

   Neil

[Neil Hodgson]

kugel:

> I'll try to describe the benefit: The change allows the g-ir-scanner program to evaluate scintilla and generate gobject-introspection (GI) data, in a .gir file.

Yes, there are benefits from making type information available. That is not what I was asking. I want to know why these particular changes are needed for exposing the type information.

It is possible to create a .gir file without the changes. Use the namespace ‘Scintilla’ on the g-ir-scanner command line and the attached Scintilla.gir is produced. Its not following the ModuleType convention but it still looks OK and builds into a typelib if a version number is added to the namespace.

Neil

[kugel]

   It is possible to create a .gir file without the changes. Use the namespace ‘Scintilla’ on the g-ir-scanner command line and the attached Scintilla.gir is produced. Its not following the ModuleType convention but it still looks OK and builds into a typelib if a version number is added to the namespace.

 

Neil,

in your Scintilla.gir you'll find that ScintillaObject isn't recognized as GObject class, it's a simple struct. This has some unwanted consquences

a) all the gobject goodies (including automatic reference counting) don't work.
b) cont is an ordnary struct but it should be hidden and treated as the base type
c) from a) and b) follows => ScintillaObject isn't recognized as a GtkWidget/GtkContainer derivate so calling GtkWidget/GtkContainer methods doesn't work, nor you can pass it to a GtkContainer (might work with languages that use the pointers directly but not with e.g. Python)
d) from a follows too that you can't connect to any of its signals
e) send_message isn't treated as an object method so you'd have to call send_message(sci, ...) instead of sci.send_message(...)

The patch I posted is totally about enabling g-ir-scanner to accept ScintillaObject as a GObject-derived type and thus integrating it into the GType system. This is vital for Gobject introspection and Gtk in general.

I think I understand now that this is the "benefits" you mentioned and wanted to learn more about. Is that right?

Is there anything you don't like about the latest patch I posted? It's essentially what you proposed in an earlier mail: took your scigirmin.patch, kept my ScintillaGTK.cxx changes except the scintilla_set_id()-related ones.

Best regards

[Neil Hodgson]

kugel:

> in your Scintilla.gir you'll find that ScintillaObject isn't recognized as GObject class, it's a simple struct. This has some unwanted consquences

Attached is a .gir file made after the changes. There is no indication in this .gir that ScintillaObject is a GObject class. It is very similar to the .gir made from the original files but missing the three symbols that weren’t translated (scintilla_set_id, scintilla_release_resources, SCINTILLA_NOTIFY)

> a) all the gobject goodies (including automatic reference counting) don't work.

Where are you finding this out from? Do you have some form of bindings that can be published?

> b) cont is an ordnary struct but it should be hidden and treated as the base type

There is a 1:1 mapping here with very few differences. cont is a field in both and the ‘<field name=“cont”’ part is identical. Is the name “_” special?

> c) from a) and b) follows => ScintillaObject isn't recognized as a GtkWidget/GtkContainer derivate so calling GtkWidget/GtkContainer methods doesn't work, nor you can pass it to a GtkContainer (might work with languages that use the pointers directly but not with e.g. Python)

Can you show this?

Do you have a .gir file that is different to the attached one?

Neil

[kugel]

Neil,

Am Mittwoch, 19. August 2015 09:27:40 UTC+2 schrieb Neil Hodgson:

kugel:

> in your Scintilla.gir you'll find that ScintillaObject isn't recognized as GObject class, it's a simple struct. This has some unwanted consquences

   Attached is a .gir file made after the changes. There is no indication in this .gir that ScintillaObject is a GObject class. It is very similar to the .gir made from the original files but missing the three symbols that weren’t translated (scintilla_set_id, scintilla_release_resources, SCINTILLA_NOTIFY)

Your .gir file is largely incomplete. How did you generate it? Did you have my patch applied? which g-ir-scanner cmdline did you run? How did your helper program (for g-ir-scanner --program) look like (if any)? See below on how I generate mine.

 

> a) all the gobject goodies (including automatic reference counting) don't work.

   Where are you finding this out from? Do you have some form of bindings that can be published?

See the .gir I attached, it picks up the the GObject (indicated by the class element), the correct parent class type, the implemented interfaces, the exposed signals, member functions and other stuff.

 

> c) from a) and b) follows => ScintillaObject isn't recognized as a GtkWidget/GtkContainer derivate so calling GtkWidget/GtkContainer methods doesn't work, nor you can pass it to a GtkContainer (might work with languages that use the pointers directly but not with e.g. Python)

   Can you show this?

Only types enclosed in a <class /> element are regognized GObject-derived types.

Is the .gir file I attach sufficient or shall I post a sample python program?
 

   Do you have a .gir file that is different to the attached one?

Sure.

I generated it using the following command line (run from the root of my scintilla clone):
g-ir-scanner -i Gtk-3.0 -DG_IR_SCANNING -DGTK --cflags-begin -include gtk/gtk.h --cflags-end -n Scintilla --program $PWD/a.out include/ScintillaWidget.h --warn-all > scintilla.gir

("-DGTK --cflags-begin -include gtk/gtk.h --cflags-end" are scintilla specific as ScintillaWidget.h doesn't include gtk.h on its own and defines the type only if GTK is defined)

a.out passed to g-ir-scanner is a small program which only creates a scintilla widget. This is required in order to get g-ir-scanner to introspect stuff that's not known from the header but can only learned about at runtime (registered signals, properties, etc).

The source for that program is attached as well. I compile it with:
g++ $(pkg-config --cflags gtk+-3.0 gobject-introspection-1.0) -Iinclude girsci.c bin/scintilla.a $(pkg-config --libs glib-2.0 gmodule-2.0 gtk+-3.0 gobject-introspection-1.0)

Best regards

[Neil Hodgson]

kugel:

> Your .gir file is largely incomplete. How did you generate it?

With the g-ir-scanner command you defined earlier:

g-ir-scanner --program ./girhelperscintilla -DGTK -Duptr_t=gulong -Dsptr_t=glong $(pkg-config --cflags gtk+-2.0) -Iscintilla/include --cflags-begin -include gtk/gtk.h --cflags-end -n Scintilla -i Gtk-2.0 -i GObject-2.0 --warn-all scintilla/include/ScintillaWidget.h -o Scintilla.gir

> Did you have my patch applied?

Yes.

> How did your helper program (for g-ir-scanner --program) look like (if any)? See below on how I generate mine.

I was using your earlier program which called scintilla_new instead of g_object_new.

The problem appears to be that I linked against gmodule-no-export-2.0 instead of gmodule-2.0.

A problem with the change to ScintillaWidget.h is that it converts sptr_t to long and uptr_t to unsigned long. This will fail on 64-bit Windows where long is 32-bits. If glib types can be used then gintptr and guintptr may be better although that will impose a minimum GTK+ version of 2.18.

Neil

[kugel]

Am Donnerstag, 27. August 2015 13:05:00 UTC+2 schrieb Neil Hodgson:

kugel:

   A problem with the change to ScintillaWidget.h is that it converts sptr_t to long and uptr_t to unsigned long. This will fail on 64-bit Windows where long is 32-bits. If glib types can be used then gintptr and guintptr may be better although that will impose a minimum GTK+ version of 2.18.

 GLib 2.18 was released over 7 years ago. The glib version that corresponds to gtk 2.24 (the last supported GTK+2 release) release is 2.28. Using these should be fine.

Will you incorporate this patch now?

[Neil Hodgson]

kugel:

> Using these should be fine.

Did you try gintptr? Did it work?

> Will you incorporate this patch now?

Not yet.

Neil

[kugel]

Am Donnerstag, 3. September 2015 11:17:19 UTC+2 schrieb Neil Hodgson:

kugel:

>  Using these should be fine.

   Did you try gintptr? Did it work?

Not yet.
 

> Will you incorporate this patch now?

   Not yet.

Let me ask another way. Will you consider it? Is there anything I can do (apart from the above gintptr test)?

Thanks for you patience.

Best regards

[Neil Hodgson]

kugel:

> Let me ask another way. Will you consider it? Is there anything I can do (apart from the above gintptr test)?

Almost all new features arrive in a testable form with it being possible to run through all the steps from patching the source through to seeing the results. The g-ir-scanner patches are not testable in this way with no build or test scripts. Even finding out how to run each step to use the patch has required tedious back-and-forth over email. For a technology that should really make testing easier, this is not sensible.

Features should always be tested before integration and it should be possible for someone other than the original author to run the tests. It doesn’t have to be me that can test this - if someone else thinks this is useful and wants to take on the role of shepherding this contribution then that would be great.

Progress could be made by adding this to a downstream project (perhaps as a fork) and then demonstrating it inside that project. I don’t think this is optimal though since it means that its mixing two sets of code and having an isolated test runner would be better. After integration the test runner would still be useful for ensuring correct introspection behaviour for each new release. The Qt bindings, for example, have a Python test runner that is used to ensure that the bindings still work for each release.

Neil

[kugel]

Makes sense. I'll try to work on that, but I'm currently busy with lots of other stuff so it may take while.

[kugel]

Neil,

I tested gintptr/guintptr and it works fine as far as I can see.

I also added a test suite as you proposed. I added a makefile under test/gi which does the .gir file generation. It has also a check against a known-good .gir file and also a python program that shows the result in action.

I hope this is acceptable to you.

[kugel]

[Neil Hodgson]

Hi kugel,

> I tested gintptr/guintptr and it works fine as far as I can see.

Good.

> I also added a test suite as you proposed. I added a makefile under test/gi which does the .gir file generation. It has also a check against a known-good .gir file and also a python program that shows the result in action.

That produces an error:
>python -u "gi-test.py"
Traceback (most recent call last):
File "gi-test.py", line 3, in <module>
from gi.repository import Scintilla
ImportError: cannot import name Scintilla

Its likely that installing the files (in an install target of the makefile) would work. The type lib files seem to go into various places in /usr but a quick search didn’t reveal which is appropriate for Scintilla.

Alternatively add a short shell script to include the current directory in search paths before running gi-test.py. Or manipulate os.environ in the Python code before the import. Adding a ‘test’ target may make this easier to understand and use.

Adding -fPIC when compiling Scintilla does have a performance cost so anyone that regards this as a problem should respond so it can be made optional if needed.

Exact comparison of the output to the .good file in the build target appears a little fragile to me as I expect the scanner will produce slightly different output in the future or with different package installations. The comparison could be included in the test target.

There are still comments that can be read as discouraging use of the current calls. I will remove any notion of deprecation before incorporating the patch.

Neil

[Neil Hodgson]

Me:

> Adding -fPIC when compiling Scintilla does have a performance cost so anyone that regards this as a problem should respond so it can be made optional if needed.

The -fPIC flag is now included in the make file in gtk.

Neil

[kugel]

Neil,

I rebased my changes and added a fixup commit. I didn't know how to amend earlier commits (hg commit --amend works only on the last one) so I decided to add a fixup change.

Sorry, for the delay. In the meantime I experimented further with gobject-introspection for Geany and I've been using this change. It becomes essential if Geany plugins (written in python via libpeas, using GI) want to do anything useful (i.e. read/write/access the text buffer and interact with scintilla object).

Best regards

[kugel]

Err, I messed up with hg export. Use these patches instead.

[Neil Hodgson]

kugel:

Err, I messed up with hg export. Use these patches instead.

<1-5776-Enable_g_ir_scanner_to_run_on_ScintillaWidget_h.patch><2-5777-Add_a_test_suite_to_check_gobject_introspection_data.patch><3-5778-Fix_issues_raised_by_review_and_some_more.patch>

   OK, committed as:

https://sourceforge.net/p/scintilla/code/ci/fb4bf7dd85afa0512b2408692ed759af9b99ac70/

https://sourceforge.net/p/scintilla/code/ci/e7ce2ccdc4beb95cb198e8eb76a84cc1f2d1ff0f/

https://sourceforge.net/p/scintilla/code/ci/14c48ef3912d3feae3e99dc1bc5e49ea7e55d745/

   The use of the …good exemplar is still fragile with the obvious

make && make test

   failing as that defaults to GTK+ 2.x and the …good file is for GTK+ 3 (first ‘include’ tag). The makefile shouldn’t stop after that problem as the file may change over time as Scintilla, GTK+, and Introspection change. Whether the script can run is interesting by itself.

   Some more changes to a typo, blank lines, a comment, and change log here:

https://sourceforge.net/p/scintilla/code/ci/8bb980856cc25c898ecc98cd403c7ef733f8a1d6/

   Neil

[kugel]

Am Dienstag, 15. Dezember 2015 11:54:19 UTC+1 schrieb Neil Hodgson:

kugel:

Err, I messed up with hg export. Use these patches instead.

<1-5776-Enable_g_ir_scanner_to_run_on_ScintillaWidget_h.patch><2-5777-Add_a_test_suite_to_check_gobject_introspection_data.patch><3-5778-Fix_issues_raised_by_review_and_some_more.patch>

   OK, committed as:

https://sourceforge.net/p/scintilla/code/ci/fb4bf7dd85afa0512b2408692ed759af9b99ac70/

https://sourceforge.net/p/scintilla/code/ci/e7ce2ccdc4beb95cb198e8eb76a84cc1f2d1ff0f/

https://sourceforge.net/p/scintilla/code/ci/14c48ef3912d3feae3e99dc1bc5e49ea7e55d745/

Awesome, thank you for your patience.

 

   The use of the …good exemplar is still fragile with the obvious

make && make test

   failing as that defaults to GTK+ 2.x and the …good file is for GTK+ 3 (first ‘include’ tag). The makefile shouldn’t stop after that problem as the file may change over time as Scintilla, GTK+, and Introspection change. Whether the script can run is interesting by itself.

Yes, but I couldn't think of a better solution. The .good file has to be updated when ScintillaObject changes of course. I hope different g-ir-scanner output depending on its version won't cause too much trouble.

As for the gtk version issue: perhaps maintain separate files for each version, like ScintillaObject-0.1.gir.gtk-2.0.good and ScintillaObject-0.1.gir.gtk-2.0.good and compare conditionally in makefile?
 

   Some more changes to a typo, blank lines, a comment, and change log here:

https://sourceforge.net/p/scintilla/code/ci/8bb980856cc25c898ecc98cd403c7ef733f8a1d6/

Cool. I would have done them too, but thanks anyway.

Will this become part of the next 3.x release or wait for 4.0?

[johnsonj]

Congratulations!

[Neil Hodgson]

kugel:

> As for the gtk version issue: perhaps maintain separate files for each version, like ScintillaObject-0.1.gir.gtk-2.0.good and ScintillaObject-0.1.gir.gtk-2.0.good and compare conditionally in makefile?

Maybe just add a comment to the makefile explaining the situation.

> Will this become part of the next 3.x release or wait for 4.0?

3.x but that won’t be until the new year due to holidays and heat exhaustion.

Neil