Page MenuHomePhabricator

Remove internally generated classes from EFL# doc
Open, Incoming QueuePublic

Description

Now EFL# doc is generated by the followings.

cd doc/docfx/
./setup.sh
./gendoc.sh

Internally generated classes exist in EFL# doc.
(e.g. Class ImageProgress_StructConversion, Class BoxNativeInherit, etc.)

I think that these internally generated classes should be removed from EFL# doc.

UPDATE:
After the DotNet Core support, there still remain the following issues:

  • *_StructConversion classes
  • *Concrete classes.
  • *NativeInherit classes.
  • On_*Evt methods: Do they need to be public?
  • register_event_proxies method
  • static_cast method
  • klass field
  • nativeInherit field
  • NativeClass property
segfaultxavi added a subscriber: lauromoura.

Yes, there are a lot of these. @lauromoura and @felipealmeida were already looking at making these classes private or internal so they don't show up in the docs, right?

segfaultxavi updated the task description. (Show Details)Mar 4 2019, 1:24 AM

@felipealmeida @lauromoura I updated the task description with a list of issues. Can you please comment on each of them so we can decide if they can be made private or internal, or I need to blacklist them in the doc generation?

segfaultxavi updated the task description. (Show Details)Mar 4 2019, 1:43 AM
segfaultxavi moved this task from TODO to InProgress on the efl: language bindings board.
  • _StructConversion classes: Do they need to be public?

No, they can be hidden as private classes inside the structs. Also, the *_StructInternal could be changed to a nested struct .NativeRepr (or another name), avoiding the name to appear twice when typing something like new MyStruct. I'll create a task for it.

  • Concrete classes.

These are required in the public API as a way to instantiate wrappers when returning interfaces.

What we could do is (for all classes) turning the public constructor receiving an IntPtr private and adding a static IFace WrapNative(IntPtr). This method returns the public iface/class wrapping the pointer. That way, the API user wouldn't be able to declare a FooConcrete variable and assign to it (at least not directly). Also another task.

  • NativeInherit classes.

Right now I can think of two approaches:

  1. These should be moved into a Efl.Internal.Namespace to be away from their regular API counterparts.
  2. They become a public nested NativeInherit class in their public API class (similar to the NativeRepr approach for structs above).

Still need to investigate more about which one would be better.

  • On_*Evt methods: Do they need to be public?

It seems to be a common pattern in C# to have protected void OnSomeEvent(EventArgs e) methods to allow subclasses to raise events from the parent. For this item, we can change them to protected and remove the _ from the method name.

  • register_event_proxies method: Does it need to be protected?

Yes, so the subclass implementation can register the parent events too. Should we change to RegisterEventProxies() to conform to C# conventions?

  • static_cast method: Does it need to be public?

It is used to mimic a "C" cast and convert from a more generic type. Maybe we could replace with an implicit operator from Efl.Object in each class, which would call the WrapNative method. Will check.

  • klass field: Does it need to be public?

This is actually not used and should be removed.

  • nativeInherit field: Does it need to be public?

No. It can be removed as it was replaced by declaring the NativeInherit class through an C# attribute.

  • NativeClass property: Does it need to be public?

Yes. Like the NativeHandle counterpart, it is used in many places and also may be overridden by child classes.

  • _StructConversion classes: Do they need to be public?

    I'll create a task for it.

Thanks!

  • Concrete classes.

    Also another task.

Thanks!

  • NativeInherit classes.

    Still need to investigate more about which one would be better.

OK

  • On_*Evt methods: Do they need to be public?

    For this item, we can change them to protected and remove the _ from the method name.

Please do!

  • register_event_proxies method: Does it need to be protected?

    Should we change to RegisterEventProxies() to conform to C# conventions?

Of course! 😁 And needs docs too!

  • static_cast method: Does it need to be public?

    Maybe we could replace with an implicit operator from Efl.Object in each class, which would call the WrapNative method. Will check.

Yes please. Anything to show a nice, clean API to the user!

  • klass field: Does it need to be public?

    This is actually not used and should be removed.

Awaiting patch then...

  • nativeInherit field: Does it need to be public?

    No. It can be removed as it was replaced by declaring the NativeInherit class through an C# attribute.

Awaiting patch then...

  • NativeClass property: Does it need to be public?

    Yes. Like the NativeHandle counterpart, it is used in many places and also may be overridden by child classes.

Then their docs need to say something like For internal usage only..

segfaultxavi updated the task description. (Show Details)Mar 17 2019, 8:07 AM
segfaultxavi updated the task description. (Show Details)Mar 17 2019, 8:12 AM
  • _StructConversion classes: Do they need to be public?

    No, they can be hidden as private classes inside the structs. Also, the *_StructInternal could be changed to a nested struct .NativeRepr (or another name), avoiding the name to appear twice when typing something like new MyStruct. I'll create a task for it.
  • Concrete classes. These are required in the public API as a way to instantiate wrappers when returning interfaces.

    What we could do is (for all classes) turning the public constructor receiving an IntPtr private and adding a static IFace WrapNative(IntPtr). This method returns the public iface/class wrapping the pointer. That way, the API user wouldn't be able to declare a FooConcrete variable and assign to it (at least not directly). Also another task.
  • Still visible, but D8550 will make their constructor private.
  • NativeInherit classes.

    Right now I can think of two approaches:
    1. These should be moved into a Efl.Internal.Namespace to be away from their regular API counterparts.
    2. They become a public nested NativeInherit class in their public API class (similar to the NativeRepr approach for structs above).

      Still need to investigate more about which one would be better.
  • Finishing a patch that will hide them as a NativeMethods nested class inside the main API class (e.g. Efl.Ui.Button.NativeMethods). They can't be protected as concrete interface implementations may call methods from multiple native methods classes (for each interface they implement).
  • On_*Evt methods: Do they need to be public?

    It seems to be a common pattern in C# to have protected void OnSomeEvent(EventArgs e) methods to allow subclasses to raise events from the parent. For this item, we can change them to protected and remove the _ from the method name.

These will be updated to remove the underscore alongiside an event rework from T7524.

  • register_event_proxies method: Does it need to be protected?

    Yes, so the subclass implementation can register the parent events too. Should we change to RegisterEventProxies() to conform to C# conventions?

It will be renamed after T7524.

  • static_cast method: Does it need to be public?

    It is used to mimic a "C" cast and convert from a more generic type. Maybe we could replace with an implicit operator from Efl.Object in each class, which would call the WrapNative method. Will check.

D8550 will remove it in place of regular C# casts.

  • klass field: Does it need to be public?

    This is actually not used and should be removed.

D8368

  • nativeInherit field: Does it need to be public?

    No. It can be removed as it was replaced by declaring the NativeInherit class through an C# attribute.

D8368

  • NativeClass property: Does it need to be public?

    Yes. Like the NativeHandle counterpart, it is used in many places and also may be overridden by child classes.

Yes.

segfaultxavi updated the task description. (Show Details)Apr 4 2019, 10:15 AM

Awesome, thanks, I updated the list in the task description. This task is almost done!