Page MenuHomePhabricator

Edje: edc text_class applied without font or font_size in style
ClosedPublic

Authored by a.srour on Nov 18 2019, 7:27 AM.

Details

Summary

The issue with text_class in Edc styles has to be within a string containing font & font_size properties to effect style, if font or font_size not presented in the same string text_class will be ignored.

So in the following Edc example, text_class will be ignored:

collections {
   text_classes {
      text_class {
         name: "tc1";
         font: "Sans";
         size: 20;
      }
   }
   styles {
      style {
         name: "style1";
         base: "color=#00FF00 text_class=tc1";
         tag: "br" "\n";
      }
   }
}

To apply text_class tc1, font and font_size has to be added to styles.style.base value, to be as follows:

...
base: "font=Serif font_size=15 color=#00FF00 text_class=tc1";
...
NOTE: The produced font will be Sans and font_size equal to 20
Test Plan

layout.edc

// compile: edje_cc layout.edc
// play: edje_player layout.edj
collections {
    text_classes {
       text_class {
          name: "tc1";
          font: "Sans";
          size: 20;
       }
    }
    styles {
        style {
            name: "style1";
            base: "color=#FFFFFF text_class=tc1";
        }
    }
    group {
        name : "group1";
        parts {
           part {
              name : "tb1";
              type: TEXTBLOCK;
              scale: 1;
              entry_mode: NONE;
              description {
                  state: "default" 0.0;
                  rel1.relative: 0.0 0.0;
                  rel2.relative: 0.5 0.5;
                  text {
                     style: "style1";
                     align: 0.0 0.0;
                     text: "Hello EFL";
                  }
              }
           }
        }
    }
}

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.
a.srour created this revision.Nov 18 2019, 7:27 AM

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/

a.srour requested review of this revision.Nov 18 2019, 7:27 AM
a.srour edited the summary of this revision. (Show Details)Nov 18 2019, 7:35 AM
a.srour edited the test plan for this revision. (Show Details)
segfaultxavi requested changes to this revision.Nov 18 2019, 7:35 AM
segfaultxavi added reviewers: smohanty, ali.alzyod.
segfaultxavi added a subscriber: segfaultxavi.

Can you please explain in the commit message what is the exact problem you are trying to fix, and how are you fixing it?
Otherwise, this is very hard to review.

This revision now requires changes to proceed.Nov 18 2019, 7:35 AM

Can you please explain in the commit message what is the exact problem you are trying to fix, and how are you fixing it?
Otherwise, this is very hard to review.

Wow Super fast, we were adding a description using web interface :)

segfaultxavi resigned from this revision.Nov 18 2019, 7:39 AM

Hahaha, I always wondered why you add the description after submitting the patch. What process do you follow?

Hahaha, I always wondered why you add the description after submitting the patch. What process do you follow?

Normally create revision with arcanist, and if the description is long, instead of writing it on the console, we do it in the web interface

ali.alzyod added inline comments.Nov 18 2019, 7:54 AM
src/lib/edje/edje_textblock_styles.c
198

Why do not we use the same old function

a.srour updated this revision to Diff 26967.Nov 18 2019, 8:00 AM

Resolve required changes

a.srour updated this revision to Diff 26968.Nov 18 2019, 8:01 AM

Remove whitespace

a.srour marked an inline comment as done.Nov 18 2019, 8:01 AM

Resolve required changes

Any update regarding this Diff?

ali.alzyod added a comment.EditedDec 21 2019, 11:16 PM

This seems fine to me, does anyone have concerns?

Maybe adding test will make @cedric happy

a.srour updated this revision to Diff 27764.Dec 23 2019, 3:04 AM

Add tests for fix

zmike requested changes to this revision.Dec 23 2019, 6:42 AM
zmike added a subscriber: zmike.

This patch looks great, and I think I understand the issue described, but the changes made here must be described in the actual patch log since the cited issue ticket may not be reachable for as long as this code persists.

In short: please describe either the change that is being made or the problem being solved (ideally both) directly in the commit log.

This revision now requires changes to proceed.Dec 23 2019, 6:42 AM
a.srour edited the summary of this revision. (Show Details)Dec 23 2019, 7:05 AM

This patch looks great, and I think I understand the issue described, but the changes made here must be described in the actual patch log since the cited issue ticket may not be reachable for as long as this code persists.

In short: please describe either the change that is being made or the problem being solved (ideally both) directly in the commit log.

Patch details updated

zmike accepted this revision.Dec 30 2019, 8:11 AM

Looks great, thanks!

This revision is now accepted and ready to land.Dec 30 2019, 8:11 AM
This revision was automatically updated to reflect the committed changes.
segfaultxavi added inline comments.Jan 3 2020, 2:34 AM
src/tests/edje/edje_test_text.c
107

The condition has been inverted! It should be && instead of ||.

Plus, this is throwing warnings during compilation. Please always be careful with warnings.

110

Same as above.