Page MenuHomePhabricator

Introduce Efl.Ui.Item_Container
Needs RevisionPublic

Authored by bu5hm4n on Thu, Jul 11, 8:11 AM.

Details

Summary

this is a new widget which aims to replace Efl.Ui.Grid / Efl.Ui.List.
The widget is split up in a widget and a interface for item placement.

Efl_Ui_Item_Position_Manager: The interface contains API which is used
by the Item_Container to place the items, there is also a set of common
tests which tests for the casual tripping wires, and ensures that the
events are emitted in the correct moments (the later part still can be
improved)

Efl_Ui_Item_Container: The widget itself, it contains the API for the
enduser to add Items to the widget, it handles the different modes for
selection type and emits the events for selection changes. The pack API
is conform with the spec unit test. An additional set of tests is
defined which should be able to be run on every widget with a specific
position_manager beeing set.

Depends on D9325

Test Plan

There are 2 elm_test tests. And the list example was adjusted.

Diff Detail

Repository
rEFL core/efl
Branch
devs/bu5hm4n/work_container
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 12274
There are a very large number of changes, so older changes are hidden. Show Older Changes
bu5hm4n requested review of this revision.Thu, Jul 11, 8:11 AM
bu5hm4n edited the test plan for this revision. (Show Details)
bu5hm4n edited the summary of this revision. (Show Details)Thu, Jul 11, 8:14 AM

Not to sure of the naming. Efl.Ui.Item_Container seems wrong. What other kind of Efl.Ui.Container would we have that would contain something different? It sounds like a boat for water kind of naming. Not really the most important bit, but that is going to take some time to discuss and find a good name.

On the technical side, I am not sure how I am going to implement the accessor for items as they will be build asynchronously on demand, I wonder how to notify the position manager that they now exist while they didn't before. I am guessing that the best approach for the CollectionView will be to use custom accessor that bind to the data model and the local storage for widget. It seems doable at this point.

src/lib/elementary/efl_ui_item_position_manager.eo
18

Is that what we described as the size cache?

47

I do not understand clearly the relation between the accessor and this add/remove function here.

SanghyeonLee added a comment.EditedThu, Jul 11, 8:22 PM

well then we could think about another names,
itemBox, itemWidget, itemScroller(?) itemCollection, itemLayout, itemController...
but most of these also have same problem...

and @bu5hm4n could you please put @beta in itemPositionManager?

On the technical side, I am not sure how I am going to implement the accessor for items as they will be build asynchronously on demand, I wonder how to notify the position manager that they now exist while they didn't before. I am guessing that the best approach for the CollectionView will be to use custom accessor that bind to the data model and the local storage for widget. It seems doable at this point.

Technically the accessor can return NULL for a element that is not ready yet, once its ready, (and can be accessed via the accessor) you call item_size_changed the two backend then fetch the size, and reapply the items to it. In case there is no item in something, 0,0 should be assumed as items size (Not tested right now).

Because of the name: This is a working title, i am happy to hear better ideas.

bu5hm4n updated this revision to Diff 23311.Fri, Jul 12, 2:36 AM
bu5hm4n edited the summary of this revision. (Show Details)

add grid position manager and tests

add beta

bu5hm4n updated this revision to Diff 23351.Sun, Jul 14, 2:00 AM
bu5hm4n retitled this revision from WIP: Introduce Efl.Ui.Item_Container to Introduce Efl.Ui.Item_Container.
bu5hm4n edited the summary of this revision. (Show Details)
  • more tests
  • feature complete
  • Most features tested

The widget meanwhile is feature complete, there are tests for the grid position manager and list position manager. The tests for now are only plain copies. Given the complexity of this widget, there will 100% be more bugs to find, and things to clarify, so I would be very happy if people could write examples for that, play with the API, do things that i did not think of. Or simply play around in the tests. Thank you :)

bu5hm4n updated this revision to Diff 23353.Sun, Jul 14, 10:05 AM

forgot a few files

bu5hm4n updated this revision to Diff 23386.Tue, Jul 16, 2:13 AM
bu5hm4n edited the summary of this revision. (Show Details)

more fixes better tests in elm_test

bu5hm4n updated this revision to Diff 23431.Wed, Jul 17, 8:01 AM

fix scroller bouncing

bu5hm4n updated this revision to Diff 23432.Wed, Jul 17, 8:23 AM

add multiselect radio

@woohyun @SanghyeonLee @zmike @segfaultxavi

I would like to merge this widget at the latest on the 27.07.

For now it's feature compatible with list / grid. I think we need to work a bit more on the items, but that is unrelated to this revision. There is right now a bunch of bugs: jumping of scrollbar, invalid arrow states. Those are in pan / scroll_manager, I will take a look at them once this is fine and ready to go.

A new name for this can also be decided, however, I don't see the name as a crucial *first-thing-to-land* requirement. I can handle that ONE rename.

segfaultxavi requested changes to this revision.Thu, Jul 18, 4:20 AM

I've made quite a few comments on the API and the docs... make sure to mark them as done so none is forgotten!

src/lib/elementary/efl_ui_grid_position_manager.eo
1

If Efl.Ui.Grid_Position_Manager is an implementation of Efl.Ui.Item_Position_Manager I would expect it to contain the words Item_Position_Manager.
How about Grid_Item_Position_Manager or Item_Position_Manager_Grid?

3
Implementation of @Efl.Ui.Item_Position_Manager for two-dimensional grids.

Every item in the grid has the same size, which is the biggest minimum size of all items.
src/lib/elementary/efl_ui_item_container.eo
11
This widget displays a list of items in an arrangement controlled by an external $position_manager object. By using different $position_manager objects this widget can show lists of items or two-dimensional grids of items, for example.

Items inside this widget can be selected according to the $select_mode policy, and retrieved with $selected_items_get.
26

So this method is like item_scroll, but you have the chance to specify where in the viewport the item will appear?
If so, this needs to be clarified (what alignment does item_scroll use?)
Or better yet, remove item_scroll, use default values for item_scroll_align, and rename it to item_scroll.

29

How does align work for grids? You can only align along a diagonal line through the viewport? that's weird...
But the only thing I can think of is that you provide here an array of align values, and the length of the array depends on the Position Manager.... ugh.

34

The item that was selected most recently.

45

User has to free the iterator after usage.

48

Position manager object that handles placement of items.

82

A $press event occurred over an item.
Same for all events below.

src/lib/elementary/efl_ui_item_position_manager.eo
10

A bit confusing... I would call it just data_set and make it a method instead of a property.
The docs could say: Sets the accessors that the object will use to retrieve the list of items and provide their calculated sizes. (if I understood correctly).

17

Just to be sure, the caller allocates the storage, passes this accessor, and this object fills that storage with the calculated item sizes, correct? This needs to be clarified.

21

Again, I would use a method viewport_set, since we have no getter.
What coordinates are used?
Items outside this viewport will not be shown.

31

What does this mean? That if we have 10 items this value goes from 0 to 10?
If I set scroll_position to 10, item #10 will be at the top of the viewport or the bottom?

41

What?
So this is like item_scroll in the container? it brings the requested widget into the viewport?
How about the alignment option that item_scroll_align had?

47

This is just telling us that there have been changes in the accessor and we need to recalculate, right?
Then this has to be explained: The accessor provided through @.data_access will contain updated items.

Also, have you considered using events emitted from the container to replace item_added, item_removed and item_size_changed?

54

The item $subobj previously at position $removed_index has been removed.

69

Emitted when the aggregate size of all items has changed. This can be used to resize an enclosing Pan object.

70

And why is this useful? I do not understand the use case.

src/lib/elementary/efl_ui_list_position_manager.eo
1

Same comment regarding the name as the Grid position manager.

This revision now requires changes to proceed.Thu, Jul 18, 4:20 AM
bu5hm4n added inline comments.Thu, Jul 18, 4:43 AM
src/lib/elementary/efl_ui_grid_position_manager.eo
1

I feel like i want to die every time i would have to write this name. A namespace would solve that way better IMO.

src/lib/elementary/efl_ui_item_container.eo
26

Agreed.

29

I am not sure what you mean, but:
Even if something is a grid, the item has a overall fixed position within the set of items that are positioned in the item_container.

The align here is an indicator on where the endposition of the new viewport position should be. And that position is defined by a one dimensional value. (We have 2D on the screen, and a orientation, which leaves 1 free Dimension).

src/lib/elementary/efl_ui_item_position_manager.eo
10

I do not see any difference in method or property here. However, as long as its called something like "data_access" or "data" i will keep it a property, if you call it "setup" then i can make it a method. but "data_set" is like a total no-go, we make things a property when they end with _set or _get.

17

No it is the "accessor for the size" so ... you can use it ... to ... access the size of a item. The value is always valid (so you do not have to check if its really what you want). And it might be changed over time...
What exactly is unclear?

21

Canvas-Space like any other viewport property.
And for the same reason as above, this will stay a property.

31

Mhm, the value goes from 0 to 1.0 like documented 5 lines below.

1.0 means the last item, 0 the first item (details obviously depend on the item_position_manager)

41

No - this API should position the item (found with id $idx). at its position in the overall set. undependend from if its in the viewport or not.

(The geometry resulting from this call will be used by the scrolling API of item_container to know where to scroll).

47

events are too slow for that.

70

It is used to set the minsize in case match_content is true for the currently used orientation.

src/lib/elementary/efl_ui_list_position_manager.eo
1

A namespace would solve this way nicer.

SanghyeonLee added inline comments.Thu, Jul 18, 5:11 AM
src/lib/elementary/efl_ui_item_container.eo
26

actaully item_scroll_align cannot cover the item_align, we might need to add -1 case for that(but I hate to adding this kind of special key value and makes default case worse).
so the difference is that,
item_scroll_align will be bring the item in given aligned position of view screen.
item_scroll also bring the item, but only till item show in the view screen, so the place will be changed as scrolling directions.
so if you call the item_scroll before the item place, scroll will move down and item will be placed bottom of view screen.
but if you call the item_scroll after the item place, scroll will move up and item will be placed top of view screen.

29

exactly.
this alignment is only for scrolling axis.

45

I think we might need to combine select and multiselect expierience in some common interface,
I was proposed,

Efl.Ui.Selectable {ItemSelect .set .get; ItemUnselect}
Efl.Ui.MultiSelectable { inerit Selectable, ItemListSelect .set .get; Selected_Items .get}

src/lib/elementary/efl_ui_item_position_manager.eo
31

note, there is typo.

zmike requested changes to this revision.Thu, Jul 18, 9:59 AM

One issue I have with this is the bool: animation param for various methods. Typically we provide some options for this kind of thing like linear, accelerate, decelerate, ...

I'm not sure how we want to handle that, but I think it needs to be discussed.

src/bin/elementary/test_ui_item_container.c
19

This should be 1154.

Also a number of lines need wrapping because they go off screen even in my 160col editor.

src/lib/elementary/efl_ui_item_position_manager.eo
22

Wrap.

41

This needs line wrapping.

segfaultxavi added inline comments.Fri, Jul 19, 8:37 AM
src/lib/elementary/efl_ui_grid_position_manager.eo
1

The user would never have to write this name, since the idea is that both Efl.Ui.Item_Container and Efl.Ui.Grid_Item_Position_Manager would be hidden behind a handy Efl.Ui.Grid, no?

src/lib/elementary/efl_ui_item_container.eo
26

Thanks @SanghyeonLee, now I understand the difference.

However, is it necessary? I find that "bring this object into the viewport and put it anywhere you want inside the viewport" is not very helpful.
In fact, I find it very confusing in some other UI toolkits when they do this. For example, I search for a file name and the window scrolls, but there are still 100 files in the window and I have to visually search for the one I want.
I prefer we only use item_scroll_align and force the user to specify the alignment (or use 0.5 as a default value).

29

Thanks, understood. The docs then should say that this align value only applies on the scrolling axis.

src/lib/elementary/efl_ui_item_position_manager.eo
10

I can live with read-only properties, but write-only properties? I think a method is more suited for that. We have over 50 _set methods in EFL already.

17

It is unclear to me the direction of obj_access and size_access. Who allocates the storage? Who fills the information? Who frees the storage and the accessor?

21

I still don't like write-only properties. I even looked it up: https://stackoverflow.com/a/4695589

31

You're right @bu5hm4n, it's documented below, sorry.

HOWEVER, this is again a write-only property :)
Why can't this one have a getter?

As for the docs: Move the viewport inside the virtual space or something like that.

In the case of Lists, which of the two values do I have to use? depends on the orientation?

41

Sorry, if this does not move the viewport I do not understand what it does. What does "This is a request to position the item at id $idx. " mean?

cedric requested changes to this revision.Fri, Jul 19, 4:20 PM
cedric added inline comments.
src/lib/elementary/efl_ui_grid_position_manager.c
189

This line scare me. How many EFL_UI_ITEM_POSITION_MANAGER_EVENT_CONTENT_MIN_SIZE_CHANGED event are going to be emitted in a burst?

194

This would need to become more asynchronous.

src/lib/elementary/efl_ui_item_container.c
138

This is dangerous as we might end up with a list pointer that change after inserting/remove items. In general the entire duplication of logic here for the accessor is not great.

Why not just "double buffer" the access to an eina accessor on the list. The idea would be to have an internal accessor and an external one. The internal one is created everytime the external one is accessed and no internal one exist. The internal one can easily be destroyed every time the list change and avoid stalled pointer and code duplication.

src/lib/elementary/efl_ui_list_position_manager.c
40

This function is going to be a source of trouble in this design. We can not have something that require walking the entire list of item and compute something in a fully blocking synchronous way. I do a see a lot of different way to solve this. My current preference is to move this cache also to the same cache and have a structure that include:

  • Size
  • Offset
  • Version

The average item size and min item size is something the position manager can maintain, but in general, it is going to be fully asynchronous (Imagine having thousands of widget that are being sized in a job that trigger an update step by step and invalidate this cache over and over). When the cache is emptied, the idea would be to update the position manager version counter and reset the job (The version counter can wrap as long as it is not to fast to wrap and we end up back on our original number before we are able to at least regenerate the cache once).

My idea would be that cache_require become a job that should not take more than 1/3 of 1/60 of a second and update what it can. At some it would maybe help to have a n levels tree system where the offset is stored in the parent. This way you do need only to update a leaf block and all of the parent. With each level having 256 entries, it should be efficient and would be likely what we will aim for at the end.

I am fine if we do not land something that complex for an internal data structure, but cache_require need to be async otherwise it is going to be in the hot blocking path.

167

Isn't there a possibility that we jump to little ahead and need to walk further?

256

Thinking more about this, why do we need two accessors? If we do have one structure, we can add the Item Evas_Object inside that structure anyway.

283

Designing the cache to not be fully invalidated on item added/removed might be something to think of.

350

That one too. This would force a full cache recalc for each object being sized asynchronously by the ContainerView. Maybe a range version of this function would be neat.

src/lib/elementary/efl_ui_list_position_manager.eo
1

Having a namespace for Efl.Ui.Position_Manager might make sense actually.