Page MenuHomePhabricator

ecore: get rid of commands in efl_task.
ClosedPublic

Authored by bu5hm4n on Dec 26 2018, 9:10 AM.

Details

Summary

Note that the usage in efl_thread.c should and could be removed.
the problem with its usage is that when the ARGUMENTS event is fired,
noone ever had the chance to subscribe to the loop of the thread yet. So
all in all this is unneccessary, since noone could ever touch that.

Depends on D7516

Diff Detail

Repository
rEFL core/efl
Branch
master
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 9116
bu5hm4n created this revision.Dec 26 2018, 9:10 AM
bu5hm4n requested review of this revision.Dec 26 2018, 9:10 AM

meson.build is modified but not any *.am? Suspicious, right?

zmike requested changes to this revision.Dec 27 2018, 9:46 AM

This modifies build system files so it should have a travis result before landing.

src/lib/ecore/efl_thread.c
749–750

nit: formatting

This revision now requires changes to proceed.Dec 27 2018, 9:46 AM
raster requested changes to this revision.Dec 27 2018, 10:19 AM
raster added a subscriber: raster.

Note that the usage in efl_thread.c should and could be removed. the problem with its usage is that when the ARGUMENTS event is fired, noone ever had the chance to subscribe to the loop of the thread yet. So all in all this is unneccessary, since noone could ever touch that.

This is wrong. please read https://phab.enlightenment.org/w/efl-loops-threads/ . I actually went to a fair bit of effort to document this.... it'd be nice if people use the docs I wrote. Look at the thread example. _th_main() is a direct mimic of efl_main() and is passed in from the "parent thread" on creation of the thread. this is copied across to the thread child and set up so it gets called once - with the same args event as the main loop so threads look just like a "child process" from this point of view and can be passed args like a process. it allows for easy migration of things to/from threads/processes and a very simple way of passing data in like arguments.

So for now I'd reject as the entire premise for this change is one of not understanding how it works. :) See the docs first.

Note that the usage in efl_thread.c should and could be removed. the problem with its usage is that when the ARGUMENTS event is fired, noone ever had the chance to subscribe to the loop of the thread yet. So all in all this is unneccessary, since noone could ever touch that.

This is wrong. please read https://phab.enlightenment.org/w/efl-loops-threads/ . I actually went to a fair bit of effort to document this.... it'd be nice if people use the docs I wrote. Look at the thread example. _th_main() is a direct mimic of efl_main() and is passed in from the "parent thread" on creation of the thread. this is copied across to the thread child and set up so it gets called once - with the same args event as the main loop so threads look just like a "child process" from this point of view and can be passed args like a process. it allows for easy migration of things to/from threads/processes and a very simple way of passing data in like arguments.

So for now I'd reject as the entire premise for this change is one of not understanding how it works. :) See the docs first.

Thank you for your effort. I am wondering right now why this thing went into the place where noone finds it ? We have wiki pages in dokuwiki and examples in tree Why hiding it in the wiki where no tutorials are, without a single link to it ?

Coming into a discussion with such a first & last sentences makes me think that the whole point of the article is to wait for someone not reading it while trying to use efl_thread. And then claiming he is to stupid to find such an article.

Further more: Even if you claim that i did not understand how it works, you might have missed that the set of functionality is still the same, the only change is that the APIs are having a different way of working, while everything that was possible before is still possible. The only difference here is:

  • The new API can be used way nicer from bindings, a accessor can easily be binded to achive something like if ('--asdf' in tmp) in python where tmp is the accessor of the command line mixin.
  • Accessing a specific position in the set of passed options is also feeling a bit more native, since a accessor can be binded into a language which enables exactly this native feeling for the api, without the need for dealing with lengh calls or get calls with specific APIs.

It is also nice that you explained how the commands should work on threads. However, i am wondering why precisely this is done, why is it neccessary to have commands to achive what you wrote above, you can do exactly the same with the native data strorage on the object, or pass it just as data to the callback ? I Also don't see any value in having the command line call from the main app in the thread, as this is most of the time used to parse the user options, i really don't see a single usecase where it is helpfull to have this informantion in a thread ... normally you parse the options in the beginning, setting global variables (which are immutable) then they are just accessed from everywhere... No reason to have the raw user input again in the thread ...

I can see what this API is made for, however, it does not make sense to me to have it there for threads, since a shared key data storage that enables more than only strings seems to be usefuller.
Further more, the commands API is used on a main app object and the exe object to specify binaries etc. for execution (or in execution) and the corresponding options. Why would that suddenly change into a API for passing messages to a thread here, that does not seem obvious nor intuitive, a command (like in command line) is totally unrelated to threads ... :)

bu5hm4n updated this revision to Diff 18222.Jan 4 2019, 3:25 AM
bu5hm4n edited the summary of this revision. (Show Details)

rebase

raster added a comment.Jan 4 2019, 3:58 AM

I am wondering right now why this thing went into the place where noone finds it ?

I sent mails to the mailing list with the page a year ago or so. It went there and i solicited feedback and made some changes to the whole api set as a result. It went there because the www doc wiki stuff was certainly not settled then and neither was this api set. it's kind of settled in. adding docs to the .eo files isn't really doable due to wanting/needing inline images.We do have wiki pages. phab's wiki pre-dates e's www wiki and it was clear that the e www wiki was for "user facing marketing like documentation and info": and phab was "for developers". i know i have said words to this effect many many times over the years. it's not hiding it - it's gone where it should have gone - certainly at the time. you would have gotten the mails about this so i think you just forgot.

Coming into a discussion with such a first & last sentences makes me think that the whole point of the article is to wait for someone not reading it while trying to use efl_thread. And then claiming he is to stupid to find such an article.

really? if you think this then i believe we can't work together. that's just insulting that i am trying to do this. consider this conversation ended. i have no desire to discuss this further. if you really think this of me after all these years then you're pretty ignorant thinking i'd ever do this or have ever done so or would intend it. end of discussion for me. rejected just on this basis alone.

I am wondering right now why this thing went into the place where noone finds it ?

I sent mails to the mailing list with the page a year ago or so. It went there and i solicited feedback and made some changes to the whole api set as a result. It went there because the www doc wiki stuff was certainly not settled then and neither was this api set. it's kind of settled in. adding docs to the .eo files isn't really doable due to wanting/needing inline images.We do have wiki pages. phab's wiki pre-dates e's www wiki and it was clear that the e www wiki was for "user facing marketing like documentation and info": and phab was "for developers". i know i have said words to this effect many many times over the years. it's not hiding it - it's gone where it should have gone - certainly at the time. you would have gotten the mails about this so i think you just forgot.

Then there could be a single comment in the source code linking to it. or the examples in a repository to find them ...

Coming into a discussion with such a first & last sentences makes me think that the whole point of the article is to wait for someone not reading it while trying to use efl_thread. And then claiming he is to stupid to find such an article.

really? if you think this then i believe we can't work together. that's just insulting that i am trying to do this. consider this conversation ended. i have no desire to discuss this further. if you really think this of me after all these years then you're pretty ignorant thinking i'd ever do this or have ever done so or would intend it. end of discussion for me. rejected just on this basis alone.

Okay - I am writing a patch, which fixes a missleading (IMO) API, this involves patching 3 classes. While patching 1 class i cannot figure out how to attach to that class, so I write this comment about it. Now the patch is *rejected* for not understanding how something (to the outside world) not documented works, and now *I* am the ignorant one ? I don't remember you being like that, and I did not say that you did this on purpose, just that your attitude here, is making me think of that. Teamworking has absolutly nothing to do with this here, I see a problem with the API as it is missleading IMO so I fixed it, for the given reasons. Yes the comment about efl_thread.c is wrong, however, the critism that a command on a thread is something syntactical and semantical different to what a command is on a execution and application still stands, this point was completly ignored (efl_thread could use some key-data storage which also enables you to set ints or other easy to share primitives). Which stands undepending from understanding efl_thread.c or not.

bu5hm4n updated this revision to Diff 18846.Jan 25 2019, 1:38 AM

general code update

bu5hm4n updated this revision to Diff 19015.Jan 29 2019, 8:59 AM

rebase & fixup

zmike requested changes to this revision.Jan 31 2019, 6:56 AM
zmike added inline comments.
src/lib/ecore/efl_exe.c
290

Should this function no-op the case of re-setting the same env object to avoid the (unlikely) scenario that the first unref destroys the object?

src/lib/ecore/efl_thread.c
283–284

nit: space before (

749–750

same as above

This revision now requires changes to proceed.Jan 31 2019, 6:56 AM
bu5hm4n marked 4 inline comments as done.Feb 7 2019, 7:15 AM
zmike accepted this revision.Feb 11 2019, 9:18 AM
This revision was not accepted when it landed; it landed in state Needs Review.Feb 12 2019, 2:20 AM
Closed by commit rEFL616381e9cfed: ecore: get rid of commands in efl_task. (authored by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain Why
This revision was automatically updated to reflect the committed changes.