Make passing data pointer to format_cb possible
AcceptedPublic
Actions

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

Details

Reviewers

bowonryu
eagleeye
zmike
cedric
raster

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

Harbormaster completed remote builds in B17551: Diff 31486.Oct 21 2021, 1:01 PM

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

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

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

Updating D12298: Make passing data pointer to format_cb possible #
Enter a brief description of the changes included in this update.
The first line is used as subject, next lines as comment. #
If you intended to create a new revision, use:
$ 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 :-)

Harbormaster completed remote builds in B17562: Diff 31497.Sat, Nov 13, 11:14 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

Harbormaster completed remote builds in B17563: Diff 31498.Sat, Nov 13, 11:22 PM

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

Revision Contents
Changeset List

			Path	Packages
M			src/lib/elementary/efl_ui_progressbar.c (50 lines)
M			src/lib/elementary/elm_progressbar_common.h (1 line)
M			src/lib/elementary/elm_progressbar_legacy.h (17 lines)
M			src/lib/elementary/elm_slider.c (71 lines)
M			src/lib/elementary/elm_slider_common.h (1 line)
M			src/lib/elementary/elm_slider_legacy.h (27 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	31486	57a5655		Oct 21 2021, 1:01 PM	★	★
Diff 2	31497	57a5655		Sat, Nov 13, 11:13 PM	★	★
Diff 3	31498	f2c5bb9	Just a minor change. In the origin Pb_Format_Wrapper_Data and…	Sat, Nov 13, 11:22 PM	★	★