Page MenuHomePhabricator

ecore: improve usability of Efl.Select_Model to provide helpers in manipulating selection information.
ClosedPublic

Authored by cedric on Apr 6 2019, 6:55 PM.

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
cedric created this revision.Apr 6 2019, 6:55 PM
cedric requested review of this revision.Apr 6 2019, 6:55 PM
segfaultxavi added inline comments.
src/lib/ecore/efl_select_model.eo
8

What is exactly contained in the iterator? uint64 is not very explicit.

18

This is the OPPOSITE of multiselect, right?

20

Better do not have docs at the property AND set/get level. Only one will be used.

The docs should be outside the {}.

cedric updated this revision to Diff 21340.Apr 12 2019, 10:30 AM

Improve doc and rebase.

code looks good, only few question about uint64 and exclusive term.

src/lib/ecore/efl_select_model.eo
15

is it okay just said exclusive not "selection_exclusive"?
as non-native speaker.. I really don't know it is easy to understand what this property does by seeing the name of it..

segfaultxavi added inline comments.Apr 18 2019, 8:03 AM
src/lib/ecore/efl_select_model.eo
15

Yeah, maybe single_selection is more explicit?
Or multi_selection (and reverse the behaviour).

cedric planned changes to this revision.Apr 18 2019, 10:46 AM
cedric added inline comments.
src/lib/ecore/efl_select_model.eo
15

I like single_selection. I will adopt that if @SanghyeonLee is fine with it too.

cedric updated this revision to Diff 21472.Apr 18 2019, 3:28 PM

Rebase and rename.

@segfaultxavi Are your concern met ? I don't want to crawl through the amount of comments here.

Yeah, all my concerns have been addressed. I will fix spelling and make the text clearer in another patch (that's my job after all).
I never reviewed the code, though.

bu5hm4n requested changes to this revision.Apr 25 2019, 4:47 AM
bu5hm4n added inline comments.
src/lib/ecore/efl_select_model.eo
8

This iterator looks faulty, in a c foreach loop over the iterator, will you have really a POINTER to a POINTER to a int that you will get back ?

This revision now requires changes to proceed.Apr 25 2019, 4:47 AM
cedric planned changes to this revision.Apr 25 2019, 8:56 AM
cedric added inline comments.
src/lib/ecore/efl_select_model.eo
8

You are right. I am still confused by the implicit meaning of iterator in eo.

cedric updated this revision to Diff 21634.Apr 25 2019, 4:10 PM
cedric edited the summary of this revision. (Show Details)

Rebase and fixup iterator.

cedric updated this revision to Diff 21635.Apr 25 2019, 4:12 PM

Actually not.

cedric added inline comments.Apr 25 2019, 4:13 PM
src/lib/ecore/efl_select_model.eo
8

So actually, the ptr is mandatory and the idea is that you pass a pointer to an uint64 to get the uint64 out of the iterator next function. This is what is expected.

bu5hm4n added inline comments.Apr 26 2019, 10:43 AM
src/lib/ecore/efl_select_model.eo
8

But ptr() in the eo file would say that the iteractor CONTAINS this pointer. but it does contain a uint64. I am meanwhile totally confused. I thought we came arround this a few times. And our reasoning was that a iterator<uint64> would not be possible, we do not do casting, hence the ptr is implicitly. (At least i remember that for events)

cedric added a subscriber: q66.Apr 26 2019, 11:42 AM
cedric added inline comments.
src/lib/ecore/efl_select_model.eo
8

I know, I am totally confused by this one too. I think it should mean that you give a reference to an uint64 and not a reference to a pointer to an uint64. So I don't know. @q66 @felipealmeida what should be the proper syntax for defining an iterator to an uint64?

SanghyeonLee added inline comments.May 2 2019, 4:07 AM
src/lib/ecore/efl_select_model.eo
15

cool. I'm find with it.

@q66 ping ping ping ping ping ping ping ping ping ping ping ping ping ping ping ping ping ping ping ping ping ping ping!

@q66 would you have time to allow for structure and base type to be authorized for iterator?

cedric updated this revision to Diff 23194.Jul 10 2019, 10:26 AM

Rebase and clean iterator type.

bu5hm4n requested changes to this revision.Jul 10 2019, 11:23 AM
bu5hm4n added inline comments.
src/lib/ecore/efl_select_model.c
261

eina_strcmp

367–368

eina_strcmp ? so we are protected again property beeing NULL ?

src/lib/ecore/efl_select_model.eo
21

can you remove set {} and get {}?

This revision now requires changes to proceed.Jul 10 2019, 11:23 AM
cedric updated this revision to Diff 23210.Jul 10 2019, 11:48 AM
cedric added a reviewer: segfaultxavi.

Remove get {} set {}.

bu5hm4n accepted this revision.Jul 10 2019, 11:51 AM

The rest is in the next revision.

This revision is now accepted and ready to land.Jul 10 2019, 11:51 AM
Closed by commit rEFL4e4210b0f349: ecore: improve usability of Efl.Select_Model to provide helpers in manipulating… (authored by cedric, committed by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain WhyJul 10 2019, 11:55 AM
This revision was automatically updated to reflect the committed changes.