Page MenuHomePhabricator

evas_object_textblock: add format strings for new efl_text_style
AbandonedPublic

Authored by woohyun on Jan 10 2020, 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.Jan 10 2020, 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.Jan 10 2020, 6:04 PM
woohyun edited the summary of this revision. (Show Details)Jan 10 2020, 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.Jan 11 2020, 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.EditedJan 11 2020, 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.EditedJan 12 2020, 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.

My opinion this approach is fine since it already:
Keep legacy work as expected.
New api work as expected + support old deprecated legacy attributes

And code logic and easy to understand and maintain because it stays in one place.

zmike requested changes to this revision.Jan 23 2020, 7:11 AM
zmike added a subscriber: zmike.

The point of having a new, designed API isn't to preserve existing codepaths and API. It's to have a single, clean entrypoint which leaves no room for misunderstandings or errors.

If we continue to provide compatibility like this, then we may as well not bother having a new API at all, since everyone will just continue to use the old API. Given that we've expressly made this distinction in all other components, it makes no sense to change our policy here.

This revision now requires changes to proceed.Jan 23 2020, 7:11 AM

To clear things up, this patch talks about: how we parse style string only

from my point of view

else if (cmd == backing_colorstr || cmd == background_colorstr)

this is very very simple to understand and do what is should be doing. if the user use backingcolor=red or background_color=red and both works fine, I do not see any problem with that.

zmike added a comment.Jan 23 2020, 8:01 AM

To clear things up, this patch talks about: how we parse style string only

from my point of view

else if (cmd == backing_colorstr || cmd == background_colorstr)

this is very very simple to understand and do what is should be doing. if the user use backingcolor=red or background_color=red and both works fine, I do not see any problem with that.

I just explained the problem with it.

Legacy style tags must be used only with legacy objects, and unified tags must be used only with unified objects.

Being "simple to understand" in the internals is not relevant in the context of API discussions. We should have readable implementation code, yes, but we cannot compromise the integrity of our API just to make the internals simpler. The API is the userspace presentation of the implementation. It is not a direct presentation of the implementation.

It would be trivial to add a filter for commands at the start of _format_command() based on whether an object is legacy, and then you would still be able to keep the added code here as-is.

Legacy style tags must be used only with legacy objects, and unified tags must be used only with unified objects.

I think this is a matter of updating legacy documentation to support new tag names.
but I do not see why the must is used in here, what if unified already understand legacy deprecated tags, what is the problem with that?

zmike added a comment.Jan 23 2020, 9:39 AM

The purpose of the unified API isn't to deprecate legacy, it's to replace legacy. We aren't intending to retain compatibility.

ali.alzyod added a comment.EditedJan 23 2020, 12:47 PM

I see and agree with it,
We do not purposely want to support legacy style strings, but it is just a side effect when update shared code between legacy and unified (it is not a bad side effect thats why we want to keep it).
I am afraid this will complex the code a bit especially in the future if we want to maintain two parts for parsing styles or adding new tags. right now this is just rename for the same tags nothing is new,
Are you still feel it is important to separate the tags names for legacy and unified, which effect code simplicity and backward compatibility?

zmike added a comment.Jan 23 2020, 3:09 PM

Like I said, you could resolve my complaint with a single line at the top of that function to pass the command to a small checker function which would verify the validity of a command based on whether an object is legacy.

This isn't an issue of code complexity. It's very simple code to write and read.

You talk about it not being a bad side effect, but it's a very bad side effect. By having two sets of APIs for use (and these style tags are API), it means that we have have duplicate API. This means we have to ensure documentation always mentions both, and we cannot remove either of them.

We are not trying to deprecate the old tags in unified widgets. We are trying to stop using them entirely.

@zmike you want to add something like this D11188 ?

Er functionally it's the same, but I was suggesting a single function which does something like

bool function(obj, cmd){
  if (is_legacy(obj)) {
     compare cmd against all valid legacy cmds here and return true if match
  }  else {
     compare cmd against all valid unified cmds here and return true if match
  }
 return false
}

This function can then be called at the start of _format_command() as I suggested, and if it returns false then the tag is treated as being invalid and ignored/errored. This allows you to keep the body of _format_command() easier to read.

Er functionally it's the same, but I was suggesting a single function which does something like

bool function(obj, cmd){
  if (is_legacy(obj)) {
     compare cmd against all valid legacy cmds here and return true if match
  }  else {
     compare cmd against all valid unified cmds here and return true if match
  }
 return false
}

This function can then be called at the start of _format_command() as I suggested, and if it returns false then the tag is treated as being invalid and ignored/errored. This allows you to keep the body of _format_command() easier to read.

but the majority of the commands are shared between the legacy and unified, only a small subset is different

zmike added a comment.Jan 24 2020, 1:04 PM

I was just explaining my idea. There's more than 5 style tags with different names between legacy and unified, which seems like the number I'd personally be considering making another function to filter like I described.

But this means to do the comparison two times for each tag, one to check if it is valid and the other to update related properties,

zmike added a comment.Jan 27 2020, 5:47 AM

If we're just talking direct pointer/bool comparisons then I don't see why that particularly matters, but I don't particularly care which method is used so long as the resulting functionality is as I've described.

If we're just talking direct pointer/bool comparisons then I don't see why that particularly matters, but I don't particularly care which method is used so long as the resulting functionality is as I've described.

For D11188 is it ok ?

zmike resigned from this revision.Jan 27 2020, 9:40 AM

Yes, that addresses the issue I raised.

woohyun abandoned this revision.Jan 28 2020, 1:10 AM

This patch would be covered by https://phab.enlightenment.org/D11188.