Page MenuHomePhabricator

efl_ui_active_view: introduce a experimental new widget
ClosedPublic

Authored by bu5hm4n on Apr 29 2019, 9:30 AM.

Details

Summary

this widget tries to replace efl.ui.stack efl.ui.flip & efl.ui.pager
In general those widgets do the same thing, they get content. And
display them with some sort of animations. The new idea here is, that
the basic widget active_view only handles the ownership etc. of the
content that gets added to this. Then there is a view_manager object. The
view_manager object gets notified over the new contents, and requests for
displaying particular contents. The transition then handles those
things.

The version here is feature complete with Efl.Ui.Stack and Efl.Ui.Pager.
Additional features can be implemented in the corresponsing transition
classes. Examples and tests will follow

Diff Detail

Repository
rEFL core/efl
Branch
devs/bu5hm4n/work
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 11183
There are a very large number of changes, so older changes are hidden. Show Older Changes
Jaehyun_Cho added inline comments.May 23 2019, 5:18 AM
src/lib/elementary/efl_ui_active_view_view_manager.eo
45 ↗(On Diff #22259)

BTW, I am also not sure if this property is necessary or not.
Because we can use Plain if we do not want transition.

I have one concern about View_Manager.

As you know, in the commercialized application or UI framework, normally it is required to support complicated transition effect.
(e.g. combination of zoom in/out and fade in/out)

Can we support it easily?
Is there any way to replace or apply transition effect to View_Manager easily? (except adding a new view_manager eo file for the new transition effect)

The documentation changes will be addresses when Xavi goes over the docs and polishes them (happens bebfore this lands.)

  1. I cannot see where page is used as a term anymore in the API, it is used in the example (simply because its for now simply copied from pager) and its used once in the docs which will be fixed, so i don't really understand where you want to have the terminology "view" (...) ?
  2. Regarding you concern. It is the idea to create a new View_Manager object if you want to have a different animation to what is there right now. However there is some thought behind this. From a bindings POV this means a user can simply create a new class that implements View_Manager and pass it in. So its super easy to handle from the bindings side. On the other side, the API calls between Container and View_Manager are heavily tested, writing your own object there (and keeping it for yourself) does not do any harm, but rather improves the abstraction of your code.
src/lib/elementary/efl_ui_active_view_container.eo
40 ↗(On Diff #22259)

This is again something i thought to be requrested, you can use this to navigate through the Container in a stack like manner. You set it to a fixed index, and nothing changes this index, new content etc. is immidiatly displayed, this enabled and set to 0 enables a almost Efl.Ui.Stack experience...

But if this is not requested, then i can surly remove it again.

55 ↗(On Diff #22259)

I am a bit confused here, I thought you explicitly requested something like that above??.... This is honestly just an shortcut, a user could emulate this behaviour by listening to transition,end and then delete the previous active content...

src/lib/elementary/efl_ui_active_view_indicator.eo
15 ↗(On Diff #22259)

Same as below.

src/lib/elementary/efl_ui_active_view_indicator_icon.eo
1 ↗(On Diff #22259)

Not right now, this is more or less the exact copy of the pager indicator. If there are any requests for this indicator, let me know, that can surly be implemented in further patches.

src/lib/elementary/efl_ui_active_view_view_manager.eo
15 ↗(On Diff #22259)

The operations for the flag del must (in the optimal case) be the exact opposite, therefore i prefered it to keep this as one function, as this seems to be less error prone, since you see the relevant parts at one go.

27 ↗(On Diff #22259)

active_index_switch sounds to be like this object should change the active_index. And this is quite unrelated to the active_index, the active_index is just some index maintained by the container object. This function here does not need to take that property into account at all. It just gets the two IDs and can work from there. Maybe one day we need to call that seperated from active_index ? I don"t know. How about a name like "perform_switch" ? It expresses that there has to be a switch, but does not necessary link it to active_index.

36 ↗(On Diff #22259)

No, we could realize that with the size hints on a object. I am not sure this property needs to be changed for that ?

The documentation changes will be addresses when Xavi goes over the docs and polishes them (happens bebfore this lands.)

I see. As you said, you do not need to fix typo in documentation now but it would be greateful if you comment on documentation which I feel difficult to understand. (e.g. bind)
It would help me to understand better how methods and codes work :)

  1. I cannot see where page is used as a term anymore in the API, it is used in the example (simply because its for now simply copied from pager) and its used once in the docs which will be fixed, so i don't really understand where you want to have the terminology "view" (...) ?

As you said, "page" is not used in API names but "page" is used in all the documentations and codes.
Since "Active_View" and "active_index" are introduced in this patch, I thought it would be better to use the same term in the documentations and codes not to make developers misunderstand.
As you said above, you do not need to modify documentations now but I think we need to modify them eventually. Before Xavi's review, let's just keep the comment for the record :)

  1. Regarding you concern. It is the idea to create a new View_Manager object if you want to have a different animation to what is there right now. However there is some thought behind this. From a bindings POV this means a user can simply create a new class that implements View_Manager and pass it in. So its super easy to handle from the bindings side. On the other side, the API calls between Container and View_Manager are heavily tested, writing your own object there (and keeping it for yourself) does not do any harm, but rather improves the abstraction of your code.

I agree with your point.
It seems that a new class inheriting View_Manager should be implemented by UI framework if UI framework wants to provide a new transition effect :)

How about separating Efl.Ui.Stack features from Efl.Ui.Active_View.Container?

I think that now Efl.Ui.Active_View.Container is simple and neat enough.
But I think that if we add features for Efl.Ui.Stack to Efl.Ui.Active_View.Container (e.g. push/pop), then it would make Efl.Ui.Active_View.Container complex.

Like you suggested before, how about providing Util class or a new class inheriting Efl.Ui.Active_View.Container?
If so, the new class contains Efl.Ui.Stack's features such as push/pop and the matched View_Manager is set in the constructor.
Then, it would satisfy the needs for Efl.Ui.Stack and also keep Efl.Ui.Active_View.Container neat.

What do you think? :)

src/lib/elementary/efl_ui_active_view_container.eo
40 ↗(On Diff #22259)

I understand your intention.
As far as I understand, this property is to control indicator update.
But Efl.Ui.Stack usecase does not require indicator up to now. So I think this property is not necessary for now.

55 ↗(On Diff #22259)

How about separating Efl.Ui.Stack features from Efl.Ui.Active_View.Container?

i.e. Like you suggested before, we can provide Util class or a new class inheriting Efl.Ui.Active_View.Container.

What do you think about it?

src/lib/elementary/efl_ui_active_view_view_manager.eo
15 ↗(On Diff #22259)

My first impression of this method name, it sounds like the existing content is updated by this API.
However, it seems that this API adds/deletes the given sub object to view manager object.

I think this is a case similar to add/del or pack/unpack.
We can make one API for add/del and pack/unpack with extra parameter (e.g. deletion) but we normally do not.
I think this conception can be adopted in this case as well.

Moreover, I think that separating this API makes less misuse because we do not need to remember the extra parameter is deletion or addition.

BTW, it seems that the parameter index is not used anywhere. Can we remove the parameter index?

27 ↗(On Diff #22259)

I understand your point so you mean that active_index is a conceptual thing used in container.
I agree with you.

Then, if I understand correctly, request_switch switches the currently displayed view from "from" view to "to" view.
Eventually, the "to" view becomes the currently displayed view.

If so, then I think that the purpose of this API is to change currently displayed view.
So I think we need to define a word or term for the currently displayed view here to use it in the API name for better understanding.
I thought that active_index would mean the currently displayed view.

About your suggestion "perform_switch", I like the word "switch" :) but I also want to add a word which contains "currently displayed view".

If there is no proper word for "currently displayed view", then I think "transit" or "view_transit" can be another option.
(Like originally this was transition object :))

36 ↗(On Diff #22259)

I haven't tested yet, but if we do not call this API, then is it able to display each view with its own size? (i.e. geometry or size hint)

segfaultxavi requested changes to this revision.May 24 2019, 4:46 AM

Fails to build for me because of implicit declaration of function ‘ck_assert_double_eq’ (Old version of libcheck, as usual).

cedric requested changes to this revision.May 24 2019, 10:22 AM

I have serious concern with the use of View in the naming as this clash with the View in the concept of MVVM (Model View ViewModel). And View is a UX class. I am bad at naming, so I don't have a great proposal for renaming, but Active_View seems bad to me.

With the next update most of the function-wise things are addressed. (Only thing left, is the switch_to thing) Xavi is working on the docs. So if you are happy with the general direction this takes, i would be happy to merge this on the 29.05. If there are other things, i am happy to patch them afterwards, but maintaining such a big patchset is quite a pain. Additionally: I really appreciate that things like "please use active_view instead of page", and i think this is a correct step into the right direction. However, I updated now my revision in this direction, and so i expect that other commit that come into phabricator or directly master also full fill this quality. Same applies btw. for documentation (strongly looking into the direction of efl_ui_grid / efl_ui_list / efl_ui_list_view).

src/bin/elementary/test_ui_active_view.c
428 ↗(On Diff #22259)

This test is just copied from pager, and adjusted a little bit, i will go with this and adjust this when we are working on check.

464 ↗(On Diff #22259)

As above

468 ↗(On Diff #22259)

As above

477 ↗(On Diff #22259)

As above

499 ↗(On Diff #22259)

As above

src/lib/elementary/efl_ui_active_view_container.eo
40 ↗(On Diff #22259)

This has nothing to do With indicator at all. The gravity is here in order to disable / enable the index manipluation.

So lets say you have 3 contents, and active_index is at position 1. now you add something at the beginning. If gravity is INDEX, then the active_index is still 1. If gravity is CONTENT, then active_index is 2.

55 ↗(On Diff #22259)

Okay, i moved this to a util class in the stack removal commit. But for the sake of getting anywhere here, can we please try to stick with the proposals we have written above?

src/lib/elementary/efl_ui_active_view_view_manager.eo
15 ↗(On Diff #22259)

The case is quite different to add/del or pack/unpack. add / del pack/unpack is about taking ownership, setting widget_parent relation. This here is just an notifier that the set of widget has changed. I can make the boolean flag a enum "State_Change" which has addition or deletion ? So its obvious ?

The index here can be used to address the position of the content directly. This is usefull as you don't have to do the backwards search if you need the index. This safes performance in case the list is quite huge.

27 ↗(On Diff #22259)

Okay, first of all: We moved away from the word transition, i am strongly against naming something again transition.

How about we name it simply swich_to ? Its part of the View_Manager, so its clear that it is about switching a view. And noone except Efl.Ui.Active_View.Container will call that, so i think this is fine ?

36 ↗(On Diff #22259)

Not right now - this would need to be implemented. As this is also not possible with either stack nor flip nor pager. We can surly implement that. However, i would prefer to not do it right now, as this already took too long here.

45 ↗(On Diff #22259)

This property is here for the sake of performance helps. So for example when a object is not finalized, its view_manager also does not need to animate things. And this object can just straight away display things.

I would not call it transition, as animation also includes the cases where animations are done based on adds or removals. Transition would only include the move from one index to another.

bu5hm4n updated this revision to Diff 22439.May 25 2019, 4:05 AM
  • make tests work without double cmp
  • rename internals that still were called pager
  • rename a API
  • remove delayed deletion again

@segfaultxavi this is now fixed.
@cedric This widget is not called view alone but rather Active_View Which refers to the widget as beeing some view to data. Which goes hand in hand with MVVM. And as you pointed out UX is a View class, so is this here. Additionally, this name was the result of 2 days discussion, and i don't feel like i will go back to this discussion, maybe we can go back when we are stabilizing this. But before, i would just merge it with this name. I am really tired of searching names with people, without even knowing if others are happy with the basics.

bu5hm4n updated this revision to Diff 22444.May 25 2019, 4:42 AM

fixed updates from master

I admit I share @cedric's concerns regarding the name View, because of the whole MVVM topic, and because we already have other "Views" in the tree (like Efl.Gfx.View).

The work on this new widget has already lasted too long and I understand everybody is tired, specially @bu5hm4n.

However this is going to be an important widget, because it will remove a lot of code from other widgets, so I think it is worth doing it right.

I ask all the people that has been involved so far (@bu5hm4n, @Jaehyun_Cho, @zmike, @cedric, @Hermet and myself) to make a final effort. We only need a name we all agree, and I will take care of the rename and the documentation.

I make yet another proposal, Booklet, because it is very easy to understand: it is a "collection" of "pages".
The classes would be Efl.Ui.Booklet.Container (active_page, page_index, ...), Efl.Ui.Booklet.PageManager (page_size, ...), Efl.Ui.Booklet.Indicator.

On Android this widget is called ViewGroup (including the StackView widget for example), but on Gtk it is called Notebook.

Please say Yes or No, or provide alternatives.

@bu5hm4n
Thank you for your work! :)
I replied your comments.
If this discussion delays this patch inefficiently, then let's discuss later. I shall request new patches later after this patch.

src/lib/elementary/efl_ui_active_view_container.eo
40 ↗(On Diff #22259)

Thank you, now I fully understand this functionality and why you added this :)

However, since we are going to provide push/pop in Util class, I think we do not need this property.

55 ↗(On Diff #22259)

Thank you so much! :)

src/lib/elementary/efl_ui_active_view_view_manager.eo
15 ↗(On Diff #22259)

In my point of view, enum and boolean are the same. If you strongly insist this form, then please leave it as boolean because this has 2 options. If I have a new better idea, then I will request a new patch.

About index, I understand what you mean, but I think it would be better remove it.
Because 1. now we do not have the case that index is used (except the test case).

  1. If caller does not have the index information, then it should search the index of the subobj anyway. It would be a burden as you mentioned if the list is huge.
27 ↗(On Diff #22259)

Then, IMO, "switch" is better than "switch_to" because "switch_to" sounds like it does not require "from" parameter and it sounds like it switches from the current state to the given "to" state.

45 ↗(On Diff #22259)

I see. Then how about "animation_enabled"?
Because property "animation" sounds like an animation instance should be assigned.

bu5hm4n updated this revision to Diff 22462.May 27 2019, 10:34 AM

rename API

I think discussion is important for the basics. But in the same moment i think the basics are quite fine now. So if this now can be merged, then your people can test it, we can leave it at rest for 1-2 weeks and revisit if we see anything after some time of doing something else. We can still add / change things :)

I now splitted content_update, since this looked like you really wanted to see it this way. However, i want to keep the index, as this really looks like a good performance indicator. You might want to change in some points if the added / removed content is next to the current displayed. And if this is the case, you want to perform some special animations. On the same time i don't really see it doing any big issues :)

With this beeing updated and the reasoning given inline about swtich vs switch_to. Are you fine with this ? :)

src/lib/elementary/efl_ui_active_view_container.eo
40 ↗(On Diff #22259)

I think we should rather keep it. Think about this:

  • A box which collect notifications
  • New notifications come in at the beginning, and can just be added with efl_pack_begin
  • it will work automatically, while you can scroll through them, which would not be in the style of push and pop.

I think we should keep this for now. This is here, and was considered usefull from xavi and me...

src/lib/elementary/efl_ui_active_view_view_manager.eo
15 ↗(On Diff #22259)

I don't understand your reasoning here. If we do not have the index, then we put the burdon of searching for the index on the user. But the Active_View.Container can provide the correct index without searching the list. This is extremly usefull for situations where you have the current index and the inserted one, now you can compare if this is right of or left of the current view.

27 ↗(On Diff #22259)

switch is not possible because its a keyword in c++. I think switch_to is decent enough. It makes sure that you will get the "to" parameter passed. And since we are in the area of navigating between linear indexes, it should be quite clear that from is the current state.

45 ↗(On Diff #22259)

Sure, will rename it to this.

I do still think View is a conflictual key word that is expected to mean something like View in MVC, MVVM and friends in any other language/framework.

SanghyeonLee added a subscriber: SanghyeonLee.EditedMay 27 2019, 6:55 PM

I do still think View is a conflictual key word that is expected to mean something like View in MVC, MVVM and friends in any other language/framework.

agreed.
what we expected by using name "view" is it sub-class of Efl.Ui.View and the component of MVVM pattern which cannot be stand-alone without model.
using same name widely over the framework can be confused for user...

As for this widget is replacement of Efl.Ui.Stack,
I suggest,
Efl.Ui.Active_Stack
Efl.Ui.Active_Page
Efl.Ui.Active_Frame from the Navi-Frame..
Efl.Ui.Active_Layout
as for each stack is layout... if not we should avoid this name either..

@segfaultxavi

I make yet another proposal, Booklet, because it is very easy to understand: it is a "collection" of "pages".
The classes would be Efl.Ui.Booklet.Container (active_page, page_index, ...), Efl.Ui.Booklet.PageManager (page_size, ...), Efl.Ui.Booklet.Indicator.

Please say Yes or No, or provide alternatives.

Thank you for suggesting :) IMO, the name Booklet is good for a widget class name. Personally, I think it would be better we use the Booklet as a widget class name.
BTW, I think PageManager is the best name for the purpose.

I discussed about naming with @SanghyeonLee and we thought that "page" is the best alternative for "view".
Our suggestion is as follows. (with @segfaultxavi 's PageManager)

  1. Efl.Ui.Pager / Efl.Ui.PageManager
    • This is like Scroller / ScrollManager
    • There is no namespace for page.
  1. Efl.Ui.PageContainer / Efl.Ui.PageManager
    • There is no namespace for page.

Please tell me your thoughts about the above suggestions.

I liked Pager from the beginning, but there's a widget already called like that. What would you call the current Pager widget?

bu5hm4n added a comment.EditedMay 27 2019, 11:20 PM

I would really prefer to keep the namespace, efl.ui already looks like a mess, due to so many widgets and non-widgets. I would rather prefer grouping the necessary class objects here under one single namespace.

I pretty much dislike pager here, you can do whatever you like in the View_Manager, you can even do animations like the android homescreen or something, that does not really remind anyone of a book or booklet.

@SanghyeonLee I cannot understand your reasoning at all. Every single widget implements the view interface, so infact, every widget *is* a view. Additionlly, view is used in other parts of EFL, so a user does need to look at the context anyways.

@segfaultxavi the pager widgets gets removed later on here.

cedric accepted this revision.May 28 2019, 9:27 AM

I think we are unable to find a better name here. Let's land it and maybe by the time we do a release someone will have found a better name. @bu5hm4n can you just create a task for next release to remind us of reviewing the naming of this classes?

I will create stabelization tickets for all types added here

segfaultxavi resigned from this revision.May 28 2019, 10:54 AM

I agree this patch must be unblocked. Don't forget to review the code, though!

I have no issues with the documentation since I am already writing an improved version (to be submitted later).

Jaehyun_Cho accepted this revision.May 29 2019, 1:11 AM

@bu5hm4n Thank you for the patch! :)

This revision is now accepted and ready to land.May 29 2019, 1:11 AM
Jaehyun_Cho requested changes to this revision.May 29 2019, 3:31 AM

Please delete callbacks in invalidate.
(This can be tested on D8906)

src/lib/elementary/efl_ui_active_view_view_manager_stack.c
123 ↗(On Diff #22462)

This callback should be deleted in invalidate.

124 ↗(On Diff #22462)

This callback should be deleted in invalidate.

This revision now requires changes to proceed.May 29 2019, 3:31 AM
bu5hm4n updated this revision to Diff 22491.May 29 2019, 5:18 AM
  • fix leaking event callbacks
  • reset progress, (this prevents weird animations)
Jaehyun_Cho added inline comments.May 29 2019, 5:20 AM
src/lib/elementary/efl_ui_active_view_container.c
398 ↗(On Diff #22491)

could you check if the pd->curr.page is the same with the given index here?

bu5hm4n added inline comments.May 29 2019, 5:23 AM
src/lib/elementary/efl_ui_active_view_container.c
398 ↗(On Diff #22491)

The reason i do want to allow the same setting of active_index twice is, that when a view_manager allows user interaction, it is super usefull to just "flush out" the current position rounded to an integer, and let the animation code deal with the rest, this safes a lot of states when you think of the view_manager as a state maschine.

Additionally, this also is usefull when some code interupts a user animation, as this is basically the same.

With my latest diff, the bug you saw should be gone.

Jaehyun_Cho added inline comments.May 29 2019, 5:25 AM
src/lib/elementary/efl_ui_active_view_container.c
398 ↗(On Diff #22491)

Thank you! I will double check the test and finish the review :)

bu5hm4n updated this revision to Diff 22495.May 29 2019, 6:08 AM

remove clip-rects

Thank you! :)

Just one more thing about view_manager_set with NULL case.

src/lib/elementary/efl_ui_active_view_container.c
589 ↗(On Diff #22495)

If we allow NULL for view manager and if we assume PLAIN as a NULL, then how about as follows?

if (transition)
  EINA_SAFETY_ON_FALSE_RETURN(efl_isa(transition, EFL_UI_ACTIVE_VIEW_VIEW_MANAGER_CLASS));
else
  transition = efl_add(EFL_UI_ACTIVE_VIEW_VIEW_MANAGER_PLAIN_CLASS, obj);
bu5hm4n updated this revision to Diff 22604.May 30 2019, 1:47 AM

insert PLAIN intead of NULL

Good Catch,

With this done, i have submitted https://travis-ci.org/Enlightenment/efl/branches. When this build succeeded, i will land the 3 accepted revisions, okay ?

Jaehyun_Cho accepted this revision.May 30 2019, 2:34 AM

Good Catch,

With this done, i have submitted https://travis-ci.org/Enlightenment/efl/branches. When this build succeeded, i will land the 3 accepted revisions, okay ?

Yes, please :)

Thank you so much!

This revision is now accepted and ready to land.May 30 2019, 2:34 AM
Closed by commit rEFL79fe0121eec5: efl_ui_active_view: introduce a experimental new widget (authored by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain WhyMay 30 2019, 2:48 AM
This revision was automatically updated to reflect the committed changes.