Details
- Reviewers
tasn cedric zmike bu5hm4n segfaultxavi woohyun ali.alzyod - Maniphest Tasks
- T8093: Name conflict between class and property
Diff Detail
- Repository
- rEFL core/efl
- Branch
- master
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 12683 Build 9112: arc lint + arc unit
Will we rename all canvas objects inside efl.Canvas namespace to have canvas word at the end like (text_canvas, image_canvas, rectangle_canvas, ... etc) ?
That sounds bad. I am bad myself at naming things, so I can't really suggest a better name. I have my hope on @segfaultxavi .
By the way, this is in its own namespace (Efl.Canvas) so why does it clash with another namespece (Efl.Text)? @lauromoura / @felipealmeida ?
They clash because c# will create a property named Text inside a class named Text, which is forbidden.
I also think Efl.Canvas.Text_Canvas is confusing, and inconsistent.
So this is the text object at the canvas level, right? Only rendered text, no editing facilities, no widgets.
Let's brainstorm... How about TextBlock, TextLabel, Label, TextOutput, StaticText, RenderedText, PrintedText... does any of these sound right?
It seems to me that this is quite a exotic limitation that only happens in csharp, not in java, cpp, etc.. what do you think of something like "csharp_prefix" that can be used to adjust the class name of the csharp classes?
Hmmmm... not that exotic, in those languages, a method named like the class is reserved for the constructor of the class, isn't it?
... constructors are most of the time without return type. (As that would be rather redundant.
I'll leave the technical details to the c# people, but I think it is a bit late in the day to change the c# bindings syntax now. Not impossible, though.
All name clashes were already solved a long time ago by renaming properties named like classes. Only these three remained (Key, Hold and Text) because they were in a blacklist in the c# generator and I forgot about them.
Yeah, this is a limitation specific of C# syntax. https://docs.microsoft.com/en-us/dotnet/csharp/misc/cs0542
One alternative would be implementing them as explicit interface implementations, like this (generated code):
class Efl.Canvas.Text : Efl.IText { public string Efl.IText { <GET/SET> } }
The drawback would be having to cast Efl.Canvas.Text to Efl.IText to get/set the property.
just as a slightly more generic point... this is what eolian_gen was meant to be for. it's job would be to appropriately MAP the efl api to the native language. For example perhaps camelcase functions if that's what that language ecosystem prefers. it's job should also be to specifically deal with these issues rather than them leak into our .eo files themselves. rename classes or types as appropriate in the tool... then it also enforces a consistent naming for such situations too which is too.
is there any reason this cannot be automated in eolian?
Explicit implemented propety can't have intellisense support. I don't think explicit type casting is a convenient way to use EFL# API.
As @raster points out, it is the job of eolian_gen to take care of any language-specific quirks. For example, for C# we could add a C prefix to all classes just like we add I to interfaces (and Evt to events).
We initially played with upper-case for classes and lower-case for properties, and things like this, but it wasn't natural for C# (and impossible for Python on Windows).
At some point we started renaming classes and properties instead of letting each language eolian_gen handle language-specific problems (and I think T6847 was the initial culprit).
Anyway, at this point, we only have one remaining clash, and that is Efl.Text.Text and some other Text properties. We can solve the clash by renaming the eolian class, or by changing eolian_mono.
If we decide to change eolian_mono, I don't like the explicit interface cast solution (it is cumbersome and lacks intellisense). I think we should move to prefixing classes with C, just like we do for interfaces and events. In hindsight, it seems obvious :)
@lauromoura Did I miss anything? Was there at the time any strong reason for renaming the eolian classes that I do not remember now?
What if we mix explicit interface implementation (so we keep the interface "pure") and add a wrapper property in classes we have this conflict so intellisense can still pick it up?
Something like this:
public interface IText { string Text { get; set; } } public class Text : IText { // Explicit implementation string IText.Text { get; set; } // Wrapper prop public string TextProp { get { return ((IText)this).Text; } set { ((IText)this).Text = value; } } } /// Usage var text = new Efl.Ui.Text(); text.TextProp = "Abc";
Won't it look weird that some properties have the Prop suffix and some others don't?
In this case, I prefer adding Prop to all properties and problem solved :D
Only properties that would have the same name as the current class.
In this case, I prefer adding Prop to all properties and problem solved :D
This must be something so minor (given the Text class rename) that I think adding prop just here and there and documenting it (helped by intellisense) outweighs adding it everywhere.
These properties also may have a disclaimer This property is a wrapper around Efl.IText.Text. It was added as a helper due to having the same name as this class.
@tasn have a plan to reshuffle text space. I think we can discuss after text class refactoring if there is still name conflict.
Even if we change the Text property this is a potential problem that would require manual intervention in the future, either by renaming the eo property or by adding it to the blacklist.
Yeah, this will come back to bite our backs for sure.
OK, then I'm fine with adding Prop to all properties named like their class. There shouldn't be many (if any) after all the renaming work, but this will save us in the future.
From the C# developer POV, these properties always had this suffix, they'll never know the EO property didn't have the suffix.
For this reason, I would not add any comment saying "This property is a wrapper around..." as this won't help the C# developers in any way.
No big issue. Just that some languages like C# do not allow a property to be called like their class (like Efl.Text.text). But as @raster already pointed out, this is a C# problem and will be fixed in the C# bindings generator.
While we come up with the best fix, though, it helps if we can avoid creating such properties in the eo classes.