Page MenuHomePhabricator

Efl.Text.Cursor
ClosedPublic

Authored by ali.alzyod on Oct 28 2019, 10:29 AM.

Details

Summary

Implementation of new cursor text object.

This Patch Contains :
1- Remove Efl.Text.Cursor & Efl.Text_Markup_Interactive interfaces and replace them with one Class Efl.Text.Cursor

=> there are some modifications on cursor methods

2- Update all related classes to use Efl.Text.Cursor object instead of the old interfaces

3- If class uses Efl.Text_Cursor_Cursor (handle), mainly annotation it will stay as it is until we update other annotations into attribute_factory

4- Add main cursor property into efl.text.interactive

5- Add cursor_new method in efl.ui.text (I think we may move it into efl.text.interactive interface)

There still some parts that need discussion: especially cursor movement functionality, I prefer to move function with Enum, instead of special function for each movement.

enum @beta Efl.Text.Cursor_Move_Type
{
   [[Text cursor movement types]]
   char_next,       [[Advances to the next character]]
   char_prev,       [[Advances to the previous character]]
   cluster_next,    [[Advances to the next grapheme cluster]]
   cluster_prev,    [[Advances to the previous grapheme cluster]]
   paragraph_start, [[Advances to the first character in this paragraph]]
   paragraph_end,   [[Advances to the last character in this paragraph]]
   word_start,      [[Advance to current word start]]
   word_end,        [[Advance to current word end]]
   line_start,      [[Advance to current line first character]]
   line_end,        [[Advance to current line last character]]
   paragraph_first, [[Advance to current paragraph first character]]
   paragraph_last,  [[Advance to current paragraph last character]]
   paragraph_next,  [[Advances to the start of the next text node]]
   paragraph_prev   [[Advances to the end of the previous text node]]
}
move {
         [[Move the cursor]]
         params {
            @in type: Efl.Text.Cursor_Move_Type; [[The type of movement]]
         }
         return: bool; [[True if actually moved]]
      }

or old way:

char_next {
         [[Advances to the next character]]
         // FIXME: Make the number of characters we moved by? Useful for all the other functions
         return: bool; [[True if actually moved]]
      }
      char_prev {
         [[Advances to the previous character]]
         return: bool; [[True if actually moved]]
      }

      char_delete {
         [[Deletes a single character from position pointed by given cursor.]]
      }

      cluster_next {
         [[Advances to the next grapheme cluster]]
         return: bool; [[True if actually moved]]
      }
      cluster_prev {
         [[Advances to the previous grapheme cluster]]
         return: bool; [[True if actually moved]]
      }

      // FIXME: paragraph_end is inconsistent with word_end. The one goes to the last character and the other after the last character.
      paragraph_start {
         [[Advances to the first character in this paragraph]]
         return: bool; [[True if actually moved]]
      }
      paragraph_end {
         [[Advances to the last character in this paragraph]]
         return: bool; [[True if actually moved]]
      }
      word_start {
         [[Advance to current word start]]
         return: bool; [[True if actually moved]]
      }
      word_end {
         [[Advance to current word end]]
         return: bool; [[True if actually moved]]
      }
      line_start {
         [[Advance to current line first character]]
         return: bool; [[True if actually moved]]
      }
      line_end {
          [[Advance to current line last character]]
         return: bool; [[True if actually moved]]
      }
      paragraph_first {
         [[Advance to current paragraph first character]]
         return: bool; [[True if actually moved]]
      }
      paragraph_last {
         [[Advance to current paragraph last character]]
         return: bool; [[True if actually moved]]
      }
      paragraph_next {
         [[Advances to the start of the next text node]]
         return: bool; [[True if actually moved]]
      }
      paragraph_prev {
         [[Advances to the end of the previous text node]]
         return: bool; [[True if actually moved]]
      }

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
ali.alzyod added inline comments.Nov 11 2019, 1:23 AM
src/lib/evas/canvas/efl_canvas_text.eo
27

done

segfaultxavi requested changes to this revision.Nov 11 2019, 2:40 AM

Good progress!
There are still some inline comments left to address, and some general ones too:

  • All documentation sentences must end with a period, start with uppercase and have no trailing or heading whitespace.
  • I am usually confused because we call "cursor" to both the external blinking object used to insert new chars and the internal thing used to mark positions in the text. There is only one "external" cursor but there can be more than one "internal" cursor. Can we call the external one caret instead?
  • Make sure all the new API has unit tests.
src/lib/evas/canvas/efl_canvas_text.eo
19

Then I would add a comment in that regard. Something like:

It is typically more convenient to obtain a cursor directly from the text object using @.cursor_create.

Otherwise, users won't know which method to use.

src/lib/evas/canvas/efl_text_cursor.eo
7

I know, I know, but since you are heavily modifying this file, it's our chance to fix that :)

16

Don't tell it to me, tell it to the user reading the docs!

55

Please write that down in the docs.

76

What is ' |' cursor mode? Please use a link to the right enum value.

This revision now requires changes to proceed.Nov 11 2019, 2:40 AM
ali.alzyod updated this revision to Diff 26802.Nov 11 2019, 4:51 AM
ali.alzyod marked 2 inline comments as done.

update documentation

I am usually confused because we call "cursor" to both the external blinking object used to insert new chars and the internal thing used to mark positions in the text. There is only one "external" cursor but there can be more than one "internal" cursor. Can we call the external one caret instead?

@segfaultxavi , the property name is main_cursor so this indicates what it do.

src/lib/evas/canvas/efl_text_cursor.eo
16

This should be known by the user who uses this enum, the term "grapheme cluster"

ali.alzyod updated this revision to Diff 26809.Nov 11 2019, 9:32 AM
ali.alzyod marked an inline comment as done.

remove un needed passed parameter

woohyun added inline comments.Nov 11 2019, 8:53 PM
src/lib/evas/canvas/efl_canvas_text.eo
19

I think we can support the both.
This is because -
"cursor_add" can be meaningful to the developer who want to re-use the cursor class instance.

segfaultxavi added inline comments.Nov 12 2019, 2:18 AM
src/lib/evas/canvas/efl_canvas_text.eo
19

Sure, I am not saying we should remove cursor_add. I'm just saying we should add a comment telling the user that the recommended way is to use cursor_create.

ali.alzyod added inline comments.Nov 14 2019, 1:42 AM
src/lib/evas/canvas/efl_text_cursor.eo
33

I think this is fine after we discuss it in IRC
Efl.Text is an interface, and it is fine to use Efl.Text namespace

I think this looks good now. If there is no more feedback, I'll close this soon.

woohyun accepted this revision.Thu, Nov 14, 8:38 PM

@ali.alzyod

Please do rebase ~ then, I'll close this.

@ali.alzyod

Please do rebase ~ then, I'll close this.

rebase done

The testcases for test creation via _efl_canvas_text_cursor_create but can a user also create it directly with a the class object ? Is that tested ? What is happening with operations called when no text object is associated? is it verified that this is not crashing, but erroring ? Additionally, is this here tested with ASAN ?

src/lib/evas/canvas/efl_text_cursor.c
89

Theoretically there is no need to memcpy. You can just *geometry2 = rc;

436

You probebly want to error in the else case.

src/lib/evas/canvas/efl_text_cursor.eo
118

I think this comment is not done.

Compare two cursors
           return <0 if cursor less than dst, 0 if cursor == dest and >0 otherwise.

Does not tell me when the cursor is less than another cursor, is it speaking about beeing *before* or *after* each other ? How does that work with RTL vs. LTR ? Additional, a fullstop after sentences, starting a new sentence with a capital letter.

213

removing the ptr(...) arround the rect here means that the iterator will still contain references to rectangles. The ptr is useless. (But blocks removing @beta at some point)

238

I'm adding the comment back here, so we know what we are talking about.
Whatever will be returned here in future. This needs a better type than Efl.Object, *or* it needs better documentation. Also keep in mind, that this way a bindings user has to handcast the object to make it useable. Which is IMO not really acceptable.

The testcases for test creation via _efl_canvas_text_cursor_create but can a user also create it directly with a the class object ? Is that tested ? What is happening with operations called when no text object is associated? is it verified that this is not crashing, but erroring ? Additionally, is this here tested with ASAN ?

The testcases for test creation via _efl_canvas_text_cursor_create but can a user also create it directly with a the class object ?

Yes he can, but he needs to add it to text object after creating it.

Is that tested ?

Actually we attended to use cursor_create in test, to show how simple to create cursor with this method, (cursor_create internally create cursor object and add it to the object ).

What is happening with operations called when no text object is associated?

Same as in legacy when you pass cursor value as null, nothing will happen

is it verified that this is not crashing, but erroring?

No errors, it just does nothing and returns

Additionally, is this here tested with ASAN ?

Yes I did, I see some errors but they are not generated from the Cursor patch.

src/lib/evas/canvas/efl_text_cursor.c
436

this is internal function so adding error here without caring about function call stack/propagation will be useful.

src/lib/evas/canvas/efl_text_cursor.eo
118

to simplify this depends on the position property of the cursor

238

If I understand your comment:
1- User at runtime time can check test object class.
2- this could be canvas/ui objects and in future, it can be another type of objects

ali.alzyod added inline comments.Sun, Nov 17, 5:25 AM
src/lib/evas/canvas/efl_text_cursor.c
436

*will be unuseful

Okay, i think it would be quite good to add these tests (about creating the textobject by hand) and then calling API without a text object, just to ensure, that there are no crashes etc.
It also might be worth thinking about if its acceptable that there are no errors, maybe this is hiding some bugs ?
Last but not least, if there are errors with ASAN (that are no leaks), then it does not really matter if this is caused by this patch or not, its not really acceptable to have a new commit that first leads to crashes in things that have been working before. These things needs clarification beforehand.

src/lib/evas/canvas/efl_text_cursor.c
436

Error are always usefull. If its not helping the API user, then its helping the next person who has to patch a few things around here. Errors are never unusefull ... :)

src/lib/evas/canvas/efl_text_cursor.eo
118

Okay, please write into the function definition, so others do not ask themselfs the same things ... :)

238

Letting the user check the type of a returned object, is not really good style nor is it something that we should aim for IMO. I know that you want to return here different types, depending on what the text_object of the cursor is. However, there should be one ground basic type that shares most of the shared API between those two objects, and this type should be returned here. This way at least for standard operations, you do not have to cast the type. Relying on the API-user to cast a object is something we should try to avoid as much as possible.

Okay, i think it would be quite good to add these tests (about creating the textobject by hand) and then calling API without a text object, just to ensure, that there are no crashes etc.

Ok, we add it to the test

It also might be worth thinking about if its acceptable that there are no errors, maybe this is hiding some bugs?

we are here doing the same behaviour as in legacy, but If it is must-have feature, I am ok to do it (or we can do it later)

Last but not least, if there are errors with ASAN (that are no leaks), then it does not really matter if this is caused by this patch or not, its not really acceptable to have a new commit that first leads to crashes in things that have been working before. These things needs clarification beforehand.

What I mean is that ASAN will print errors when running ninja test even on the master branch, so it is something not related to this patch.

src/lib/evas/canvas/efl_text_cursor.c
436

I am not saying errors are not useful, but since EFL function does not return errors anyway, this can be confusing who actually creates the error.
Anyway since you suggest it is useful I will add it.

src/lib/evas/canvas/efl_text_cursor.eo
118

I did.
Return <0 if cursor position less than dst, 0 if cursor == dest and >0 otherwise.

238

Ok, I will set it as Efl.Canvas.Object

segfaultxavi requested changes to this revision.Tue, Nov 19, 2:52 AM

There are still many open comments.

src/lib/elementary/efl_text_interactive.eo
9

I think this needs a bit more detail. For example:

The cursor visible to the user, used to modify text. Any number of cursors can be obtained for any text object using @.cursor_create, but @.main_cursor is the only visible one.
src/lib/evas/canvas/efl_text_cursor.eo
16

Without empirical data I cannot rebate this.

My thought is that a user wanting to move a cursor through a text will arrive to this enum and start reading the different options. I am not convinced the majority of users will know what a "grapheme cluster" is... keep in mind these are not required for English or western European languages.

Anyway, I would just add: ... grapheme cluster (A character sequence rendered together. See https://unicode.org/reports/tr29/).

This revision now requires changes to proceed.Tue, Nov 19, 2:52 AM
ali.alzyod updated this revision to Diff 26978.Tue, Nov 19, 3:20 AM
ali.alzyod marked an inline comment as done.

update documentation

src/lib/elementary/efl_text_interactive.eo
9

This interface does not have ((cursor_create))

There are still many open comments.

I do not find any open comment

ali.alzyod added inline comments.Tue, Nov 19, 3:42 AM
src/lib/elementary/efl_text_interactive.eo
9

I feel if we start mention methods in other classes, this will confuse the user rather than helping him.

segfaultxavi added inline comments.Tue, Nov 19, 4:04 AM
src/lib/elementary/efl_text_interactive.eo
9

I think it is worth stating the difference between the main_cursor and all the other cursors.

My text is a suggestion. You can replace @.cursor_create with the appropriate reference (To Canvas.Text, for example?).

segfaultxavi requested changes to this revision.Tue, Nov 19, 4:10 AM

I did find open comments in efl_text_cursor.eo for example.

src/lib/evas/canvas/efl_text_cursor.eo
16

"Grapheme cluster" appears twice now.

This revision now requires changes to proceed.Tue, Nov 19, 4:10 AM
ali.alzyod added a comment.EditedTue, Nov 19, 5:57 AM

I did find open comments in efl_text_cursor.eo for example

I already commented about this, and I was waiting for your feedback.
This is general concepts, and I do not think it is part of our rule to define these concepts.
This link can become invalid at anytime (unicode link), and we will not know about that.
Anyway I already add it.

ali.alzyod added inline comments.Tue, Nov 19, 6:01 AM
src/lib/elementary/efl_text_interactive.eo
9

I feel mentioning other classes property in this interface is not good thing to do
First why user would mix between interface properties and classes properties.
Second Will we keep mention other classes in future if the deal with cursor in this propery documentation?

Anyway if you insist about it , I am ok to add it.

segfaultxavi resigned from this revision.Tue, Nov 19, 7:49 AM

I think this is OK regarding the API. I will take care of the rest of the documentation concerns in future patches.

I will let somebody else finish the implementation review.

This revision is now accepted and ready to land.Tue, Nov 19, 7:49 AM
woohyun accepted this revision.Fri, Nov 22, 12:43 AM
This revision was automatically updated to reflect the committed changes.

This patch seems to have caused troubles to Travis:
https://travis-ci.org/Enlightenment/efl/jobs/615447302

../src/lib/elementary/efl_ui_internal_text_interactive.c:827:20: warning: implicit declaration of function 'efl_text_cursor_word_end'
This is the code:

if defined(__APPLE__) && defined(__MACH__)
        if (altgr) efl_text_cursor_word_end(cur);
#else
        /* If control is pressed, go to the end of the word */
        if (control) efl_text_cursor_move(cur, EFL_TEXT_CURSOR_MOVE_TYPE_WORD_END);
#endif

@segfaultxavi

Thanks for sharing the issue. I would handle it soon.

It is also breaking build if C# bindings are enabled.

woohyun added a comment.EditedFri, Nov 22, 3:47 AM

It is also breaking build if C# bindings are enabled.

@segfaultxavi

The error message is -
src/bindings/mono/efl_canvas_text.eo.cs(504,68): error CS0234: The type or namespace name TextCursorHandle' does not exist in the namespace Efl'. Are you missing an assembly reference?

Am I right ?

And I fix Travis problem in https://phab.enlightenment.org/D10725. :)

Yes, that's the problem we're seeing.
Your fix for the Mac build has already landed, thanks!

Yes, that's the problem we're seeing.
Your fix for the Mac build has already landed, thanks!

@segfaultxavi
Woops. the binding problem was also fixed (b22594a10bc55c1083d15354a956bb8d84f18052)

Thanks @lauromoura !!!! :)

@ali.alzyod While extending the documentation I realized I do not know what happens if you assign a text cursor to a text object (with cursor_add) and then assign it again to a different text object.
Is this scenario handled correctly? What's the outcome?

ali.alzyod added a comment.EditedThu, Nov 28, 1:33 AM

@ali.alzyod While extending the documentation I realized I do not know what happens if you assign a text cursor to a text object (with cursor_add) and then assign it again to a different text object.
Is this scenario handled correctly? What's the outcome?

It will be removed from the old one, and assigned to the new one, in other words cursor will effect last text object only

Thanks! I've updated the docs with this information.