Page MenuHomePhabricator

efl_canvas_text: rename to efl_canvas_text_canvas
AbandonedPublic

Authored by YOhoho on Aug 14 2019, 4:42 AM.

Details

Summary

Efl.Canvas.Text -> Efl.Canvas.TextCanvas in C#.
This patch will fix name conflict with Efl.Text.Text proeprty.
Legacy c api(evas_object_textblock) is not changed.

ref T8093
Depends on D9565

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
YOhoho created this revision.Aug 14 2019, 4:42 AM
YOhoho requested review of this revision.Aug 14 2019, 4:42 AM

Feel free to suggest new class name!

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) ?

cedric requested changes to this revision.Aug 14 2019, 1:29 PM
cedric added subscribers: lauromoura, felipealmeida.

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 ?

This revision now requires changes to proceed.Aug 14 2019, 1:29 PM

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.

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.

raster added a subscriber: raster.Aug 15 2019, 8:53 AM

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.

YOhoho abandoned this revision.Aug 18 2019, 1:01 PM

We can rename classes in eolian_mono to solve limitation of C# syntax.

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?

Explicit implemented propety can't have intellisense support. I don't think explicit type casting is a convenient way to use EFL# API.

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

Won't it look weird that some properties have the Prop suffix and some others don't?

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.

In T8093#140311, @tasn wrote:

@woohyun, I'm going to reshuffle things in the text space so I wouldn't worry about making a decision/change yet. I'll take a look at the WPF naming scheme when changing though.

@tasn have a plan to reshuffle text space. I think we can discuss after text class refactoring if there is still name conflict.

In T8093#140311, @tasn wrote:

@woohyun, I'm going to reshuffle things in the text space so I wouldn't worry about making a decision/change yet. I'll take a look at the WPF naming scheme when changing though.

@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.

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.

tasn added a comment.Sep 2 2019, 8:59 AM

Not sure I follow the issue here, but take a look at T8151 (as @YOhoho suggested), and if the issue is still there with the Efl2.* classes, please let me know and we can try to find a solution.

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.

I have no objection of Prop suffix.

tasn added a comment.Sep 3 2019, 12:55 AM

@segfaultxavi, yup, if it's a C# problem, it should be fixed in the generator.