Page MenuHomePhabricator

efl_ui_multi_selectable: clean this up
ClosedPublic

Authored by bu5hm4n on Thu, Nov 14, 12:20 PM.

Details

Summary

this commit merges common functions from efl_ui_multi_selectable and
efl_ui_multi_selectable_async. Additionally, the two different aspects
of accessing the elements in a multi_selectable widget (numerical or
object based) are now abstracted into interfaces called range_numeric and
range_object. numeric APIs are also prefixed with id's, so its possible
for one widget to implement both (if there will ever be the demand to do
that in future).

The main reason for this split is:

  • there is no good common path between mvvm based multi_selectable and

object based multi_Selectable, so there is no way that both sides would
benefit, without the other one suffering.

  • If we find later on the demand to implement both on one widget, we now

can fully do that

  • Common API is available for both types, so its less API and less

confusion for the API user.

ref T7871
ref T8265

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.Thu, Nov 14, 12:20 PM
bu5hm4n requested review of this revision.Thu, Nov 14, 12:20 PM
cedric requested changes to this revision.Thu, Nov 14, 1:06 PM
cedric added inline comments.
src/lib/elementary/efl_ui_multi_selectable_async.eo
0

The prefix id_ make this API awful. I don't have any idea right now, but will share if I find something. @segfaultxavi any suggestion?

This revision now requires changes to proceed.Thu, Nov 14, 1:06 PM
bu5hm4n added inline comments.Thu, Nov 14, 1:40 PM
src/lib/elementary/efl_ui_multi_selectable_async.eo
0

We need some prefix. And well it's a "id" range it's a iterator of selection "id"s. So I don't really see this is making it awful ?

SanghyeonLee added inline comments.
src/lib/elementary/efl_ui_multi_selectable_async.eo
0

what about using class prefix numeric and object?
though in this case, selected_numeric or selected_index seems more natural than numeric_selected or index_selected..
we could pair it like this.

selected_numeric(index)_iterator_new
selected_object_iterator_new

numeric(index)_range_select
object_range_select

Efl.Ui.Multi_Selectable_Range_Numeric/Object sounds a bit weird... How about:

Efl.Ui.Multi_Selectable_Index_Range and Efl.Ui.Multi_Selectable_Object_Range?

src/lib/elementary/efl_ui_multi_selectable_async.eo
0

I agree with @SanghyeonLee, as the docs say, "The iterator gives indices of selected children." therefore maybe this should be called selected_index_iterator_new or selected_ndx_iterator_new, no?

Efl.Ui.Multi_Selectable_Index_Range and Efl.Ui.Multi_Selectable_Object_Range?

I like this proposal.

Renamed:

  • Efl.Ui.Multi_Selectable_Range_Numeric --> Efl.Ui.Multi_Selectable_Index_Range
  • Efl.Ui.Multi_Selectable_Range_Object --> Efl.Ui.Multi_Selectable_Object_Range
  • id prefixes --> ndx (instead of "index", because this API is already way too verbose)

This is again ready for review!

Thank you for taking this over :)

Okay, i am totally fine with this. I would say lets go with this one.

@cedric are you ok with that ?

cedric accepted this revision.Mon, Nov 25, 3:27 AM

Looks better this way.

This revision is now accepted and ready to land.Mon, Nov 25, 3:27 AM
Closed by commit rEFLf7868fd28cfc: efl_ui_multi_selectable: clean this up (authored by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain WhyMon, Nov 25, 4:31 AM
This revision was automatically updated to reflect the committed changes.