Page MenuHomePhabricator

Make passing data pointer to format_cb possible
AcceptedPublic

Authored by MaxPerl on Oct 21 2021, 1:01 PM.

Details

Summary

Hello,
For my perl binding it is important that one can pass a data pointer to all callbacks, especially to "Format_Cbs" as in elm_slider_units_format_function_set(), elm_slider_indicator_format_function_set() of elm_progressbar_unit_format_function_set(). Another "problematic" function would be elm_calendar_format_function_set().

Enclosed you find a approach to solve this problem.

It would be wonderful, if the Efl-libraries could make data pointers also in format cbs possible...

Thanks in advance,
Max

Diff Detail

Repository
rEFL core/efl
Branch
work
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 17563
Build 11825: arc lint + arc unit
MaxPerl created this revision.Oct 21 2021, 1:01 PM

It seems that this patch has no reviewers specified. If you are unsure who can review your patch, please check this wiki page and see if anyone can be added: https://phab.enlightenment.org/w/maintainers_reviewers/

MaxPerl requested review of this revision.Oct 21 2021, 1:01 PM

I hope this is the right way to specify reviewers ;-)

raster added a subscriber: raster.Mon, Nov 1, 1:25 PM

I saw this - marked it. Just haven't gotten to it. :)

raster requested changes to this revision.Sun, Nov 7, 2:21 AM

see all my comments. you don't introduce a new type for the cb - you change an existing one. you need to introduce a new one. you can go casting ptrs around inside efl but the api has to be cleaner than this. :)

src/lib/elementary/efl_ui_progressbar.c
959

you are using the exact same callback types... but it's a new one that passes in data to the cb.. ?

src/lib/elementary/elm_progressbar_common.h
7–8

Small comment - you do not need the unused tag here ... it'd be in ther actual callback implementation itself :)

src/lib/elementary/elm_progressbar_legacy.h
206

ad above - types for cb's have not changes

src/lib/elementary/elm_slider.c
105

why did you remove the return here?

150

same here - why remove the if + return?

183

and here?

1573–1574

also removing if + return here .. ?

src/lib/elementary/elm_slider_common.h
7–8

now this is bad - you change the function signature for an existing callback type... you need a new type. you can't do this as existing code will now complain that the func type signature mismatches. and it'd be right in that complaint. if you fix this then all my other comments about not using a new type are now valid - so you need a new cb type. :)

src/lib/elementary/elm_slider_legacy.h
175

also cb type here - no data ptr

313

same here - type of cb

This revision now requires changes to proceed.Sun, Nov 7, 2:21 AM
MaxPerl updated this revision to Diff 31497.Sat, Nov 13, 11:13 PM
  1. Updating D12298: Make passing data pointer to format_cb possible #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment. #
  4. If you intended to create a new revision, use:
  5. $ arc diff --create

Thank you very much for your review. I tried to change my patch as wished. With regard to removing the return I really don't know what was happened :-)

MaxPerl updated this revision to Diff 31498.Sat, Nov 13, 11:22 PM

Just a minor change. In the origin Pb_Format_Wrapper_Data and Slider_Format_Wrapper_Data structs we don't need any more a func_data member

raster accepted this revision.Sun, Nov 14, 7:18 AM

so much needed fixing... i just fixed it :) i suggest you check the patch after i've fixed things... :)

This revision is now accepted and ready to land.Sun, Nov 14, 7:18 AM

Sorry for asking (this is my first patch...), but landing doesn't work for me. I followed the guide at https://www.enlightenment.org/contrib/devs/arcanist-guide.md and got these errors, when I tried to land:

STRATEGY Merging with "squash" strategy, the default strategy.

SOURCE  Landing the current branch, "arcpatch-D12298".
ONTO REMOTE  Landing onto remote "origin", the default remote under Git.
ONTO TARGET  Landing onto target "master", the default target under Git.
INTO REMOTE  Will merge into remote "origin" by default, because this is the remote the change is landing onto.
INTO TARGET  Will merge into target "master" by default, because this is the "onto" target.
FETCH  Fetching "master" from remote "origin"...
 
 $   git fetch --no-tags --quiet -- origin master
 
 
INTO COMMIT  Preparing merge into "master" from remote "origin", at commit "f2c5bb93a6a3".
LANDING  These changes will land:
 
 *   D12298 Make passing data pointer to format_cb possible
         4452377be3cd  Make passing data pointer to format_cb possible
 
>>> Land these changes? [y/N/?] y

[2021-11-15 05:34:30] EXCEPTION: (ConduitClientException) ERR-CONDUIT-CALL: <harbormaster.buildable.search> Conduit API method "harbormaster.buildable.search" does not exist. at [<arcanist>/src/conduit/ConduitFuture.php:70]
arcanist(head=master, ref.master=a028291f8e5e)

#0 ConduitFuture::didReceiveResult(array) called at [<arcanist>/src/future/FutureProxy.php:40]
#1 Future::updateFuture() called at [<arcanist>/src/future/FutureProxy.php:35]
#2 FutureProxy::isReady() called at [<arcanist>/src/conduit/ConduitSearchFuture.php:62]
#3 ConduitSearchFuture::isReady() called at [<arcanist>/src/future/Future.php:63]
#4 Future::updateFuture() called at [<arcanist>/src/future/FutureIterator.php:224]
#5 FutureIterator::next() called at [<arcanist>/src/hardpoint/ArcanistHardpointEngine.php:215]
#6 ArcanistHardpointEngine::updateFutures() called at [<arcanist>/src/hardpoint/ArcanistHardpointEngine.php:176]
#7 ArcanistHardpointEngine::waitForRequests(array) called at [<arcanist>/src/runtime/ArcanistRuntime.php:858]
#8 ArcanistRuntime::loadHardpoints(array, ArcanistHardpointRequestList) called at [<arcanist>/src/workflow/ArcanistWorkflow.php:2327]
#9 ArcanistWorkflow::loadHardpoints(array, array) called at [<arcanist>/src/land/engine/ArcanistLandEngine.php:542]
#10 ArcanistLandEngine::confirmBuilds(array) called at [<arcanist>/src/land/engine/ArcanistLandEngine.php:524]
#11 ArcanistLandEngine::confirmRevisions(array) called at [<arcanist>/src/land/engine/ArcanistLandEngine.php:1220]
#12 ArcanistLandEngine::execute() called at [<arcanist>/src/workflow/ArcanistLandWorkflow.php:344]
#13 ArcanistLandWorkflow::runWorkflow(PhutilArgumentParser) called at [<arcanist>/src/workflow/ArcanistWorkflow.php:227]
#14 ArcanistWorkflow::executeWorkflow(PhutilArgumentParser) called at [<arcanist>/src/toolset/ArcanistPhutilWorkflow.php:21]
#15 ArcanistPhutilWorkflow::execute(PhutilArgumentParser) called at [<arcanist>/src/parser/argument/PhutilArgumentParser.php:492]
#16 PhutilArgumentParser::parseWorkflowsFull(array) called at [<arcanist>/src/runtime/ArcanistRuntime.php:171]
#17 ArcanistRuntime::executeCore(array) called at [<arcanist>/src/runtime/ArcanistRuntime.php:37]
#18 ArcanistRuntime::execute(array) called at [<arcanist>/support/init/init-arcanist.php:6]
#19 require_once(string) called at [<arcanist>/bin/arc:10]

Thanks in advance for your help,
Max