StefanSalewski / gintro

High level GObject-Introspection based GTK3/GTK4 bindings for Nim language
MIT License
298 stars 20 forks source link

Unable to connect to a signal whose handler gets several values #14

Open lscrd opened 7 years ago

lscrd commented 7 years ago

The "connect" template works fine for simple signals as "clicked" for a Button, but I have not been able to connect to a signal such as "row-deleted" for a TreeModel.

Here is a small example which doesn’t compile:

import gintro/[gtk, gobject]

# Handler.
proc handler(model: TreeModel, path: TreePath, data: pointer) =
  discard

# Connection.
proc p(view: TreeView) =
  discard view.connect("row-deleted", handler, pointer(nil))

I think that "connect" may be currently restricted to simple signals. Is this the case or is there a special way to define the handler (with varargs for instance) ?

StefanSalewski commented 7 years ago

Connect should be not restricted. (It was in the initial release indeed.)

See https://github.com/StefanSalewski/nim-chess4/blob/master/board.nim#L136

onDrawEvent() gets the cairo context as additional parameter. So it was my hope that it would work generally, but indeed this call was my only non trivial test case until yet. Will investigate your issue tonight or tomorrow.

Have forwarded the other issue with conflicting procs to Araq, see

https://github.com/nim-lang/Nim/issues/6393

StefanSalewski commented 7 years ago

And of course

https://github.com/StefanSalewski/gintro/blob/master/examples/connect_args.nim

But maybe some combinations of arguments do not work yet.

StefanSalewski commented 7 years ago

Ah yes, TreeModel and TreeView! They may be incompatible, I think I have tested only identical objects. Have to leave now, will investigate later...

lscrd commented 7 years ago

Sorry, my example is wrong as "row-deleted" is not a signal for "TreeView". It should be:

import gintro/[gtk, gobject]

# Handler.
proc handler(model: TreeModel, path: TreePath, data: pointer) =
  discard

# Connection.
proc p(model: TreeModel) =
  discard model.connect("row-deleted", handler, pointer(nil))

So, only "TreeModel" is involved here.

And you are right with "draw". I have already used this signal successfully. And, indeed, this compiles:

import gintro/[gtk, gobject, cairo, glib]

# Handler.
proc handler(area: DrawingArea, cr: cairo.Context, data: pointer): bool =
  discard

# Connection.
proc p(area: DrawingArea) =
  discard area.connect("draw", handler, pointer(nil))

So, there is something special with TreeModel and/or "row-deleted".

lscrd commented 7 years ago

I have found what is wrong. In "gtk.nim", there is no procedure "scRowDelete". It’s also the case for the other four signals emitted by a TreeModel.

Adding a "scRowDelete" in "gtk.nim" and the associated data in "gisup.nim", I have been able to retrieve the signal in the handler.

In my program, there is still a difficulty, nevertheless. I use a ListStore and it is not recognized as a TreeModel, so the handler is not attached. This is a more general problem: the TreeModel methods are apparently not recognized by a ListStore. I will check and create a report for this later.

StefanSalewski commented 7 years ago

Great that you found it. Indeed I was not aware that Interfaces provide signals. I have code dealing with signals only for objects. Structs and Unions seems to have indeed no signals, but interfaces have. Will try to fix that soon, maybe I just can use most code from objects.

What you may notice later: I have currently no code for dealing with properties. There are functions in gobject-introspection like g_interface_info_get_n_properties(), so I guess I should uses them. I have never done much with properties in the past, so I ignored them at the beginning, but I will try to add that code soon.

StefanSalewski commented 7 years ago

Done.

Now we should have support for 75 additional interface signals. I have just copied the code for object signals to interface proc, with minimal changes.

Old test programs still compiles, but I have tested no one of the 75 new signals yet.

lscrd commented 7 years ago

I have tested and it works, at least for two signals: "row-deleted" and "row-inserted" from TreeItem. Thanks for the addition.

StefanSalewski commented 7 years ago

That is great. I was a bit worried that there may something special about interface signals which may need additional changes.

lscrd commented 7 years ago

Unfortunatley, I have encountered a problem with signal "editing-started" from a CellRenderer.

Here is an example with the "row-deleted" signal and the "editing-started" message. The first one compiles, not the second one.

import gintro/[gtk, gobject, glib]

proc handler1(model: TreeModel, path: TreePath) = discard

proc p1(m: Treemodel) = m.connect("row-deleted", handler1)

proc handler2(renderer: CellRenderer, editable: CellEditable, path: string) = discard

proc p2(r: CellRenderer) = r.connect("editing-started", handler2)
StefanSalewski commented 7 years ago

Thanks, that seems to be a bug in the macro definition. I will try to fix it this evening.

StefanSalewski commented 7 years ago

Seems to be an easy and funny bug. Character | is used when more than one data type is allowed as parameter, and at the same time I used that character in module gisup.nim to group data. Will fix that tomorrow.

StefanSalewski commented 7 years ago

Unfortunately this bug is hard and still unsolved. See

https://forum.nim-lang.org/t/3183

lscrd commented 7 years ago

This seems indeed complicated. There is still the solution to create a procedure for each type, as you suggest, but the concept way would be better… if it works.

For now, I have managed to adapt my "simpleConnect" procedure to work in this case, using "g_object_get_qdata" to retrieve the Nim object from the GTK3 object. This way, I can continue my conversion.

StefanSalewski commented 7 years ago

Maybe fixed now. I broke the proc into multiple instances. I am not really convinced if the concept use as suggested by lando forum user would make much sense. The plain OR notation would be fine, but does not work.

Your example compiles now, and chess board and my other examples still compile and work, so maybe it is OK now.

lscrd commented 7 years ago

Yes it works. It seems that I could retrieve the widget itself and not a CellEditable which is nearly unusable (an Entry would be better). I will try this tomorrow, but presently I have been able to draw the TreeView (a table in fact), to edit the cells and to update the ListStore.

lscrd commented 7 years ago

So, I have tried to replace CellEditable by Entry, but it doesn’t work. The signal may be emitted by an "Entry" but, in the handler interface, only a CellEditable is accepted. But a CellEditable is nearly useless except if there exists some way to retrieve the actual widget from this CellEditable (in order to get access to its methods).

StefanSalewski commented 7 years ago

Ah yes, it is clear that it can not work.

From

https://forum.nim-lang.org/t/3183

we see that in the first proc with cdecl in its name parameter type CellEditable is also used.

We could investigate the parameters of the handler proc inside the macro if we could pass it typed. But unfortunately my macro compiles only when I pass it untyped. I have to investigate that.

lscrd commented 7 years ago

I have been able to cast "CellEditable" to "Editable" without nasty effects (but I don’t know why it works). Thus I have a way to do what I need but I don’t like to use unsafe constructs in normal application code. Now, I’m able to "cut", "copy" from cells and "paste" into cells as I have access to the Editable methods.

StefanSalewski commented 7 years ago

There is again some hope for a clean solution: I found the reason why typed parameter for the handler did not work: Typed works, when we use "connect(app, "activate", modulename.activate)". Problem is, that procs with name activate are contained in library modules, and without module name prefix not the local one is used. With prefix, it compiles again, and due to typed parameter it is easy to extract parameters for handler proc. So we can check parameters and generate matching procs.

The needed module name prefix -- I needed some hours to find the reason, and think it is a bug. But I can not provide a minimal example for Araq, and Araq generally recommends the AST API for macro construction, while I am still using string concatenation and parseStmt().

StefanSalewski commented 7 years ago

You may test again.

A new version of gimpl.nim is available. Now we pass the handler typed and inspect the parameter list. Unfortunately I have no real test data. Your test compiles now with Entry widget, my examples and chess board compile too, when we ensure that handler names for connect macro are unique or have module name prefix. Generally activate() proc is a problem, we need for example connect(app, "activate", button.activate). That is a bit ugly indeed, and error messages are strange when name conflict occurs. Araq recommends to rewrite the connect macro with AST API, maybe then we do not need module prefix any longer. Connect macro has still echo procs, so while compiling you can see how the generated procs look and maybe you can see what is wrong when it does not work. That macro is not really easy and it has to cover many different handler shapes, so testing is necessary unfortunately.

lscrd commented 7 years ago

I am exactly in the wrong configuration with procedures whose name are not unique, but I have not been able to find a working prefix for them, so I have changed their name to make sure they are unique. After that, changing "CellEditable" to "Entry" works fine. Unfortunately, for my specific usage, I need an Editable and so I cannot avoid a cast from CellEditable or Entry to Editable. But there is a real improvement nevertheless even if we have to make sure that the handler names are unique (I would like to know how qualify the handler name though).

StefanSalewski commented 7 years ago

The prefix for the handlers should just be the module name, to tell the compiler which to use. My example was indeed misleading, connect(app, "activate", button.activate) was from the module button.nim.

This needed prefix is ugly, and the compiler error messages when prefix is missing is very confusing for me. Araq is not very interested in this problem, and as long as I can not provide a minimal example to show that strange behaviour he will not investigate it currently.

The GTK interfaces are also confusing, it is difficult to find clear relations. The fact that widgets provide interface is still easy, but relation of CellEditable to Editable is still not very clear to me. I can remember it was difficult to use in Ruby GTK also.

StefanSalewski commented 7 years ago

Well, maybe it is not that difficult:

From https://developer.gnome.org/gtk3/stable/GtkEntry.html#GtkEntry.implemented-interfaces we have

Implemented Interfaces

GtkEntry implements AtkImplementorIface, GtkBuildable, GtkEditable and GtkCellEditable.

So maybe we should offer for Entry widget a safe conversion to these 4 interfaces. Similar for other widgets which implements interfaces. So the user would have to type myentry.editable.someEditableFunc(). But that is not to bad. We could even try to use converters for this, so myentry.someEditableFunc() would work.

lscrd commented 7 years ago

Yes it would be a solution. But, it’s clear that there is still some work to do to hide the low level details and provide a really nice interface.

As regards the prefix, it doesn’t wok in my program as I used the same name for two procedures in the same module. Changing the name was not a bad thing actually, so I don’t care much about that.

StefanSalewski commented 7 years ago

I tried to fully fix it.

Basically it was only

359,364d357
< 
<         if interfaceProvider.contains(h):
<           let provider = interfaceProvider[h]
<           for i in provider: discard mangleType(i)
<           h = h & " | " & provider.join(" | ")

Now all entities providing an interface should be a drop in replacement for the interface instance itself. For example you should be able to pass an entry whenever an Editable is desired, as Entry provides the Editable interface.

That is theoretical behaviour -- I had not the time to create real new testing code for that.

Connect macro still needs unique symbols. I was able to create a minimal example, see https://forum.nim-lang.org/t/3192 But that seems to be the expected behaviour. Would be OK when error messages would be more clear. For the examples proc activate is now named appActivate(). I think for other handler procs problem is not so big, as callback are often named onButtonClick() which is often a unique name. I hope later something like ThisModule.activate() will work where ThisModule refers always to the current file.

lscrd commented 7 years ago

I have not been able to use a CellEditable as an Editable, the relation between them being not very clear. So, I have replaced the CellEditable with an Entry as I’m sure that the edited widget is an Entry. And, indeed, I’m now able to use the clipboard methods of Editable on these entries. That’s impressive ! Thanks a lot ! We are now able to write cleaner code without resorting to a cast at some moment.

As regards the naming constraint, it’s really a small one and I think quite acceptable.

StefanSalewski commented 7 years ago

I have not been able to use a CellEditable as an Editable

Yes, as both are interfaces. But for example it should be able to use an entry wherever an CellEditable or an Editable is needed. I think this should cover most cases.

I have added a simple listview example to the tutorial and made a few more fixes to the generator script.