Generate Signals

[mjakeman]

This PR currently generates signals with the correct syntax, and with the entire project compiling.

The codegen part is itself complete (minus some edge-case features that we may want to consider down the road), however I haven't yet implemented the generic SignalArgs-based argument classes for signals with additional parameters.

TODO:

• Generate Signal descriptors
• Generate Signal event handlers
• Generate SignalArgs-based classes
• Generate SignalHandler delegates that use the above (assume return values are unsupported for now)
• GValue Marshalling
• Test this all in practice
• Create custom eventhandler which do not require any null value handling or casting for custom SignalArgs
• Make SignalArgs fields pascal case
• Make SetArgs method internal and provide protected Value[] to extract the data from

[badcel]

Could be merged if it is working with the latest version of the code 👍

[mjakeman]

Run into a bit of trouble. Seems like Parameters aren't getting serialised in GSignal while the exact same code (and XML elements) works for GMethod. For that matter, neither are Return Values.

Adding this in GenerateClasses in GObjectGenerator should illustrate what I mean:

// GObjectGenerator.cs
// Inside GenerateClasses()
if (cls is GClass)
{
    if (cls.Name == "Object")
        System.Console.WriteLine("Detected: GObject");

    foreach (var signal in (cls as GClass)!.Signals)
    {
        if (signal?.Parameters != null)
        {
            System.Console.Write("Parameter!");
        }
    }
}

Not too sure what's going on. I might be missing something obvious here.

Any ideas?

Edit: In regards to merging, I want to try and get SignalArgs done first, since it seems like it'll be a bit more difficult than I hoped.

[badcel]

@firox263 You have to add the namespace of the repository to the properties. The glib:signal xml element is explicity in the glib namespace. I suppose that the return-value and parameters elements are not part of this namespace and therefore not found.

I added the parameters in your branch. It should be working now.

[mjakeman]

@firox263 You have to add the namespace of the repository to the properties. The glib:signal xml element is explicity in the glib namespace. I suppose that the return-value and parameters elements are not part of this namespace and therefore not found.

I added the parameters in your branch. It should be working now.

Ahh thanks, that makes a lot more sense thinking about it.

I've managed to get the SignalArgs generation to a good point, however the final blocker before this can be merged is how we marshal between GValue and the individual unboxed types of each parameter.

// DBusConnection.Generated.cs

/// <summary>
/// Signal (Event) Arguments for OnClosed
/// </summary>
public sealed class ClosedSignalArgs : SignalArgs
{

    /// <summary>
    /// %TRUE if @connection is closed because the
    ///     remote peer closed its end of the connection
    /// </summary>
    public bool remote_peer_vanished;


    /// <summary>
    /// a #GError with more details about the event or %NULL
    /// </summary>
    public GLib.Error? error;


    public override void SetArgs(Value[] args)
    {
        // FIXME: Marshalling
        // We need to find a way to marshal from GValues to the concrete
        // type of the parameter. This will probably involve a generator-side
        // type dictionary which stores rich type information as we
        // walk through the introspection data.

        remote_peer_vanished = args[1].To</* Type Goes Here */>();
        error = args[2].To</* Type Goes Here */>();
    }
}

Since we're generating one SignalArgs-based class for every signal with non-default properties, I'm debating whether using GClosure is actually helpful, since we're not really using GValues and they only serve to add more overhead. I might experiment with using Signals or CClosures directly later on, but right now GValues should be more than fine.

The main point is finding a way to determine the type of marshal for a given type (e.g. GLib.Error should use g_value_get_boxed from what I can gather).

[badcel]

Regarding SignalArgs I agree to your findings. We should do as you suggest because we don't have more information available. We have a GValue and a target type, that's it. So our custom code has to do the rest: Get the value in case of a primitive data type, class or struct.

[mjakeman]

Thanks @badcel for the review.

Unfortunately, I don't think I'll have the time to work on this for at least the next two weeks. If you guys would like to pick this up and work on it in the meantime, feel free to commit in this branch, otherwise I'll try and get back to it later this month.

Currently, signal generation is almost complete. The last step is, as I mentioned above, is marshalling to/from GValues, which will probably need some changes to the generator's type resolver.

[badcel]

I think it looks quite interesting and I would like to see how it is done. As it is not hurting we can have it available. But at least from my current point of view it will be used quite seldom.

[na2axl]

Ok @badcel I will try it, this may end to the need to modify a bit the signal generator since I will have to introduce a feature I planned to implement in the Toolkit layer, SignalIndexerDescriptor (to connect/disconnect signals) and PropertyIndexerDescriptor (for MVVM binding on properties).

[badcel]

@na2axl Perhaps you can do a separate pull request into this branch for the SignalIndexerDescriptor and PropertyIndexerDescriptor so we can evaluate before merging?

[badcel]

I updated the code to support concrete signal handlers.

Now we generate a lot of indexers but this has a positive effect: We can't add a SignalHandler for a Button in the Notbook object and vice versa. Beforhand it was possible to add the SignalHandler for the Button.ClickedSignal into the Notebook object. 😎

[na2axl]

Hi @badcel, @firox263, talking about indexer descriptors, I think it will be better to finally implement it in the Toolkit layer, since it will add more complexity to the Core layer, and they are not really directly related to GNOME libraries...

[badcel]

@na2axl Hi @badcel, @firox263, talking about indexer descriptors, I think it will be better to finally implement it in the Toolkit layer, since it will add more complexity to the Core layer, and they are not really directly related to GNOME libraries...

Okay 👍 In this case I would prefer to see the code in the toolkit layer, too (if possible).

It is done. For me this is the code we can start to improve on. From my point of view the other pull requests are now obsolete. If you agree i would discard them @firox263 @na2axl ?

The next qustion would be if we merge this into develop? Then we can push further progress directly into develop and don't need to make PRs for PRs all the time.

I would be okay with merging this into develop as the general direction of the code is now settled and we have some working samples.

[na2axl]

@badcel what if we merge all theses PRs recursively to finally merge #52 in develop ? Beside that, I've no problem for merging this directly in develop 🙂.

[mjakeman]

Adding to what @na2axl said, I would be in favour of doing it this way, merging #52 into develop. I think we've managed to get a very solid base for moving forward with.

When we're ready to merge, I plan on spending some time on getting our documentation up to date. I've been wanting to revisit the website for some time and I'll look into generating an API reference while we're at it.

[badcel]

Okay we can do it recursively 👍 Then we don't loose all our comments which are still an important part of this.

Do you guys think that we are actually ready to merge or do we want to continue polishing things up with more stacking of pull requests?

My vote is for merging, because I think we are like rebooting the project and the code in develop is giving quite a false impression of this project in the moment.

[na2axl]

I agree to merge. We have managed to get a good code base for further improvements. For cleaning code and other stuffs we can consider doing this on that new code base.

[mjakeman]

I agree. I think we should merge now, and work on fixing any remaining issues in develop.

If we're all in agreement, would you like to go ahead and merge? @badcel

[na2axl]

@badcel, please wait a bit before merge, I'm currently refactoring all the code according to our .editorconfig. I will open a PR for this branch when finished.

[na2axl]

@badcel, @firox263 I've noticed that the generation of properties is disabled, it is intended ? I didn't know about that...

[badcel]

@na2axl I've noticed that the generation of properties is disabled, it is intended ? I didn't know about that...

Yes at the current stage this is intended. We agreed to start without property generation here (at least nobody argued against it): #68 (comment)

I'm still waiting for feedback regarding my last proposal from you guys regarding property generation. I think this could be a possible solution: #68 (comment)