Page MenuHomePhabricator

efl.ui.spotlight.container
Open, TODOPublic

Description

class Efl.Ui.Active_View.Container
├ (P) view_manager
├ (P) indicator
├ (P) active_index
├ (P) active_view_size
├ (M) push
├ (M) pop
├ (E) transition,start
├ (E) transition,end
  • Do we really want the widget to inherit from Layout ? (The theme of the spotlight is rather useless, inheriting from widget directly will safe up more memory)
  • We need some policy on the minsize of this widget. How about the minsize is the max of the minsize of each element, (And spotlight_size)
  • what are we doing about the push and pop name ? (@segfaultxavi)
  • Should we invert the push and pop semantics ? (right now we are inserting at 0 instead of inserting at the max number)
  • I would like to think about leaving indicator @beta for now, until we have a indicator widget and EFL, and see how we can integrate this.
  • I would also like to leave spotlight manager @beta for not
  • introduce Efl.Ui.Stack Efl.Ui.Pager which uses the proper spotlight managers, without exposing too many internals to the stable world.
  • The animation object spotlight manager is going to be introduced after everything above is done.
bu5hm4n created this task.May 30 2019, 4:31 AM
bu5hm4n triaged this task as TODO priority.
bu5hm4n updated the task description. (Show Details)
bu5hm4n renamed this task from efl.ui.active_view.container to efl.ui.spotlight.container.Jul 30 2019, 11:43 PM

@Jaehyun_Cho @woohyun What do you think of changing active_index to actaully take a object and not a index ? It feels a little bit weird to me that we have to use the index here, and i am currently playing with this widget arround, and i have to use
active_index_set(obj, efl_pack_index_get(subobj)); all the time all the time ...

It is true that an integer index is only used in active_index and in the transition event data, so it should be easy to change it to use an object instead.
However, I am sure somebody will come up with an example showing that using objects is more inconvenient than indices :)

Can we add an active_object property and let the user choose whichever he prefers?

I kind of agree, but: Spotlight needs to access its internal list with nth, which costs performance, in most of the cases the user will need to get its own index, which ends in a eina_list_find call. So both times a O(n) call.
So right now:
Have a object: 2 times O(n)
Have a index : 1 time O(n) 1 time O(1)
With object:
Have a object: 2 times O(1)
Have a index : 1 time O(n) 1 time O(1)

So ... the solution with having a object is more performant.

I'm OK with that. But then we also change the indices inside the events, so everything works with objects.

Jup, good point.

Sorry about replying late.. ;)

I am OK on using object instead of indices for active_index.

Since index can be changed but object is not changed, I think that using object might be better not only performance but also from some aspects.

BTW, like @segfaultxavi 's comment, it seems that using object may require quite a bit of code modifications.

I have done the migration now in the attached revision. However, I left out the events for the reason given in the commit. We might need to fill invalid pointers to have correct values, which is weird. we do not have this issue with the index.

@cedric @Jaehyun_Cho @segfaultxavi @stefan_schmidt @woohyun @zmike

here are a few things we probably want to discuss here:

  • Do we really want the widget to inherit from Layout ? (The theme of the spotlight is rather useless, inheriting from widget directly will safe up more memory)
  • We need some policy on the minsize of this widget. How about the minsize is the max of the minsize of each element, (And spotlight_size)
  • what are we doing about the push and pop name ? (@segfaultxavi)
  • Should we invert the push and pop semantics ? (right now we are inserting at 0 instead of inserting at the max number)
  • I would like to think about leaving indicator @beta for now, until we have a indicator widget and EFL, and see how we can integrate this.
  • I would also like to leave spotlight manager @beta for not, and introduce Efl.Ui.Stack Efl.Ui.Pager which uses the proper spotlight managers, without exposing too many internals to the stable world.
  • The animation object spotlight manager is going to be introduced after everything above is done.

For context, I introduce you to... The Push/Pop conundrum:
Efl.Ui.Spotlight_Container implements the Efl.Pack_Linear interface, so pages can be added anywhere using pack_begin, pack_at, etc.
For convenience, we also provide push and pop methods which mimic the behavior of a STACK (by inserting and removing from position 0, and making position 0 current afterwards).

However, the existing push and pop methods allow "pushing into" and "popping from" ANY position. This might be handy, but I think it is an abuse of the meaning of the words "push" and "pop".
If we want to keep this ability to "insert and make current" from anywhere, I think we need another name for it. "Push" and "pop" should be reserved to LIFO STACKs.
Any other opinion?

Regarding whether the active position in a stack should be the first or the last one, I don't think it matters much. I would leave it as it is.

Okay, as an general update:
First and second bullet point are in the revisions attached here. The inheritance from Efl.Ui.Layout is removed, which safes quite a lot of performance and memory.

If we take it that precise with push and pop then we should also consider that in normal stacks, the new element is put "on top" of everything else, so its the highest element (efl_pack_end), which is the most right and not might left. Also, looking at every UI i could find, the back direction is also on the left side, which means, the direction things are pushed is to the right side.
In the case of the push and pop name, i do not find them confusing, so if you want to see them changed, please propose a name or something we should do.

So if nothing else pops up, i would merge the revisions above during next week. And start creating the classes for Efl.Ui.Stack & Efl.Ui.Pager.

@Jaehyun_Cho I would wait a few more weeks with removing @beta, to see how things work out. However, from my side, i do not have any further plans in regards of Efl.Ui.Spotlight or Efl.Ui.Stack or Efl.Ui.Pager.

Alright, alright, you convinced me. Let's keep using push and pop and make the last element the current one, instead of element 0.
Just make sure that docs are also updated when you change this :)

bu5hm4n updated the task description. (Show Details)Mon, Dec 2, 8:25 AM

@bu5hm4n @segfaultxavi
Thank you very much :)

I would like to think about leaving indicator @beta for now, until we have a indicator widget and EFL, and see how we can integrate this.
-> I agree with this (let's leave it @beta) :)
-> I think we can decide if Efl.Ui.Pager is required to set indicator automatically in constructor or not later.

I would also like to leave spotlight manager @beta for now
-> As you discussed, if Efl.Ui.Stack and Efl.Ui.Pager sets spotlight manager automatically in constructors, then I agree with this (let's leave it @beta) :)

Should we invert the push and pop semantics ? (right now we are inserting at 0 instead of inserting at the max number)
-> Pushing an element to the right end is exactly what I wanted! :)
-> I have a question to ask you related to this. What do you think about adding push and pop only to Efl.Ui.Stack not in Efl.Ui.Spotlight.Container?

-> I have a question to ask you related to this. What do you think about adding push and pop only to Efl.Ui.Stack not in Efl.Ui.Spotlight.Container?

It is theoretically possible. However, the implementation in Efl.Ui.Spotlight.Container is *way* more performent, and does not need event subscriptions at all ..., its a matter or priority i guess. What if we call them "animated_insert" and "animated_remove", and move the name push and pop to Stack, where we can also ensure that @segfaultxavi's concerns are met ?

Thank you for your reply :)

It is theoretically possible. However, the implementation in Efl.Ui.Spotlight.Container is *way* more performent, and does not need event subscriptions at all ..., its a matter or priority i guess. What if we call them "animated_insert" and "animated_remove", and move the name push and pop to Stack, where we can also ensure that @segfaultxavi's concerns are met ?

Thank you for your reply :)

Thank you for the reply :)
If we have "animated_insert" and "animated_remove" in spotlight_container, then I think we don't need to add "push" and "pop" to Stack additionally because those methods are the same :)

If we consider changing the method names "push" and "pop" to new names (e.g. "animated_insert" and "animated_remove"), then I have a suggestion about the new names!
What do you think "active_element_pack" and "active_element_unpack" (combinding the existing method names) ? I am not sure which one is the correct order "active_element_pack" or "pack_active_element" ^^;;

Thank you! :)

I'm getting lost... let me summarize what we've got:

  • Adding pages is done through the Pack interface, and this is not animated.
  • Transitioning from one page to another is done through Spotlight Container and THIS IS animated.
  • Push and Pop are a combination of "add/remove page" + "move to the new page". This is animated, because there is a transition.

I think "animated_insert/remove" is confusing because insert/remove operations are not animated: Transitions are animated.
We already spent some time thinking about this (at least I did) and we didn't find a better name, so I am OK with keeping Push and Pop (even when inserting/removing in the middle of the list).

I am not sure which one is the correct order "active_element_pack" or "pack_active_element" ^^;;

In EFL, the rule is to put the verb at the end, so "active_element_pack" ^_^

Thank you for the summery. I kind of agree with Xavi on that, so lets keep push and pop in the place and with the name how it is right now ? @Jaehyun_Cho