Page MenuHomePhabricator

Efl.Access mixin removal
Open, Incoming QueuePublic

Description

This task aggregates all activities related to removal of MIXINs from Efl.Access.* namespace.

There are few fundamental reasons for such change:

  1. (major) The C# Binding generator cannot utilize mixins correctly
  2. (major) Efl.Access interfaces causes many name clashes and "pollute" current Efl.Widget and Efl.Widget.Item namespace. As accessibility features seems to be quite separate functionality, it make sense to provide separate class for it.
  3. Few mixins are currently composed of other interfaces (eg. Efl.Access.Component which uses Efl.Gfx.*), which in fact do not make any sense, since standalone Efl.Access objects will not support majority of Efl.Gfx.* methods.

@Jaehyun_Cho could You elaborate if (1) is still an issue? While talking with @bu5hm4n it occurred that even core EFL members do not fully understand the problem. It is assumed that recent changes to eo/eolian already solved it.

The refactoring process will contain many steps. The final changes will contain:

  1. The Efl.Access.Object will be a regular class
  2. The Efl.Access.Object will be created (refed) & destroyed (unref) by Efl.Ui.Widget and Elm.Widget.Ite. The lifetime of those objects is
  3. The Efl.Access.Object will be created on demand by Efl.Ui.Widget and Elm.Widget.Item
  4. The basic method for obtaining Efl.Access.Object from Efl.Ui.Widget and Elm.Widget.Item will be by using efl_object_provider_find interface, the snippet:
Evas_Object *button = efl_add(EFL_UI_BUTTON_CLASS, parent);
Elm_Access_Object *access = efl_object_provider_find(button, EFL_ACCESS_OBJECT_CLASS);
efl_access_object_description_set(access, "By clicking this button, the form will be uploaded to server");
  1. The Efl.Ui.Widget and Elm.Widget.Item will have overloaded efl_object_provider_find function which in case of EFL_ACCESS_OBJECT_CLASS will return already created efl_access_object or call a constructor method which could be overloaded in derived classes:
class Efl.Ui.Widget {
methods  {
      access_object_create @protected {
          retun; Efl.Access.Object;
      }
}
}
_efl_ui_widget_efl_object_provider_find(Eo *obj, ......, Eo_Class *klass)
{
    if (klass == EFL_ACCESS_OBJECT_CLASS) {
        if (!pd->access_object)
             pd->access_object = efl_ui_widget_access_object_create();
        return pd->access_object;
    }
}

_efl_ui_widget_access_object_create(Eo *obj, ......, )
{
   Efl_Access_Object *access = efl_add(EFL_ACCESS_OBJECT_CLASS, obj);
   efl_access_object_role_set(access, EFL_ACCESS_ROLE_INVALID);
   return access;
}

@Jaehyun_Cho , could You check if above changes make sens in case of C# bindings and will not cause additional problems?

stanluk created this task.Jan 23 2019, 2:28 AM
stanluk added a subscriber: kimcinoo.

The plan looks very detailed and good to me!

The usage of the Efl.Access.Object looks very simular to the usage of Efl.Ui.Focus_Manager, and Efl.Ui.Scroll_Manager, I like it a lot that we start to use those patterns.

A few questions so I understand this correctly:

  • The widget / widget_item class will not have any API from access beside the access_object_create methods ? (Maybe its worth to make a mixin out of them and extend from it in Elm_WIdget_Item and Efl.Ui.Widget and Efl.Ui.WIdget_Item)
  • The outside world will not see any API of access unless they explicitly get the object via the provider Find method ?

@bu5hm4n

To answer Your questions:

  1. Yes, there will be no public methods, only single "access_object_create" protected method. It can be extracted into mixin, however I'm afraid it may again cause problems with C# bindings generator.
  2. Yes, the plan is to use efl_provider_find method as generic way to get access object from other objects. Alternatively, the dedicated method "access_obect_get" can be created, however I assumed that "provider_find" method fit this concept so may be reused.

Cool! yeah provider_find is likely the better way to do this.
Regarding the mixins: I am still not sure what the problem is, so i wold say using a mixin there is just fine, as all problems regarding this do seem to be resolved after talking to @felipealmeida.

@bu5hm4n

Yesterday I was talking with @Jaehyun_Cho about the MIXINS and C#. I was able to understand C# binding generator a bit better. As it is already known, the C# bindings will
provide binding functionality (the possibility to call C code from C#), as well as extensions functionality (allow to call c# from c code).

The obvious issue with mixins is that concept do not exists in c# language. It have few implications:

  • the EFL mixin have to be mapped to other concept (class/interface/??) existing in C#, which increases complexity of the bindings and may confuse developers as inheritance/interface tree do not match the one existing in C/EFL world.
  • the implementation of function is separated from class implementation and according to mr @Jaehyun_Cho this is non-obvious for C# developers and makes debugging experience worse.
  • the other problem is how to make mixins usable from C# point of view. As an example, imagine for example that C# developer creates new widget:
class MyWidgetInCSharp : Efl.Ui.Widget
{
}

In EFL we have something like 'Efl.Ui.Focus.Composition' mixin, which, as far as I understand the code, may be used to implement specific focus behaviour. The usage is it just attached to any class inheriting from Efl.Ui.Widget and it just works. Now assume that someone want to utilize this mixin in C# code. It cannot be done when mixin is a class, as C# do not support multi-inheritance. It cannot be done when mixin is an interface as it will require its implementation in C# and makes a whole mixin usage broken.

Off top of my head ideas to solve mixin problems are as follows:

  • Do not expose mixins in C# at all. The will become implementation detail of the class, the mixin methods may be just copy-pased into binded C# class implementation. Such solution may be problematic as mixins provide a lot of functionality (there are ~30 mixins in source tree) and also the mixin is a type itself in EFL, so it may be referenced by other classes.
  • [drastic] Remove mixins from EFL type system totally. It might be a bad design decision to make such construct on the first place. If the goal was to allow easy bindings creation the type system should be as minimalistic as possible.
  • any other ideas???

@stanluk
As you know, currently EFL C# converts mixin in .eo to interface in .eo.cs.
This works well but as I told you, app developers may be confused when they investigate or debug the C# interface which is originally mixin in .eo.

  • Do not expose mixins in C# at all. The will become implementation detail of the class, the mixin methods may be just copy-pased into binded C# class implementation. Such solution may be problematic as mixins provide a lot of functionality (there are ~30 mixins in source tree) and also the mixin is a type itself in EFL, so it may be referenced by other classes.

As I understand, this method either converts mixin to interface or moves mixin's methods to class' methods/properties.
In my opinion, many mixin cases in EFL can be modified as above (converted to interface or moves into class' method/properties) and its design would be better.
This is basically what I want but we cannot always modify as above because as you mentioned, there are cases using mixin is better (e.g. to reduce too many duplicate codes)
So I suggested you not to use mixin in Efl.Access as much as possible.

In my opinion, many mixin cases in EFL can be modified as above (converted to interface or moves into class' method/properties) and its design would be better.

I totally agree, the main reason why Efl.Access.Object is a mixin is because it should have to be implemented in Elm.Widget.Item and Efl.Ui.Widget which do not share any base class which I could modify.

I've made a quick look on how mixins are used in elementary and some mixins can be removed already:

[removable] Efl.Ui.Dnd_Container -> seems unused in the code base.
[removable?] Efl.Ui.Widget_Focus_Manager -> seems theoretically replacable with something like Efl.Ui.FocusManagerWidget, however not 100% sure
Efl.Ui.Focus.Composition -> seems non-removalbe without big code dupliation or major refactor
Efl.Ui.Focus.Object -> used in Efl.Ui.Widget and few Elm.Widget.Item derived classes. Again as in case of Efl.Access.Object no common base class between Elm.Widget.Item and Efl.Ui.Widget is the reason.
[removable] Efl.Ui.Dnd -> can be made a interface and be implemented in Efl.Ui.Widget
Efl.Ui.Focus.Layer -> removes code duplication between Elm.Notify and classes inheriting from Efl.Ui.Layout
Elm.Interface_Scrollable -> seems non-removalbe without big code dupliation or major refactor
Efl.Ui.Focus.Manager_Sub -> seems non-removalbe without big code dupliation or major refactor
[removable] Efl.Ui.Selection -> not really required to be a mixin, can be made an interface with the implementation in Efl.Ui.Widget

Well, mixins in efl-focus are basically used when tehre is no common ground. Which also applies to Efl.Ui.Widget_Focus_Manager. And as @Jaehyun_Cho wrote, a few cannot or should not be removed, as this would lead to code duplication.

@Jaehyun_Cho I can understand your point, that this may be confusing to C# app developers. However the concept we have looks very simular to what https://www.c-sharpcorner.com/UploadFile/b942f9/how-to-create-mixin-using-C-Sharp-4-0/ proposes. Therefore I would claim that this should be common group for a c# app developper, as this seems to be some sort of pattern which is even documented :)

Jaehyun_Cho added a comment.EditedFeb 20 2019, 10:42 PM

@bu5hm4n

Thank you for the link https://www.c-sharpcorner.com/UploadFile/b942f9/how-to-create-mixin-using-C-Sharp-4-0/ !!
It is brilliant! :)

However, the proposed mixin and our eo's mixin has still a huge difference about instantiation.
As the proposed mixin described in the link, the mixin is defined as follows.

A mixin is a class which adds functionality to other classes but which cannot itself be instantiated.

However, our eo's mixin calls constructor and destructor per its instance.

e.g. In the proposed mixin of using C#,

public static class AgeProvider
{
    static AgeProvider()
    {
        /* This is called only once.
         * Therefore, any data allocated here is not instance specific data. */
    }
}

e.g. In our eo's mixin,

EOLIAN static Eo * _age_provider_efl_object_constructor(...)
{
    /* This is called whenever its instance is instantiated.
     * Therefore, any data allocated here may be instance specific data. */
}
stanluk added a comment.EditedFeb 21 2019, 12:44 AM

@Jaehyun_Cho
Aren't examples that You provided only the implementation detail that C# developer should not care about? From C# dev perspective mixin only adds method to class, nothing else.

The eo constructor and destructor are not a problem as all C# interfaces already inherit from something like IEoWrapper (or something), which implicate that every C# class must have in fact internal eo class. The more problematic are mixins that require some higher level objects like Focus mixins requiering Efl.Ui.Widget.

There is also other issue I'm warried about. Currently the EO mixins provides the methods that are generally virtual. As far as I know the static methods cannot be overrided in C#, so in C# we will loose this functionality. There are already cases that this happens in source code eg. Efl.Access.Object mixin and Efl.Ui.Widget, Efl.Ui.Focus.Composition mixin and Elm.Grid. My idea is to maybe split interface from implementation and for every mixin generate two types in fact

// mixin.eo 
// mixin Mixin requires Widget
// methods {
//      methodA {}
//      methodB {}
// }

interface Mixin : IWrapper
{
   void MethodA();
   int MethodB(int a);
}

static class MixinImplementation
{
     static void MethodA(Widget self);  // this and below should use eo_super level internally to avoid recursive calls
     static int MethodB(Wdiget selft, int a);
}

so Mixin can be used such way:

class MyCustomClass : Widget, Mixin
{
   void MethodA()
    {
           MixinImpementation.MethodA(this)
    }
   
   int MethodB(int a)
    {
           return MixinImpementation.MethodB(this, a)
    }
}

And mixin methods can be overrided in standard way:

class MyDerivedCustomClass : MyDerivedCustomClass
{
   void MethodA()
    {
          /// do something else if required
    }

There are obvious downsides of such approach:

  1. The Mixin itself do not carry information about Widget class requirement so it may be missused by C# dev
  2. The implmentation of Mixin interface is tedious

@stanluk

Thank you for suggesting your new idea :)
However, I am afraid that the downsides are bigger than now I think..

As I understand, we need 1 interface, 1 static class, and 1 regular class for 1 mixin. Is that right?
(mixin Mixin is converted into interface Mixin, static class MixinImplementation, and class MyCustomWidget)

If so, then I think it requires many interface and classes for 1 mixin. Moreover, it may mislead C# dev to use interface Mixin incorrectly as you mentioned.
So IMHO, it would be better to convert 1 mixin to 1 interface and implement the method in regular class. (This is current approach)
(e.g. mixin Mixin is converted into interface Mixin and any class implementing the interface Mixin implements the methods MethodA() and MethodB())

IMHO, we have either 2 choices to handle Eo's mixin in C# bindings.

  1. Replace the mixin Mixin with class Mixin or interface Mixin in Eo. (This is case by case. I do not insist that all mixin should be replaced.)

OR

  1. Keep the mixin Mixin in Eo and C# binding converts the mixin Mixin to interface Mixin (This is current approach)