Page MenuHomePhabricator

efl_text_interactive: selection enhancment
Needs ReviewPublic

Authored by ali.alzyod on Thu, Dec 26, 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
Branch
arcpatch-D10968
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 15330
Build 10547: arc lint + arc unit
ali.alzyod created this revision.Thu, Dec 26, 7:38 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.Thu, Dec 26, 7:38 AM
  • update + rebase
woohyun added inline comments.Sun, Dec 29, 9:47 PM
src/lib/elementary/efl_ui_internal_text_interactive.c
918

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.Mon, Dec 30, 7:32 AM
src/lib/elementary/efl_text_interactive.eo
30

typo

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

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.Mon, Jan 6, 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.Mon, Jan 6, 3:58 AM
ali.alzyod updated this revision to Diff 27974.Mon, Jan 6, 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.Thu, Jan 9, 1:35 AM

update documentation

ali.alzyod marked an inline comment as done.Thu, Jan 9, 1:35 AM
ali.alzyod added a comment.EditedThu, Jan 9, 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.EditedFri, Jan 10, 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.EditedSat, Jan 11, 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.EditedSun, Jan 12, 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)