This patch is set to rename some properties of Efl.Text_Font & Efl.Text_Format interfaces.
1- efl_text_font_set/get become (efl_text_font_family_set/get, efl_text_font_size_set/get)
2- efl_text_valign/halign become efl_text_vertical/horizontal_align
3- efl_text_halign_auto_type become efl_text_horizontal_align_auto_type
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
| src/lib/efl/interfaces/efl_text_font.eo | ||
|---|---|---|
| 64–65 | this line should be removed | |
| 83 | this line should be removed | |
| src/lib/evas/canvas/efl_canvas_text.eo | ||
| 224–225 | this needs full name too (horizontal_align_auto_type) | |
| src/lib/evas/canvas/evas_object_text.c | ||
| 500 | Append condition to the first If statement | |
| 2219 | I think no need for this line | |
| src/lib/evas/canvas/evas_textgrid_eo.h | ||
| 122 | should we mention font_family too? | |
| src/lib/evas/canvas/evas_textgrid_eo.legacy.h | ||
| 115 | should we add _font_family ? | |
| src/lib/evas/canvas/evas_object_text.c | ||
|---|---|---|
| 2219 | To force cleaning and recalculate layout, i think we need to set this to NULL | |
Some minor changes required, overall seems good.
@cedric can you double check the edje parts?
| src/lib/evas/canvas/evas_object_text.c | ||
|---|---|---|
| 458 | This should be EINA_SAFETY_ON_NULL_RETURN(font); | |
| src/lib/evas/canvas/evas_object_textblock.c | ||
| 15983 | This should have a safety check for font. Also, I don't think it's guaranteed that font is stringshared here, which means this needs strcmp? | |
| 16015 | This should be a safety check if size <= 0 | |
| src/lib/evas/canvas/evas_object_textgrid.c | ||
| 1201–1202 | This should be a safety check. | |
| 1238 | Safety check. | |
| src/lib/evas/canvas/evas_object_textblock.c | ||
|---|---|---|
| 15983 | This is not part of this patch ?! | |
| src/lib/evas/canvas/evas_object_textblock.c | ||
|---|---|---|
| 15983 | This patch changes the functionality of this function such that the code here needs alteration. | |
I think that this patch includes every requests from reviews now.
Any other opinions ? :)
No, there's still one comment I made which was ignored.
Once that's resolved, I have no further complaints.
| src/lib/evas/canvas/evas_object_textblock.c | ||
|---|---|---|
| 15983 | This still has not been addressed. | |
Done
| src/lib/evas/canvas/evas_object_textblock.c | ||
|---|---|---|
| 15983 | Sorry, i thought safety check was added here | |
Great, thanks! @woohyun I don't have any further issues with this patch, will leave for HQ experts to check.
This patch add a bunch of warning when trying genlist elementary test due to some unexpected legacy usage. Would be nice to get that fixed too.
Can you please provide more information, how to produce these warnings.
I try to use genlist in elementary_test but did not detect any warnings in the console
You mean this :
sel item data [0x50] on genlist obj [0x40000002f3fa], item pointer [0x400000071d06], index [9] selected: 0x400000071d06
I am getting a tons of: ERR<1489125>:eina_safety ../src/lib/evas/canvas/evas_object_text.c:2355 evas_object_text_font_set() safety check failed: font == NULL
actually these EINA_CHECKS are new and have been requested by the reviewers.
Old
if (!font || size <= 0) return; /*Condition for legacy object*/
New
EINA_SAFETY_ON_NULL_RETURN(font); EINA_SAFETY_ON_TRUE_RETURN(size <= 0);
After the double-check, I correct my statement Nobody requested to change legacy code, So this is the caused by this patch.
I will submit new patch to fix this part