Page MenuHomePhabricator

efl.ui.textbox: replace hoversel with popup
ClosedPublic

Authored by ali.alzyod on Jan 13 2020, 12:47 AM.

Details

Summary

efl.ui.textbox: replace hoversel with popup
I think this may need some changes, please let me know what you think

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.
ali.alzyod created this revision.Jan 13 2020, 12:47 AM

It seems that this patch has no reviewers specified. If you are unsure who can review your patch, please check this wiki page and see if anyone can be added: https://phab.enlightenment.org/w/maintainers_reviewers/

ali.alzyod requested review of this revision.Jan 13 2020, 12:47 AM
bu5hm4n requested changes to this revision.Jan 13 2020, 4:06 AM

That's a good start, a few things should be fixed however.

src/lib/elementary/efl_ui_textbox.c
1029

This will not work with translation, text will not be Copy for example.

1071–1072

can you rename to hoversel to popup ?

1082

Mhm, what do you think of listening to the selection changed events of the items in the list, that would safe you the code above that needs to do string compares.

This revision now requires changes to proceed.Jan 13 2020, 4:06 AM
ali.alzyod added inline comments.Jan 13 2020, 4:46 AM
src/lib/elementary/efl_ui_textbox.c
1082

are their events for Item click?

bu5hm4n added inline comments.Jan 13 2020, 4:50 AM
src/lib/elementary/efl_ui_textbox.c
1082

EFL_UI_EVENT_SELECTED_CHANGED is emitted on the *item* object.

ali.alzyod updated this revision to Diff 28154.Jan 14 2020, 4:03 AM
ali.alzyod marked 5 inline comments as done.

update

I notice that EFL_UI_POPUP_EVENT_BACKWALL_CLICKED does not work with right mouse click

I am not sure if the backwall should emit clicked when rightclicked, it normally is just meant to react on left click.

This looks like a good start, a few more things then we are good to go i guess. With the changes now mentioned this should also cleanup a fair amount of code.

src/lib/elementary/efl_ui_textbox.c
908–909

@zmike is it possible that this positioning code is basically available via popup internals ?

909

Do you know if this job is really needed, i am not sure why we have it, the popup will be deleted in _backwall_clicked anyways.

1047

Can you refactor

EFL_UI_TEXT_DATA_GET(data, sd);
efl_del(sd->popup_list);
efl_del(sd->popup);
sd->popup_list = NULL;
sd->popup = NULL;

Into a static inline void function ehre and call it in every _cb where they are the same, that would make the code a little bit smaller :)

ali.alzyod marked 2 inline comments as done.Jan 14 2020, 7:50 AM

it seems EFL_UI_SELECTABLE_EVENT_SELECTION_CHANGED is not fired on EFL_UI_LIST_DEFAULT_ITEM_CLASS

ali.alzyod added inline comments.Jan 14 2020, 8:31 AM
src/lib/elementary/efl_ui_textbox.c
1021

this cause error in log when called on EFL_UI_EVENT_SELECTED_CHANGED

ERR<1606>:eina_safety ../src/lib/elementary/efl_ui_layout.c:1166 _efl_ui_layout_base_efl_layout_signal_signal_emit() safety check failed: efl_invalidated_get(obj) is true
ERR<1606>:eina_safety ../src/lib/evas/canvas/evas_focus.c:226 _efl_canvas_object_seat_focus_add() safety check failed: !efl_invalidating_get(eo_obj) && !efl_invalidated_get(eo_obj) is false
bu5hm4n added inline comments.Jan 14 2020, 8:39 AM
src/lib/elementary/efl_ui_textbox.c
1021

Mhm, crap, i will check it tomorrow.

bu5hm4n accepted this revision.Jan 14 2020, 10:36 PM

I think we can go ahead and land it the way it is, the fix for the error will be outside this here i guess.

src/lib/elementary/efl_ui_textbox.c
1021

https://phab.enlightenment.org/D1913 This is causing it, i wrote all the details to T8576, lets wait for answers.

This revision is now accepted and ready to land.Jan 14 2020, 10:36 PM
Closed by commit rEFL1a1868ce57a3: efl.ui.textbox: replace hoversel with popup (authored by ali.alzyod, committed by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain WhyJan 15 2020, 1:50 AM
This revision was automatically updated to reflect the committed changes.