Page MenuHomePhabricator

C#: Mark protected methods as protected
Open, HighPublic

Description

Currently, protected methods are being generated as public.

lauromoura triaged this task as TODO priority.
woohyun added a subscriber: woohyun.Jan 8 2019, 9:43 PM

@lauromoura

I know you are very busy, but -
please care about this problem together :)

lauromoura raised the priority of this task from TODO to High.Mar 15 2019, 7:38 AM
YOhoho added a subscriber: YOhoho.Jun 19 2019, 1:23 AM

I tried to write patch to support eo access modifiers(like @protected) on EFL#. however there is a problem.
Method of C# interface have to be public.

Interface members are automatically public, and they can't include any access modifiers. Members also can't be static. (https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/interfaces/index)

but there is protected method of Eo interface. e.g. hint_size_restricted_min_set. you will get CS0106 Compiler error.(https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/compiler-messages/cs0106)

I have no idea how to solve this problem.

I think this is (again) an example of something that EO supports and one of the binding languages does not.

I see two options:

  1. Make the C# generator ignore @protected tags in interfaces.
  2. Modify Eolian so that it does not accept @protected tags in interfaces, and then we remove that tag from hint_size_restricted_min_set.

I agree with Microsoft here, I don't think protected methods on an interface make much sense. Therefore I prefer option 2.
However, if this is too much work now, we can create a task to do that later and proceed with option 1 for now.

What do you think @q66?

YOhoho added a comment.EditedJun 19 2019, 2:20 AM

Note that current state is option 1. eolian_mono ignore @protected tags and make interface method public. Ah, you mean it is only interface case.
I also prefer option 2. end user shouldn't use @protected methods directly.

Below is a list of places (left col) where the documentation refers to @protected methods from interfaces (right col).

These @protected methods would not be generated leading to cref misses when generating C# docs.

Reference locationprotected interface method
Efl.IPackLayout.LayoutRequest()Efl.IPackLayout.UpdateLayout
Efl.IPackLayout.LayoutRequest()Efl.IPackLayout.UpdateLayout
Efl.IPackLayoutConcrete.LayoutRequest()Efl.IPackLayout.UpdateLayout
Efl.IPackLayoutConcrete.LayoutRequest()Efl.IPackLayout.UpdateLayout
Efl.Ui.Box.LayoutRequest()Efl.IPackLayout.UpdateLayout
Efl.Ui.Box.LayoutRequest()Efl.IPackLayout.UpdateLayout
Efl.Ui.Calendar.ApplyFormattedValue()Efl.Ui.IFormat.GetFormattedValue
Efl.Ui.Calendar.GetFormattedValue(Eina.Strbuf, Eina.Value)Efl.Ui.IFormat.GetFormattedValue
Efl.Ui.Collection.LayoutRequest()Efl.IPackLayout.UpdateLayout
Efl.Ui.Collection.LayoutRequest()Efl.IPackLayout.UpdateLayout
Efl.Ui.IFormatConcrete.ApplyFormattedValue()Efl.Ui.IFormat.GetFormattedValue
Efl.Ui.IFormatConcrete.GetFormattedValue(Eina.Strbuf, Eina.Value)Efl.Ui.IFormat.GetFormattedValue
Efl.Ui.IL10nConcrete.UpdateTranslation()Efl.Ui.IL10n.UpdateTranslation
Efl.Ui.LayoutPartLegacy.UpdateTranslation()Efl.Ui.IL10n.UpdateTranslation
Efl.Ui.LayoutPartText.UpdateTranslation()Efl.Ui.IL10n.UpdateTranslation
Efl.Ui.Progressbar.ApplyFormattedValue()Efl.Ui.IFormat.GetFormattedValue
Efl.Ui.Progressbar.GetFormattedValue(Eina.Strbuf, Eina.Value)Efl.Ui.IFormat.GetFormattedValue
Efl.Ui.RelativeLayout.LayoutRequest()Efl.IPackLayout.UpdateLayout
Efl.Ui.RelativeLayout.LayoutRequest()Efl.IPackLayout.UpdateLayout
Efl.Ui.Spin.ApplyFormattedValue()Efl.Ui.IFormat.GetFormattedValue
Efl.Ui.Spin.GetFormattedValue(Eina.Strbuf, Eina.Value)Efl.Ui.IFormat.GetFormattedValue
Efl.Ui.Table.LayoutRequest()Efl.IPackLayout.Update'Layout
Efl.Ui.Table.LayoutRequest()Efl.IPackLayout.UpdateLayout
Efl.Ui.Tags.ApplyFormattedValue()Efl.Ui.IFormat.GetFormattedValue
Efl.Ui.Tags.GetFormattedValue(Eina.Strbuf, Eina.Value)Efl.Ui.IFormat.GetFormattedValue
Efl.Ui.ViewFactory.CreateWithEvent(Efl.Ui.IFactory, Eina.Iterator<Efl.IModel>, Efl.Gfx.IEntity)Efl.Ui.IFactory.Create
Efl.Ui.ViewFactoryEfl.Ui.IFactory.Create
Efl.Ui.Widget.UpdateTranslation()Efl.Ui.IL10n.UpdateTranslation
Efl.Ui.WidgetPartEfl.IPart.GetPart

Btw, I'm also in favor of option 2 (removing @protected stuff from interfaces).

Hmmm.... @lauromoura, Efl.Ui.Format is a mixin, and its protected method formatted_value_get is only referenced from other protected methods. I see no problem here.

I understand the C# implementation of mixins involves creating interfaces. If we forbid protected methods from interfaces, what will happen to mixins in C#?

I've fixed the Efl.IPackLayout.UpdateLayout reference. This was really a public method referencing a protected method.

Efl.IPart.GetPart is a protected method in an EO interface, which we said we should forbid.

The rest of problems (Efl.Ui.IFormat.GetFormattedValue and Efl.Ui.IL10n.UpdateTranslation) only exist because C# mixins use interfaces, and should be fixed from the C# generator.

lauromoura added a subscriber: felipealmeida.

I've pushed the tentative fix to the devs/lauromoura/csharp-interface-protected branch.

The first commit turns Efl.Part.part_get public as we use it in C# MVVM bindings code, so we need to access it publicly through Efl.IPart. Without this commit the next one below would break C# build. If it is not possible to make it public, we could try something not so straightforward and keep it protected.

The second one actually removes non-public methods from generated C# interfaces (aka Eo Mixins and Interfaces). They are generated only for generated classes that would implement these interfaces. The test case adds a protected method to an interface and tests both acessing it directly and through C.

What do you think @segfaultxavi and @YOhoho?

I think the approach is OK.
It makes protected eo methods also protected in C# (which will remove them from the docs), and the only downside is that these methods also disappear from C# interfaces, so user-created C# classes won't be able to fully implement eo interfaces.
I think this downside is acceptable, since the most common use case would be for users to inherit from an EFL# class (which already includes all eo mixins and interfaces), not to create new classes from scratch implementing eo interfaces and including eo mixins.
Am I correct?