Page MenuHomePhabricator

Efl.Ui.Text : all related interfaces
ClosedPublic

Authored by ali.alzyod on Thu, Nov 21, 12:59 AM.

Details

Summary

Change summary:

Removed :

  • efl_ui_text_selectable.eo and add it functionality into efl_text_interactive.eo
  • efl_ui_text_editable.eo because it is same as efl_ui_text.eo (just set one property by default)

Modifications:

  • Move all Text Input enums and functionality from efl_ui_text class into its own interface, this interface will be implemented at efl_ui_internal_text_interactive class.
  • Rename selection_allowed property to selectable (same as other "editable" property) in efl_text_interactive
  • Add select_all function into efl_text_interactive interface
  • Add have_selection property into efl_text_interactive interface
  • Move user_change , selection_start/change/clear and undo/redo events into efl_text_interactive interface.
  • Move methods and events of copy/paste/cut into efl_ui_text
  • Fix password-related functionality
  • Remove context menu clear and add_item methods. (these should be added later with better design)
  • Remove Validation event from EFL_UI_TEXT. (these should be added later with better design)

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.
There are a very large number of changes, so older changes are hidden. Show Older Changes
bu5hm4n added inline comments.Sun, Dec 1, 11:49 PM
src/lib/elementary/elm_entry.c
1670

As explaind above, this breaks event submission for evas_object_smart_callback_add events.

2457

As explaind above, this breaks event submission for evas_object_smart_callback_add events.

This revision now requires changes to proceed.Sun, Dec 1, 11:49 PM
ali.alzyod added inline comments.Mon, Dec 2, 12:07 AM
src/lib/elementary/efl_text_interactive.eo
73

I understand your point (I think),

First : my argument about the waste of power, we will keep running callback in user code even if he is not interested in.
For example, user register call back for selection_clear, his call back will be called if selection is cleared, but if we combine all events in one event, his call back will be called for (selection_clear,selection_start,selection_change), and he need to check what is this event using custom structure in event data then return( this is what I mean by waste if process/power).

I like to think if, like a mouse event, we do not combine them in one event, because it will simplify writing the code, and a lot of time user is not interested in all events (he just need to listen to some of them).

src/lib/elementary/elc_fileselector_entry.c
63

@bu5hm4n Can you please provide more explanation I do not get what is the problem.
Currently, there are no EFL_UI_EVENT_SELECTION_ events. also why selection event is emitted in legacy with a copy ?

ali.alzyod marked an inline comment as done.Mon, Dec 2, 12:12 AM
ali.alzyod added inline comments.
src/lib/elementary/efl_text_interactive.eo
16

I wanted to use the same naming convention for boolean properties.
we have editable, predictable. @segfaultxavi Can you please help with good naming for this boolean property which can be mixed with the others.

woohyun added inline comments.Mon, Dec 2, 12:13 AM
src/lib/elementary/efl_text_interactive.eo
70–81

@ali.alzyod
You already did give good explanation.

@segfaultxavi
For better understanding, you can check the link below.
https://github.com/flutter/flutter/issues/40153

In the link, you can see a movie with editing Korean word.
In the movie, you can see "underline" just under the Korean character.
It shows that the word is still in combining.

This state means "preedit".
I hope this will make you understand well.

bu5hm4n requested changes to this revision.Mon, Dec 2, 12:35 AM
bu5hm4n added inline comments.
src/lib/elementary/efl_text_interactive.eo
73

Lets say the struct for the event is simply the selection + the number of characters in the selection, the code the user then executes, is 1 redirection, plus one conditional branch, plus one ret, that is way less code that gets executed compared to more event subscriptions, so let me repeat again, the argument of waste of process/power is invalid.

The reason mouse events are not combined is because it simply does not make sense, we normally have one event per property, this property here is the selection of the text field, if this is changed, a event is going to be emitted. Same for mouse, if the position changed, a event is emitted, if it gets pressed a event is going to be emitted. The mouse usecase and the selection usecase here are complete orthogonal things...

src/lib/elementary/elc_fileselector_entry.c
63

The current symbol is EFL_UI_EVENT_SELECTION_CUT, which means, for legacy callbacks "selection,cut" will be emitted. Which is not the case anymore with the new event. Which is why you now need to handemit "selection,*" events with evas_object_smart_callback_call. (Not only here, also in the entry code.

This revision now requires changes to proceed.Mon, Dec 2, 12:35 AM
ali.alzyod added inline comments.Mon, Dec 2, 12:47 AM
src/lib/elementary/elc_fileselector_entry.c
63

So If I rename the events to EFL_UI_TEXT_SELECTION_COPY this will fix the issue?

ali.alzyod added inline comments.Mon, Dec 2, 12:50 AM
src/lib/elementary/efl_text_interactive.eo
73

What is the use case to listen to all selection events?

bu5hm4n added inline comments.Mon, Dec 2, 2:19 AM
src/lib/elementary/efl_text_interactive.eo
73

I don't have a usecase in mind, but it's possible, which is enough to think about that. On a general note, prefiltering a event in the library code is always a bad idea, performance wise and API wise. If the user wants to know about when selection starts, compare the old value to the new value, and we are good. A user could have all sorts of filtering needs, giving him the full power (and beeing inline with the rest of the project) seems to be the better option compared to this. Esp. since we had quite a few event related performance fixes, and I fear, this is just going to be the next one of those.

src/lib/elementary/elc_fileselector_entry.c
63

Yes.

q66 added inline comments.Mon, Dec 2, 2:49 AM
src/lib/efl/interfaces/efl_input_text.eo
272

i will look at this, i have a rough idea of what's wrong

q66 added inline comments.Mon, Dec 2, 4:38 AM
src/lib/efl/interfaces/efl_input_text.eo
272
segfaultxavi requested changes to this revision.Mon, Dec 2, 4:53 AM

We're still missing some unit tests, either in this patch or in a dependent one.

src/lib/elementary/efl_text_interactive.eo
16

This is a particular case, because selectable is already used by the Collections and Items API (as @bu5hm4n says). Reusing the term would be confusing.
We historically used selection for text and we can probably keep using it. In this case, I think selection_allowed is good enough..

70–81

Thanks for the explanation. All clear now. I'll add the explanation to the docs in another patch once this lands.

ali.alzyod updated this revision to Diff 27223.Mon, Dec 2, 6:09 AM
ali.alzyod marked 30 inline comments as done.

updates

ali.alzyod added inline comments.Mon, Dec 2, 6:09 AM
src/lib/elementary/efl_text_interactive.eo
73

If there are no good use case to combine these events, then we will combine them, if events are separated and user may listen to subset of them then we should not combine them.

src/lib/elementary/efl_ui_internal_text_interactive.c
858

I think the selection_clear phase is not important anyway, if something is already selected, then extend the selection

ali.alzyod marked 2 inline comments as done.Mon, Dec 2, 6:10 AM
a.srour added a subscriber: a.srour.Mon, Dec 2, 6:23 AM
bu5hm4n requested changes to this revision.Mon, Dec 2, 6:59 AM
bu5hm4n added inline comments.
src/lib/elementary/efl_text_interactive.eo
73

Okay, it seems my message above was not clear enough:

  • You are emitting more than 1 event in the same situation, for the same thing, which will walk the whole event list twice, which is a waste of time, you can just let the user do the filter he needs, you can even pass a struct, with the information needed for doing that nice and quick.

Beside from that: Isn't the selection also "changed" when its cleared, it goes form *something* to *nothing* so for a causal joe average user, and the documentation given here, i would 100% believe this is the event i want to listen to, to get notifications about CHANGES to the SELECTION, but no, i would need to listen to others, or more events.

So for the sake of performance, and sanity, make this *one* event, with a struct as event_info. Where you can pass on how many characters are selected, maybe the delta before, and maybe the position or the cursor where everything started, with this, a user can perfectly well deal with whatever he wants to filter out.

This revision now requires changes to proceed.Mon, Dec 2, 6:59 AM
ali.alzyod added inline comments.Mon, Dec 2, 7:17 AM
src/lib/elementary/efl_text_interactive.eo
73

You do not have a use case for this, what you are suggesting is wasting process too.

You notify a user about events that he is not interested about, so this is waste of calls (for each event change).

Normally user is interested in selection,start or seleciton,clear to enable or disable ui components for selection.

ali.alzyod requested review of this revision.Mon, Dec 2, 7:17 AM
bu5hm4n requested changes to this revision.Mon, Dec 2, 7:24 AM
bu5hm4n added inline comments.
src/lib/elementary/efl_text_interactive.eo
73

You seem to not have read my comment before, or any comment before.

I am saying again and again, that walking the event subscription list twice, is *WAY* more expensive, then letting the user do the filter that he wants. You can even read up above how many instructions this will take.
Additionally, emitting the event, even if noone subscribed to it, is going to walk the list of event subscriptions.
So: NO! What i am suggesting is *not* a waste of process (actually, i am not sure if you mean performance or memory with that)

This revision now requires changes to proceed.Mon, Dec 2, 7:24 AM
ali.alzyod added inline comments.Mon, Dec 2, 7:30 AM
src/lib/elementary/efl_text_interactive.eo
73

with your current argument, all events should be combined, (like copy/paste/cut) and users do filtration will be more optimal but you are arguing only about this event, so this is not right for this event.

Please provide a use case. (these events are rarely needed together, so we should not combine them)

ali.alzyod added a comment.EditedMon, Dec 2, 7:31 AM

@bu5hm4n other than SELECTION events do you have other requested changes.

@bu5hm4n other than SELECTION events do you have other requested changes.

Unit tests. Things like:

  • verify event emission
  • verify that selection works
  • [...]
src/lib/elementary/efl_text_interactive.eo
73

No, copy/paste/cut are different things, because they are not emitted within the same action, not within the same moment, and they are not referencing to the same "thing". The reasons why i am requesting these changes to these events and not to others are given more than once above.

You are still asking for a usecase, so you still have not understood an essential thing, its not about the usecase, its about the fact, that you emit 2 different events in the same situation.

ali.alzyod added inline comments.Mon, Dec 2, 8:51 AM
src/lib/elementary/efl_text_interactive.eo
73

Ok now how about two events instead of 3 ?
selection_status_changed : selection start or ended
selection_content_changed : selection extend or shrink

almost all users are interested in selection_status_changed but it is rare to listen to selection_content_changed

bu5hm4n added inline comments.Mon, Dec 2, 9:14 AM
src/lib/elementary/efl_text_interactive.eo
73

i would call them have_selection,changed and selection,changed, then this would at least follow the guidlines of the naming. However, the problem of have_selection,changed and selection,changed would still be emitted within the same action.

ali.alzyod added inline comments.Mon, Dec 2, 9:48 AM
src/lib/elementary/efl_text_interactive.eo
73

So have_selection,changed will be the event for the user to know about selection start/clear. (fired only when selection started or ended)
and on the other hand selection,changed will be fired each and every change to selection happened, and for special kind of application user will listen to this event.

How do you feel about it ? (it is the best I can think of to keep it both more usable for developers and less expensive for EFL)

@segfaultxavi @bu5hm4n @zmike
I will add more tests in other patch tomorrow is that ok? or add it to this patch (I do not want to increase the large of this patch anymore)

ali.alzyod updated this revision to Diff 27241.Tue, Dec 3, 1:16 AM

replace selection_start,selection_clear with have_selection event
selection_changed will contain range of selected text index
have_selection_changed will contains boolean indicate if it has selectio(start) or not (clear)

bu5hm4n added inline comments.Tue, Dec 3, 1:20 AM
src/lib/elementary/efl_text_interactive.eo
73

Well, i still do not like this, as the initial concern is not met (not even addressed).

However, can we just leave these events a little bit longer @beta, I am sick of this discussion, and i am not willing to say that i am happy with this, and later be in the sucky situation to fix event-performance stuff again.

ali.alzyod updated this revision to Diff 27279.Wed, Dec 4, 1:38 AM

rebase + fix cut event

@segfaultxavi @bu5hm4n @zmike @woohyun
Please let me know for any more concerns related to this patch. (Test founded in D10776)

@bu5hm4n making have_selection_changed event as beta is ok for now. right?

ali.alzyod updated this revision to Diff 27281.Wed, Dec 4, 1:46 AM
This comment was removed by ali.alzyod.
bu5hm4n resigned from this revision.Wed, Dec 4, 7:54 AM
bu5hm4n added a subscriber: stefan_schmidt.

As i said before, my concerns regarding the events "have_selection,changed" have not been met, so i will remove them on my own. Can someone else @cedric @zmike @stefan_schmidt @segfaultxavi also apply this patch locally and run it a few days and see if there are legacy behavior changes.

ali.alzyod edited the summary of this revision. (Show Details)Thu, Dec 5, 1:25 AM

I have this example calculator application which I am adapting to these changes in a branch:
https://git.enlightenment.org/tools/examples.git/tree/apps/c/calculator/src/calculator.c?h=devs/xartigas/adapting-to-text-changes

  • After this patch, I have to manually move the cursor to the end of the line before inserting anything (previously I didn't have to). Is this intended?
  • I add characters to the Efl.Ui.Textwidget using a cursor and efl_text_cursor_text_insert. However, my _screen_changed_cb is never called, no matter if I connect to EFL_TEXT_INTERACTIVE_EVENT_CHANGED_USER or to EFL_UI_TEXT_EVENT_CHANGED. Is this intended?

@segfaultxavi Thanks a lot for sharing the demo.

I have this example calculator application which I am adapting to these changes in a branch:
https://git.enlightenment.org/tools/examples.git/tree/apps/c/calculator/src/calculator.c?h=devs/xartigas/adapting-to-text-changes

  • After this patch, I have to manually move the cursor to the end of the line before inserting anything (previously I didn't have to). Is this intended?

This was bug and I fix it, the problem was that efl_text_set was not modify main cursor position, now we move main cursor to the end if we use efl_text_set or efl_text_markup_set.

  • I add characters to the Efl.Ui.Textwidget using a cursor and efl_text_cursor_text_insert. However, my _screen_changed_cb is never called, no matter if I connect to EFL_TEXT_INTERACTIVE_EVENT_CHANGED_USER or to EFL_UI_TEXT_EVENT_CHANGED. Is this intended?

This was a bug, Now EFL_UI_TEXT_EVENT_CHANGED will be called for any change in the content.
EFL_TEXT_INTERACTIVE_EVENT_CHANGED_USER will be called only when user change the text (using keyboard or clipboard)

I think this seems ok now :)

I will wait for a day ~ and if there is no more feedback, then I'll accept and close this.

woohyun accepted this revision.Sun, Dec 8, 6:08 PM
This revision was not accepted when it landed; it landed in state Needs Review.Sun, Dec 8, 6:12 PM
This revision was automatically updated to reflect the committed changes.

The Calculator example still does not work fine.
I do not need to move the cursor to the end of the line before inserting, that's fixed.
However, I still do not get the CHANGED events when I press a number button, and I get two at the same time sometimes.
Also, efl_text_get() on the widgets seems to always return "0".

ali.alzyod added a comment.EditedMon, Dec 9, 6:11 AM

The Calculator example still does not work fine.
I do not need to move the cursor to the end of the line before inserting, that's fixed.
However, I still do not get the CHANGED events when I press a number button, and I get two at the same time sometimes.

After some investigation, find out adding text using cursor will not force the efl_canvas_text_event_changed

Also, efl_text_get() on the widgets seems to always return "0".

I could not reproduce this one, it seems to work fine.

I made small modification so text change callback will not change text (so it will not enter infinite loop)
https://github.com/Ali-Alzyoud/efl_calculator/blob/master/source.c
, The fix will be in D10834 to emit change event when change text using cursor

Thanks for the fix. I also fixed the infinite recursion problem in the calculator code and landed it in the examples repo.