Page MenuHomePhabricator

efl_text_interactive: selection enhancment
ClosedPublic

Authored by ali.alzyod on Dec 26 2019, 7:38 AM.

Details

Summary

1- Implement setting selection range programmatically by modifying selection cursors from efl_text_interactive_selection_cursors_get
2- Add setter with efl_text_interactive_selection_cursors_set to set the range at once (modify start and end)

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

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.Dec 26 2019, 7:38 AM
  • update + rebase
woohyun added inline comments.Dec 29 2019, 9:47 PM
src/lib/elementary/efl_ui_internal_text_interactive.c
937

I think that this _sel_clear will emit "selection changed", and _sel_extend (in the last line of this func) will also emit the same signal.

segfaultxavi added inline comments.Dec 30 2019, 7:32 AM
src/lib/elementary/efl_text_interactive.eo
30

typo

ali.alzyod updated this revision to Diff 27953.Jan 4 2020, 11:45 PM
ali.alzyod marked 2 inline comments as done.
  • update
ali.alzyod added inline comments.Jan 4 2020, 11:45 PM
src/lib/elementary/efl_ui_internal_text_interactive.c
937

We added a new parameter to _sel_clear to prevent the emit of events.

We need this call. because if there was partial selection then user hit select all, we need to allow selection_start cursor to reposition again.

bu5hm4n requested changes to this revision.Jan 6 2020, 3:58 AM

Looks good otherwise.

Why are you freezing the cursor objects around copying them ? There is no event emitted when copying the cursor ...

src/lib/elementary/efl_text_interactive.eo
31

You should write here as well that setting the cursor objects will only copy the position over, modifying the cursor objects (those that you set) *later on* does not change the selection of the text interactive.

src/tests/elementary/efl_ui_test_text.c
92

Can you ensure that the selection changed events are emitted correctly during these operations ?

This revision now requires changes to proceed.Jan 6 2020, 3:58 AM
ali.alzyod updated this revision to Diff 27974.Jan 6 2020, 6:01 AM
ali.alzyod marked 2 inline comments as done.

update

@bu5hm4n
I think @ali.alzyod have updated for your comments.
Do you have another idea ?
I think this looks good for me :)

I am still missing a answer to the question why things here have been freezed ?

src/lib/elementary/efl_text_interactive.eo
31

The comment is not really done. The comment requested here is meant to be in the set{} block, (so it's not displayed in the getter).

Additionally, this still misses the mentioning, that later changes to the setter are not going to be applied to the internal cursor objects.

ali.alzyod updated this revision to Diff 28040.Jan 9 2020, 1:35 AM

update documentation

ali.alzyod marked an inline comment as done.Jan 9 2020, 1:35 AM
ali.alzyod added a comment.EditedJan 9 2020, 1:46 AM

I am still missing a answer to the question why things here have been freezed ?

Sorry, I missed question,
No need to freeze because of copy does not submit change events, (We do not even have copy anymore :) ), these should be replaced with set_position

@bu5hm4n after D11034 land, this patch needs rebase anyway

@ali.alzyod
D11034 was landed ~ so please do rebase :)

bu5hm4n added a comment.EditedJan 10 2020, 2:25 AM

One thing is the rebase, the other thing are these freeze and thaws arround the copy. You say they are not needed, but they are still there, so they are planned to be removed ?

ali.alzyod added a comment.EditedJan 11 2020, 5:52 AM

One thing is the rebase, the other thing are these freeze and thaws arround the copy. You say they are not needed, but they are still there, so they are planned to be removed ?

As you know there are no copy method in cursor, so we are setting position and we need to freeze the events

@ali.alzyod

As you know there are no copy method in cursor, so we are setting position and we need to freeze the events

You may not want to callback_call for CURSOR_EVENT_CHANGED below. Am I right ?

static void _evas_textblock_cursor_object_changed(Efl_Text_Cursor_Handle *cur)  
{                                                                               
   if (!cur || !cur->cur_objs) return;                                          
   Eina_List *l;                                                                
   Eo *cur_obj;                                                                 
                                                                                
   EINA_LIST_FOREACH(cur->cur_objs, l, cur_obj)                                 
     efl_event_callback_call(cur_obj, EFL_TEXT_CURSOR_EVENT_CHANGED, NULL);     
}

One thing I'm concerned is whether "cursor changed" event will not be called after efl_event_thaw(en->sel_start).
That is, while event is freezed, callback_call would be ignored ? or saved in an event queue to be called later after event_thaw ?

ali.alzyod added a comment.EditedJan 12 2020, 11:04 PM

@ali.alzyod

As you know there are no copy method in cursor, so we are setting position and we need to freeze the events

You may not want to callback_call for CURSOR_EVENT_CHANGED below. Am I right ?

static void _evas_textblock_cursor_object_changed(Efl_Text_Cursor_Handle *cur)  
{                                                                               
   if (!cur || !cur->cur_objs) return;                                          
   Eina_List *l;                                                                
   Eo *cur_obj;                                                                 
                                                                                
   EINA_LIST_FOREACH(cur->cur_objs, l, cur_obj)                                 
     efl_event_callback_call(cur_obj, EFL_TEXT_CURSOR_EVENT_CHANGED, NULL);     
}

this is the only place to submit this event, since cursor can be changed internally, for example content removed, all cursor that are not set to position 0, will be moved to 0.
Do you see a problem in this call ?

One thing I'm concerned is whether "cursor changed" event will not be called after efl_event_thaw(en->sel_start).
That is, while event is freezed, callback_call would be ignored ? or saved in an event queue to be called later after event_thaw ?

As far as I know, no events will be submitted after event_thaw. (they are frozen and died :) )

One thing is the rebase, the other thing are these freeze and thaws arround the copy. You say they are not needed, but they are still there, so they are planned to be removed ?

As you know there are no copy method in cursor, so we are setting position and we need to freeze the events

Yeah - i guessed that - however. When i am setting the selection cursors on a textbox object, they will not emit events for the position to be changed, i guess that cannot be the goal of that ? We should ensure that the event works correctly there.

One thing is the rebase, the other thing are these freeze and thaws arround the copy. You say they are not needed, but they are still there, so they are planned to be removed ?

As you know there are no copy method in cursor, so we are setting position and we need to freeze the events

Yeah - i guessed that - however. When i am setting the selection cursors on a textbox object, they will not emit events for the position to be changed, i guess that cannot be the goal of that ? We should ensure that the event works correctly there.

I am not sure I fully understand your comment, Line 92 in test already test the selection change event. (position change event for cursor should no be submited)

ali.alzyod updated this revision to Diff 28277.Jan 19 2020, 8:01 AM

update have_selection changed when reduce selection area

ali.alzyod updated this revision to Diff 28278.Jan 19 2020, 8:25 AM

add more test cases

@ali.alzyod
Could you do rebase :)

@bu5hm4n
Are you ok with current patch ?

remove all freeze events

woohyun accepted this revision.Jan 20 2020, 2:43 PM
This revision was not accepted when it landed; it landed in state Needs Review.Jan 20 2020, 2:44 PM
This revision was automatically updated to reflect the committed changes.
bu5hm4n reopened this revision.Jan 21 2020, 8:13 AM

This did not pass tests. Lets revisit it.

ali.alzyod updated this revision to Diff 28326.Jan 21 2020, 8:39 AM

update selection

ali.alzyod updated this revision to Diff 28328.Jan 21 2020, 9:23 AM

clean up + update

ali.alzyod updated this revision to Diff 28329.Jan 21 2020, 9:26 AM

remove empty lines

ali.alzyod added a subscriber: stefan_schmidt.EditedJan 21 2020, 9:27 AM

@bu5hm4n @segfaultxavi @stefan_schmidt
If anyone one of you can run test on this patch with many many thanks :)

init watch sel value

bu5hm4n accepted this revision.Jan 23 2020, 12:04 AM

Has passed the stress test here!

This revision is now accepted and ready to land.Jan 23 2020, 12:04 AM
Closed by commit rEFL4cdd5505e957: efl_text_interactive: selection enhancment (authored by ali.alzyod, committed by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain WhyJan 23 2020, 12:05 AM
This revision was automatically updated to reflect the committed changes.