Page MenuHomePhabricator

csharp: add a interface to start apps
ClosedPublic

Authored by bu5hm4n on Dec 20 2018, 9:33 AM.

Details

Summary

until to today you had to call init functions and a run function which
were static function in a class called Efl.Ui.Config.

However, calling those init functions there is not really OOP style.
Right now things have changed into a manner where you are defining you
application class with inheriting from the Application /
SimpleApplication abstract.

This enables you to call launch() on your application class, calling
launch there leads to a call to the args function, you can call and use
the Efl classes in there, everything is booted up.
Option parsing and dependency start can still be done in the main method
or application constructor, just ensure that you never call any efl
class / function outside the launch function.

A commit that demonstrates the usage can be found at

ref T7204

https://git.enlightenment.org/tools/examples.git/log/?h=devs/bu5hm4n/POC

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.
There are a very large number of changes, so older changes are hidden. Show Older Changes
bu5hm4n requested review of this revision.Dec 20 2018, 9:33 AM

However, note that there is a bug right now which is worked arround by commit:
https://git.enlightenment.org/core/efl.git/commit/?h=devs/bu5hm4n/POC&id=ac01b9c721a8be5d793e35966d6c67d9a3ff2b14
Until the correct fix for the above is added this added interface will not work as expected.

lauromoura added inline comments.
src/bindings/mono/efl_mono/efl_csharp_application.cs
82

Maybe add a check for to avoid running elm_run without elm_init if not using UI?

But in this case (having a NOP run), how would the control flow back to the app, starting the main loop? Calling Efl.App.MainLoop.Begin()?

87

Shouldn't these methods be Uppercase, to follow C# guidelines?

cedric requested changes to this revision.Dec 20 2018, 12:53 PM

Additional question when do you choose to go with upper case and not in C#? Is there a rules for this?

src/bindings/mono/efl_mono/efl_csharp_application.cs
61

Where are the command line arguments being propagated to ? Also you might want to call ecore_init_ex(argc, argv); somewhere.

82

I think it would be better to follow what EFL_MAIN_EX macro does and call Begin directly indeed. Also Begin does return the exit code as an Eina_Value, maybe it would be good to propagate that back out of Run() and launch().

This revision now requires changes to proceed.Dec 20 2018, 12:53 PM
bu5hm4n added inline comments.Dec 20 2018, 1:10 PM
src/bindings/mono/efl_mono/efl_csharp_application.cs
61

This is just copied from efl_all.cs. I completly agree with you, further more, this probebly should use efl_ api instead of the current stable API.

82

@lauromoura Well this is just copied from the efl_all.cs file, the problem is not new, but rather inherited from the old solution... :)

87

yep :) will fix

bu5hm4n updated this revision to Diff 18043.Dec 20 2018, 1:45 PM

review updates

bu5hm4n updated this revision to Diff 18094.Dec 25 2018, 2:02 AM

updating to review comments

bu5hm4n added inline comments.Dec 25 2018, 2:18 AM
src/bindings/mono/efl_mono/efl_csharp_application.cs
82

@cedric not possible that we return the Eina.Value from the launch method, eina is not initializatied there anymore, which means, calls to eina there are doing to be a problem, or we just return some sort of container that either contains a string or a int, which requires no eina interaction.

akanad added a subscriber: akanad.Dec 26 2018, 2:57 AM
akanad added inline comments.Dec 26 2018, 8:07 PM
src/bindings/mono/efl_mono/efl_csharp_application.cs
89

I guess that "protected" seems more proper for the visibility of methods for manipulating an application such as resume, terminate, pause. because it's a kind of callback methods.

and also renaming like as 'onResume' looks necessary to take place due to same reason of the above opinion.

98

launch method calls basically static methods internally. so that public int static launch(...) look more proper.

120

could I know how user use this class, I mean in point of caller's view.

akanad added inline comments.Dec 27 2018, 2:29 AM
src/bindings/mono/efl_mono/efl_csharp_application.cs
89

I guess that "protected" seems more proper for the visibility of methods for manipulating an application such as resume, terminate, pause. because it's a kind of callback methods.

I mean that the methods seems like manipulating an application, but are actually callback methods.

I don't have a strong opinion on either onPause etc. or Pause. However, we have nowhere else the convention of "on" beside a few APIs in elm. In efl_io things are also not named with on in the beginning, (like seek in efl_io_positioner, or update in efl_observer.)

src/bindings/mono/efl_mono/efl_csharp_application.cs
89

Yeah i will make them protected :)

98

the launch method cannot be static, as it calls the none static methods Args, Pause etc. etc..
Init and Shutdown are only static because it does not depend on any Interna of the implementing class, and thus can be perfectly inlined by the compiler without growing the vtables :).

120
bu5hm4n updated this revision to Diff 18223.Jan 4 2019, 4:02 AM
bu5hm4n edited the summary of this revision. (Show Details)

add arguments passed to the application

This patch adds something that I have been wanting for a long time, i.e., something similar to the EFL_MAIN macro in C, which allows getting rid of the horrible static Efl.Ui.Config.Init() methods.
Some comments:

  • I don't think the Pause, Resume and Terminate callbacks are very useful, since it is extremely easy for the user to just register to those events in the Efl.App.AppMain object. Furthermore, we use events everywhere else in the EFL API: using abstract methods which you need to override is a new paradigm which will be confusing for the user. I would just make Efl.App.AppMain available as a parameter to the Args method (for convenience) and let the user register to the PauseEvt, ResumeEvt and TerminateEvt if he wants to.
  • If some of this code has been copied from efl_all.cs, shouldn't that code be removed from that file in this same patch?
  • Missing autotools support :)
  • Thanks for providing two examples, I will use them to update all other examples in the repo.
src/bindings/mono/efl_mono/efl_csharp_application.cs
28

No need to write Note: if you are inside a <remarks> tag.

44

All these *.Config.Init() methods will then be hidden from the user, right? I don't want them to show up in the docs!

61

Can you do all the fixes you propose in this same patch? In this way we will wrap up all this task in a single patch and forget about it.

79

Correction: "Called when the application is started. Arguments from the command line are passed here."

81

I don't like that the main application method is called Args. Why not EflMain or just Main?

bu5hm4n marked 2 inline comments as done.Jan 7 2019, 8:47 AM
bu5hm4n added inline comments.
src/bindings/mono/efl_mono/efl_csharp_application.cs
44

This can be done - but that is a bit more work, which would IMO just polute this revision, i am going to tackle this in a seperated task. Ok ?

61

elm_policy_set is not having a efl namespaced candidate, which "forces" us to call the elm_ function.

segfaultxavi added inline comments.Jan 7 2019, 8:49 AM
src/bindings/mono/efl_mono/efl_csharp_application.cs
44

Perfectly OK.

bu5hm4n updated this revision to Diff 18256.Jan 7 2019, 8:53 AM

update with the review comments.

bu5hm4n added a subscriber: zmike.Jan 10 2019, 1:51 AM

It seems that this almost fixed all review commits, the only thing left is, if Pause Resume Termiante should be functions in the abstract, or the users should rather subscribe to the event themself.
@felipealmeida @Jaehyun_Cho @akanad @woohyun @cedric @zmike

I still don't like that the entry point is called Args. I prefer Main or EflMain.

Also, the callbacks should be definitely be called OnPause, OnResume and OnTerminate. If we never used this notation in other places of EFL is because we used Events instead of overridable methods :)

bu5hm4n added a comment.EditedJan 10 2019, 8:48 AM

I still don't like that the entry point is called Args. I prefer Main or EflMain.

Also, the callbacks should be definitely be called OnPause, OnResume and OnTerminate. If we never used this notation in other places of EFL is because we used Events instead of overridable methods :)

It is not called Main, because it can be called mutliple times, when for examples new parameters are passed to the application, main is usally called once, and once it exits the application terminates, not the case here. What do you think of Startup EntryPoint onNewArgs onStart Fireup ?

The similarities with EFL_MAIN then are really thin... how can you pass new arguments to your program?

What is the name used in Tizen for this event? Maybe use that.

On IRC @bu5hm4n explained that EFL is able to receive new command line arguments on the fly (seriously, dudes?), so an Args event is needed.
However, that is uncommon in other frameworks, where commandline args are received by the main entry point, so we agreed on making the initial commandline args available to the entry point too.
Also, in C efl_pause, efl_resume and efl_terminate are methods which your app implements, so I guess the C# counterpart should follow the same mechanism.
So the current proposal is:

  • protected OnInitialize(args)
  • protected OnPause()
  • protected OnResume()
  • protected OnTerminate()
  • protected OnArgs(args)

The Application class will provide default NOP implementations for all method except OnInitialize, so apps can override whichever methods they desire, and then there is no need for the SimpleApplication class.

bu5hm4n updated this revision to Diff 18359.Jan 11 2019, 2:59 AM
bu5hm4n edited the summary of this revision. (Show Details)

update to latest review

https://git.enlightenment.org/tools/examples.git/log/?h=devs/bu5hm4n/POC

^ This is an example of how it looks on the user side

segfaultxavi requested changes to this revision.Jan 14 2019, 4:45 AM

Some minor documentation nitpicks. Also, the override methods are not protected yet.

src/bindings/mono/efl_mono/efl_csharp_application.cs
90

*Called when the application is not going to be displayed, or is not used by a user for some time.

94

*Called before an application is used again after a call to OnPause().

103

*This call will result in a call to OnInitialize(), which you application should override.

This revision now requires changes to proceed.Jan 14 2019, 4:45 AM
bu5hm4n updated this revision to Diff 18403.Jan 14 2019, 10:09 AM
bu5hm4n edited the summary of this revision. (Show Details)

review comments

bu5hm4n updated this revision to Diff 18405.Jan 14 2019, 11:05 AM

adjust code sample

segfaultxavi accepted this revision.Jan 15 2019, 1:59 AM
segfaultxavi added inline comments.
src/bindings/mono/efl_mono/efl_csharp_application.cs
103

*your

MY MISTAKE! SORRY! DON'T HATE ME!

This revision was not accepted when it landed; it landed in state Needs Review.Jan 16 2019, 3:49 AM
Closed by commit rEFL6a7c273ec76f: csharp: add a interface to start apps (authored by Marcel Hollerbach <mail@marcel-hollerbach.de>, committed by segfaultxavi). · Explain Why
This revision was automatically updated to reflect the committed changes.

@bu5hm4n

What do you think about removing the namespace "Csharp"? (if so the file name is also changed to efl_application.cs)

Because this is already .cs file so it seems that it is not necessary to use the namespace "Csharp".

Please let me know if I miss or misunderstand here~

@Jaehyun_Cho the reason i have added this under the csharp namespace is that it *cannot* be confused with autogenerated EFL applications, and we prevent later collisions in case we want to name a class efl.application.

(To see what i mean, right now there is a efl.ui.config class in csharp which is not the autogenerated one, which means, we cannot add such a class to efl)

@bu5hm4n
I understand your point. Thank you :)
When I have some new brilliant ideas about the namespace Csharp, then I will share with you.

@bu5hm4n @segfaultxavi

I made a patch D8463 which moves the enum Efl.Csharp.Components out of the namespace Csharp.
(i.e. Efl.Csharp.Components -> Efl.Components)

I think there is no need to put the enum Components under the namespace Csharp.
Because the enum Components is mainly used as a parameter of Efl.All.Init().

Since @segfaultxavi requested to discuss about D8463 on this thread, I left the above here :)

As far i understood the situation, Efl.All is going to be internal, and this interface is the thing that should be used. So all in all the components declaration can stay here.

As far i understood the situation, Efl.All is going to be internal, and this interface is the thing that should be used. So all in all the components declaration can stay here.

Indeed.

Initially we had this Efl.All.Init/Shutdown scheme to explicitly initialize and shut down EFL. With the Application class, we changed to from this old behavior in order to better respond to the Arguments events, abstract the app initialization etc.

Once T7524 and T7655 are resolved (already in progress), we can go ahead and phase out the Efl.All scheme (although with a similar approach through a new Efl.Csharp.Application.Setupto load the modules - automatically called by Launch if needed).

@bu5hm4n @segfaultxavi

I made a patch D8463 which moves the enum Efl.Csharp.Components out of the namespace Csharp.
(i.e. Efl.Csharp.Components -> Efl.Components)

I think there is no need to put the enum Components under the namespace Csharp.
Because the enum Components is mainly used as a parameter of Efl.All.Init().

Since @segfaultxavi requested to discuss about D8463 on this thread, I left the above here :)

While indeed there is no Efl.Components right now, we put them in the Efl.Csharp namespace exactly to avoid adding C#-specific bits to upstream EFL namespaces that could eventually conflict, just like we did with Application.

Btw, the library user even wouldn't notice it exists as for the common case of creating Elementary apps it would be hidden as the default value for Application methods.