Page MenuHomePhabricator

Efl.Ui.Textbox class
Closed, ResolvedPublic

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
D11236: efl.ui.textbox: add and use keyboard bindings
D11216: efl.ui.textbox: replace ecore_job with Efl_Future
D11217: efl.ui.textbox: clean up all evas_object related functions from stable methods/interfaces
D11218: efl.ui.textbox: replace strncmp with strcmp for Part
D11153: efl.ui.textbox: move file implementation in to internal class
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
D11328 / rEFL69d6ca28ab34: move stabelized items out of @beta
D11141 / rEFLb3c0da13d804: efl_ui_textbox / efl_ui_image_zoomable: remove duplicated code
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
There are a very large number of changes, so older changes are hidden. Show Older Changes
ali.alzyod added a comment.EditedDec 29 2019, 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.EditedDec 29 2019, 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.EditedJan 14 2020, 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!

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)

Mhm, yeah. This needs a solution, @zmike @segfaultxavi can you help here ?

The problem is: when the func is called there, the Object passed to the function pointer is not the object you added the callback on. I think we need to solve that. Any ideas how ?

Additionally, the implemenations in efl_ui_textbox.c are equal to efl_ui_layout.c and the efl_ui_image_zoomable implementation. Both inherit from layout, could we just remove these implementations ? (Note, edje_entry == wd->resize_obj)?

The other things do look fine, thank you.

Is it possible to annotate interface implemnetaiton as @beta ? (this is for file interface)

The problem is: when the func is called there, the Object passed to the function pointer is not the object you added the callback on.

Why? Can you give more details?

Why? Can you give more details?

Well, you add a signal callback to object X. The callback gets called. However, object Y Gets passed as object parameter, in EFL, the object parameter is normally always the object you attached the callback on. But Y is not X, so this is a bug.

Yeah, I get that :)
My question was "why does this happen?". Is it an implementation bug or a design issue?

Ah, sorry :)
This happens because we simply attach the callback in the impl to the internal resize object, and do not handle the emitting by hand at all...

OK, thanks.
Well, the obvious solution is that the textbox should intercept the resize obj's signals and emit its own signals, no?
The user does not know about any internal object, so the textobj must proxy everything.
And the other classes with the same problem should do the same :(

Don't we have event forwarders exactly for this reason?
From the docs of efl_event_callback_forwarder_priority_add:

This allow object that hide internally another object to easily be able to
propagate an event without the need to add custom handler.

Signals are sadly no events.

The AIs I am thinking about for this:

  • remove the implementations in image_zoomable / textbox (they are equal to layout)
  • implement intercepting behaviour in layout I think we can get this rather performant, just a bit sucky when it comes to memory usage. (Allocate a struct, stuff it into a hash with the func as pointer, free this memory when deleting (you can find it from the hash again)

@bu5hm4n @segfaultxavi @zmike

If we don't have a plan to remove "_efl_ui_textblox_efl_layout_signal_signal_callback_add",
I hope to talk about this in another task to focus on textbox relating things :)

Plus, can anyone give @ali.alzyod 's question ?

Is it possible to annotate interface implemnetaiton as @beta ? (this is for file interface)

I will try to remove it today, i have just too many things to do, its hard to get anything done tbh.

I talked with ali on irc, and we talked about that he could seperate the implementation to something outside the .eo file, so we keep the code and the feature, just not due to the file interface.

@bu5hm4n

Thank you very much !
I'll get info from @ali.alzyod on how to separate the implemetation.

@ali.alzyod

Thanks - and another opinion ?
I think everything seems ok now :)

I've lost track of all the things we said already, but there's been a lot of work in this widget... it must be very good by now :)

Nothing API related does show up, there are a few implementation things that need fixing however...

Nothing API related does show up, there are a few implementation things that need fixing however...

Can you be more specific?

Just by skimming over it:

  • We are still not using the keybindings interface
  • There is still legacy API arround, ecore_job etc. the job future will be easier here with efl_future_then (also evas_object_* API).
  • The macro usage of STRCMP should be reviewed, do we *really* only want to compare the first n charaters of X with Y or the two strings absolutly?
ali.alzyod added a comment.EditedJan 28 2020, 4:40 AM

We are still not using the keybindings interface

What is this?

There is still legacy API arround, ecore_job etc. the job future will be easier here with efl_future_then (also evas_object_* API).

D11217 + D11216

The macro usage of STRCMP should be reviewed, do we *really* only want to compare the first n charaters of X with Y or the two strings absolutly?

D11218

ali.alzyod added a comment.EditedJan 29 2020, 3:03 AM

Now can we stabilize this one? :)

@bu5hm4n Are there anymore concerns you have?

There is still the thing with the keybindings (Take efl.ui.button as an example):

  • you have a abstract array defining what key-related actions you have (array named key_actions)
  • ELM_WIDGET_KEY_DOWN_DEFAULT_IMPLEMENT will bind key down events to the actual actions.
  • in config files there are groups called Elm_Config_Bindings_Widget, which specify the actual keys that are ending up calling the action.

With all this, a "deployer" has the power of deciding which keys should trigger which action, additionally, you do not need to care about differences like mac vs. the rest of the world, thats handled by the config then. Same for the event flags, they are also handled somewhere else. (And, last but not least, the access implementation is way easier when you have that array)

There is still the thing with the keybindings (Take efl.ui.button as an example):

  • you have a abstract array defining what key-related actions you have (array named key_actions)
  • ELM_WIDGET_KEY_DOWN_DEFAULT_IMPLEMENT will bind key down events to the actual actions.
  • in config files there are groups called Elm_Config_Bindings_Widget, which specify the actual keys that are ending up calling the action.

    With all this, a "deployer" has the power of deciding which keys should trigger which action, additionally, you do not need to care about differences like mac vs. the rest of the world, thats handled by the config then. Same for the event flags, they are also handled somewhere else. (And, last but not least, the access implementation is way easier when you have that array)

Wow, we already have this in EFL :), actually, I put this in my list for the future, because our hardcoded keys/actions were not flexible anyway, and we do not provide the user with customization options.
Thanks a lot

There is still the thing with the keybindings (Take efl.ui.button as an example):

  • you have a abstract array defining what key-related actions you have (array named key_actions)
  • ELM_WIDGET_KEY_DOWN_DEFAULT_IMPLEMENT will bind key down events to the actual actions.
  • in config files there are groups called Elm_Config_Bindings_Widget, which specify the actual keys that are ending up calling the action.

    With all this, a "deployer" has the power of deciding which keys should trigger which action, additionally, you do not need to care about differences like mac vs. the rest of the world, thats handled by the config then. Same for the event flags, they are also handled somewhere else. (And, last but not least, the access implementation is way easier when you have that array)

I hope this D11236 will fix it

ali.alzyod added a comment.EditedFeb 4 2020, 1:54 AM

I think now we need to remove (or at least move) accessibility interfaces into internal class, since we will not stibalize them

Accessibility is still @beta so this does not get exposed.

Accessibility is still @beta so this does not get exposed.

After this we are ready for stabilization, right ?

@bu5hm4n I think now everything is ready for stabilization. right ?

I dont feel comfy doing any final call on that @woohyun @zmike @segfaultxavi any last comments ?

I did a last review of the API and I cannot see anything wrong.
We have not talked about the selection_copy/cut/paste methods at the end. I guess these align well with the new C&P API, @bu5hm4n?

They do not really care for the C&P stuff, they are just doing the operation which uses C&P in the background. But they look fine.

In that case, I'm OK with the current state of this.

I think this looks good now :) And we can move this to "stabilized'.

Thank you very much !!!

woohyun moved this task from Evaluating to Stabilized on the efl: api board.Feb 9 2020, 4:41 PM
ali.alzyod closed this task as Resolved.Apr 30 2020, 1:56 AM