Page MenuHomePhabricator

Add a tab/pageview widget
Open, TODOPublic

Description

We need a tab/pageview widget that has the ability to contain a list of view.

Details

Differential Revisions
D5430: efl_ui_pager: add a new class
cedric created this task.Mar 30 2017, 2:38 AM
cedric mentioned this in T5318: Improve flip.
cedric raised the priority of this task from TODO to Normal.Jul 10 2017, 3:08 PM
eunue added a comment.EditedAug 4 2017, 12:57 AM

I have been working on building classes to support page view in efl and just pushed the results so far.

git checokut devs/eunue/interface

You can check out the concept easily by running the demo on top of the log.
This is just a simple implementation among many possible concepts and I want to develop it in a better way with your help.
I will update a document soon about the process, property specification, to-dos, etc.

jpeg added a comment.Aug 18 2017, 1:24 AM

Next week let's have a look at this together. I'm so sorry I couldn't check it yet...

eunue added a comment.Nov 6 2017, 6:34 AM

I rebuilt the code from the scratch and here is the new branch:

git checkout devs/eunue/pager

You can checkout the behavior in elementary_test as follows:

elementary_test -to pager
jpeg added a comment.Nov 6 2017, 11:29 PM

Can you open a Diff on phabricator so that I can review line by line?

eunue added a comment.Nov 6 2017, 11:54 PM

Here is the link to the differential:
https://phab.enlightenment.org/D5430

@bowonryu

Could you update your Efl.Ui.TabPager Class definition here to get the review from @cedric and @eunue together :)

As we discussed with @jpeg before, we want to eliminate elm_widget_item and manage tabs and pages internally with index based.
Please give feedback on my class definition.
Diff is currently being written and will be uploaded this week.

This is the definition of Efl.Ui.Tab_Pager.

class Efl.Ui.Tab_Pager (Efl.Ui.Pager, Efl.Ui.Layout, Efl.Ui.Direction)
{
   [[Tab Pager calss]]
   methods {
       tab_pages_count @const {
           [[Get the number of tab pages in a tab_pager]]
           return: uint;
       }
       tab_page_append {
           [[Append tab and page to the tab_pager]]
           return: bool;
           params {
               @in label: string;
               @in icon: string @optional;
               @in page: Efl.Canvas.Object;
               @in func: Evas_Smart_Cb @optional;
               @in data: const(void_ptr) @optional;
           }
       }
       tab_page_insert {
           [[Insert tab and page to the index of the tab_pager]]
           return: bool;
           params {
               @in index: int;
               @in label: string;
               @in icon: string @optional;
               @in page: Efl.Canvas.Object;
               @in func: Evas_Smart_Cb @optional;
               @in data: const(void_ptr) @optional;
           }
       }
       tab_page_remove {
           [[Remove tab and page at the index of the tab_pager]]
           return: bool;
           params {
               @in index: int;
           }
       }
       @property current_tab_page {
           values {
               index: int;
           }
       }
       @property tab_label {
           keys {
               index: int;
           }
           values {
               label: string;
           }
       }
       @property tab_icon {
           keys {
               index: int;
           }
           values {
               icon: string;
           }
       }
       @property page_content {
           keys {
               index: int;
           }
           values {
               page: Efl.Canvas.Object;
           }
       }
   }
   implements {
      Efl.Object.constructor;
      Efl.Object.finalize;
      Efl.Object.destructor;
      Efl.Gfx.position { set; }
      Efl.Gfx.size { set; }
   }
}
woohyun added a comment.EditedMar 12 2018, 10:58 PM

Dear. @cedric
I would give some information for better understanding.
Efl.Ui.Pager (@eunue is creating) supports Efl.Pack interface to insert contents in it.
So, the above Efl.Ui.Tab_Pager should support this interface together.

So, here is one thing ambiguous - whenever a content is packed, an empty tab should be created automatically or not ?

We (@bowonryu and I) concluded that an empty tab should be created and developers can set text(or icon) in it.
So, there are tab_label and tab_icon with index.

About "index" ... we didn't want to create new Item Class for Efl.Ui.Tab_Pager.
So, all methods are based on index.

tab_page_append/insert/remove ...
These are for developer convenience.
We'd think many developers might want to create tabs with these kindes of methods.
(But, I don't know func and data are something needed)

If you have any opinion on this, please leave some comment here :)

Dear. @bowonryu
Some feedback on the class definition. Please refer them.

  1. We don't need "page_content" because there is pack_content_get already inside Efl.Ui.Pager.
  2. tab_label -> indicator_text (?)
  3. tab_icon -> indicator_icon (?)
  4. tab_page_xxx -> tab_xxx (?)
  5. current_tab_page -> current_tab_index (?)
  6. tab_append/prepend/insert_at/remove (?)

I don't like this idea for many reason, but mostly because it is discarding the goal of interfaces work to simplify the amount of different API we have and not add thousand of new one for each case we forgot. So with that in mind, here we have only a simple text, what will happen the day when we want underline ? What happen when we want to disable an item ? I don't think this is going to work.

I did like the design discussion we had last time I was in Korea, where we did propose to have an tab page, not an item, but a container for packing content in it with the pack API. Splitting things in four object. One to handle one page, Efl.Ui.Tab.Page. One to play the indicator, Efl.Ui.Tab.Indicator (Which display the icon and text from an Efl.Ui.Tab.Page). One to handle navigation between page and page content itself, Efl.Ui.Tab.Pager. And finally, one to contain them all, Efl.Ui.Tab (Basically to put the indicator, the pager at the right place and redirect pack operation to the pager). Efl.Ui.Tab.Page would have a part "indicator" that provide a text and image API so that you have the full featureset here.

To avoid code duplication, using efl_object_composite_attach can be used to get to redirect call from some of the API, like text, image or packing directly to the object doing the work internally. This shouldn't be to much work, would not duplicate API, fully support all possible case.

Let me know if there is anything you don't get as communicating via text new idea is not so trivial, I may have not provided enough detail or missed some point.

woohyun added a comment.EditedMar 13 2018, 5:23 PM

@cedric
If my understanding is correct developers don't need to know about the existence of Efl.Ui.Tab.Indicator and Efl.Ui.Tab.Pager,
because developers will do (in C#) ....

// Create a tab
efl.ui.Tab tab = new efl.ui.Tab();

// Create a page with indicator text and icon
efl.ui.Tab_Page pg = efl.ui.Tab_Page();
pg.part("indicator").text_set("Tab1");            // I don't know how to use efl_part in C#. Just for explanation.
pg.part("indicator").icon_set("/icon_path");
pg.content_set(content_obj);     // content_obj is something that would be shown as a tab content. (layout or box ..)

// Pack the page into the tab.
tab.pack_begin(pg);

Please give feedback whether my understanding is right or not.

Then, I think Efl.Ui.Tab_Pager (which extends Efl.Ui.Pager and includes Efl.Ui.Tab.Indicator in it) and Efl.Ui.Tab_Page are enough as new classes.
Later, If other classes want Efl.Ui.Tab.Indicator, then we can consider to expose it.

I don't like to create Efl.Ui.Tab because I don't like to use composite_attach.
This is because the methods (which are supported by composite_attach) could not be used by subclasses. (Am I right ?)

How do you think about my opinion ?

Internally you will need to do the Efl.Ui.Tab.Indicator, if you don't want to expose it outside (by installing the .eo and exposing the .h), I am fine with that.

I think the problem with composite attach should be ok, if it is done in the constructor after calling the efl_super constructor and returning the object. Now, I could be wrong and I would have to look at how composite attach implementation.

Otherwise I agree with your example. I just hope we could get C# binding to do something like : pg.indicator.text_set("Tab1") for part. Would be neat I think.

woohyun added a comment.EditedMar 13 2018, 11:17 PM

@cedric

Hmm. I want to get the spec of Efl.Ui.Tab.Indicator.
Based on your basic idea of this, does the class include UI or not ?
If it includes UI, then what is the difference between the new class and old elm_toolbar ?

I'm asking this becuase -

  1. We concluded to remove elm_toolbar from efl_ui_xxx world.
  2. But I think that kind of thing is necessary if we need to give freedom about Tab.Indicator UX (for example two texts or two icons). Then, what we can consider is that Efl.Ui.Tab.Indicator itself cannot work alone (i.e. it should be set into the Efl.Ui.Tab). And different type of Tab.Indicator can support different UX ...

How do you think about this ?

  • About efl_part in C#, I will discuss with Felipe :)

@cedric
This patch is not including the result of above discussion. (That's because we have not finalized it yet.)

I think that efl_part(page, "indicator") is enough with text_set, icon_set, and content_set.
That is, we don't need to think about text2_set or icon2_set.

Based on this assumption, we can consider several combinations among classes.

Let's choose one of them :)

[1] Tab_Pager + Page

efl.ui.Tab_Pager pager = new efl.ui.Tab_Pager();
pager.shrink_mode_set(efl.ui.Tab_Pager.SHRINK_EXPAND); // I don't like this honestly.

efl.ui.Tab_Page pg = efl.ui.Tab_Page();
pg.indicator.text_set("Tab1");
pg.indicator.icon_set("/icon_path");
pg.content_set(content_obj);

pager.pack_begin(pg);

[2] Tab_Pager + Indicator + Page

efl.ui.Tab_Pager pager = new efl.ui.Tab_Pager();

efl.ui.Tab_Indicator_Expand indicator = new efl.ui.Tab_Indicator_Expand();

// Or .... 
//efl.ui.Tab_Indicator indicator = new efl.ui.Tab_Indicator();
//indicator.shrink_mode_set(efl.ui.Tab_Indicator.SHRINK_EXPAND);  


pager.indicator_set(indicator);

efl.ui.Tab_Page pg = efl.ui.Tab_Page(); // Page only can work with specific Tab_Indicator because of efl_part limitation
pg.indicator.text_set("Tab1");
pg.indicator.icon_set("/icon_path");
pg.content_set(content_obj);

pager.pack_begin(pg);

[3] Tab(includes Pager inside) + Indicator + Page

efl.ui.Tab tab = new efl.ui.Tab();
efl.ui.Tab_Indicator_Expand indicator = new efl.ui.Tab_Indicator_Expand();

tab.indicator_set(indicator);

efl.ui.Tab_Page pg = efl.ui.Tab_Page(); 
pg.indicator.text_set("Tab1");
pg.indicator.icon_set("/icon_path");
pg.content_set(content_obj);

tap.pack_begin(pg);

Previously, I thought [1] looks good ....
but, if we think about lots of mode_set APIs in old elm_toolbar, [2] and [3] look good.

IMHO, I think [2] is the best. (because I don't like to use composit in Tab class)

@cedric
How do you think about this ?

I am fine with either [2] or [3] at this point.

woohyun added a comment.EditedMar 27 2018, 12:57 AM

@cedric
Ok. I will ask @bowonryu to modifiy his patch based on the idea of [2].

@bowonryu
Let's go with idea of [2].
I think we need to make basic Tab_Indicator class and Tab_Page classes,
and support other types of them later.

Let's make a plan for this asap, and update them on https://phab.enlightenment.org/D5847.

zmike edited projects, added Restricted Project; removed efl.Jun 11 2018, 6:58 AM
zmike edited projects, added efl: widgets; removed Restricted Project.Jun 11 2018, 8:16 AM
jpeg added a comment.Nov 27 2018, 1:59 AM

The test case is cool and works fine at first but exhibits many bugs and tons of ERR messages along the way (eg. unpack_all does not delete the current page, etc...)

zmike lowered the priority of this task from Normal to TODO.Jan 17 2019, 10:16 AM
bu5hm4n added a subscriber: bu5hm4n.Fri, Oct 4, 2:44 AM

I think this is done ?