Page MenuHomePhabricator

Polish text interface methods
ClosedPublic

Authored by a.srour on Nov 21 2019, 8:12 AM.

Details

Summary

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

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 changed the visibility from "Public (No Login Required)" to "Subscribers".Nov 21 2019, 8:18 AM
ali.alzyod edited subscribers, added: ali.alzyod, a.srour; removed: committers, cedric, reviewers.
ali.alzyod added inline comments.Nov 21 2019, 3:17 PM
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 ?

ali.alzyod added a subscriber: woohyun.
a.srour added inline comments.Nov 24 2019, 12:36 AM
src/lib/evas/canvas/evas_textgrid_eo.h
122

I think, we don't need to mention font_size here instead i will change it to font_family

src/lib/evas/canvas/evas_textgrid_eo.legacy.h
115

I think, we don't need to mention font_size here instead i will change it to font_family

a.srour added inline comments.Nov 24 2019, 1:10 AM
src/lib/evas/canvas/evas_object_text.c
2219

To force cleaning and recalculate layout, i think we need to set this to NULL

a.srour updated this revision to Diff 27079.Nov 24 2019, 5:00 AM

Resolve requests, revert _evas_text_efl_text_font_font_set() changes

a.srour marked 10 inline comments as done.Nov 24 2019, 5:01 AM

Done

ali.alzyod edited the summary of this revision. (Show Details)Nov 24 2019, 5:09 AM
ali.alzyod changed the visibility from "Subscribers" to "Public (No Login Required)".Nov 24 2019, 7:30 AM

@woohyun @segfaultxavi @bu5hm4n
Do you have any feedback regarding this patch?

zmike requested changes to this revision.Nov 25 2019, 5:44 AM
zmike added a reviewer: cedric.
zmike added a subscriber: zmike.

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.

This revision now requires changes to proceed.Nov 25 2019, 5:44 AM
ali.alzyod added inline comments.Nov 25 2019, 6:31 AM
src/lib/evas/canvas/evas_object_textblock.c
15983

This is not part of this patch ?!

a.srour updated this revision to Diff 27095.Nov 25 2019, 7:21 AM
a.srour edited the summary of this revision. (Show Details)

Using EINA_SAFETY macro

a.srour marked 4 inline comments as done.Nov 25 2019, 7:26 AM

Done

a.srour updated this revision to Diff 27096.Nov 25 2019, 7:27 AM

Fix warning

zmike added inline comments.Nov 25 2019, 8:52 AM
src/lib/evas/canvas/evas_object_textblock.c
15983

This patch changes the functionality of this function such that the code here needs alteration.

a.srour updated this revision to Diff 27098.Nov 25 2019, 9:15 AM

Add eina_streq to font check

a.srour marked 3 inline comments as done.Nov 25 2019, 9:16 AM

Done

bu5hm4n requested changes to this revision.Nov 26 2019, 3:07 AM
bu5hm4n added inline comments.
src/lib/evas/canvas/evas_object_text.c
461

font is always != NULL

497

EINA_SAFETY check to ensure size is > 0

This revision now requires changes to proceed.Nov 26 2019, 3:07 AM
a.srour updated this revision to Diff 27111.Nov 26 2019, 3:36 AM

Resolve requests

a.srour marked 2 inline comments as done.Nov 26 2019, 3:37 AM

Done

bu5hm4n resigned from this revision.Nov 26 2019, 5:49 AM

Concerns have been met.

I think that this patch includes every requests from reviews now.

Any other opinions ? :)

zmike requested changes to this revision.Nov 27 2019, 5:41 AM

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.

This revision now requires changes to proceed.Nov 27 2019, 5:41 AM
a.srour updated this revision to Diff 27155.Nov 27 2019, 5:52 AM

Add Eina_Safety check

a.srour marked an inline comment as done.Nov 27 2019, 5:53 AM

Done

src/lib/evas/canvas/evas_object_textblock.c
15983

Sorry, i thought safety check was added here

zmike resigned from this revision.Nov 27 2019, 10:06 AM

Great, thanks! @woohyun I don't have any further issues with this patch, will leave for HQ experts to check.

@zmike

Thank you very much ! I'll check and close this soon.

  • fix merge conflict
woohyun accepted this revision.Nov 27 2019, 11:42 PM
This revision is now accepted and ready to land.Nov 27 2019, 11:42 PM
This revision was automatically updated to reflect the committed changes.

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.

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

cedric added a comment.Dec 6 2019, 9:39 AM

Just execute "elementary_test -to genlist".

Just execute "elementary_test -to genlist".

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

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);

Seems like this is indeed incorrect.

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

Seems like this is indeed incorrect.

D10826 will retain old legacy behaviour (No eina error checks)