Page MenuHomePhabricator

eolian_mono: Add support for C# style properties
ClosedPublic

Authored by lauromoura on Nov 29 2018, 6:13 PM.

Details

Summary

Syntatic sugar around the Get/Set functions for now.

Test Plan

Run efl-mono-suite

Diff Detail

Repository
rEFL core/efl
Branch
new_style_properties
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 8291
Build 7548: arc lint + arc unit
lauromoura created this revision.Nov 29 2018, 6:13 PM
lauromoura requested review of this revision.Nov 29 2018, 6:13 PM
lauromoura edited the summary of this revision. (Show Details)
lauromoura updated this revision to Diff 17704.Nov 29 2018, 6:24 PM

Update removing dead code from a previous mockup for this patch.

I want this patch very much, but it is failing make check for me:

tests/eolian/eolian_parsing.c:423:F:Eolian Parsing:eolian_complex_type:0: Failure '!(unit = eolian_state_file_parse(eos, TESTS_SRC_DIR"/data/complex_type.eo"))' occurred

I applied the patch on top of the latest master.

segfaultxavi requested changes to this revision.Nov 30 2018, 3:06 AM

Also, since this is adding new API, could you add examples and unit tests?
I don't see the new properties in the generated C# files (I was expecting to find a Content property in the Efl.Ui.Button class, instead of the SetContent() and GetContent() methods).

This revision now requires changes to proceed.Nov 30 2018, 3:06 AM
lauromoura planned changes to this revision.Nov 30 2018, 5:13 AM

Also, since this is adding new API, could you add examples and unit tests?

There are test in Eo.cs, both using Efl.Object.Name and generated properties from Dummy.TestObject.

I don't see the new properties in the generated C# files (I was expecting to find a Content property in the Efl.Ui.Button class, instead of the SetContent() and GetContent() method

Nice catch. Events interfaces aren't being declared. I'll add it.

Another thing: Some properties would lead to invalid names in C#, with the same name as the enclosing class (e.g. Efl.Text.Text). I'm thinking about adding a Prop suffix for these, like:

myButton.PropText = "something";
// The getter version would not be changed
myButton.SetText("Something");

Ouch! I am starting to feel extremely tired of all these name collisions...

I do not like the Efl.Text.PropText because then ALL properties will need the Prop prefix for consistency.
I would rather change the type name (to TextContainer, TextHolder, TextWidget, or whatever).

I know I always ask for the same thing but, before making a decision, can we know how many clashing properties do we have, and how many have no problems? With a little pyolian or lua magic maybe? 😊

lauromoura added a comment.EditedNov 30 2018, 12:16 PM

Ouch! I am starting to feel extremely tired of all these name collisions...

I do not like the Efl.Text.PropText because then ALL properties will need the Prop prefix for consistency.
I would rather change the type name (to TextContainer, TextHolder, TextWidget, or whatever).

Another alternative would be to make - in C# only - interfaces have the I prefix again (Efl.IText), like MS's coding style recomends. To avoid those "exceptions", all interfaces would follow this scheme.

I know I always ask for the same thing but, before making a decision, can we know how many clashing properties do we have, and how many have no problems? With a little pyolian or lua magic maybe? 😊

  • Efl.Canvas.Scene3d: scene3d
  • Efl.Config: config
  • Efl.Content: content
  • Efl.File: file
  • Efl.Gfx.Color_Class: color_class
  • Efl.Gfx.Color: color
  • Efl.Gfx.Fill: fill
  • Efl.Gfx.Path: path
  • Efl.Gfx.Size_Class: size_class
  • Efl.Gfx.Text_Class: text_class
  • Efl.Input.Hold: hold (Not Interface, regular class)
  • Efl.Input.Key: key (Not Interface, regular class)
  • Efl.Io.Closer_Fd: closer_fd
  • Efl.Io.Positioner_Fd: positioner_fd
  • Efl.Io.Reader_Fd: reader_fd
  • Efl.Io.Sizer_Fd: sizer_fd
  • Efl.Io.Writer_Fd: writer_fd
  • Efl.Orientation: orientation
  • Efl.Playable: playable
  • Efl.Text_Cursor: text_cursor
  • Efl.Text: text
  • Efl.Ui.Cursor: cursor
  • Efl.Ui.Direction: direction
  • Eldbus.Model.Method: method

Updated version supporting interface properties.

But still blacklisting Efl.Text.Text and similarly conflicting properties.

(Marked as DONOTMERGE until we sort this out).

Well, just renaming the interfaces wouldn't be enough, as implementing classes like Efl.Ui.Text would still conflict when they implement the interface property.

Only renaming the interfaces won't be enough. As per IRC discussion, these three properties have conflicts: Efl.Ui.Text.Text, Efl.Input.Key.Key and Efl.Input.Hold.Hold. I guess the easiest solution is to just rename either the property or the type (whatever is easier) instead of trying to come up with a clever C# trick. I'm working on a proposal.

Only renaming the interfaces won't be enough. As per IRC discussion, these three properties have conflicts: Efl.Ui.Text.Text, Efl.Input.Key.Key and Efl.Input.Hold.Hold. I guess the easiest solution is to just rename either the property or the type (whatever is easier) instead of trying to come up with a clever C# trick. I'm working on a proposal.

Meanwhile, as the patch just adds new functionality, we could merge it, and once these three properties are dealt with a new commit would remove them from the blacklist and turn the Getters/Setters private.

segfaultxavi accepted this revision.Dec 4 2018, 2:56 AM

Meanwhile, as the patch just adds new functionality, we could merge it, and once these three properties are dealt with a new commit would remove them from the blacklist and turn the Getters/Setters private.

OK, I agree, let's proceed with this. I confirm make, make check and make examples work for me, and the new C# classes have the expected properties available. I cannot review the code, though.

This revision is now accepted and ready to land.Dec 4 2018, 2:56 AM
segfaultxavi requested changes to this revision.Dec 4 2018, 4:26 AM

On a second thought... properties should have documentation :)

This revision now requires changes to proceed.Dec 4 2018, 4:26 AM
lauromoura updated this revision to Diff 17760.Dec 4 2018, 9:58 AM

Update generating documentation for properties.

Also added EINA_UNUSED in some test function implementations.

vitor.sousa accepted this revision.Dec 4 2018, 10:42 AM

Seems fine now and works OK in my computer.
Anyway, I will wait for @segfaultxavi approval before landing this.

segfaultxavi requested changes to this revision.Dec 5 2018, 1:48 AM

Almost done, but not there yet :)
The value of the property is not being documented. You need to use the <value> tag after the <summary> tag:
https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/xmldoc/value

/// <summary>The Name property represents the employee's name.</summary>
/// <value>The Name property gets/sets the value of the string field, _name.</value>
public string Name {
    get { return _name; }
    set { _name = value; }
}

The content of this tag should be the one from the values section in the EO file.

Also, the patch does not apply to latest master.

This revision now requires changes to proceed.Dec 5 2018, 1:48 AM
vitor.sousa edited the summary of this revision. (Show Details)Dec 5 2018, 4:19 AM

Curious, arc is still trying to apply D7389 even after you removed the comment from the summary.

vitor.sousa added a comment.EditedDec 5 2018, 4:33 AM

Ouch! That sucks.
I am using --skip-dependencies for now.
I will look further on removing this no longer needed dependency.

vitor.sousa added a comment.EditedDec 5 2018, 4:42 AM

OK, edited Related Revisions.
Apparently arc is no longer trying to apply D7389.

Another rebase.

Regarding the <value> tags, we discussed on irc of leaving it to a future commit as Xavi is already working
heavily in the documentation.hh header.

vitor.sousa accepted this revision.EditedDec 14 2018, 12:14 PM

Well, since there is an agreement about the <value> tag now, I think I can proceed and land this commit.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 14 2018, 12:31 PM
This revision was automatically updated to reflect the committed changes.