Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
lscrd opened this issue Sep 17, 2017 · 28 comments
Open

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

lscrd opened this issue Sep 17, 2017 · 28 comments

Comments

@lscrd
Copy link

lscrd commented Sep 17, 2017

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
Copy link
Owner

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

nim-lang/Nim#6393

@StefanSalewski
Copy link
Owner

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
Copy link
Owner

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
Copy link
Author

lscrd commented Sep 17, 2017

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
Copy link
Author

lscrd commented Sep 17, 2017

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Author

lscrd commented Sep 17, 2017

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

@StefanSalewski
Copy link
Owner

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

@lscrd
Copy link
Author

lscrd commented Sep 18, 2017

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
Copy link
Owner

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

@StefanSalewski
Copy link
Owner

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
Copy link
Owner

Unfortunately this bug is hard and still unsolved. See

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

@lscrd
Copy link
Author

lscrd commented Sep 20, 2017

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
Copy link
Owner

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
Copy link
Author

lscrd commented Sep 20, 2017

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
Copy link
Author

lscrd commented Sep 21, 2017

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
Copy link
Owner

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
Copy link
Author

lscrd commented Sep 21, 2017

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Author

lscrd commented Sep 22, 2017

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Author

lscrd commented Sep 23, 2017

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
Copy link
Owner

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
Copy link
Author

lscrd commented Sep 28, 2017

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
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants