Page MenuHomePhabricator

Efl.Text.Attribute_Factory
ClosedPublic

Authored by ali.alzyod on Nov 11 2019, 8:47 AM.

Details

Summary

Implementation of new Efl.Text.Attribute_Factory class which replace the annotation interface.

Currently, we have two public methods:

void efl_text_attribute_factory_attribute_insert(const Efl_Text_Cursor *start, const Efl_Text_Cursor *end, const char *format)
unsigned int efl_text_attribute_factory_attribute_clear(const Efl_Text_Cursor *start, const Efl_Text_Cursor *end);

Other methods will be internal methods, for the time being, we will redesign internal methods

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
woohyun added inline comments.Tue, Nov 19, 9:36 PM
src/lib/evas/canvas/efl_text_attribute_factory.c
32

Yeah, removing "ret =" and "(void) ret;" would be right.

ali.alzyod marked 6 inline comments as done.

apply review changes

ali.alzyod added inline comments.Wed, Nov 20, 12:29 AM
src/bin/elementary/test_efl_ui_text.c
8

bin/elementary/test_events.c already use internal methods.

We leave it there to keep check internal behavior for annotation, (since methods became internal)

If it must change I will remove them.

src/lib/evas/canvas/efl_text_attribute_factory.c
226

I think there must be an Item but I am not sure we must have a format.
I will add a condition for the Item

src/lib/evas/canvas/efl_text_attribute_factory.eo
5

Perfect thank you a lot.
I did some small modifications
from widgets supporting styled text (Those implementing the @Efl.Text_Markup interface?).
and used "Efl.Canvas.Text.style_set" instead of "@Efl.Canvas.Text.style_set" until we approve that patch

14

I think passing NULL will be confusing for what will happen, It is hard to Imagine the right behaviour

15

But because Efl.Canvas.Text.style_set is from other patch I will add it as "Efl.Canvas.Text.style_set" then I will change it to "@Efl.Canvas.Text.style_set"

ali.alzyod added a comment.EditedWed, Nov 20, 12:32 AM

It seems, I need to rebase again on Cursor Patch :(
It seems using arc diff --update directly on patch based on other batch is not valid

ali.alzyod updated this revision to Diff 26998.Wed, Nov 20, 1:39 AM

update based on review

segfaultxavi resigned from this revision.Wed, Nov 20, 3:40 AM

I think I have no further API concerns. I'll let you finish the code review.

src/lib/evas/canvas/efl_text_attribute_factory.eo
5

Or you can make this patch depend on the Text_Style one. As you prefer.

8

Beware of long lines! The limit is 120 chars.

14

OK, it was just an idea. I will create a low-priority task to continue discussing in the future.

ali.alzyod updated this revision to Diff 26999.Wed, Nov 20, 4:09 AM
ali.alzyod marked 4 inline comments as done.

change class to abstract

close reviews

ali.alzyod marked 3 inline comments as done.Wed, Nov 20, 5:10 AM
ali.alzyod added inline comments.
src/bin/elementary/test_efl_ui_text.c
8

I removed the included internal test

ali.alzyod updated this revision to Diff 27002.Wed, Nov 20, 5:19 AM
ali.alzyod marked an inline comment as done.

remove internal header in test

bu5hm4n added a subscriber: q66.Thu, Nov 21, 3:02 AM
bu5hm4n added inline comments.
src/lib/efl/interfaces/efl_text_types.eot
24–25

@q66 please have a look here.

src/lib/evas/canvas/efl_text_attribute_factory.c
226

If format can be NULL, then you need a check further down for "eina_strbuf_append_printf" or that will create something like "(NULL) href=asdf"

q66 added inline comments.Thu, Nov 21, 4:37 AM
src/lib/efl/interfaces/efl_text_types.eot
24–25

You could make it an opaque struct handle?

ali.alzyod updated this revision to Diff 27036.Thu, Nov 21, 4:42 AM
ali.alzyod marked an inline comment as done.

remove internal functions

ali.alzyod added inline comments.Thu, Nov 21, 4:43 AM
src/lib/efl/interfaces/efl_text_types.eot
24–25

@q66
Just write :
type @extern @beta Efl.Text_Attribute_Handle; ??

src/lib/evas/canvas/efl_text_attribute_factory.c
226

I will remove this function for now. (we keep them for future use)

ali.alzyod updated this revision to Diff 27037.Thu, Nov 21, 5:09 AM

remove __undefine handle

ali.alzyod updated this revision to Diff 27061.Fri, Nov 22, 4:43 AM

remove unused file

ali.alzyod updated this revision to Diff 27062.Fri, Nov 22, 4:57 AM

remove cursor handle (not used public anywhere)

@segfaultxavi @bu5hm4n @woohyun
Do you have any more comments regard this patch, I do not see any more comments.

Additionally, some comment above from me is saying that instead of "ERR" you should be able to use EINA_SAFETY_MACROS which will also print the condition which is failing, which is rather usefull for the user. (Given the problems that there are with the tooling, i am fine with an additional patch for that).

No more comments so far, I am just wondering if the feature scope of this is okay, as its way less than legacy, and way less than Tom's proposal.

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

Here should be a EAPI (this is a convention in EFL, if something i EAPI we write that in the header as well as in the impl.)

427

Here should be a EAPI (this is a convention in EFL, if something i EAPI we write that in the header as well as in the impl.)

ali.alzyod updated this revision to Diff 27092.Mon, Nov 25, 4:47 AM
  • add EAPI to internal functions
woohyun added a comment.EditedMon, Nov 25, 4:48 AM

@bu5hm4n
Thanks for the review comment first :)

Additionally, some comment above from me is saying that instead of "ERR" you should be able to use EINA_SAFETY_MACROS which will also print the condition which is failing, which is rather usefull for the user. (Given the problems that there are with the tooling, i am fine with an additional patch for that).

@ali.alzyod
Could you share your idea on this ? If it is simple to modify code, I also think that updating EINA_SAFETY_MACROS looks good.

No more comments so far, I am just wondering if the feature scope of this is okay, as its way less than legacy, and way less than Tom's proposal.

@ali.alzyod and I have talked about feature scope of this attribute_factory several times,
and concluded that current definition is enough for basic usage. (The others are just for convenience, and can be supported later if they are requested).
Plus, Tom's proposals had some conflicts among them, so it was impossible to apply them
without any change.

Additionally, some comment above from me is saying that instead of "ERR" you should be able to use EINA_SAFETY_MACROS which will also print the condition which is failing, which is rather useful for the user. (Given the problems that there are with the tooling, i am fine with an additional patch for that).

using Eina_safe check can make code look ugly (split each condtions, or || them inside macro), especially for parts like this :

if (!efl_text_cursor_handle_get(start) || !efl_text_cursor_handle_get(end) ||
        efl_text_cursor_handle_get(start)->obj != efl_text_cursor_handle_get(end)->obj)
     {
        ERR("Used invalid cursors");
        return NULL;
     }

anyway, lets see how things go in additinal patch about it.

No more comments so far, I am just wondering if the feature scope of this is okay, as its way less than legacy, and way less than Tom's proposal.

First: There are no API for text annotation in legacy
Second: Yes this has less APIs than @tasn proposal, but it does not use handles (using handles could be challenging for user using annotations, because user could delete handles and reuse them, for example when he set new text), So now we will have harmless APIs that can be used to do the needed work, then we will add more in the future.

ali.alzyod marked 2 inline comments as done.Mon, Nov 25, 4:53 AM

done

zmike added a subscriber: zmike.Mon, Nov 25, 5:21 AM

Additionally, some comment above from me is saying that instead of "ERR" you should be able to use EINA_SAFETY_MACROS which will also print the condition which is failing, which is rather useful for the user. (Given the problems that there are with the tooling, i am fine with an additional patch for that).

using Eina_safe check can make code look ugly (split each condtions, or || them inside macro), especially for parts like this :

if (!efl_text_cursor_handle_get(start) || !efl_text_cursor_handle_get(end) ||
        efl_text_cursor_handle_get(start)->obj != efl_text_cursor_handle_get(end)->obj)
     {
        ERR("Used invalid cursors");
        return NULL;
     }

anyway, lets see how things go in additinal patch about it.

The point of using safety checks here is that each individual case can be evaluated to trigger an error, immediately informing developers what and where the issue is.

Please update the code to use safety checks in all recommended cases, thanks.

@zmike is it possible to print error message with EINA_SAFTY methods ?

ali.alzyod updated this revision to Diff 27094.Mon, Nov 25, 6:49 AM

replace ERR with EINA_SAFETY

@zmike @bu5hm4n
Done, change ERR to EINA_SAFETY

In future we will add new patch to use both.

ali.alzyod marked 22 inline comments as done.Tue, Nov 26, 12:15 AM
ali.alzyod edited the summary of this revision. (Show Details)Tue, Nov 26, 1:47 AM
bu5hm4n resigned from this revision.Tue, Nov 26, 6:01 AM

I think my comments have been met.

zmike added a comment.Tue, Nov 26, 8:32 AM

@woohyun This patch seems okay to me. I'll leave it to you to check with HQ experts and land if you don't find any issues.

@bu5hm4n @zmike @segfaultxavi

Thanks for giving good review, and I'll do test and land this if there is no problem.

woohyun accepted this revision.Tue, Nov 26, 8:04 PM
This revision is now accepted and ready to land.Tue, Nov 26, 8:04 PM
This revision was automatically updated to reflect the committed changes.