Details
- Reviewers
Jaehyun_Cho SanghyeonLee zmike - Maniphest Tasks
- T8057: RFC: Efl.Ui.Selectable
- Commits
- rEFL35f9fc26e3d3: efl_ui_multi_selectable: add APIs for selecting
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.
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.
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 :)
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:
- specify clearly in the docs that end must be less than the total number of items in the list
- 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).
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. | |