Page MenuHomePhabricator

elm: add a logic to load modules for ui inspection
Needs ReviewPublic

Authored by akanad on Feb 3 2020, 6:18 PM.

Details

Summary

elm has a external ui inspection module named clouseau.
however there is a need for a kind of a infra which enables loading another module.
this patch add a logic to load multiple modules while initializing elementary.

Diff Detail

Repository
rEFL core/efl
Branch
arcpatch-D11276
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 16341
Build 10954: arc lint + arc unit
akanad created this revision.Feb 3 2020, 6:18 PM
akanad requested review of this revision.Feb 3 2020, 6:18 PM
bu5hm4n requested changes to this revision.Feb 7 2020, 6:55 AM
bu5hm4n added a subscriber: bu5hm4n.
bu5hm4n added inline comments.
src/lib/elementary/elm_main.c
310

That should be static.

313

Why is this a seperated function? Its only called once.

330

The parameter should have void here.

333

This should support intree loading.

339

The parameter should have void here.

349

That is leaking memory.

This revision now requires changes to proceed.Feb 7 2020, 6:55 AM
akanad marked 4 inline comments as done.Feb 10 2020, 10:54 PM
akanad added inline comments.
src/lib/elementary/elm_main.c
313

I'd like to use another external ui-inspection tool rather than clouseau.
this function will be utilized for it.

333

previous code returns in case of IN_TREE situation.
I thought that you guys think that loading modules while building isn't necessary.

akanad updated this revision to Diff 28939.Feb 10 2020, 10:55 PM
  • modify parameters
  • fix a leak stuff
  • add phab comments
akanad updated this revision to Diff 29082.Feb 16 2020, 5:21 PM
  • rebasing
  • ping

I need to test that with closeau, which can take a bit.

akanad updated this revision to Diff 29119.Feb 18 2020, 6:24 PM
  • rebase onto the latest

Have you tried checking if this module *ever* gets loaded ?

bu5hm4n requested changes to this revision.Feb 19 2020, 7:09 AM
This revision now requires changes to proceed.Feb 19 2020, 7:09 AM

My comment was probably a bit short:

  • That can NEVER EVER work. clouseau/api is not the name of the module you have added here.
  • Loading modules means, you have to add them to the config, means, whenever you want to debug your application you need to know which modules are running right now in order to overwrite the modules string correctly.

If you want to add a application that does the same as clouseau (btw. why not just extend / patch clouseau, that needs some love, but the same applies to a new application). However, if it *must* be a new application, then why not just call the file as well clouseau_debug? then the existing code will just work.

akanad updated this revision to Diff 29537.Mar 17 2020, 2:39 AM
  • rebasing
  • add the module name into config (I forget to add this cause I was using ELM_MODULES env)

I'd like to introduce another inspection tool later.
with this patch, It is possible to show the tool without interrupting the use of the exsiting one, clouseau.

Still:

  1. why not just use clouseau, and write a extension for it ?
  2. Loading modules means, you have to add them to the config, means, whenever you want to debug your application you need to know which modules are running right now in order to overwrite the modules string correctly.

All in all, i am still not sure why we want that. If you want to write your own inspection tool (which I really dont get why you want to have that). Why dont you just write the same file as closeau, and take the infra we have right now ?

Still:

  1. why not just use clouseau, and write a extension for it ?
  2. Loading modules means, you have to add them to the config, means, whenever you want to debug your application you need to know which modules are running right now in order to overwrite the modules string correctly.

I have a project(ui inspection tool) which has been developing while commercializing a wearable product in Tizen.
because it is originally developed from a scratch, it has almost nothing similar with clouseau.
that is a reason why I can't write some patches on clouseau to show the tool.
efl community might decide to overwrite clouseau with it later, however I guess that 'not this time'

All in all, i am still not sure why we want that. If you want to write your own inspection tool (which I really dont get why you want to have that). Why dont you just write the same file as closeau, and take the infra we have right now ?

if we have to have just one inspection tool only for efl, right, what you said works. I agree.
but I don't agree that we have to have just one inspection tool only for efl.
and that is the reason why I don't get a reason why you are not sure about this.

IMHO,
In general, a competition is a good way to improve something among similar stuffs.
so that it's good for us to have a kind of infra that enables us to have multiple inspection tools easier. isn't it?

All you wrote still does not help for the problem i pointed out the 2. point.

What i would do (after knowing the full picuture), is refactoring _elm_clouseau_load into something where you pass the name of the debug module to load, and a pointer to the struct which is right now:

struct {
     Eina_Module *handle;
     Eina_Bool (*init)(void);
     Eina_Bool (*shutdown)(void);
     Eina_Bool is_init;
} _clouseau_info;

That way you can either add 1 single line in code that loads *another* dbging tool, *or* we can make that depended on a env var. However, i would not use elementary modules just for loading that, mainly because: you load a nearly empty .so just to load another .so, isn't that somewhat weird ?
As of right now, Clouseau is loaded per default when its installed, (that might be not the best idea tbf), we already have a config value enabling / disabling the old Clouseau module, we might want to do the same for the "new" Clouseau module.

zmike added a subscriber: zmike.Mar 19 2020, 9:17 AM

I'm interested in seeing this new inspection tool. Do you have plans to push it upstream or ?