Page MenuHomePhabricator

edje/style: optimize style_update function.
ClosedPublic

Authored by smohanty on Aug 19 2019, 11:41 PM.

Details

Summary

If the style is readonly then we know for sure it dosen't have any text_class/color_class.
If a style has text_class tag then call update only once not for each text_class tag it has.

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.
smohanty created this revision.Aug 19 2019, 11:41 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/

smohanty requested review of this revision.Aug 19 2019, 11:41 PM

Note:

Just by static analysis the style_update()  looks broken.
As the text_class can be overridden by the individual object. @see  _edje_text_class_find() below
Edje_Text_Class *
_edje_text_class_find(Edje *ed, const char *text_class)
{
   Edje_Text_Class *tc;

   if ((!ed) || (!text_class)) return NULL;

   /* first look through the object scope */
   tc = eina_hash_find(ed->text_classes, text_class);
   if (tc) return tc;

   /* next look through the global scope */
   tc = eina_hash_find(_edje_text_class_hash, text_class);
   if (tc) return tc;

   /* finally, look through the file scope */
   if (ed->file)
     tc = eina_hash_find(ed->file->text_hash, text_class);
   if (tc) return tc;

   return NULL;
}

As all the object modifies the same Evas_Style Data it will not work if more than one edje are overridden the same text_style and given different value then only the last modification will be reflected.

@cedric or @ali.alzyod , can comment more on that.

anyway this issue is for next patch.

If the style is readonly then we know for sure it dosen't have any text_class/color_class.
In edje_textblock_styles.c I do not see check for color_class (only text class)

If the style is readonly then we know for sure it dosen't have any text_class/color_class.
In edje_textblock_styles.c I do not see check for color_class (only text class)

color_class is special flavour of Tizen :)

Now I know you are not familiar with the tizen code base :) you should get familiar ;)

anyway if you want will remove color class from commit message

This is seems fine to me.
Just simple note if you think it is right (else forget about it), move the if (stl && !stl->readonly) inside the function _edje_textblock_style_member_add

This is seems fine to me.
Just simple note if you think it is right (else forget about it), move the if (stl && !stl->readonly) inside the function _edje_textblock_style_member_add

I understand your point .. For me style_add() is the api and rest are just helper function .. that's why the logic and checks I put in the Api .. and helper function should just do the job.
For safety I don't mind putting assert (stl && !stl->readonly) inside the member_add.

cedric requested changes to this revision.Aug 20 2019, 10:01 AM

This is seems fine to me.
Just simple note if you think it is right (else forget about it), move the if (stl && !stl->readonly) inside the function _edje_textblock_style_member_add

I understand your point .. For me style_add() is the api and rest are just helper function .. that's why the logic and checks I put in the Api .. and helper function should just do the job.
For safety I don't mind putting assert (stl && !stl->readonly) inside the member_add.

I agree with @ali.alzyod as this will reduce duplicated code and potential future bad use of the helper. The if should be in _edje_textblock_style_member_add.

This revision now requires changes to proceed.Aug 20 2019, 10:01 AM

This is seems fine to me.
Just simple note if you think it is right (else forget about it), move the if (stl && !stl->readonly) inside the function _edje_textblock_style_member_add

I understand your point .. For me style_add() is the api and rest are just helper function .. that's why the logic and checks I put in the Api .. and helper function should just do the job.
For safety I don't mind putting assert (stl && !stl->readonly) inside the member_add.

I agree with @ali.alzyod as this will reduce duplicated code and potential future bad use of the helper. The if should be in _edje_textblock_style_member_add.

How adding a state in a helper function is a good coding style ?
This kind of style leads to below helper function.

void
_edje_textblock_style_update(Edje *ed, Edje_Style *stl, Eina_Bool force)
{
   Eina_List *l;
   Eina_Strbuf *txt = NULL;
   Edje_Style_Tag *tag;
   Edje_Text_Class *tc;
   char *fontset = _edje_fontset_append_escaped, *fontsource = NULL;

   if (!ed->file) return;

   /* Make sure the style is already defined */
   if (!stl->style) return;

   /* we are sure it dosen't have any text_class */
   if (stl->readonly) return;

   /* No need to compute it again and again and again */
   if (!force && stl->cache) return;
}

This function checks 3 bools that itself is 8 states . In future if someone has to refactor this code he has to know all the history of these flags . but if you see what this helper actually does its just creates a style string. so all the state its actually checking has nothing to do with what this function actually does.

So if this kind of coding style you are encouraging then it leads to bad code not good code.

So I don't agree with your review comments (because you didn't give any technical reason) but if that's my only choice to pass this review will update the patch

smohanty updated this revision to Diff 24281.Aug 20 2019, 5:01 PM

updated with if checks

cedric requested changes to this revision.Aug 20 2019, 5:22 PM

I agree with @ali.alzyod as this will reduce duplicated code and potential future bad use of the helper. The if should be in _edje_textblock_style_member_add.

How adding a state in a helper function is a good coding style ?

As said, you are duplicating code. Every caller to the helper has to not forget to add this if (stl && !stl->readonly). This is bad practice. Duplicating code and requiring every user of the function to add such code is a bad practice.

So I don't agree with your review comments (because you didn't give any technical reason) but if that's my only choice to pass this review will update the patch

Technical reason is code duplication and potential source of error in the future use of the helper. If the called function should not work when in a specific state, then that function should refuse to work in that state which is not the case here. Calling that helper is not guaranteed to honor the state of the object and the caller has to enforce that before doing the call.

This revision now requires changes to proceed.Aug 20 2019, 5:22 PM

As said, you are duplicating code. Every caller to the helper has to not forget to add this if (stl && !stl->readonly). This is bad practice. Duplicating code and requiring every user of the function to add such code is a bad practice.

I think before you add some review comment please go through the logic and check what it does just giving blanket statement won't help .

  • If the style is read only and we call style_member_add() thats a potential bug (lets discuss about that ) .**

So It's the caller function responsibility to call these helper functions when they really need rather than just call it and put all the checks in the leaf function to decide whether to do the work or not.

That make both logic and code not maintainable as leaf functions will end up states for different higher level functions (depending on how many place you call it and what the individual caller function usecase).

How this will lead to code duplication ? (you mean state duplication ?) state duplication is totally fine and its clean as individual higher level function manage its own state so the state scope is limited to that function.

Note: As I said earlier I have already updated the patch with the modification , if you don't have any more review comment lets close this issue and move on.

smohanty requested review of this revision.Aug 20 2019, 6:47 PM
cedric resigned from this revision.Aug 20 2019, 8:09 PM

So It's the caller function responsibility to call these helper functions when they really need rather than just call it and put all the checks in the leaf function to decide whether to do the work or not.

Relying on the caller to call a function properly leads to bugs. That is why every free function does allow call on NULL. This is why we do have Eo. Because there is a lot of caller point and only one callee. Getting the check in the callee lead to less potential bugs.

That make both logic and code not maintainable as leaf functions will end up states for different higher level functions (depending on how many place you call it and what the individual caller function usecase).

When this happen you break down the leaf function to have two entry function that wrap with the different proper state requirement checked in that one.

How this will lead to code duplication ? (you mean state duplication ?) state duplication is totally fine and its clean as individual higher level function manage its own state so the state scope is limited to that function.

As in: "more line of code are necessary to call that function". Every added line of code add at least one bug. The more code line, the more bugs.

src/lib/edje/edje_textblock_styles.c
331

This is a duplicate of line 312 and could be at 273.

340

This is a duplicate of line 302 and could be at 273.

smohanty updated this revision to Diff 24367.Aug 21 2019, 5:26 PM

Refactor the code to incorporate review comment.

smohanty marked 2 inline comments as done.Aug 21 2019, 5:27 PM
ali.alzyod added inline comments.Aug 25 2019, 9:24 AM
src/lib/edje/edje_textblock_styles.c
295

Why do not you use original function name _edje_textblock_style_member_add ?

307

according to original code, I think this should be _edje_textblock_style_member_remove

smohanty marked 2 inline comments as done.Aug 25 2019, 6:00 PM
smohanty added inline comments.
src/lib/edje/edje_textblock_styles.c
295

because it dosen't convey any meaning . thats why refactored it out.
If you know why its called style_member then will happy to put some comment and rename back. I thought this name was not proper.

307

same as above.

Hermet accepted this revision.Aug 25 2019, 10:22 PM
This revision is now accepted and ready to land.Aug 25 2019, 10:22 PM
Closed by commit rEFLe742bf67e441: edje/style: optimize style_update function. (authored by subhransu mohanty <sub.mohanty@samsung.com>, committed by Hermet). · Explain WhyAug 25 2019, 10:22 PM
This revision was automatically updated to reflect the committed changes.
ali.alzyod added inline comments.Aug 25 2019, 11:27 PM
src/lib/edje/edje_textblock_styles.c
251

Why this function still exist ? you already replace it with _edje_textblock_style_add