Page MenuHomePhabricator

csharp: add a interface to start apps
Needs ReviewPublic

Authored by bu5hm4n on Thu, Dec 20, 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
Branch
devs/bu5hm4n/POC
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8802
bu5hm4n created this revision.Thu, Dec 20, 9:33 AM
bu5hm4n requested review of this revision.Thu, Dec 20, 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.Thu, Dec 20, 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.Thu, Dec 20, 12:53 PM
bu5hm4n added inline comments.Thu, Dec 20, 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.Thu, Dec 20, 1:45 PM

review updates

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

updating to review comments

bu5hm4n added inline comments.Tue, Dec 25, 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.Wed, Dec 26, 2:57 AM
akanad added inline comments.Wed, Dec 26, 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.Thu, Dec 27, 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.Fri, Jan 4, 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.Mon, Jan 7, 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.Mon, Jan 7, 8:49 AM
src/bindings/mono/efl_mono/efl_csharp_application.cs
44

Perfectly OK.

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

update with the review comments.

bu5hm4n added a subscriber: zmike.Thu, Jan 10, 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.EditedThu, Jan 10, 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.Fri, Jan 11, 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.Mon, Jan 14, 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.Mon, Jan 14, 4:45 AM
bu5hm4n updated this revision to Diff 18403.Mon, Jan 14, 10:09 AM
bu5hm4n edited the summary of this revision. (Show Details)

review comments

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

adjust code sample

segfaultxavi accepted this revision.Tue, Jan 15, 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!