Live change for setChecked/setCheckable checkbox

[F-JJTH]

Hi,

I'm playing with Canvas GUI, and I'm missing 2 features about the canvas checkbox:

first case:
Create a canvas window and insert a checkbox, with "setCheckable()" to false. I want that this checkbox become checkable as soon as another checkbox is checked.
Unfortunately it's not possible because "setCheckable()" doesn't listen to my other checkbox.

second case:
Again, a canvas window with a checkbox and "setChecked()" to false. I want that this checkbox become checked without user action (the user don't click on the checkbox, instead he press a button on his joystick).
Unfortunately it's not possible because "setChecked()" doesn't listen to a property.


In order to fit my need I implemented this:

Code: Select all

diff --git a/Nasal/canvas/gui/widgets/Button.nas b/Nasal/canvas/gui/widgets/Button.nas
index e185323..3e594b4 100644
--- a/Nasal/canvas/gui/widgets/Button.nas
+++ b/Nasal/canvas/gui/widgets/Button.nas
@@ -24,6 +24,13 @@ gui.widgets.Button = {
     me._checkable = checkable;
     return me;
   },
+  setCheckableProp: func(prop)
+  {
+    setlistener(prop, func(v){
+      me._checkable = v.getBoolValue();
+    });
+    return me;
+  },
   setChecked: func(checked = 1)
   {
     if( !me._checkable or me._down == checked )
@@ -36,6 +43,14 @@ gui.widgets.Button = {
     me._onStateChange();
     return me;
   },
+  setCheckedProp: func(prop)
+  {
+    setlistener(prop, func(v){
+      me._down = v.getBoolValue();
+      me._onStateChange();
+    });
+    return me;
+  },
   setDown: func(down = 1)
   {
     if( me._checkable or me._down == down )


With this change I have everything working as I want.
Does it look ok ? can I commit it ?

Regards,
Clément

[Hooray]

Actually, you would normally implement such things separately by wrapping your me reference in a closure so that it can be used by the callback registered for listener/timer.

Thus, the "conventional" method would be to register a listener that updates the other checkbox.

(unless I am missing something here obviously)

[Philosopher]

You need to make sure that the setlistener gets destroyed on .del(), otherwise it will be leaked and not freed. I'm not sure if each element's .del() is called when the window is destroyed... if it isn't, then we should probably add a method to C++ to keep track of a listener (Element.setlistener or Widget.setlistener or something) so that the listener will be freed when the element or widget is deleted.

[F-JJTH]

You need to make sure that the setlistener gets destroyed on .del(), otherwise it will be leaked and not freed.

Yep it was the thing I mainly wondered, I would need to investigate, help is welcome because right now I have no idea how to delete my listener when I close my canvas window.
Example of code are welcome

[Philosopher]

For setlistener I would do something like this:

Code: Select all

var <Class> = {
    new: func(...) {
        me.listeners = [];
    },
    setlistener: func(arg...) {
        var id = call(setlistener, arg);
        append(me.listeners, id);
    },
    del: func() {
        ...;
        foreach (var l; me.listeners)
            removelistener(l);
    },
    ...
};


(sorry if the code is too long, it's basically "id = setlistener(fn); removelistener(id)")

[F-JJTH]

I almost did what you say yesterday without success. But I see that you use

Code: Select all

var id = call(setlistener, arg);


where I use

Code: Select all

var id = setlistener(prop, func(v){
  *some nasal here*
});


Anyway here is the current state of my patch:

Code: Select all

diff --git a/Nasal/canvas/gui/Widget.nas b/Nasal/canvas/gui/Widget.nas
index bcbd1c4..1812c7a 100644
--- a/Nasal/canvas/gui/Widget.nas
+++ b/Nasal/canvas/gui/Widget.nas
@@ -9,6 +9,7 @@ gui.Widget = {
       _hover: 0,
       _enabled: 1,
       _view: nil,
+      _listeners: [],
       _pos: [0, 0],
       _size: [32, 32]
     });
@@ -121,6 +122,13 @@ gui.Widget = {
       me._view = nil;
     }
 
+    print("onRemove()");
+    foreach(var l; me._listeners)
+    {
+      print("onRemove::removelistener");
+      removelistener(l);
+    }
+
     if( me._focused )
       me.getCanvas()._focused_widget = nil;
   },
@@ -173,6 +181,13 @@ gui.Widget = {
       );
     return me;
   },
+  _registerListener: func(id)
+  {
+    print("_registerListener()");
+    append(me._listeners, id);
+    print("nb of listener: "~size(me._listeners));
+    return me;
+  },
   _windowFocus: func
   {
     var canvas = me.getCanvas();
diff --git a/Nasal/canvas/gui/widgets/Button.nas b/Nasal/canvas/gui/widgets/Button.nas
index e185323..888ac69 100644
--- a/Nasal/canvas/gui/widgets/Button.nas
+++ b/Nasal/canvas/gui/widgets/Button.nas
@@ -24,6 +24,13 @@ gui.widgets.Button = {
     me._checkable = checkable;
     return me;
   },
+  setCheckableProp: func(prop)
+  {
+    setlistener(prop, func(v){
+      me._checkable = v.getBoolValue();
+    });
+    return me;
+  },
   setChecked: func(checked = 1)
   {
     if( !me._checkable or me._down == checked )
@@ -36,6 +43,16 @@ gui.widgets.Button = {
     me._onStateChange();
     return me;
   },
+  setCheckedProp: func(prop)
+  {
+    var id = setlistener(prop, func(v){
+      print(v.getBoolValue());
+      me._down = v.getBoolValue();
+      me._onStateChange();
+    });
+    me._registerListener(id);
+    return me;
+  },
   setDown: func(down = 1)
   {
     if( me._checkable or me._down == down )


and this is the result in the console:

Code: Select all

Enabling ATI viewport hack
Starting automatic scenery download/synchronization. Using built-in SVN support. Directory: '/home/clement/FlightGear/TerraSync'.
environment init
onRemove()     <-- here I open my canvas window
_registerListener()  <-- this is automaticalled at same time I opened my canvas window
nb of listener: 1
1  <-- here I press the button of my joystick, everything works as I want
0
1
0
onRemove()  <-- here I close my canvas window, I should see   "print("onRemove::removelistener");" also but it looks like the foreach loop doesn't work :/
onRemove()  <-- here I open again my canvas window
_registerListener()
nb of listener: 1
1  <-- here I press the button of my joystick
1  <-- and now 2 listeners are executed... that's what I have to fix
0
0
1
1
0
0


So I conclude that the listener is correctly created (because he works) and registered ( "_registerListener()") but when I want to delete my listener it doesn't work because it seems that me._listeners is empty that's why the foreach() is not executed.

[EDIT] I added a "size(me._listeners)" in the onRemove() method and it return 0. So I don't understand what append... in the _registerListener() method it works as expected (append(me._listeners, id); size(me._listeners, id); give me "1" as expected) but when I want to retrieve my me._listeners vector in the onRemove() method I discover that me._listeners is empty


Any suggestion ?

[TheTom]

I don't like the idea of extending the API to support properties directly. What if the next one wants to connect other methods to properties? I would like to see something more general, probably something like listener objects, which automatically destroys the associated listenere if it gets garbage collected. You would just have to add the listener object to the widget/object/etc. to whose lifetime it should be bound and would automatically get it cleaned up if the object is garbage collected. I already got some problems with methods called on destruction and want to let the garbage collector handle as much as possible.

Maybe we could add something similar to maketimer or qt connect:

Code: Select all

# Let the listener be destroyed once listener_ref is garbage collected
var listener_ref = makelistener(<prop>[, <self>], <func>);

# Wrapper around makelistener to tie the resulting object to the lifetime of an object
# (For hashes this would be trivial. I'm not yet sure what the best solution would be
# for ghosts. We would need space for an additional naRef per ghost instance
# somewhere, or an global lookup table...)
connect(<prop>, <obj>, <func>);


[Hooray]

I agree completely, and having a dedicated OO API for listener management, analogous to maketimer(), would be in line with Zakalawe's proposal here:

Listener objects

Dumb question of the day. Someone recently pointed out that the setlistener() / removelistener() API makes it easy to leak resources. So I wondered about making an alternate API where the return value from setlistener must be kept, or the listener is removed. I can imagine this with a helper object

var myL = setlistener2("some/prop", func { ... }} )
myL.addprop("some/other/prop");
myL.addprop("yet/another/prop");

Now you need to retain a ref to myL or the listeners on all the props are removed. I don't think we can retro-fit this to the existing API, because I suspect many places just ignore the return value and would break with this change.

(Or even pass a vec of property names to setlistener2, to avoid all the discrete calls to 'addprop', that's a seperate detail really)

It seems like this would work, and be easy enough to implement - question is if it gives enough benefit to be worth the confusion.

[F-JJTH]

Ok, so my working solution is not welcome.
Do you plan to commit a working solution that you like soon ? I'm asking because I can't continue to work on my project until this issue is not addressed.

[Hooray]

Given that you have commit access to $FG_SRC, you'd only need to expose SGPropertyChangeListener via cppbind, for which you can refer to the cppbind/maketimer() implementation wrapping SGTimer ($FG_SRC/Nasal / FGNasalSys) according to the proposal outlined above: http://wiki.flightgear.org/Nasal/CppBind

[F-JJTH]

Well to be honest I'm not interested to dig into CppBind :/ I never looked at this part of the source code so I have no clue what happen in this part of SG but not really seduced to look at it.
TheTom is working on this source code since lot of month, it's his baby and I wouldn't touch his baby at least for respect.

[TheTom]

I started to work on this, but it turned out to be a bit more complicated than I thought, due to issues with the garbage collector. But I think they should be solveable and I already got an idea (have a look at the mailing list if you are interested...).