Page MenuHomePhabricator

Revert "csharp: Property Indexer implementation"
ClosedPublic

Authored by YOhoho on Jan 16 2020, 6:31 PM.

Details

Summary

This reverts commit 0954e501fd4008c40b3848de1f2c91bcd53b2f71.

According to Framework Design Guidelines of MS, most of indexed properties are
not recommended in EFL#.
(see, https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/property)

It is better to leave properties which have a key as methods.

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
YOhoho created this revision.Jan 16 2020, 6:31 PM
YOhoho requested review of this revision.Jan 16 2020, 6:31 PM

After reading that guideline, I see that indexers are only discouraged in some cases.
Index through a tuple to fit multiple keys indeed looks very convoluted and should be avoided.
However, I think indexers are still useful in simple cases (like when there is only one key, and it's a string). Can we consider keeping them for these cases?

I thought @woohyun preferred properties over methods because of XAML or something like this?

@segfaultxavi For strings and integers that could make sense. What do you think @YOhoho @woohyun ?

@segfaultxavi @felipealmeida

Thanks for sharing the idea ~

C# reviewers had indicated two things.

  1. Property with indexer looks not good.
  2. They cannot know why XXXIndexer classes (such as FilterDataIndexer) should exist.

For supporting property with indexer, we need to create Indexer classes for each property.
And I don't think it's not that beneficial for later management.

So, I hope to keep them as methods instead of properties.

How do you think about this idea ?

@woohyun What happens then with XAML? Can it work with methods?

@segfaultxavi

@YOhoho listed up "the indexer only with one key" cases. (Please refer to below properties)
And those seem ok without XAML support.
Most of XAML supports are related with Efl.Ui.XXX classes for layouting with widget classes. And below look a bit far from that purpose.

....
Efl.Layout.IGroup.GroupData { get; } key : string
Efl.Layout.IGroup.PartExist { get; } key : string
Efl.Canvas.GestureRecognizer.Config { get; } key : string
Efl.Canvas.IScene.Device { get; } key : string
Efl.Canvas.IScene.Seat { get; } key : int
Efl.Gfx.IColorClass.ColorClassDescription { get; } key : string
Efl.Gfx.IFilter.FilterData { get; } key : string
Efl.Gfx.IFilter.FilterSource { get; } key : string
Efl.Canvas.GestureManager.Config { get; set; } key : string
Efl.Gfx.IMapping.MappingCoordAbsolute { get; set; } key : int
Efl.Gfx.IMapping.MappingUv { get; set; } key : int
Efl.Gfx.IMapping.MappingColor { get; set; } key : int
Efl.Input.Pointer.ButtonPressed { get; set; } key : int
Efl.IConfig.config { get; set; } key : string
...

OK! Then I'm OK with using methods instead of properties when there are keys involved.

woohyun accepted this revision.Jan 20 2020, 2:00 PM

@felipealmeida @segfaultxavi

Ok. Then, let's go with this patch :)

This revision is now accepted and ready to land.Jan 20 2020, 2:00 PM
Closed by commit rEFL292f4bc0da8b: Revert "csharp: Property Indexer implementation" (authored by Yeongjong Lee <yj34.lee@samsung.com>, committed by woohyun). · Explain WhyJan 20 2020, 2:26 PM
This revision was automatically updated to reflect the committed changes.