Page MenuHomePhabricator

Efl.Access: Separate event handle function from Efl.Access.Object.
AbandonedPublic

Authored by jsuya on Dec 6 2018, 12:10 AM.

Details

Summary

Efl.Access.Object's event handling function simply handles
the list of callbacks declared as static.
Objects that inherit Efl.Access.Object interface will have unnecessary
and meaningless functions as members.
Improvements to Efl.Access.Object class structure
should be made in the future.

Test Plan

N/A

Diff Detail

Repository
rEFL core/efl
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 8459
Build 7602: arc lint + arc unit
jsuya created this revision.Dec 6 2018, 12:10 AM
jsuya requested review of this revision.Dec 6 2018, 12:10 AM
kimcinoo requested changes to this revision.Dec 6 2018, 12:54 AM
kimcinoo added inline comments.
src/lib/elementary/elm_priv.h
747

IMO we could remove both _efl_access_shutdown and _efl_access_event_shutdown.
Because only _elm_atspi_bridge_shutdown is calling _efl_access_shutdown.
And please reconsider the name 'shutdown' because there is not 'init'. How about _efl_access_event_handler_free?

This revision now requires changes to proceed.Dec 6 2018, 12:54 AM
jsuya updated this revision to Diff 17811.Dec 6 2018, 1:56 AM

Update the code according to @kimcinoo feedback.

jsuya marked an inline comment as done.Dec 6 2018, 1:57 AM
jsuya added inline comments.
src/lib/elementary/elm_priv.h
747

Update code :)

kimcinoo requested changes to this revision.Dec 6 2018, 3:46 AM

I am sorry there would be a small typo.

src/lib/elementary/efl_access_event.eo
11 ↗(On Diff #17811)

This would be not "mixin"

This revision now requires changes to proceed.Dec 6 2018, 3:46 AM
Jaehyun_Cho requested changes to this revision.Dec 6 2018, 3:51 AM

@jsuya

Class name cannot be used as a namespace.

Here, Efl.Access.Event is a class and also it is used as a namespace. (e.g. Efl.Access.Event.State_Changed.Data, Efl.Access.Event.Geometry_Changed.Data, Efl.Access.Event.Children_Changed.Data)

You can see the namespace error if you run autogen.sh with --enable-csharp-bindings.

The solution for this case is replace "." with "_" not to use namespace. (e.g. Efl.Access.Event_State_Changed_Data)
However, when I modified like above, the following errors were displayed and I don't understand why..

lib/elementary/efl_access_object.eo.cs(106,23): error CS0066: `Efl.Access.Object.VisibleDataChangedEvt': event must be of a delegate type
lib/elementary/efl_access_object.eo.cs(110,23): error CS0066: `Efl.Access.Object.AddedEvt': event must be of a delegate type
lib/elementary/efl_access_object.eo.cs(112,23): error CS0066: `Efl.Access.Object.RemovedEvt': event must be of a delegate type

Maybe @lauromoura can help with that error since he has been working on the C# bindings.

jsuya updated this revision to Diff 17831.Dec 7 2018, 1:03 AM
jsuya marked an inline comment as done.

I update code according to @Jaehyun_Choi.
(I tried that changed the class name to Efl.Access.Events and build it. But same error occured.)

The same error occurs as @Jaehyun_Choi mentioned.
I do not think this error is caused by this revision.(Maybe?;;)
We need @lauromoura 's help.

lib/elementary/efl_access_window.eo.cs(12,23): error CS0066: `Efl.Access.Window.WindowCreatedEvt': event must be of a delegate type
lib/elementary/efl_access_window.eo.cs(14,23): error CS0066: `Efl.Access.Window.WindowDestroyedEvt': event must be of a delegate type
lib/elementary/efl_access_window.eo.cs(16,23): error CS0066: `Efl.Access.Window.WindowActivatedEvt': event must be of a delegate type
lib/elementary/efl_access_window.eo.cs(18,23): error CS0066: `Efl.Access.Window.WindowDeactivatedEvt': event must be of a delegate type
lib/elementary/efl_access_window.eo.cs(20,23): error CS0066: `Efl.Access.Window.WindowMaximizedEvt': event must be of a delegate type
lib/elementary/efl_access_window.eo.cs(22,23): error CS0066: `Efl.Access.Window.WindowMinimizedEvt': event must be of a delegate type
lib/elementary/efl_access_window.eo.cs(24,23): error CS0066: `Efl.Access.Window.WindowRestoredEvt': event must be of a delegate type
lib/elementary/efl_access_text.eo.cs(91,23): error CS0066: `Efl.Access.Text.AccessTextCaretMovedEvt': event must be of a delegate type
lib/elementary/efl_access_text.eo.cs(97,23): error CS0066: `Efl.Access.Text.AccessTextSelectionChangedEvt': event must be of a delegate type
lib/elementary/efl_access_selection.eo.cs(41,23): error CS0066: `Efl.Access.Selection.SelectionChangedEvt': event must be of a delegate type
lib/elementary/efl_ui_list_view.eo.cs(458,7): error CS0539: `Efl.Access.Selection.SelectionChangedEvt' in explicit interface declaration is not a member of interface
lib/elementary/efl_ui_list_view.eo.cs(467,7): error CS0539: `Efl.Access.Selection.SelectionChangedEvt' in explicit interface declaration is not a member of interface

jsuya updated this revision to Diff 17877.Dec 9 2018, 8:21 PM

update code

jsuya updated this revision to Diff 17878.Dec 9 2018, 10:30 PM

Change to class name for build error.

Jaehyun_Cho added inline comments.Dec 16 2018, 9:15 PM
src/lib/elementary/efl_access_event_handler.eo
4 ↗(On Diff #17878)

please change handler to Handler.

jsuya updated this revision to Diff 17934.Dec 16 2018, 10:22 PM

update code

jsuya marked an inline comment as done.Dec 16 2018, 10:23 PM

@jsuya

This new class name is Efl.Access_Event_Handler.

It looks fine by itself. But other Efl.Accss class names and interface names are not synchronized with this.
(e.g. Efl.Access.XXX Access is used as a namespace)

If we use Access as a namespace, then we need to change the class name Efl.Access_Event_Handler.
(But we cannot use Efl.Access.Event_Handler because Event_Handler is already reserved word for event handler.)

I just have a general questions why it is designed like this? eo comes with the code to work with events, the class Efl.Access_Event_Handler seems to to do just that again.

You could have there a singleton pattern with one event on the class which is something like ACCESS,EVENT. which carries another event type as event-data, with this you would not need to have this list maintained by hand, and you get callback generations and other things that eo provides for free :)

jsuya added a comment.Dec 17 2018, 4:28 PM

@bu5hm4n Thank you for your advice.

The purpose of this commit is to separate the code that is not related to class instantiation from Efl.access.object while maintaining usability.

As you say, we need to improve the efl.access.* structure.
Please let me know if you have a case I can refer to.

jsuya updated this revision to Diff 17958.Dec 17 2018, 4:28 PM

I changed class name(Efl.Access.Event_Handler->Event_Manager)
because the class name generated by efl.access.* conflicts with
the existing system, eo(base?) class name.

jsuya abandoned this revision.Jan 2 2019, 10:52 PM