There is no need to discuss something like "please make use of a macro here" or "this should be tested", nor are these *suggesstions*, they changes that i am requesting.
I am also not saying that discussions are not allowed, but coming to a discussion with a argument "its also like this somewhere else" is not going to be usefull at any rate.
Replying to a inline comment where we would like to have a change with "we will do it later" is also not really a discussion, why would you start later on again on old code file fixing something that was said ahead of time, even if it could have been fixed before this piece of code would have even landed ? That makes no sense to me. Fix it now, no need to discuss things, no need to revisit things, and more testing, review can happen.

Okay, i am starting to be annoyed by replies like this. I am called here to do review, i do review, and all that is coming back is "we will do it later" "its used somewhere else like this" "we should not document concepts like this".
There is no golden rule about what we declare as easy to read code, however, this block is the perfect example of what it is NOT, it takes ONE single macro to get this cleaned up, where you have something like MY_MACRO("off", EVAS_TEXT_STYLE_PLAIN, EFL_TEXT_STYLE_EFFECT_TYPE_NONE) one call for each line up there. Which is WAY more readable than what is there right now. We also do EXACTLY this macro stuff ALL over the place, so the argument of "used in other part of text" is completely irrelevant and does not mean anything.
Other people have to READ that code again, have to MAINTAIN that code in some way, and someone on a bug-hunt will likely come across it, why should we make the life of these people harder ?
Have you ever heard of the "Broken windows theory" ? Whenever there is something broken or ugly, someone else, who does not want to make it any better will exactly refer to this code "eh, it was done like this before", could you please try to get off that attitude a bit ? This code here will last a long time, and why would you want to start with a hard-to-read version if you can invest 5 min. and make it nice ? (Same applies to other comments and revisions regarding tests, or string parsing, or or or )

make xavi happy

I am pretty much fine with this,i think we can still move to python if there are issues. I am also not sure if the doc build works anywhere except linux ... :/

No problem, @zmike is on vacation ... :)

In D10607#200161, @AbdullehGhujeh wrote:

@bu5hm4n

can you check that all branches of the parsing function is checked and tested (also with asan enabled)?

I have tested only new code.

I think the errors in efl_text_attribute_factory.c should be done using EINA_SAFETY_ON_FALSE_RETURN. Which will also print the conditions, otherwise these errors are not really helpful.

more docs

As a first review, its quite hard to review here anything, as cursor changes seems to have mixed into this here.

Is it possible that this here is totally mixed up ? there are also changes from the cursor in here ?

Thank you :)

No no, you should not, I just wondered if you did before ... (but you you didn't, which is correct).

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.

I am not sure why people are seeing this (you are not the first one), there is not a single call within the efl-build that is loading old e* libraries from the system to compile based on that. And the -I / -L flags are added as first by meson, only the user CFLAGS go higher in the priority, have you set custom CFLAGS that are including the libraries from these directories? (Or is your compiler not honoring the ordering of flags like (for example) mine) ?

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 ?

I am going to merge this on the 20th of November, if there is no voice raised against this.

I am going to merge this on the 20th of November, if there is no voice raised against this.

This looks overall good. However, it took me quite a bit to get the difference of last and previous.

I am wondering here. Doesnt that mean that we are not emitting visibility,changed events anymore ? :/

Hi, additionally to my comments, can you check that all branches of the parsing function is checked and tested (also with asan enabled)? The tests for example do not cover "style=" which has quite a bit of string parsing code. I am also wondering if there is some guideline on what is happening when attributes of the style are not within the normal behavior, is there normally a error ?

Thank you for the update, however I would still prefer to have the 4 lines refactored into a single function, and not copy and pasted across the same file :)

Oh - i dont mind of the duplicated blocks are in both files. But they should *not* be repeated within the same file ... :)

Okay, i now see what you want to fix. Thank you for the fix & the explanation.

Can you elaborate in the commit message what exactly is the issue this is fixing ?

Does replacing desc == EFL_CANVAS_GROUP_EVENT_MEMBER_ADDED with the a check if the name of the event is "theme,changed" fix the issue ?

Seems fine. However, i am not a laptop for some time, so someone else needs to land this i guess.

After a little discussion with @cedric on IRC:

finalize player removal

finalize player removal

Why did that compile for you ? o.O Works for me now.

I think this is pretty fine like it is now, there are 2 more things that need to migrate away from player, but that does not block this revision here.

asdfasdf

its efl canvas now.

Okay, i just thought because we prefix here something with animation that is not in our animation mixin ... :) But as i said, i am fine with this :)

rename

renames

renames

I am very fine with this.

The assert is not found ...

more tests, and a simple fix

rebase

Can you add a test for that ?

Why did this never show up in any test ?

fix animation. position is now returned *always* from 0.0 to 1.0

add testcases, and fix more wrong double handling

In T8126#146903, @cedric wrote:

Before all that kind of optimization, I think we will need to revise the API of the position manager and its internal a bit. Main issue I am seeing:

• The collection needs to throttle position manager sizing information request and make sure batch request are not overwhelming the main loop.

Nice that you took care of these performance critical whitespace changes! :-D

lovely.

mhm, then i have to create an entire test suite, as there are no test cases so far :(

The revision i have linked above now contains pause. You can check the example that got added in this revision and press p for pause (press twice for toggle) or press r for reverse, which will reverse the animation, just as you wished to do that. IMO the code in there look super easy and nice. The speed parameter is essential for the reversing, if i remove it again, the reversing code will be way harder. Would something like the revision be acceptable for you ? If yes, then i would continue removing the player object, as this usage as we have it right now is way easier.

add a example

update