Page MenuHomePhabricator

efl_ui_multi_selectable: add APIs for selecting
ClosedPublic

Authored by bu5hm4n on Aug 20 2019, 11:12 PM.

Details

Summary

this can be used to select / unselect a range or all selectables in a
container. The range selectable APIs do not have a strong ordering on a
and b, b does not have to come after a.

ref T8057

Depends on D9723

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.
bu5hm4n created this revision.Aug 20 2019, 11:12 PM
SanghyeonLee added a comment.EditedAug 22 2019, 5:30 AM

One suggestion for the future is, using data structures then giving two fixed parameters..

we probably support range index bases,
so for that we might need some data type Eina.Range { int start, int end};
also we could give some collections of ranges like, Eina.Range.Series[Eina.Range(1,3), Eina.Range(5,6), Eina.Range(10,104), ....]
Eina.Range can be extended for Object, Like Eina.Range.Int{int start, int end} and Eina.Range.Obj{eo* start, eo *end}
so we could extend our method for object or int or series of ranges, instead of giving two parameters.

That can surly be added later on. However, for now does this here look fine to you ?

I think current implements looks good to me. should we have to wait another review?

Mhm, i so not know, there are already a few places this is used and implemented, I am quite happy with it, I would continue :)

zmike requested changes to this revision.Aug 23 2019, 10:48 AM

A small but significant nit: there's no specification in the doc about what happens if end is greater than the total number of items. In _range_selection_find you handle this by verifying the end marker for the range and erroring if it isn't found (without making the change). This seems like probably not what app developers expect, and it certainly is not specified in the docs.

I think this should either:

  1. specify clearly in the docs that end must be less than the total number of items in the list
  2. permit end to be larger and clamp to the last item in the list

Personally, I think #2 would be fine since it enables developers to potentially skip having to do the clamping in the app itself.

In either case, however, I think _range_selection_find() should immediately jump into _selectable_range_apply(). If option #1 is used, provide the error immediately by using a counter and calculating eina_list_count(pd->items) - start_idx in order to avoid a long double list walk. If there's concerns over whether the selectable belongs to the collection, check efl_parent_get() on both selectables before iterating (probably should be done anyway).

This revision now requires changes to proceed.Aug 23 2019, 10:48 AM
bu5hm4n updated this revision to Diff 24463.Aug 23 2019, 11:42 AM
bu5hm4n edited the summary of this revision. (Show Details)

more docs and different walking

zmike requested changes to this revision.Aug 23 2019, 12:06 PM

Looks pretty good

src/lib/elementary/efl_ui_collection.c
946

This can just be a normal EINA_LIST_FOREACH now since the function is only used for full list walks. Also no length param is needed and the name can probably change.

This revision now requires changes to proceed.Aug 23 2019, 12:06 PM
zmike accepted this revision.Aug 26 2019, 4:55 AM
This revision is now accepted and ready to land.Aug 26 2019, 4:55 AM
Closed by commit rEFL35f9fc26e3d3: efl_ui_multi_selectable: add APIs for selecting (authored by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain WhyAug 26 2019, 5:44 AM
This revision was automatically updated to reflect the committed changes.