Papierkorb / bindgen

Binding and wrapper generator for C/C++ libraries
GNU General Public License v3.0
179 stars 18 forks source link

Overloaded Qt signals #59

Closed HertzDevil closed 4 years ago

HertzDevil commented 4 years ago

While using qt5.cr I noticed that #on_SIGNAL methods do not work properly if the signal has multiple signatures. One example is the overloads of QComboBox::activated:

The Qt processor translates them to something along the following lines:

module Qt
  lib Binding
    fun bg_QComboBox_CONNECT_activated_CrystalProc_void_int(_self_ : QComboBox*, _proc_ : CrystalProc) : QMetaObjectConnection*
    fun bg_QComboBox_CONNECT_activated_CrystalProc_void_const_QString_R(_self_ : QComboBox*, _proc_ : CrystalProc) : QMetaObjectConnection*
  end

  class ComboBox
    def on_activated(&_proc_ : Proc(Int32, Void)) : SignalConnection
      SignalConnection.new(unwrap: Binding.bg_QComboBox_CONNECT_activated_CrystalProc_void_int(self, BindgenHelper.wrap_proc(_proc_)))
    end

    def on_activated(&_proc_ : Proc(String, Void)) : SignalConnection
      SignalConnection.new(unwrap: Binding.bg_QComboBox_CONNECT_activated_CrystalProc_void_const_QString_R(self, BindgenHelper.wrap_proc(_proc_)))
    end
  end
end

However, Crystal does not support method overloading through different type restrictions on a block, so the second on_activated definition overwrites the first, which means code like this fails to compile:

cb = Qt::ComboBox.new
cb.on_activated(&->(x : Int32) { puts "item index #{x}" })
# Error: expected block argument's argument #1 to be String, not Int32

The int overload itself works if the class is reopened and the method definition is repeated under a different name, but this is most certainly not end users are expected to do themselves.

docelic commented 4 years ago

Hey @HertzDevil that's an interesting question. @Papierkorb do you have a preference on how you'd like to see this issue solved/worked around?

HertzDevil commented 4 years ago

I took a look at PyQt5. It exposes signals and slots directly, and quite conveniently gives the following example for QComboBox:

from PyQt5.QtWidgets import QComboBox

class Bar(QComboBox):

    def connect_activated(self):
        # The PyQt5 documentation will define what the default overload is.
        # In this case it is the overload with the single integer argument.
        self.activated.connect(self.handle_int)

        # For non-default overloads we have to specify which we want to
        # connect.  In this case the one with the single string argument.
        # (Note that we could also explicitly specify the default if we
        # wanted to.)
        self.activated[str].connect(self.handle_string)

    def handle_int(self, index):
        print "activated signal passed integer", index

    def handle_string(self, text):
        print "activated signal passed QString", text

In Python str is a class, and a particular overload is selected by the classes that go between the []. I think we could imitate this by requiring classes themselves as arguments to #on_SIGNAL:

module Qt
  class ComboBox
    def on_activated(_type1_ : Int32.class, &_proc_ : Proc(Int32, Void)) : SignalConnection    
    def on_activated(_type1_ : String.class, &_proc_ : Proc(String, Void)) : SignalConnection
  end
end

cb = Qt::ComboBox.new
cb.on_activated(Int32) { |x| puts "item index #{x}" }
cb.on_activated(String) { |x| puts "item text #{x}" }

The Qt processor would need to detect all such overloads and inject those dummy type arguments only for signals that are overloaded; signals with unique signatures could stay the same. Classes are constant values in Crystal, so this approach should have little to no overhead while remaining readable. The only possible ambiguity is when one of the overloads takes no arguments, as the generated method would then look like one without overloads (it probably doesn't matter).

I don't know how the PyQt5 documentation defines what the default overload is. I don't think we need that either.

Papierkorb commented 4 years ago

I think passing the type as argument for overloaded signals is a clever solution! I don't think there needs to be a "default" signal, as we can't predict the most useful overload so let's not.

For an overload where one signal comes with no arguments the resulting on_method could simply take no arguments beyond the block.