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

[RFC002] Static GProperty Descriptors #57

Closed
na2axl opened this issue Aug 21, 2020 · 12 comments · Fixed by #58, #52, #70 or #75
Closed

[RFC002] Static GProperty Descriptors #57

na2axl opened this issue Aug 21, 2020 · 12 comments · Fixed by #58, #52, #70 or #75
Assignees
Labels
Discussion Plan the future Enhancement New feature or request

Comments

@na2axl
Copy link
Member

na2axl commented Aug 21, 2020

Static GProperty Descriptors

RFC

Please see the RFC here: https://github.com/gircore/rfcs/blob/master/rfc-002-static-properties.md
It explains the proposal and some potential solutions.

Summary

To allow Gir.Core be a part of the current C# UI ecosystem, the implementation of GProperties can be done using Static Properties, to match what it's done now using DependencyProperty in other libraries. For that we have the following goals:

  • Declare GProperties of current Wrappers using Static Properties;
  • Allow library users to declare GProperties of theirs Subclass using Static Properties;
  • An high-level API will provide access to the GProperty value, to retrieve and edit it through the corresponding Property Descriptor.
  • Property Descriptors must know the type (the C# type) of the GProperty;
  • Property Descriptors must know the type (the C# type) of the GObject in which it will be registered;
  • Property Descriptors must know the name of the GProperty;
  • Property Descriptors must know if the GProperty is write-only, read-only, or read-write;
  • Property Descriptors must allow an easy registration and initialization of the GProperty in the GObject;
  • Property Descriptors must also describe child GProperties;
  • All of these points must be implemented in an easy to use, DependencyProperty-like manner.

These goals can lead us to these achievements:

@na2axl na2axl linked a pull request Aug 22, 2020 that will close this issue
@na2axl na2axl self-assigned this Aug 22, 2020
@na2axl na2axl added Discussion Plan the future Enhancement New feature or request labels Aug 22, 2020
@na2axl
Copy link
Member Author

na2axl commented Aug 23, 2020

I've noticed that we have talked about properties without seeing another important guys, signals.

For now I've seen that to register to signals in wrappers we call RegisterEvent() in constructor, but what if the end user want to create custom signals in a C# subclass ? I know this can not be necessary since C# events can do the same work as C signals, but if is something we have to consider, I think it will be doable in the same way properties descriptors are implemented now.

What do you think ?

@na2axl
Copy link
Member Author

na2axl commented Aug 23, 2020

For my idea to make signals works like properties, this is the implementation I'm testing (not pushed on #58 for now)

using System;
using System.Collections.Generic;

namespace GObject
{
    public class SignalEventArgs : EventArgs
    {
        public object[] Args { get; }

        internal SignalEventArgs(params object[] args)
        {
            Args = args;
        }

        internal SignalEventArgs(params Sys.Value[] args)
        {
            Args = new object[args.Length];

            for (int i = 0; i < args.Length; i++)
            {
                // TODO: Args[i] = args[i].Value;
            }
        }
    }

    /// <summary>
    /// Describes a GSignal.
    /// </summary>
    public sealed class Signal
    {
        #region Fields

        private static readonly Dictionary<EventHandler<SignalEventArgs>, ActionRefValues> _handlers = new Dictionary<EventHandler<SignalEventArgs>, ActionRefValues>();

        #endregion

        #region Properties

        /// <summary>
        /// The name of the GSignal.
        /// </summary>
        public string Name { get; }

        /// <summary>
        /// The signal flags.
        /// </summary>
        public Sys.SignalFlags Flags { get; }

        /// <summary>
        /// The return type of signal handlers.
        /// </summary>
        public Sys.Type ReturnType { get; }

        /// <summary>
        /// The type of parameters in signal handlers.
        /// </summary>
        public Sys.Type[] ParamTypes { get; }

        #endregion

        #region Constructors

        private Signal(string name, Sys.SignalFlags flags, Sys.Type returnType, Sys.Type[] paramTypes)
        {
            Name = name;
            Flags = flags;
            ReturnType = returnType;
            ParamTypes = paramTypes;
        }

        #endregion

        #region Methods

        public static Signal Register(string name, Sys.SignalFlags flags, Sys.Type returnType, params Sys.Type[] paramTypes)
        {
            return new Signal(name, flags, returnType, paramTypes);
        }

        public static Signal Register(string name, Sys.SignalFlags flags = Sys.SignalFlags.run_last)
        {
            return new Signal(name, flags, Sys.Type.None, Array.Empty<Sys.Type>());
        }

        public void Connect(Object o, Action action, bool after = false)
        {
            if (action == null)
                return;

            o.RegisterEvent(Name, action, after);
        }

        public void Connect(Object o, ActionRefValues action, bool after = false)
        {
            if (action == null)
                return;

            o.RegisterEvent(Name, action, after);
        }

        public void Connect(Object o, EventHandler<SignalEventArgs> action, bool after = false)
        {
            if (action == null)
                return;

            if (_handlers.TryGetValue(action, out ActionRefValues callback))
                return;

            callback = (ref Sys.Value[] values) => action(o, new SignalEventArgs(values));
            o.RegisterEvent(Name, callback, after);
            _handlers[action] = callback;
        }

        public void Disconnect(Object o, Action action)
        {
            if (action == null)
                return;

            o.UnregisterEvent(action);
        }

        public void Disconnect(Object o, ActionRefValues action)
        {
            if (action == null)
                return;

            o.UnregisterEvent(action);
        }

        public void Disconnect(Object o, EventHandler<SignalEventArgs> action)
        {
            if (action == null)
                return;

            if (!_handlers.TryGetValue(action, out ActionRefValues callback))
                return;

            o.UnregisterEvent(callback);
            _handlers.Remove(action);
        }

        #endregion
    }
}

And a simple example to use this in a wrapper will be:

using System;
using GObject;

namespace Gtk
{
    public class Button : Widget
    {
        public static readonly Signal ClickedSignal = Signal.Register("clicked");

        public event EventHandler<SignalEventArgs> Clicked
        {
            add => ClickedSignal.Connect(this, value, true);
            remove => ClickedSignal.Disconnect(this, value);
        }

        // ...
}

@badcel, @firox263 what do you think about the syntax ? It can be ease you the generation of wrappers ?


EDIT: The use of this syntax works as expected.

@badcel
Copy link
Member

badcel commented Aug 23, 2020

This looks good to me, we should see how the signals and properties integrate into: #52

@na2axl
Copy link
Member Author

na2axl commented Aug 23, 2020

Good, I will rebase you recent changes on #52 into #58 and try to continue on this way then :-)

@mjakeman
Copy link
Member

One thing I would like to try and keep are strongly typed signal args. It makes the developer experience much nicer as it integrates with intellisense, etc

For example:

class DestroySignalArgs : SignalArgs
{
    string someRelevantArg;
    int someOtherRelevantArg;
} 

@badcel
Copy link
Member

badcel commented Aug 23, 2020

👍

I think you could just try to use the new properties / signals in Window and Button, this is enough for the small quickstart demo in the samples.

With this we could check the overall design and decide if this is what we want.

@na2axl
Copy link
Member Author

na2axl commented Aug 23, 2020

One thing I would like to try and keep are strongly typed signal args. It makes the developer experience much nicer as it integrates with intellisense, etc

Good point, but these ones can be generated? Or they will be hand crafted?

@badcel
Copy link
Member

badcel commented Aug 23, 2020

Good point, but these ones can be generated? Or they will be hand crafted?

For the proof of concept we can hand craft them. Later we can try to generate as much as possible if there is no API tradeoff.

@na2axl na2axl linked a pull request Aug 24, 2020 that will close this issue
@na2axl
Copy link
Member Author

na2axl commented Aug 27, 2020

@firox263

One thing I would like to try and keep are strongly typed signal args. It makes the developer experience much nicer as it integrates with intellisense, etc

While it will be totally possible to do that, It will break the experimental feature I've added in #58 for the definition of signals callbacks using indexers. If we want theses features together, a possible workaround will be to generate theses indexers at the same time we will generate signal descriptors:

// In the Container class...

// The signal descriptor will need to know the type of the SignalArgs
public static readonly Signal<AddedSignalArgs> AddSignal = Signal<AddedSignalArgs>.Register("add");

// This one need to be generated, or added manually
public EventHandler<AddedSignalArgs> this[Signal<AddedSignalArgs> signal]
{
    set => signal.Connect(this, value, true);
}

@mjakeman
Copy link
Member

mjakeman commented Aug 27, 2020

I think it's reasonable to generate this automatically. It's definitely possible as we have the necessary type annotations in Gir data.

We should register a normal event handler so we still get the += syntax, and use this inside the indexer.

Edit: We should write these manually for now and add them to the generator once we're satisfied on the exact syntax

@na2axl
Copy link
Member Author

na2axl commented Aug 27, 2020

@firox263 the complete syntax is this:

// The signal descriptor
public static readonly Signal<AddedSignalArgs> AddSignal = Signal<AddedSignalArgs>.Register("add");

// The event handler
public event EventHandler<AddedSignalArgs> Added
{
    add => AddSignal.Connect(this, value, true);
    remove => AddSignal.Disconnect(this, value);
}

// The signal indexer
public EventHandler<AddedSignalArgs> this[Signal<AddedSignalArgs> signal]
{
    set => signal.Connect(this, value, true);
}

But I don't think it's possible use the event handler directly into the indexer 🤔

@mjakeman
Copy link
Member

That's perfectly fine in my opinion. As long as we support the traditional use case of event handlers as well as the indexer, I don't think there's a problem here. Lgtm 👍

@badcel badcel mentioned this issue Aug 28, 2020
This was linked to pull requests Aug 31, 2020
@na2axl na2axl linked a pull request Oct 16, 2020 that will close this issue
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment