Page MenuHomePhabricator

introduce efl_ui_tab_bar_default_item
ClosedPublic

Authored by bu5hm4n on Fri, Aug 23, 10:46 AM.

Details

Summary

Tab bar used a direct layout to implement all this functionality by its
own. However, it seems that we can reuse a lot of object functionality
that is already part of efl.ui.item. With this commit the features that
are not part of Efl.Ui.Item are moved to Efl.Ui.Tab_Bar_Default_Item.

The tab bar is changed in a way that you do not need to pass the icon
and label by hand anymore, you can rather just pass the tab_bar default
item. Additionally, the item for a tab_page can now directly be
generated from a page.

This is the first commit in order to cleanup efl_ui_tab_bar, tab_page &
tab_pager. The goal is also to implement our interfaces for inserting
elements on those objects. So our common test suites can also be used.

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.Fri, Aug 23, 10:46 AM
bu5hm4n requested review of this revision.Fri, Aug 23, 10:46 AM
segfaultxavi requested changes to this revision.Tue, Aug 27, 2:31 AM
segfaultxavi added inline comments.
src/lib/elementary/efl_ui_tab_bar_default_item.eo
3

Missing docs.

Come on! Give me something I can use to write proper docs later on!

src/lib/elementary/efl_ui_tab_page.eo
26

I don't fully understand how this works, but it looks a bit weird that you have to pass a "default" item.
I would expect that you have to pass an "item" and there exists a "default" implementation that you can use if you don't want to write your own.

This revision now requires changes to proceed.Tue, Aug 27, 2:31 AM
woohyun added a comment.EditedTue, Aug 27, 5:16 AM

This design was also considered when we did architecture work for efl_ui_tab_pager.

There were following reasons that we did not choose this design.

  1. We did not have efl_ui_item at that time. So, we did not want to create it only for tab (This looks ok now because we already have efl_ui_item)
  1. Tab's appearance should be decided by Tabbar to keep consistency among tabs in it.

    But, with current design, tab's appearance is decided by tab_bar_default_item.

    For exmaple - if a tab with "tab_bar_NEW_item (extends tab_bar_default_item)" is inserted among tabs with "tab_bar_default_item",

    we can not guarantee a good appearance for Tabbar.

    Plus, if we want to extend the functionality of Tabbar (for example, add "More" tab when number of tabs are exceeding the limitation),

    we cannot keep design consistency because Tabbar never knows what kinds of items would be inserted.
  1. We made setter/getter for Tabbar because we considered the muliple Tabbars with different appearance and functionality

    (as in #2, we can make new tabbar with "More" tab support).

    This is related to D9731, and I think this should be supported for "different functionality". I don't think creating new tab_pager for new tabbar is not good way.

In conclusion, I think the major issue is whether we allow to insert different item classes in the same Tabbar.

I don't object to this design ~ and I also think this can give better flexibility to user.
But ! please think about removing setter for tab_bar (D9731) again. It is important for supporting variety UX of tabbar.

src/lib/elementary/efl_ui_tab_bar.eo
16

"item" would be proper instead of "icon".

src/lib/elementary/efl_ui_tab_bar_default_item.eo
5

I'm not sure whether this "icon" would be conflicted with icon part in Efl.Ui.Default_Item.

src/lib/elementary/efl_ui_tab_page.eo
1

I think this enum needs to be modified (or removed) because there can be more properties in Tab_Bar_Default_Item if it's extended.

9

I think it would be better to pass Efl.Ui.Tab_Bar_Default_Item with current architecure.

This design was also considered when we did architecture work for efl_ui_tab_pager.

There were following reasons that we did not choose this design.

  1. We did not have efl_ui_item at that time. So, we did not want to create it only for tab (This looks ok now because we already have efl_ui_item)

That makes sense.

  1. Tab's appearance should be decided by Tabbar to keep consistency among tabs in it.

    But, with current design, tab's appearance is decided by tab_bar_default_item.

    For exmaple - if a tab with "tab_bar_NEW_item (extends tab_bar_default_item)" is inserted among tabs with "tab_bar_default_item",

    we can not guarantee a good appearance for Tabbar.

    Plus, if we want to extend the functionality of Tabbar (for example, add "More" tab when number of tabs are exceeding the limitation),

    we cannot keep design consistency because Tabbar never knows what kinds of items would be inserted.

I wanted to do something simular for grid and list, however we do lack the time to do that based on items. (My idea was that the added container tells the item the details of the group it should have, in a way that we minimize the edje calls)
For now with this patch the behaviour is the same as in list and grid, the garantee to have a uniform appearance can still be patched later on, then, unified over all containers, with one single feature.
The feature with the more tab still can be realized, the item can just be seen as the "model" that owns the content and text. If we want to display it within another item, we can just reparent it to the next item, and use it there.

  1. We made setter/getter for Tabbar because we considered the muliple Tabbars with different appearance and functionality

    (as in #2, we can make new tabbar with "More" tab support).

    This is related to D9731, and I think this should be supported for "different functionality". I don't think creating new tab_pager for new tabbar is not good way.

That can still happen once we have a usecase for that. However, i do not see a reason to have that right now, and we can lower the barrier of entry with the automatic creation of that tab bar. Later on we can still make a constructor call, to insert a custom tabbar into the tab_pager (ensureing there are no items yet). However, right now the API seemed problematic to me, repairing it would have been simular to rewriting it, so removing it was the fasted solution for me.

In conclusion, I think the major issue is whether we allow to insert different item classes in the same Tabbar.

I would allow that, there could be also a seperator item, which is just a "|" between items.

I don't object to this design ~ and I also think this can give better flexibility to user.
But ! please think about removing setter for tab_bar (D9731) again. It is important for supporting variety UX of tabbar.

I have given my reasoning why it was removed above and in the commit message, if there is a strong need of that, i can add something similar back, would that be okay for you ?

src/lib/elementary/efl_ui_tab_bar.eo
16

That is true - however, i am removing this function anyways in a later revision where i am replacing this API with the implementation of Efl.Pack_Linear, would you be okay with keeping that just as it is?

src/lib/elementary/efl_ui_tab_bar_default_item.eo
5

Parts are in general in a different "klass" of conflicts than properties, so this should not do any harm.

src/lib/elementary/efl_ui_tab_page.eo
1

I remove that in a later revision :)

9

I remove that in a later revision :) (Please see my reasoning there why)

As I talked ~ I'm ok to go with new Item class :)

I will take a look at other patches after this :)

Always thanks for your effort !!!

src/lib/elementary/efl_ui_tab_bar.eo
16

Sure, I'm ok :)
I have not seen other patches yet - so, I commented like above.

Anyway~ I don't think the modification is necessary, either.

src/lib/elementary/efl_ui_tab_bar_default_item.eo
5

Yeah, you are right.

But, IconPart and Icon will change the same icon - then, I think only supporting IconPart would give no confusion to application developer.
How do you think about this ?

src/lib/elementary/efl_ui_tab_page.eo
1

Thanks :)

9

Thanks :)

bu5hm4n updated this revision to Diff 24579.Wed, Aug 28, 11:40 PM
bu5hm4n edited the summary of this revision. (Show Details)

address comments, add docs

bu5hm4n added inline comments.Wed, Aug 28, 11:42 PM
src/lib/elementary/efl_ui_tab_bar_default_item.eo
5

I just tried that, and it feels kind of weird. Part is only one way where you can access the icon. You can also just access it via normal content_set. So right now you have icon_set next to content_set. The docs also express that previous content is deleted. Does that look fine ?

woohyun accepted this revision.Thu, Aug 29, 4:11 AM

I think this looks ok for me - if there would be no more feedback in some hours - I'll close this :)

src/lib/elementary/efl_ui_tab_bar_default_item.eo
5

Ok :) Then, it makes sense.

src/lib/elementary/efl_ui_tab_page.eo
26

@segfaultxavi

I think it's something to keep consistency with list_default_item and grid_default_item.
So, I feel ok with this for now.
How do you think about my opinion?

segfaultxavi added inline comments.Thu, Aug 29, 4:41 AM
src/lib/elementary/efl_ui_tab_page.eo
26

This is exactly what I mean. Efl.Ui.Collection and company accept Efl.Ui.Item. Then users can provide their own items, or use the already-made Efl.Ui.Default_Item, Efl.Ui.List_Default_Item or Efl.Ui.Grid_Default_Item.
But this is not the same behavior in this patch. Here all tabs must be Efl.Ui.Tab_Bar_Default_Item or derivatives, which is weird.
Can't we just rename it to Efl.Ui.Tab_Bar_Item?

Also does the icon property justify creating yet another type? can't we just use Efl.Ui.List_Default_Item and change the icon through parts?

bu5hm4n added inline comments.Thu, Aug 29, 4:45 AM
src/lib/elementary/efl_ui_tab_page.eo
26

The type is not because of icon, the type is because the theme is different, and has a different look and feel for the case that it is selected. I added icon because it was there before, and it seems to be a nice and helpfull helper as a user...
I can Rename it to Efl.Ui.Tab_Bar_Item, before we just kept default in the name when it inherits from Efl.Ui.Default_Item, but i do not have strong feelings on that tbh.

segfaultxavi added inline comments.Thu, Aug 29, 4:51 AM
src/lib/elementary/efl_ui_tab_page.eo
26

OK, let's rename it at a later stage then. But not too late! :D

This revision was not accepted when it landed; it landed in state Needs Review.Thu, Aug 29, 5:08 AM
Closed by commit rEFLef3d88dfc9b7: introduce efl_ui_tab_bar_default_item (authored by Marcel Hollerbach <mail@marcel-hollerbach.de>, committed by woohyun). · Explain Why
This revision was automatically updated to reflect the committed changes.