Page MenuHomePhabricator

csharp: Change to new class API.
ClosedPublic

Authored by lauromoura on Nov 12 2018, 8:02 AM.

Details

Summary

As discussed in T7204:

  • Eo Interfaces/mixins -> C# Interfaces with concrete class implementations
  • Eo Regular/Abstracts -> Proper C# classes
  • Added some new generators and helper methods.
  • Refactored the class generator, splitting into helper methods

Eo handles now are stored only in the "root" class in any given
inheritance tree (generally, Efl.Object), and accessible to each child.
Methods also are defined in a single place instead of repeatedly
generated in everyfile, reducing the size of the generated .dll from
30MB to around 4.5MB.

Mixins are generated as C# interfaces but any regular class it inherits
from is lost, as we can't have interfaces inheriting from regular
classes. This will be dealt with in a later commit.

Summary of API Changes:

  • Merged Inherit/Concrete classes. (These suffixes disappear from regular classes).
  • Interface still have implementations with 'Concrete' suffix for when they are returned from methods.
  • Removed 'I' from interface names.
  • Removed interfaces for regular/abstract Eo classes.
  • Concrete classes for interfaces/mixins hold the event argument struct.
  • Removed '_' from classes, enums, structs, etc, as indicated in C# naming conventions.
  • Namespaces are now Camel.Cased.
  • Renamed IWrapper's raw_handle/raw_klass to NativeHandle/NativeClass

Also renamed the test classes as after the namespace change, the
test namespace Test can conflict with the helper Test namespace.
(And use more meaningful names than Test.Testing...)

Also Fixes T7336 by removing a deprecated example and adding
efl_loop_timer_example to build system.

Fixes T7451 by hiding the class_get DllImports and renaming the IWrapper
fields. The native handlers are used in the manual binding.

Still need to work:

  • As there are still some events names clashing (e.g. Efl.Ui.Bg with "resize" from Efl.Gfx.Entity and Efl.Gfx.Image), Events are currently declared on the interface and implemented "namespaced" in the classes, requiring the cast to the interface to access the event.
  • The Mixin Conundrum. Mixin inheritance will be dealt in a future commit.

Depends on D7260

Diff Detail

Repository
rEFL core/efl
Branch
csharp-new-classes
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 8222
Build 7508: arc lint + arc unit
lauromoura created this revision.Nov 12 2018, 8:02 AM
lauromoura requested review of this revision.Nov 12 2018, 8:02 AM

Update revision after conflict with newer version of parent revision.

@lauromoura Thanks a lot, I am very much tempted to merge this already, but there are a couple of questions:

  • efl.input.Interface.KeyDownEvt_Args-->Efl.Input.InterfaceConcrete.KeyDownEvt_Args. Is there any way to avoid this? I think this is the only place where the Concrete class is needed and is very confusing.
  • win.ResizeEvt += ResizeEvt;-->((Efl.Gfx.Entity)win).ResizeEvt += ResizeEvt;. This forces the user to know the inheritance hierarchy. I have to grep all the time to know who emits each event. We definitely need a solution for this. What are your plans?

@lauromoura Thanks a lot, I am very much tempted to merge this already, but there are a couple of questions:

  • efl.input.Interface.KeyDownEvt_Args-->Efl.Input.InterfaceConcrete.KeyDownEvt_Args. Is there any way to avoid this? I think this is the only place where the Concrete class is needed and is very confusing.

The problem is that interfaces can hold only methods, events, properties and indexers, but not other classe/structs (like the *Event_Args ones).

Maybe we could change them "inlining" the class name, like Efl.Input.InterfaceKeyDownEvt_Args. What do you think?

  • win.ResizeEvt += ResizeEvt;-->((Efl.Gfx.Entity)win).ResizeEvt += ResizeEvt;. This forces the user to know the inheritance hierarchy. I have to grep all the time to know who emits each event. We definitely need a solution for this. What are your plans?

This still depends on T7454 (events collision when inheriting).

Maybe we could change them "inlining" the class name, like Efl.Input.InterfaceKeyDownEvt_Args. What do you think?

I like this solution. Let's try it.

This still depends on T7454 (events collision when inheriting).

Event name clashes are blocking you because they produce compiler errors in your new code, correct?
Can you write here the list of these errors so I can fix the name clashes and you can proceed?
In this way, T7454 will not block you.

segfaultxavi added a comment.EditedNov 15 2018, 7:50 AM

@lauromoura How's that list of conflicting event names coming along?
By using eolian I got this list of duplicated events, but it does not mean that they are in the same hierarchy, so some are not actual collisions:

eolian: efl_canvas_scene.eo:231:7: event 'focus,in' conflicts with another symbol (at efl_input_interface.eo:63:7)
eolian: efl_canvas_scene.eo:232:7: event 'focus,out' conflicts with another symbol (at efl_input_interface.eo:64:7)
eolian: efl_gfx_image.eo:208:7: event 'resize' conflicts with another symbol (at efl_gfx_entity.eo:107:7)
eolian: efl_ui_focus_manager.eo:40:9: function 'move' conflicts with another symbol (at efl_gfx_entity.eo:106:7)

Also, is converting Eolian properties into C# properties instead of setter and getter functions still in the roadmap?

@lauromoura How's that list of conflicting event names coming along?
By using eolian I got this list of duplicated events, but it does not mean that they are in the same hierarchy, so some are not actual collisions:

eolian: efl_canvas_scene.eo:231:7: event 'focus,in' conflicts with another symbol (at efl_input_interface.eo:63:7)
eolian: efl_canvas_scene.eo:232:7: event 'focus,out' conflicts with another symbol (at efl_input_interface.eo:64:7)
eolian: efl_gfx_image.eo:208:7: event 'resize' conflicts with another symbol (at efl_gfx_entity.eo:107:7)
eolian: efl_ui_focus_manager.eo:40:9: function 'move' conflicts with another symbol (at efl_gfx_entity.eo:106:7)

I got a bunch more clashes with the pyolian script from P244

  • Collisions for class Elm.Toolbar
    • event: selection,changed: Efl.Access.Selection, Efl.Ui.Selectable
  • Collisions for class Efl.Ui.List_View
    • event: selection,changed: Efl.Access.Selection, Efl.Ui.Selectable
  • Collisions for class Evas.Canvas
    • event: focus,out: Efl.Canvas.Scene, Efl.Input.Interface
    • event: focus,in: Efl.Canvas.Scene, Efl.Input.Interface
  • Collisions for class Elm.Combobox
    • event: filter,done: Elm.Combobox, Elm.Genlist
    • event: expanded: Elm.Combobox, Elm.Genlist
    • event: dismissed: Elm.Combobox, Elm.Hover
    • event: activated: Elm.Entry, Elm.Genlist
  • Collisions for class Efl.Ui.Text
    • event: selection,changed: Efl.Ui.Selectable, Efl.Text_Interactive
  • Collisions for class Efl.Ui.Bg
    • event: resize: Efl.Gfx.Entity, Efl.Gfx.Image
  • Collisions for class Efl.Ui.Win
    • event: focus,out: Efl.Canvas.Scene, Efl.Input.Interface
    • event: focus,in: Efl.Canvas.Scene, Efl.Input.Interface
  • Collisions for class Elm.Entry
    • event: changed: Elm.Entry, Elm.Interface_Scrollable
  • Collisions for class Elm.Genlist
    • event: selection,changed: Efl.Access.Selection, Efl.Ui.Selectable
  • Collisions for class Elm.Scroller
    • event: edge,right: Elm.Scroller, Efl.Ui.Scrollable
    • event: scroll,up: Elm.Scroller, Efl.Ui.Scrollable
    • event: scroll,left: Elm.Scroller, Efl.Ui.Scrollable
    • event: scroll,right: Elm.Scroller, Efl.Ui.Scrollable
    • event: scroll,down: Elm.Scroller, Efl.Ui.Scrollable
    • event: edge,left: Elm.Scroller, Efl.Ui.Scrollable
  • Collisions for class Elm.List
    • event: selection,changed: Efl.Access.Selection, Efl.Ui.Selectable
  • Collisions for class Efl.Net.Server_Fd
    • event: error: Efl.Loop_Fd, Efl.Net.Server

This forces the user to know the inheritance hierarchy. I have to grep all the time to know who emits each event.

(from a previous comment). Doesn't this happens too in C? Maybe we could generate wrapper events for events without duplication, forcing explicit cast for the clashing ones.

Also, is converting Eolian properties into C# properties instead of setter and getter functions still in the roadmap?

Sure. That's exactly what I'm working now. And there are some clashes there (like Efl.Input.Key.Key and Efl.Input.Hold.Hold) too.

As this property clash is more related to C#'s Camel.CaseMethod style (while the event clash is a class structure issue), I'm not fond of modifying these properties only for the sake of C#'s tidiness. Maybe add a prefix (Efl.Input.Key.KeyProp?) or do not generate the property for these.

Filtering out legacy classes which we should not be building for C#, and merging with my items above, the whole list seems to be:

  • event: selection,changed: Efl.Access.Selection, Efl.Ui.Selectable, Efl.Text_Interactive
  • event: focus,out: Efl.Canvas.Scene, Efl.Input.Interface
  • event: focus,in: Efl.Canvas.Scene, Efl.Input.Interface
  • event: resize: Efl.Gfx.Entity, Efl.Gfx.Image
  • event: error: Efl.Loop_Fd, Efl.Net.Server
  • event: move: Efl.Ui.Focus_Manager, Efl.Gfx.Entity

I will try to fix the duplicated event names in T7476. When that is done, can this patch be merged? I would put Properties in a different patch.

lauromoura updated this revision to Diff 17606.Nov 26 2018, 1:37 AM

Update after Xavi's comments on events.

  • Removed the need to cast when accessing interface events.
  • Moved the event argument structure up a level. It will be a sibling of the class declaring the event, with the latter as a prefix (e.g. Efl.Input.IntefaceKeyDownEvt_Args)

Also refactored event code to user EventHandlerList internally as it
is simpler than using proxy events for our use case.

lauromoura updated this revision to Diff 17620.Nov 26 2018, 9:42 AM

Update removing remaining event cast and changing name_helpers.hh comparison to case-sensitive. It was case insensitive when namespaces were lower cased.

vitor.sousa added inline comments.Nov 27 2018, 8:30 AM
src/bin/eolian_mono/eolian/mono/klass.hh
387

Always true?

vitor.sousa added inline comments.Nov 27 2018, 9:19 AM
src/bin/eolian_mono/eolian/mono/name_helpers.hh
55
src/bindings/mono/eo_mono/workaround.cs
376

Should it be uppercase?

396

Gengrid?

lauromoura updated this revision to Diff 17640.Nov 27 2018, 5:20 PM

Update after Vitor comments.

  • Removed extra space
  • Removed the "cls_has_string" variables from generator. All root classes now have this field to prepare for future classes inheriting from them.
  • Cleaned up a bunch of deprecated items from workaround.cs

Update after removing return "abc" from name_helpers

lauromoura updated this revision to Diff 17690.Nov 29 2018, 2:00 PM

Update to use any_of in event_is_unique after suggestion by Vitor.

vitor.sousa accepted this revision.Nov 29 2018, 2:58 PM
This revision is now accepted and ready to land.Nov 29 2018, 2:58 PM
This revision was automatically updated to reflect the committed changes.