Bug 108934

Created attachment 189058 [details] Patch

rakuco, could you please take a look?

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.

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

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.

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.

(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.

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 :(

Created attachment 189103 [details] Patch Take feedback into consideration.

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.

Created attachment 192663 [details] Patch Take Rakuco's feedback into consideration.

Comment on attachment 192663 [details] Patch Clearing flags on attachment: 192663 Committed r145564: <http://trac.webkit.org/changeset/145564>

All reviewed patches have been landed. Closing bug.