Page MenuHomePhabricator

efl_ui_stack: remove!
Needs ReviewPublic

Authored by bu5hm4n on Sun, May 19, 4:00 AM.

Details

Summary

this can now be done with active_view. This is done in order to reduce
the LOC in elementary that basically do the same.

Depends on D8919

Diff Detail

Repository
rEFL core/efl
Branch
devs/bu5hm4n/work3
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 11769
bu5hm4n created this revision.Sun, May 19, 4:00 AM
bu5hm4n requested review of this revision.Sun, May 19, 4:00 AM
bu5hm4n added a reviewer: woohyun.

Noo... i just found a bug in this widget. :P

@singh.amitesh That makes you a perfect tester for this new widget that should replace it. Can you test it / check it out ? :)

bu5hm4n updated this revision to Diff 22443.Sat, May 25, 4:06 AM
bu5hm4n edited the summary of this revision. (Show Details)

introduce a new class to do pop / push

I thought that the util class would be either

  1. a regular class inherits from Efl.Ui.Active_View.Container
    • it sets Efl.Ui.Active_View.View_Manager_Stack in constructor or finalize?
    • it makes view_manager_set disabled (e.g. it overrides view_manager_set and makes other view manager cannot be set)

Or

  1. a regular class attaches Efl.Ui.Active_View.Container as a composite object
    • it sets Efl.Ui.Active_View.View_Manager_Stack in constructor or finalize?
    • it makes view_manager_set disabled (I am not sure how to do it)

What do you think about above?

I don't think it makes sense to create a class called Util that is creatable, and has additionally pop & push, where is there the difference to just having pop & push on the normal object? It makes no sense to restrict the usage of push and pop to something that only can have the stack View_Manager. Why shoudn't someone just use the Scroll View_Manager instead of the stack? It could looks nice and serve the purpose.

I can create here another util function that just returns a ready to use Active_View with a setted stack, nothing against that. But there is no need for another object.

@bu5hm4n

I see. I left comment incorrectly about util class. Please forget about what I talked about util class.

I think that utility class which provides push/pop methods is not a better option than regular class for stack usecase.
Because, as you know, using utility class is not OOP approach so it is not easier than using push/pop from a regular class.
e.g.

  1. Support push/pop by utility class in C#
Efl.Ui.ActiveView.Container container = new Efl.Ui.ActiveView.Container(...);
Efl.Ui.ActiveView.Util.Push(container, content);
  1. Support push/pop by regular class in C#
//Let's assume Efl.Ui.ActiveView.Stack inherits from Efl.Ui.ActiveView.Container
Efl.Ui.ActiveView.Stack stack = new Efl.Ui.ActiveView.Stack(...);
stack.Push(content);

IMO, the second regular class usecase is easier to use for stack usecase not only it adopts general OOP approach but also we can get help from tools like intellisense in Visual Studio.
Basically, I like the class Efl.Ui.ActiveView.Container and its concept. And the reason why I think it is good because we can extend the class for specific usecases.

In the beginning, I thought that the Efl.Ui.ActiveView.Stack's ViewManager is set in constructor and should not be changed.
But now I think that you are right about the ViewManager also can be changed by developer. But I still think the default ViewManager should be set in constructor in Efl.Ui.ActiveView.Stack for convenience.
Because most developers just want to use the default transition. If they want to change the transition, then they can modify it by set another ViewManger as you commented.

If the new Efl.Ui.ActiveView.Stack class inheriting Efl.Ui.ActiveView.Container is beyond this patch, then please you do not need to consider it. I will request a new patch for it later.

Okay, lets say we overwork this one. Can you accept the previous ones, so they can land ? :)
This revision here will stay, and we overwork the approach here.

I am not in favor of a new class inheriting from active_view, just with 2 more APIs push and pop. Push and pop do look general useful to me, and belong IMO either on the Efl.Ui.Active_View.Container class, or like here in a util class.
I think its a bad idea to add a new class because:

  1. There is no real difference in the functionallity the class provides, there are just two more util functions that do not change anything in terms of the internal state.
  2. A function here in this util class like "stack_gen" could just return a Active_View.Container with VIEW_MANAGER_STACK_CLASS as view_manager. Then we would not need another type/class in our tree.
  3. A way more performant and IMO better way for all this can be implemented when its in Active_View.Container.

The Push and Pop behavior can be already implemented by setting the gravity property to index, setting the active_index to 0, and then doing "Push" with pack_begin and "Pop" with unpack_at(0) (all this is documented in the new docs).

If there are animation issues with the above approach, I would rather fix them than adding new classes (inherited or utilities), because it looks like we have almost everything we need already.

@segfaultxavi

As you mentioned, we have animation issues on pop with only using gravity. Because the unpacked view cannot be in the pop animation.

I am afraid it is difficult to support pop animation without adding new methods/properties or supporting new classes.

@segfaultxavi

I thought more about your suggestion about fix.

And I realized that it would be fine if the unpacked view is waiting until transition is finished if the the unpacked view is active view (active_index).

I will test it!! Thanks :)

@segfaultxavi

I tested it and I found a problem.
As you know, unpack does not delete the content so the content should be deleted by developer manually.
Therefore, it becomes the same as developer registers callback for transition finish and delete the content manually ;(

To support push/pop, I write specification for push and pop.
(The names, push and pop, can be changed)

push

  • sets active index to the pushed view (packed view)
  • animation can be controlled to be displayed or not (This can be controlled by "animation_enabled" of View_Manager)

pop

  • sets active index from popped view (unpacked view) to another view
  • can control to remove the popped view (unpacked view) after pop is finished
  • The number of popped view can be more than one (i.e. pop_to() pops multiple views)
  • animation can be controlled to be displayed or not (This can be controlled by "animation_enabled" of View_Manager)

@bu5hm4n @segfaultxavi
How about we add active_view_pack and active_view_unpack methods to Active_View.Container?
If so, active_view_gravity is removed. (we only use content gravity policy)

active_view_pack {
   params {
      view : Efl.Gfx.Entity;
   }
}
active_view_unpack {
   return: Efl.Gfx.Entity;
}
@property active_view_unpack_autodel {
   [[This flag can be used to delete view unpacked by active_view_unpack.
     If the autodel is true, then active_view_unpack deletes unpacked view after transition is finished.]]
   values {
      autodel: bool;
   }
}

If we support above methods, then the C# API would be PackActiveView() and UnpackActiveView(). They look good to me :)
We can add some more methods for convenience.

active_view_pack_at {
   params {
      view : Efl.Gfx.Entity;
      index : int;
   }
}
active_view_unpack_at {
   params {
      index : int;
   }
   return: Efl.Gfx.Entity;
}
active_view_unpack_to {
   [[Unpack views from the current active index view to the given index view]]
   params {
      index : int;
   }
   return: Efl.Gfx.Entity;
}

I think we can decide if active_view_pack packs the given view to the beginning or to the end. (also active_view_unpack)
If we synchronize with other containers such as box, then we need to pack it to the end.

What do you think about adding the above methods to Active_View.Container?

bu5hm4n added a comment.EditedWed, Jun 5, 4:57 AM

What is active_view_pack / active_view_unpack doing ?

What is active_view_pack / active_view_unpack doing ?

active_view_pack packs an active view so the packed content becomes an active view (active index's view).
active_view_unpack unpacks the active view so the active view (active index's view) is unpacked.

pack and unpack simply packs and unpacks the content but active_view_pack and active_view_unpack packs and unpacks the active view (active index's view).

I have thought about APIs for push/pop and I think that the following 3 methods are sufficient for our push/pop usecase.

active_view_pack {
   [[Adds a new view to this widget and sets the new view to be the active view.

     The new view is added to the beginning of this widget.
   ]]
   params {
      view : Efl.Gfx.Entity; [[View to add and set to be the active view.]]
   }
   return : bool; [[$false if $view could not be packed.]]
}
active_view_unpack {
   [[Removes the active view from this widget.

     The active index is not changed.
     e.g. The active index is 0, then the new active index is still 0.
     If the active index is removed, then it is changed.
     e.g. There are 2 views and active index is 1, then index 1 is removed so the new active index is 0.
   ]]
   return : bool; [[$false if there is no view in this widget or the active view couldn't be removed.]]
}
@property active_view_unpack_autodel {
   [[This flag can be used to delete view unpacked by active_view_unpack.
     If the autodel is true, then active_view_unpack deletes unpacked view after transition is finished.
   ]]
   values {
      autodel: bool; [[ $true if $active_view_unpack deletes the active view automatically.]]
   }
}

What do you think about the above?

For the usecase pop multiple views, we can do efl_pack_unpack multiple views and do efl_ui_active_view_unpack().

//we have 0, 1, 2, 3 index views and active view index is 0 and want to delete 0, 1, 2 index views
efl_pack_unpack(index 1's view);
efl_pack_unpack(index 2's view);
efl_ui_active_view_unpack();

That works for me, I will implement those APIs later today and test them and look over them at the weekend. We then can merge it on Monday. Thank you for putting time into it :)

To be clear: I will implement those APIs on the efl_ui_active_view_container object. I still think it should not be a separated object. (But I will also provide helper methods in until to get a stack like object)

bu5hm4n updated this revision to Diff 22672.Fri, Jun 7, 10:04 PM

implement active_unpack / active_pack

@Jaehyun_Cho I have implemented this now. I basically took your proposal just with three different things :

  • There is no method for autodeleting something, it feels weird to me to have that in a property, having it in a parameter sounds more sensible, since the usecase "I want to delete the content" is IMO not really bound to the widget, rather to a special situation, so it might be different from time to time.
  • The functions do not return true / false anymore. Reason for this is: The API is async, it triggers a animation, that might be immidiatly done, but it also might take a few frames to finish, that means, telling the user *yeah everything worked* while not have the animation / deletion finished, is not something i would do
  • The unpack method now returns an promise. The promise will be resolved once the animation is done, carrying either the Efl.Gfx.Entity (if autodel is false), or simply empty when the autodel is true. In this way the user has a easy way to find out when the operation is done. Without the need to register to the transition,done event, and unregister from it again. (I will add cedric as an reviewer, to verify that i did not do any mistakes here ... :))

@bu5hm4n

Thank you for the implementation! :)
I haven't used future on C# bindings so I think I need to check it on C# as well.

I have been trying to test the C# stuff myself, it seems that it does not work, since Object is not understood by the eina_value implementation of C#, we can say we keep it for now as a promise, the initial proposal did not address that feature anyways, so the main-thing that was requested is there i guess. Right now a user can still get the correct point of time, when he is listens to the next transition,end event ... :)

@bu5hm4n
Basically, I agree with you. I think that the returned future is good.
However, I am concerning if it is available to use EINA_VALUE_TYPE_OBJECT in EFL C# before we release EFL C# bindings publicly.
If the future is returned by active_view_unpack(), then developers want to know what is the eina_value in their eina_future_then callback. (now the Eina_Value argument cannot be used as an object in C#)

@felipealmeida @vitor.sousa
For now, EINA_VALUE_TYPE_OBJECT is not supported in EFL C#. (not supported in eina_value.cs in manual bindings)
What do you think about supporting EINA_VALUE_TYPE_OBJECT in EFL C#? (supporting in eina_value.cs in manual bindings)

I think we should definitely support the object type in the Rina value, mvvm also uses this kind of stuff afair. If it turns out that we don't need it, then I can still update this patch :)

Jaehyun_Cho accepted this revision.Wed, Jun 12, 5:33 AM

@bu5hm4n
I see~ You are right we have time to change before release (although it might not be that long! :( ) Thank you :)
Just a rebase is needed to push this~

Do we need to wait for @cedric 's comments on the usage of future/promise in this patch?

This revision is now accepted and ready to land.Wed, Jun 12, 5:33 AM

@cedric can you say something about the promise usage ?

segfaultxavi requested changes to this revision.Thu, Jun 13, 3:14 AM

I also think adding push & pop to the container makes sense.
I do not like the names active_view_pack/unpack though. They can be confused with the normal pack/unpack from the Efl.Pack_Linear interface.
I think the names push & pop are perfectly understood by everybody, why not use active_view_push/pop? It's less confusing.

I like the Eina_Future, but I think we are unnecessarily complicating things. Why don't we just return the view immediately from pop (just like unpack does), but we keep an extra reference until the animation is finished?
As you have found, support for Eina Futures and Values in C# is still incomplete.

The gravity property needs to be removed if we use these new two methods.

Needs rebasing.

src/lib/elementary/efl_ui_active_view_container.eo
20

Please add this text after line 20:

Adding views through the @Efl.Pack_Linear methods does not change the currently active view (@.active_index). If you want to add a view and immediately activate it you can use @.active_view_push for convenience.
61

This property needs to be removed.

83
Packs a new view at the beginning and sets the @active_index to 0.

This is the same behavior as a push operation on a stack.

An animation might be triggered to make the new active view come into position.
92
Removes the active view from the widget.

The views behind it naturally flow down so the next one becomes the active view. This is the same behavior as a pop operation on a stack.
When combined with @.active_view_push you don't have to worry about @.active_index since only the first view is manipulated.

An animation might be triggered to make the new active view come into position and the old one disappear.

The removed view can be returned to the caller or deleted (depending on $delete_on_transition_end).
100

I prefer delete_on_transition_end.

102

Missing doc for the return value, but see my comment regarding Futures first.

This revision now requires changes to proceed.Thu, Jun 13, 3:14 AM

After talking with @bu5hm4n I now understand we cannot keep an extra ref to the view while animating, because the user can do all sorts of strange things with it.

But still, I would like to avoid using Futures. @Jaehyun_Cho, would it be an option for you if active_view_pop did NOT return the popped view, so it is always deleted after animation?
I think this is by far the most usual case, and it makes sense to make it simple. If any user wants to push and pop views and still use them after popping, he can do it manually (with pack/unpack, active_index, and listening to the transition,end event.

@Jaehyun_Cho Can you answer if you are happy with pop / push ?

@segfaultxavi in all honesty, not returning the promise is not an option here, if we require that in future we need a new API, which basically does the same. It is perfectly possible right now to do that, bindings do lack this feature, but they should add that.

@bu5hm4n I'm not arguing against using a Promise, I'm just trying to see if there are alternatives :)

So, if we DO return a promise containing the popped view, and the user does not care about this view (I think this would be the most usual case), what does the user need to do? Does he need to attach a callback to the promise just to unref the view? What happens if he just ignores the returned promise?

@segfaultxavi this depends on the parameter, when delete_on_transition_end is true, then the user does not need to care about the value of the promise, only the point of time. When the user passes false as argument, then the user *needs* to handle the value of the promise. However, you have to note here, that when the user does not *want* to take care of that returned value, the widget that got unpackt will just float arround on the UI, which is probebly not what a user wants ... :)