Page MenuHomePhabricator

evas_object_textblock: add format strings for new efl_text_style
Needs ReviewPublic

Authored by woohyun on Fri, Jan 10, 6:04 PM.

Details

Summary

These new format strings are for renamed properties in efl_text_style.
Additionally, this work also considered new value types for each property.
(ex: "none" and "solid_color" for new background_type property)

ref T8523

Test Plan
  1. ninja test
  2. check the result of efl_canvas_textblock_style test

Diff Detail

Repository
rEFL core/efl
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 15329
Build 10546: arc lint + arc unit
woohyun created this revision.Fri, Jan 10, 6:04 PM

It seems that this patch has no reviewers specified. If you are unsure who can review your patch, please check this wiki page and see if anyone can be added: https://phab.enlightenment.org/w/maintainers_reviewers/

woohyun requested review of this revision.Fri, Jan 10, 6:04 PM
woohyun edited the summary of this revision. (Show Details)Fri, Jan 10, 6:05 PM

I am not sure what are you trying to accomplish here.
Almost all EO properties related to text style are available as format strings:
For example, you have efl_text_strikethrough_type_set(obj, EFL_TEXT_STYLE_STRIKETHROUGH_TYPE_SINGLE) and also efl_canvas_textblock_style_apply(obj, "strikethrough=single").
I agree the names are sometimes slightly different. But I like it this way, because the C API is meant to be concise (In the above example, "this is the type of the strikethrough decoration") whereas the format string is meant to be short ("strikethrough=single" is short and clear).
Adding both options to the formatting string I think will be confusing... We would need to document both!
If you think the slightly different names is a problem, then I would prefer we use the exact same names in both APIs.

Some random thought: could we replace ALL the style_apply parsing with introspection? This should remove a ton of code, and the C and string API would then be forever in sync.

ali.alzyod added inline comments.Sat, Jan 11, 5:46 AM
src/lib/evas/canvas/evas_object_textblock.c
3350

Update needed for this method too, where we convert style into string

ali.alzyod added a comment.EditedSat, Jan 11, 11:23 PM

Also update to documentation should be done for the new names.

From my side, I think this update is fine since it will allow both styles "strikethrough=single" and "strikethrough_type=single" which gives more flexibility when used.

woohyun added a comment.EditedSun, Jan 12, 5:25 AM

@segfaultxavi

efl_canvas_textblock_style_apply(obj, "strikethrough=single") is not valid.
(strikethrough only works with "on" or "off")
efl_canvas_textblock_style_apply(obj, "strikethrough_type=single") is available.

I need to check my understanding in T8523.
There are some diffs between property and attribute.

  1. different names between property and attribute.
    • such as "backing" and "background_type"
  2. different values for property and attribute
    • such as "on and off (for "strikethrough")" and "none and single (for "strikethrought_type")"

Don't you want to sync them all together or only parts of them ?

I assumed that you might want to sync them all, but if you want to change only parts of them (such as name of "backing" to "background") ~ then, please let me know :)

@woohyun shouldn't we split this somehow between the unified API and the legacy API ? so the legacy API only works with the attributes equal to the function name, and the same for the unified API ?

I feel this here alone would cause a bit of inconstancy (But i also feel like the state we have right now in master *is* inconsistent)

efl_canvas_textblock_style_apply(obj, "strikethrough=single") is not valid.
(strikethrough only works with "on" or "off")
efl_canvas_textblock_style_apply(obj, "strikethrough_type=single") is available.

We should fix this!
We can discuss if the properties and the attributes should have the same name, but the values they accept should be absolutely the same!
I think the correct ones are the ones in the enums, and we should modify the ones in the attributes (and the style_apply docs).

  1. different names between property and attribute.
    • such as "backing" and "background_type"

backing should be renamed to background everywhere. Other things can be discussed.

  1. different values for property and attribute
    • such as "on and off (for "strikethrough")" and "none and single (for "strikethrought_type")"

I think these should be absolutely the same and we should fix them.

Don't you want to sync them all together or only parts of them ?

I assumed that you might want to sync them all, but if you want to change only parts of them (such as name of "backing" to "background") ~ then, please let me know :)

So, to summarize:

  1. Legacy names cannot be modified, obviously. If the code becomes too cumbersome, we should separate the implementations for Legacy and Unified.
  2. We have to decide if we want the same name for properties and attributes, or we allow some of them to be different. If we allow them to be different, I do not like that the parser accepts both versions.
  3. Values should be the same for properties and attributes.