Page MenuHomePhabricator

mixer: factorize backend emix infra
ClosedPublic

Authored by michael.bouchaud on Jan 22 2019, 3:59 AM.

Details

Summary

The mixer gadgets suffer of a bad design between old and new gadget infra.
This commit refactorize the mixer backend to get only one instance
between old and new gadget. This resolve many problems as e_client_volume
integration or the default_sink choosen in one of this modules.
Now it will be easier to maintain this code and better support when we will
remove old gadget infra.

Diff Detail

Repository
rE core/enlightenment
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 8990
Build 7798: arc lint + arc unit
michael.bouchaud requested review of this revision.Jan 22 2019, 3:59 AM
  • Add missing vars
  • Remove debug print
zmike requested changes to this revision.Jan 22 2019, 6:03 AM

minor nits

src/modules/mixer/e_mod_main.c
9–10

This needs to be EINTERN, as do any other module-internal variables that aren't static.

127

This feels a bit fragile where perhaps a race condition could occur if a list item has been marked for deletion somewhere else while this is running?

src/modules/mixer/gadget/mixer.c
72

Same as above.

src/modules/mixer/gadget/mod.c
13

This leaks the above two lines of init.

This revision now requires changes to proceed.Jan 22 2019, 6:03 AM
  • Add EINTERN to _e_emix_log_domain var
  • Avoid memleak on e_modapi_gadget_init, and clean some emix_init and emix_shutdown uneeded anymore
  • Track sink changes to update popup list, and more cleanings vars uneeded

Why/how will sharing the same instance be an improvement/make transitioning easier? It seems to me not having co-dependence of any kind will make the removal of the old module easier? Perhaps ensuring independence of each at this point would be the better route?

I see many things to do this

First and the biggest issue about the old and new, both try to catch pulse input_sink to add it to an e_client. This go to make 2 e_client_volume instances for each e_client associated with some sink_input. It works but consume only more ram and have no benefit.
The new gadget don't create his e_actions before we create a gadget. So some users in future need to add this gadget to control the volume with keyboard or whatever they want.
Old and new gadget, have a default_sink saved into the emix_config which is only one config not 2.
And for futur sharing code instead of massive cnp. The old and new gadget only use gui code and no more emix things. So if we fix for one we fix it for both.
For futur too we init the backend into e_mod_main.c and mod.c too, if we go to remove the old gadget. Just rm old gadget file and we are good.

I see many things to do this

First and the biggest issue about the old and new, both try to catch pulse input_sink to add it to an e_client. This go to make 2 e_client_volume instances for each e_client associated with some sink_input. It works but consume only more ram and have no benefit.
The new gadget don't create his e_actions before we create a gadget. So some users in future need to add this gadget to control the volume with keyboard or whatever they want.
Old and new gadget, have a default_sink saved into the emix_config which is only one config not 2.
And for futur sharing code instead of massive cnp. The old and new gadget only use gui code and no more emix things. So if we fix for one we fix it for both.
For futur too we init the backend into e_mod_main.c and mod.c too, if we go to remove the old gadget. Just rm old gadget file and we are good.

Fair enough

michael.bouchaud marked 4 inline comments as done.Jan 31 2019, 12:45 AM
src/modules/mixer/e_mod_main.c
127

I track changes on sinks now and update the ui accordingly

zmike accepted this revision.Mar 18 2019, 5:54 AM
This revision is now accepted and ready to land.Mar 18 2019, 5:54 AM
Closed by commit rEfb23acf87417: mixer: factorize backend emix infra (authored by Michael Bouchaud (yoz) <yoz@efl.so>, committed by zmike). · Explain WhyMar 18 2019, 6:03 AM
This revision was automatically updated to reflect the committed changes.