Page MenuHomePhabricator

Efl.Ui.Textbox class
Open, Incoming QueuePublic

Description

class @beta Efl.Ui.Textbox
(P) scrollable
(P) context_menu_disabled
(P) cnp_mode
(P) selection_handler_disabled
(P) item_factory
(M) cursor_add
(M) cursor_create
(M) selection_copy
(M) selection_paste
(M) selection_cut
(E) selection,paste;
(E) selection,copy;
(E) selection,cut;
(E) changed:void;
(E) context,open;

Details

Differential Revisions
D10968: efl_text_interactive: selection enhancment
D11127: efl.ui.textbox: using efl_provider_find
D11109: efl.ui.textbox: update _part_is_efl_ui_textbox_part
D11103: efl.ui.textbox: theme code cleanup
D11102: efl.ui.textbox: part implementation comments clean up!
D11092: efl.ui.textbox: add efl.input text as composite interface
D11072: efl.ui.textbox: replace hoversel with popup
D11068: efl.ui.textbox: paste in mouse button 2
D11067: efl.ui.textbox: legacy cleanup
D11042: efl.ui.textbox: rename selection_handler to selection_handles
D11035: efl.ui.textbox: clean text box from unused interfaces implementation (FILE interface)
D11025: efl_ui_textbox: replace elm_obj stuff and focuse stuff
D11023: efl.ui.textbox: clean up (remove unused vars and methods)
D11003: efl.ui.textbox: replace legacy scroller type check
D10986: elementary_text: Efl.Ui.Textbox Input Field
D10985: efl.text.cursor: emit events CANVAS_TEXTBLOCK_CHANGED when insert text using efl_text_cursor_markup_insert
D10982: efl_ui_textbox: replace legacy calls with new ones
D10923: efl.ui.textbox: rename _disabled properties into _enabled
D10979: efl.text.interactive: remove event freeze when keyboard button is pressed
D10981: efl_ui_text: scroller mode clean up
Commits
D11107 / rEFL0b389a8338eb: efl_ui_textbox: rename efl_ui_text_part to efl_ui_textbox_part
D11093 / rEFLd412f73a9974: efl_ui_textbox: mark @beta to cnp_mode and item_factory
D10987 / rEFL5154b5a8fca0: efl_ui_text: support focus navigation

How about remove beta tag & release it. ?
Please let me know

segfaultxavi moved this task from Backlog to Evaluating on the efl: api board.Dec 18 2019, 5:54 AM

There are a few things I am not sure if they can be stabilized. What is the state of:

  • context_menu_disabled
  • cnp_mode
  • selection_handler_disabled
  • item_factory

I dislike "negative" properties, so I would rename those disabled to enabled and make them true by default.

There are a few things I am not sure if they can be stabilized. What is the state of:

  • context_menu_disabled

I think we definitly want to keep that beta for now. We do not have a menu class yet.

  • cnp_mode

I also think we also want to wait until the cnp rework is done.

  • selection_handler_disabled

What is this doooooing ?

  • item_factory

Mhm, how is the ownership of these things ? Nothing is notated or documented right now.

I dislike "negative" properties, so I would rename those disabled to enabled and make them true by default.

Mhm - disabled is also used in widget and quite "spread" over the whole efl API :/

The only instances of disabled I could find in our current Unified API are here and in Efl.Ui.Widget (I think we can treat Efl.Ui.Widget.disabled as a special case since this is an extremely common property in all toolkits).

The only instances of disabled I could find in our current Unified API are here and in Efl.Ui.Widget (I think we can treat Efl.Ui.Widget.disabled as a special case since this is an extremely common property in all toolkits).

True, cool for me, thank you for checking :)

But of content_menu_disabled, this is can work independent of Menu_Class, this is just to enable or disable the menu on right-click. (menu object can be exposed later on)

There are a few things I am not sure if they can be stabilized. What is the state of:

  • context_menu_disabled
  • cnp_mode
  • selection_handler_disabled
  • item_factory

You copy 4 lines instead of two :)
I think this disabled term came from legacy definitions in Elm_Entry.

D10923 to rename properties

This is should be ready to stabilize, any comments ?

@bu5hm4n and I raised some concerns some days ago which haven't been addressed, I think.

@bu5hm4n and I raised some concerns some days ago which haven't been addressed, I think.

Is it possible to raise them again?

Could you answer the questions here T8522#148377 ?

ali.alzyod added a comment.EditedFri, Dec 27, 10:45 AM

There are a few things I am not sure if they can be stabilized. What is the state of:

  • context_menu_disabled

I think we definitly want to keep that beta for now. We do not have a menu class yet.

This can work independent of Menu_Class, this is just to enable or disable the menu on right-click/longpress. (menu object can be exposed later on)

  • cnp_mode

I also think we also want to wait until the cnp rework is done.

Currently, we need to allow the user to use Plain text or Markup text or image mode.

  • selection_handler_disabled

What is this doooooing ?

these are UI components used by user to modify selection range.

  • item_factory

Mhm, how is the ownership of these things ? Nothing is notated or documented right now.

How to notate Setter that ref passed value?

There are a few things I am not sure if they can be stabilized. What is the state of:

  • context_menu_disabled

I think we definitly want to keep that beta for now. We do not have a menu class yet.

This can work independent of Menu_Class, this is just to enable or disable the menu on right-click/longpress. (menu object can be exposed later on)

But we might have a better solution later on, like just unsetting the menu object, or getting it and appending the own options. I would rather wait with this until we have a menu class, as solution then might be easier and nicer. (@woohyun is there a strong requirement for having this ?)

  • cnp_mode

I also think we also want to wait until the cnp rework is done.

Currently, we need to allow the user to use Plain text or Markup text or image mode.

But we cannot. Efl_Ui_Selection_Format is beta, and should stay beta, this is simply copied over from X11, but is basically incompatible with Wayland and (and maybe other platforms).

  • selection_handler_disabled

What is this doooooing ?

these are UI components used by user to modify selection range.

But why would someone on a touchscreen disable the selection handlers, either you disable making selection, or you have them. Having a selection but no way to alter it seems a bit weird ?.

  • item_factory

Mhm, how is the ownership of these things ? Nothing is notated or documented right now.

How to notate Setter that ref passed value?

What?

Another question: When the entry is scrollable, how do I tell it to "show" a specific region in the textbox ?

bu5hm4n added a comment.EditedSat, Dec 28, 2:09 AM

Further more:

  • changed event is only emitted in weird cases (like backspace pressed, but not when abcd is pressed).
  • The implementation is using legacy API all over the place, which is then checking again via elm_widget_is_legacy the stype of the widget, just to call the Efl.Ui API on it.
  • The implementation of the context menu calls API for scroll stuff like elm_widget_scroll_freeze_push, are you 100% sure you dont want to call that on the internal scroller object ? (Considering the performance overhead)
  • _efl_ui_textbox_cnp_mode_set seems a little bit broken, EFL_UI_SELECTION_FORMAT_HTML is not supported. However, the all target still allows to have HTML (Same for VCARD)?
  • _viewport_region_get is checking for ELM_INTERFACE_SCROLLABLE_MIXIN which is the legacy scroll version, which seems wrong here
  • _efl_ui_textbox_efl_ui_widget_disabled_set also calls legacy scroll APIs.
  • evas_object_ref is also something you likely dont need and dont want to have.
  • You likely want to get rid of the hoversel selection there, and use something like Efl.Ui.Popup which has a anchor property.
  • _mouse_down_cb button 2 should not use the clipboard CNP buffer, that will mess up how things work in X
  • Code structures like top = elm_widget_top_get(data); and if (efl_isa(top, EFL_UI_WIN_CLASS)) can be replaced with top = efl_provider_find(widget, EFL_UI_WIN_CLASS) No type check needed.
  • _efl_ui_textbox_efl_layout_signal_signal_emit This FIXME there looks quite critical.
  • _efl_ui_textbox_efl_layout_signal_signal_callback_add You are adding two callbacks here,
  • The callback_add and callback_del operations are looking weird, the Efl_Layout_Signal object in the callbacks will be wrong
  • _evas_object_intercept_call calls are not needed anymore, this is a Efl.Ui widget.
  • what is text_table, and why is it used ?
  • You should not theme in _efl_ui_textbox_efl_object_finalize, but rather in theme_apply
  • All part swallow things should be done in theme_apply.
  • the part implementation is done with FIXMEs and by hand instead of the usally used helpers, any reasons for that ?

Overall, this widget uses soooo much legacy API, this should all use unified API, (like idler usage, evas_object API usage, etc. etc.)

@zmike can you review _efl_ui_textbox_efl_canvas_group_group_calculate if all the correct hints are used ?

Additionally, https://phab.enlightenment.org/T8538 should be resolved before we stabilize this. Additionally, focus seems a bit messed up here, In efl_ui_editor, sometimes canvas-focus sometimes ends up in some Efl.Canvas.Layout object, instead of the Efl.Ui.Internal.Text.Interactive object.

Long story short: This widget needs *way* more tests / testing / work before we can declare it stable.

Additionally the second: in elm_test Efl.Ui.Textbox Input Field

  • if i cnp something from the lower textbox to the upper one, the guide text is not removed, additionally,
  • error in console at startup
  • if i cnp the text from below in the upper very often there, the scroller does not move anymore at some point, i also cannot click, nor move the cursor anymore.
ali.alzyod added a comment.EditedSun, Dec 29, 2:09 AM

Overall, this widget uses soooo much legacy API, this should all use unified API, (like idler usage, evas_object API usage, etc. etc.)
Long story short: This widget needs *way* more tests / testing / work before we can declare it stable.

@bu5hm4n thanks for your feedback. these text changes are built at the top of what has been done there, and we try to minimize changes as much as possible (because some of these classes have been there for a long time over four years).
Your feedback is helpful to show me exactly what is blocking stabilization in old source code.

But why would someone on a touchscreen disable the selection handlers, either you disable making selection, or you have them. Having a selection but no way to alter it seems a bit weird ?.

Even on touch screen, you could have a keyboard and you make the selection with it (not using a mouse or touch screen).
This property will show selection handles (handlers), regardless of the interactive way (or programmatic way) of making the selection.

What?

The property item_factory_set(Eo *x), will efl_ref(x) passed an object, how to notate this property in the EO file?

Another question: When the entry is scrollable, how do I tell it to "show" a specific region in the textbox?

The current scrolling options are either to enable or disable scrolling only, So for your question, you can not directly scroll to a specific region.

changed event is only emitted in weird cases (like backspace pressed, but not when abcd is pressed).

D10979 Should fix it.

The implementation is using legacy API all over the place, which is then checking again via elm_widget_is_legacy the stype of the widget, just to call the Efl.Ui API on it.

@woohyun textbox is not used by legacy entry widget, (It can not be legacy), So I think we should remove all legacy code, what do you think ?

The implementation of the context menu calls API for scroll stuff like elm_widget_scroll_freeze_push, are you 100% sure you dont want to call that on the internal scroller object ? (Considering the performance overhead)
_viewport_region_get is checking for ELM_INTERFACE_SCROLLABLE_MIXIN which is the legacy scroll version, which seems wrong here
_efl_ui_textbox_efl_ui_widget_disabled_set also calls legacy scroll APIs.
evas_object_ref is also something you likely dont need and dont want to have.
You likely want to get rid of the hoversel selection there, and use something like Efl.Ui.Popup which has a anchor property.
Code structures like top = elm_widget_top_get(data); and if (efl_isa(top, EFL_UI_WIN_CLASS)) can be replaced with top = efl_provider_find(widget, EFL_UI_WIN_CLASS) No type check needed.

So all these legacy stuff have replacement, I will check them one by one.

_evas_object_intercept_call calls are not needed anymore, this is a Efl.Ui widget.

I made a search and found this used all over the place, even in Efl.Ui widget itself.

_efl_ui_textbox_cnp_mode_set seems a little bit broken, EFL_UI_SELECTION_FORMAT_HTML is not supported. However, the all target still allows to have HTML (Same for VCARD)?

I think this is same as legacy, hmmmm maybe broken in both. So you suggest remove them ?

_mouse_down_cb button 2 should not use the clipboard CNP buffer, that will mess up how things work in X

You mean paste text on mouse button 2 click?

_efl_ui_textbox_efl_layout_signal_signal_emit This FIXME there looks quite critical.
_efl_ui_textbox_efl_layout_signal_signal_callback_add You are adding two callbacks here,
The callback_add and callback_del operations are looking weird, the Efl_Layout_Signal object in the callbacks will be wrong
You should not theme in _efl_ui_textbox_efl_object_finalize, but rather in theme_apply
All part swallow things should be done in theme_apply.
the part implementation is done with FIXMEs and by hand instead of the usally used helpers, any reasons for that ?

This is old code, I will try to check what was the problem

woohyun added a comment.EditedSun, Dec 29, 9:05 PM

@ali.alzyod

@woohyun textbox is not used by legacy entry widget, (It can not be legacy), So I think we should remove all legacy code, what do you think ?

In my opinion, removing legacy code is always good for new widgets to cut off the dependency with old features. :)

sudo

Additionally the second: in elm_test Efl.Ui.Textbox Input Field

  • if i cnp something from the lower textbox to the upper one, the guide text is not removed, additionally,
  • error in console at startup
  • if i cnp the text from below in the upper very often there, the scroller does not move anymore at some point, i also cannot click, nor move the cursor anymore.

D10986 D10985

Additionally, https://phab.enlightenment.org/T8538 should be resolved before we stabilize this. Additionally, focus seems a bit messed up here, In efl_ui_editor, sometimes canvas-focus sometimes ends up in some Efl.Canvas.Layout object, instead of the Efl.Ui.Internal.Text.Interactive object.

D10987 (I still feel at least we should provide the user with the option to disable this (disabled by default))

@bu5hm4n I really appreciate for giving great feedback !!!!

  • changed event is only emitted in weird cases (like backspace pressed, but not when abcd is pressed).

This should be a critical bug. it needs to be fixed.

  • The implementation is using legacy API all over the place, which is then checking again via elm_widget_is_legacy the stype of the widget, just to call the Efl.Ui API on it.

This is correct. The code should be modified.

  • The implementation of the context menu calls API for scroll stuff like elm_widget_scroll_freeze_push, are you 100% sure you dont want to call that on the internal scroller object ? (Considering the performance overhead)

I think all the scrollers (including internal scroller) upon ui_textbox should be freezed together.

  • _efl_ui_textbox_cnp_mode_set seems a little bit broken, EFL_UI_SELECTION_FORMAT_HTML is not supported. However, the all target still allows to have HTML (Same for VCARD)?

I hope to set "cnp_mode" as beta and release later. It will take longer to test all the modes.

  • _viewport_region_get is checking for ELM_INTERFACE_SCROLLABLE_MIXIN which is the legacy scroll version, which seems wrong here

This is correct. The code should be modified.

  • _efl_ui_textbox_efl_ui_widget_disabled_set also calls legacy scroll APIs.

This is correct. The code should be modified.

  • evas_object_ref is also something you likely dont need and dont want to have.

This is correct. The code should be modified.

  • You likely want to get rid of the hoversel selection there, and use something like Efl.Ui.Popup which has a anchor property.

This can take a bit longer to implement. I hope it would be handled later.

  • _mouse_down_cb button 2 should not use the clipboard CNP buffer, that will mess up how things work in X

I think we can remove the code for button 2.

  • Code structures like top = elm_widget_top_get(data); and if (efl_isa(top, EFL_UI_WIN_CLASS)) can be replaced with top = efl_provider_find(widget, EFL_UI_WIN_CLASS) No type check needed.

This is correct. The code should be modified.

  • _efl_ui_textbox_efl_layout_signal_signal_emit This FIXME there looks quite critical.
3e5cfb83c092652cf0306b7f4b1a0b230fdf21bf added "signal_emit", but I cannot understand what was the issue.
So, we can remove the message_process and check the result of it.
  • _efl_ui_textbox_efl_layout_signal_signal_callback_add You are adding two callbacks here,

One for base edje and the other for scroller ? then no problem maybe ?

  • The callback_add and callback_del operations are looking weird, the Efl_Layout_Signal object in the callbacks will be wrong

I am a bit confused with this, @bu5hm4n Could you explain a bit more ?

  • _evas_object_intercept_call calls are not needed anymore, this is a Efl.Ui widget.

This seems proper feedback. We need to modify.

  • what is text_table, and why is it used ?

@ali.alzyod Could you answer for this question ?

  • You should not theme in _efl_ui_textbox_efl_object_finalize, but rather in theme_apply

This seems proper feedback. We need to modify.

  • All part swallow things should be done in theme_apply.

This seems proper feedback. We need to modify.

  • the part implementation is done with FIXMEs and by hand instead of the usally used helpers, any reasons for that ?

I think @ali.alzyod had no idea on this, so @bu5hm4n could you give a guideline for this ?

Again! I really appreciate for your polite feedback.

  • The implementation of the context menu calls API for scroll stuff like elm_widget_scroll_freeze_push, are you 100% sure you dont want to call that on the internal scroller object ? (Considering the performance overhead)

I think all the scrollers (including internal scroller) upon ui_textbox should be freezed together.

Correct, which is why i would call this on the internal scroller. As this then skips the widget tree introspection.

  • _efl_ui_textbox_cnp_mode_set seems a little bit broken, EFL_UI_SELECTION_FORMAT_HTML is not supported. However, the all target still allows to have HTML (Same for VCARD)?

I hope to set "cnp_mode" as beta and release later. It will take longer to test all the modes.

Sounds good to me, this will likely also undergo changes when we work on the new cnp stuff.

  • You likely want to get rid of the hoversel selection there, and use something like Efl.Ui.Popup which has a anchor property.

This can take a bit longer to implement. I hope it would be handled later.

Mhm, it should not be too difficult, i can help with it if you want to.

  • _mouse_down_cb button 2 should not use the clipboard CNP buffer, that will mess up how things work in X

I think we can remove the code for button 2.

I think we just want to have that code use EFL_UI_SELECTION_TYPE_PRIMARY instead of EFL_UI_SELECTION_TYPE_CLIPBOARD.

  • _efl_ui_textbox_efl_layout_signal_signal_emit This FIXME there looks quite critical.
3e5cfb83c092652cf0306b7f4b1a0b230fdf21bf added "signal_emit", but I cannot understand what was the issue.
So, we can remove the message_process and check the result of it.

That would be a good start i think :)

  • _efl_ui_textbox_efl_layout_signal_signal_callback_add You are adding two callbacks here,

One for base edje and the other for scroller ? then no problem maybe ?

The problem is, if both are added, you *might* emit both, which would be a bug i think.

  • The callback_add and callback_del operations are looking weird, the Efl_Layout_Signal object in the callbacks will be wrong

I am a bit confused with this, @bu5hm4n Could you explain a bit more ?

Well, the function pointer is just passed on to another object. However, the second argument in the function pointer is just the object where the signal is called on. If i add this callback on the textbox object, i would expect that this second argument is the textbox object, not the edje or scroller object.

  • the part implementation is done with FIXMEs and by hand instead of the usally used helpers, any reasons for that ?

I think @ali.alzyod had no idea on this, so @bu5hm4n could you give a guideline for this ?

Sure, @ali.alzyod, can you ping me on irc in the next week, i guess we can just talk about it there, (i dont know exactly where i need to start to explain things :))

Again! I really appreciate for your polite feedback.

np :) It just takes a loooooooot of time to get to it :)

One last comment: I would rename selection_handler_enabled to selection_handles_enabled. I think we should be talking about selection handles (a graphical gadget that helps you select text) and not selection handlers (an abstract "handler" that nobody knows what it does).

One last comment: I would rename selection_handler_enabled to selection_handles_enabled. I think we should be talking about selection handles (a graphical gadget that helps you select text) and not selection handlers (an abstract "handler" that nobody knows what it does).

Done in D11042

woohyun added a comment.EditedTue, Jan 14, 12:37 AM

I hope to keep "item_factory" as @beta one.
This is because current definition of Textblock_Factory can be developed with better way (T8568).

I don't think there would be any problem even though we would not support this now, since this feature is not such a basic feature for ui_textbox.

Plus, as we talked above, "cnp_mode" should be set as @beta together :)

@segfaultxavi @bu5hm4n @ali.alzyod
How do you think about this ?

I still don't know how factories work, I cannot help much here.
If you say it is not urgent, we can keep it beta, sure.

I agree, lets keep these @beta.

what is text_table, and why is it used ?

it is a container for guided text and internal interactive text

You should not theme in _efl_ui_textbox_efl_object_finalize, but rather in theme_apply
All part swallow things should be done in theme_apply.

Let's discuss exactly what should be changed in D11103

what is text_table, and why is it used ?

it is a container for guided text and internal interactive text

Mhm, table is quite expensive just to stack two elements above each other. i will check if i can find something that improves this.

You should not theme in _efl_ui_textbox_efl_object_finalize, but rather in theme_apply
All part swallow things should be done in theme_apply.

Let's discuss exactly what should be changed in D11103

Okay

Something else discovered due to reviewing D11107. _part_is_efl_ui_textbox_part returns valid parts for each and every part name. However, _efl_ui_textbox_text_set and _efl_ui_textbox_text_get do only work for "efl.text_guide" or "efl.text". I think we should change _part_is_efl_ui_textbox_part that it only works for "efl.text_guide" and "efl.text", for the rest EINA_FALSE should be returned.

Something else discovered due to reviewing D11107. _part_is_efl_ui_textbox_part returns valid parts for each and every part name. However, _efl_ui_textbox_text_set and _efl_ui_textbox_text_get do only work for "efl.text_guide" or "efl.text". I think we should change _part_is_efl_ui_textbox_part that it only works for "efl.text_guide" and "efl.text", for the rest EINA_FALSE should be returned.

D11109

This should be ready for stabilization , any more comments?

I'm also ok with current definition :)

Another opinion ?

I am okay with the API side, only one thing impacts the API right now:

  • _efl_ui_textbox_efl_layout_signal_signal_callback_add still has a problem, the object of the function callback will be wrong

From the impl side:

  • Code structures like top = elm_widget_top_get(data); and if (efl_isa(top, EFL_UI_WIN_CLASS)) can be replaced with top = efl_provider_find(widget, EFL_UI_WIN_CLASS) No type check needed. (That should enhance the performance)
  • efl_ui_textbox.c:825 this variable is buggy now, its not assigned anymore, however, the result of efl_provider_find(obj, EFL_UI_WIN_CLASS); is geranteed to be a window. (efl_ui_textbox.c:1295 also can be replaced with provider find.) .

Otherwise it looks good!

I am okay with the API side, only one thing impacts the API right now:

  • _efl_ui_textbox_efl_layout_signal_signal_callback_add still has a problem, the object of the function callback will be wrong

what do you suggest? I look at other parts of the toolkit, and they use it the same way as in efl.ui.textbox (like efl_ui_image_zoomable and efl_ui_layout_base)

From the impl side:

  • Code structures like top = elm_widget_top_get(data); and if (efl_isa(top, EFL_UI_WIN_CLASS)) can be replaced with top = efl_provider_find(widget, EFL_UI_WIN_CLASS) No type check needed. (That should enhance the performance)

I remember changing this part O_o D11127

  • efl_ui_textbox.c:825 this variable is buggy now, its not assigned anymore, however, the result of efl_provider_find(obj, EFL_UI_WIN_CLASS); is geranteed to be a window. (efl_ui_textbox.c:1295 also can be replaced with provider find.) .

I do not understand well your comments, efl_ui_textbox.c:825 and efl_ui_textbox.c:1295 already uses efl_provider_find, but I found using of uninitialized var D11127

Otherwise it looks good!