Page MenuHomePhabricator

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

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

Diff Detail

Repository
rEFL core/efl
Branch
T7376-devs/cedric/selectmodel
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 11037
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.Thu, Apr 25, 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.Thu, Apr 25, 4:47 AM
cedric planned changes to this revision.Thu, Apr 25, 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.Thu, Apr 25, 4:10 PM
cedric edited the summary of this revision. (Show Details)

Rebase and fixup iterator.

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

Actually not.

cedric added inline comments.Thu, Apr 25, 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.Fri, Apr 26, 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.Fri, Apr 26, 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.Thu, May 2, 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!