Bug 108934 – [EFL][WK2] Add popup menu support to MiniBrowser

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Chris Dumez]

We should add support for popup menus (when clicking a <select> element) in EFL's MiniBrowser by leveraging the ewk_popup_menu API.

[Chris Dumez]

Created attachment 189058 [details]
Patch

[Chris Dumez]

rakuco, could you please take a look?

[Gyuyoung Kim]

Comment on attachment 189058 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189058&action=review

> Tools/MiniBrowser/efl/main.c:98
> +    Evas_Object *elm_popup_menu;

Should you use elm_ prefix ?

> Tools/MiniBrowser/efl/main.c:790
> +destroy_elm_menu(Browser_Window *window)

I wonder if you want to use this function for other menus as well as popup menu. If this function is only for popup menu, isn't destroy_popup_menu better ?

> Tools/MiniBrowser/efl/main.c:801
> +on_menu_discarded(void *user_data, Evas_Object *obj, void *event_info)

How about on_popup_menu_discarded ?

> Tools/MiniBrowser/efl/main.c:812
> +on_popup_item_clicked(void *user_data, Evas_Object *obj, void *event_info)

How about on_popup_menu_item_clicked ?

> Tools/MiniBrowser/efl/main.c:825
> +populate_elm_menu(Evas_Object *elm_menu, Ewk_Popup_Menu *ewk_menu)

Aren't populate_popup_menu_items() or similar things more clear ?

> Tools/MiniBrowser/efl/main.c:838
> +                Elm_Object_Item* item = elm_menu_item_add(elm_menu, NULL, NULL, ewk_popup_menu_item_text_get(ewk_item), NULL, NULL);

Wrong * place.

> Tools/MiniBrowser/efl/main.c:841
> +                Elm_Object_Item* item = elm_menu_item_add(elm_menu, NULL, NULL, ewk_popup_menu_item_text_get(ewk_item), on_popup_item_clicked, INT_TO_PTR(index));

ditto.

[Chris Dumez]

Comment on attachment 189058 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189058&action=review

>> Tools/MiniBrowser/efl/main.c:98
>> +    Evas_Object *elm_popup_menu;
> 
> Should you use elm_ prefix ?

I found it clearer to easily distinguish a ewk_popup_menu and an elm_menu. We can use popup_menu though, I don't really mind, if people think it is better.

>> Tools/MiniBrowser/efl/main.c:790
>> +destroy_elm_menu(Browser_Window *window)
> 
> I wonder if you want to use this function for other menus as well as popup menu. If this function is only for popup menu, isn't destroy_popup_menu better ?

This is really specific to elm_popup_menu member of the Browser_Window so it is not used for other menus. Yes, I can rename to destroy_popup_menu().

>> Tools/MiniBrowser/efl/main.c:801
>> +on_menu_discarded(void *user_data, Evas_Object *obj, void *event_info)
> 
> How about on_popup_menu_discarded ?

ok

>> Tools/MiniBrowser/efl/main.c:812
>> +on_popup_item_clicked(void *user_data, Evas_Object *obj, void *event_info)
> 
> How about on_popup_menu_item_clicked ?

ok

>> Tools/MiniBrowser/efl/main.c:825
>> +populate_elm_menu(Evas_Object *elm_menu, Ewk_Popup_Menu *ewk_menu)
> 
> Aren't populate_popup_menu_items() or similar things more clear ?

I'll switch to populate_popup_menu() as "populate_popup_menu_items()" doesn't seem to make much sense to me. We are really populating a *menu*, not its items.

>> Tools/MiniBrowser/efl/main.c:838
>> +                Elm_Object_Item* item = elm_menu_item_add(elm_menu, NULL, NULL, ewk_popup_menu_item_text_get(ewk_item), NULL, NULL);
> 
> Wrong * place.

ok

>> Tools/MiniBrowser/efl/main.c:841
>> +                Elm_Object_Item* item = elm_menu_item_add(elm_menu, NULL, NULL, ewk_popup_menu_item_text_get(ewk_item), on_popup_item_clicked, INT_TO_PTR(index));
> 
> ditto.

ok

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 189058 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189058&action=review

I think the code can be simplified in a way that avoids the current calls to evas_object_data_{get,set} and the rest of the code is easier to read:

  struct Browser_Window {
      ...
      struct {
          Evas_Object *menu;
          Ewk_Popup_Menu *ewk_menu;
      } popup;
  };

  static void
  popup_menu_populate(...)
  {
      const Eina_List *items = ...;

      ...

      EINA_LIST_FOREACH(...) {
          ...
          if (regular_menu_item) {
              Elm_Object_Item *item = elm_menu_item_add(window->popup.menu, ..., on_popup_item_clicked, window);
              elm_object_item_data_set(item, "position", index);
          }
          ...
      }
  }

  static Eina_Bool
  on_popup_menu_show(...)
  {
      Browser_Window *window = ...;

      if (window->popup.menu)
          evas_object_del(window->popup.menu);

      window->popup.menu = elm_menu_add(...);
      popup_menu_populate(window->popup.meu, ewk_menu);

      window->popup.ewk_menu = ewk_menu;

      ...
  }

You can then stop worrying about closing and deleting the elm_menu, since it is automatically hidden when an item is selected or an area outside the menu is clicked (I don't think there's a problem with keeping the menu object alive until its parent is deleted).

> Tools/MiniBrowser/efl/main.c:827
> +    const Eina_List* ewk_items = ewk_popup_menu_items_get(ewk_menu);

Wrong pointer placement.

> Tools/MiniBrowser/efl/main.c:836
> +        if (ewk_popup_menu_item_type_get(ewk_item) == EWK_POPUP_MENU_SEPARATOR)
> +            elm_menu_item_separator_add(elm_menu, NULL);
> +        else {

Isn't it possible to use a switch() here?

> Tools/MiniBrowser/efl/main.c:838
> +                Elm_Object_Item* item = elm_menu_item_add(elm_menu, NULL, NULL, ewk_popup_menu_item_text_get(ewk_item), NULL, NULL);

Wrong pointer placement.

> Tools/MiniBrowser/efl/main.c:839
> +                elm_object_item_disabled_set(item, EINA_TRUE);

Why not leave it enabled? I think it makes more sense for it to be confused with a normal menu item (ie. non-label) than a disabled one.

> Tools/MiniBrowser/efl/main.c:841
> +                Elm_Object_Item* item = elm_menu_item_add(elm_menu, NULL, NULL, ewk_popup_menu_item_text_get(ewk_item), on_popup_item_clicked, INT_TO_PTR(index));

Wrong pointer placement.

[Chris Dumez]

Comment on attachment 189058 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189058&action=review

>> Tools/MiniBrowser/efl/main.c:839
>> +                elm_object_item_disabled_set(item, EINA_TRUE);
> 
> Why not leave it enabled? I think it makes more sense for it to be confused with a normal menu item (ie. non-label) than a disabled one.

Because otherwise mouse cursor changes and it it clickable. What I really want is for it to be disabled but look as if it wasn't :) I'm not sure how to do that though.

[Chris Dumez]

(In reply to comment #5)
> (From update of attachment 189058 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189058&action=review
> 
> I think the code can be simplified in a way that avoids the current calls to evas_object_data_{get,set} and the rest of the code is easier to read:
> 
>   struct Browser_Window {
>       ...
>       struct {
>           Evas_Object *menu;
>           Ewk_Popup_Menu *ewk_menu;
>       } popup;
>   };
> 
>   static void
>   popup_menu_populate(...)
>   {
>       const Eina_List *items = ...;
> 
>       ...
> 
>       EINA_LIST_FOREACH(...) {
>           ...
>           if (regular_menu_item) {
>               Elm_Object_Item *item = elm_menu_item_add(window->popup.menu, ..., on_popup_item_clicked, window);
>               elm_object_item_data_set(item, "position", index);
>           }
>           ...
>       }
>   }
> 
>   static Eina_Bool
>   on_popup_menu_show(...)
>   {
>       Browser_Window *window = ...;
> 
>       if (window->popup.menu)
>           evas_object_del(window->popup.menu);
> 
>       window->popup.menu = elm_menu_add(...);
>       popup_menu_populate(window->popup.meu, ewk_menu);
> 
>       window->popup.ewk_menu = ewk_menu;
> 
>       ...
>   }

I'll give this a try, thanks.

> 
> You can then stop worrying about closing and deleting the elm_menu, since it is automatically hidden when an item is selected or an area outside the menu is clicked (I don't think there's a problem with keeping the menu object alive until its parent is deleted).

Well, I still need callbacks in order to call ewk_popup_menu_close() anyway. We really need to notify WebKit that the menu was closed. It is required by our implementation.

[Chris Dumez]

rakuco, I'm having trouble with elm_object_item_data_set(). First of all, it seems to take only 2 arguments, not 3. I tried to use it anyway:

Elm_Object_Item *item = elm_menu_item_add(elm_menu, NULL, NULL, ewk_popup_menu_item_text_get(ewk_item), on_popup_menu_item_clicked, window);
elm_object_item_data_set(item, INT_TO_PTR(index));

However, this causes index to be passed as user_data to on_popup_menu_item_clicked callback, instead of window :(

[Chris Dumez]

Created attachment 189103 [details]
Patch

Take feedback into consideration.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 189103 [details]
Patch

This version looks much better, and it looks like elm_menu_item_index_get() has saved the day :-)

You could probably get rid of destroy_popup_menu() since the elm_menu will be deleted at the end anyway. I'd also rename some of the functions to match EFL's Yoda style (eg. populate_popup_menu -> popup_menu_populate). Other than that, LGTM.

[Chris Dumez]

Created attachment 192663 [details]
Patch

Take Rakuco's feedback into consideration.

[WebKit Review Bot]

Comment on attachment 192663 [details]
Patch

Clearing flags on attachment: 192663

Committed r145564: <http://trac.webkit.org/changeset/145564>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.