Picking font on textrun, will now give priority into font picked by the user, regardless of script type.
picking font due script can cause many inconvenient results
Example of wrong results: (User font is NotoColorEmoji)
After Change results: (User font is NotoColorEmoji)
Also now the following lines will be shown exactly the same, regardless of characters order
"가123A321" "A321가123" "123가A321" "A가123321"
#include <Elementary.h>
/*
gcc -o example test.c `pkg-config --cflags --libs elementary`
*/
EAPI_MAIN int
elm_main(int argc EINA_UNUSED, char **argv EINA_UNUSED)
{
Evas_Object *win, *en;
elm_policy_set(ELM_POLICY_QUIT, ELM_POLICY_QUIT_LAST_WINDOW_CLOSED);
win = elm_win_util_standard_add("", "");
elm_win_autodel_set(win, EINA_TRUE);
en = elm_entry_add(win);
elm_entry_scrollable_set(en, EINA_TRUE);
evas_object_size_hint_weight_set(en, EVAS_HINT_EXPAND, EVAS_HINT_EXPAND);
evas_object_size_hint_align_set(en, EVAS_HINT_FILL, EVAS_HINT_FILL);
elm_entry_text_style_user_push(en,"DEFAULT='font=NotoColorEmoji font_size=30 color=red'");
elm_object_text_set(en, "☪123456a");
evas_object_show(en);
elm_object_content_set(win, en);
evas_object_resize(win, 400, 200);
evas_object_show(win);
elm_run();
return 0;
}
ELM_MAIN()| No Linters Available |
| No Unit Test Coverage |
| Buildable 15939 | |
| Build 10782: arc lint + arc unit |
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/
No, they should not look the same.
The first one will change the previous character to red color.
The second one work as expected when it will not change the color of the old text.
Oh, it's showing a completely different gif when I click on it compared to when I just view it on the page!
What I'm seeing:
Anyway though, a gif is a pretty annoying format because it can't be easily viewed side by side... Even a video would be better (do you have one) because at least I can easily compare frame by frame, while the gif is just going in a loop and hard to make sense of it.
So this change just makes it so different text runs have different fonts set? Looking at the code, the change looks wrong. The idea there is to choose the optimal font for a text run. Consider for example this text: "... 편집실" (3 dots followed by Korean hangul).
With your change, if the default font doesn't have hangul glyphs, they will not render correctly. While before your change, they would, because the system would fall-back to a font that has them.
I'm not saying the existing code is perfect, there must be a better solution that fixes both issues, but just pointing our what your code probably breaks. That code is there for a reason, and I bet that if you git blame (or comment) to find why it was added you'll find more information, though even if not, try to think of the reason.
Another example of cases where this would fail (I'm not 100% sure on this one, but pretty sure). Consider you have two emoji fonts, neither of which supports all of the emojis. Let's consider two glyphs A (only supported on font a) and B (only supported on font b). Now consider the string "AB". I believe that with your change A will render correctly and B not, though without your change, both will render as expected.
I'm not sure what they issue you are describing is because it was hard to follow the gifs, so maybe you can try better explaining, preferably with just a comparison of two screenshots rather than gifs? In general, you want to make the job of the reviewer easier, so one good example is much better than a video that contains many. If you need more than one example, have more screenshots. It's hard to decipher what you mean otherwise.
| src/lib/evas/common/evas_font_query.c | ||
|---|---|---|
| 26 | I guess this comments need to go too. | |
Oh, it's showing a completely different gif when I click on it compared to when I just view it on the page!
What I'm seeing:
Anyway though, a gif is a pretty annoying format because it can't be easily viewed side by side... Even a video would be better (do you have one) because at least I can easily compare frame by frame, while the gif is just going in a loop and hard to make sense of it.
Description updated to use still images instead.
So this change just makes it so different text runs have different fonts set? Looking at the code, the change looks wrong. The idea there is to choose the optimal font for a text run. Consider for example this text: "... 편집실" (3 dots followed by Korean hangul).
With your change, if the default font doesn't have hangul glyphs, they will not render correctly. While before your change, they would, because the system would fall-back to a font that has them.
Depend on your example I am not sure what is render correctly output you are expecting, from my point of view these dots should be render with defaul font anyway, but when we reach hangul characters these are only rendered with a new font ( and this is what i see in text editors for example like MSWord)
I'm not saying the existing code is perfect, there must be a better solution that fixes both issues, but just pointing our what your code probably breaks. That code is there for a reason, and I bet that if you git blame (or comment) to find why it was added you'll find more information, though even if not, try to think of the reason.
This part of the function is written with the first version of the function, So I know exactly what is this function doing, but for remove part, in particular, I am not sure why do we need to choose font for the text-run script while it can be rendered automatically with the default font.
Another example of cases where this would fail (I'm not 100% sure on this one, but pretty sure). Consider you have two emoji fonts, neither of which supports all of the emojis. Let's consider two glyphs A (only supported on font a) and B (only supported on font b). Now consider the string "AB". I believe that with your change A will render correctly and B not, though without your change, both will render as expected.
Again this is a difference in our POV, in what is render as expected, in this case, it is dependent what is the default font.
Anyway, this change should not affect your example
I'm not sure what they issue you are describing is because it was hard to follow the gifs, so maybe you can try better explaining, preferably with just a comparison of two screenshots rather than gifs? In general, you want to make the job of the reviewer easier, so one good example is much better than a video that contains many. If you need more than one example, have more screenshots. It's hard to decipher what you mean otherwise.
As I mention, I updated the description with still images and I hope it is clear now, the main idea is that
1- We will not change old content fonts for text component when new content added. (it will be more consistent at textrun level)
1- Picking font for any textrun, will always give priority to default font over other fonts, (I think default font is what user expected all(or most) of the content will be rendered with).
Much better!
Depend on your example I am not sure what is render correctly output you are expecting, from my point of view these dots should be render with defaul font anyway, but when we reach hangul characters these are only rendered with a new font ( and this is what i see in text editors for example like MSWord)
With your changes the hangul changes wouldn't render at all (given the example I said), no? The font instance is chosen per text run, and you changed the code to choose based on the first character, so if it happens to choose a font that doesn't have hangul, that's it. The current code will correctly search the right font. This is from memory, but the code seems to agree with me.
Even worse, try starting it with an esoteric control character (like the comment "/* 0x1F is the last ASCII contral char, just a hack in" mentions) and it will very likely break. You can really just look at the code you are removing and try to break your code using it...
This part of the function is written with the first version of the function, So I know exactly what is this function doing, but for remove part, in particular, I am not sure why do we need to choose font for the text-run script while it can be rendered automatically with the default font.
As said in my previous comment and just above here. You see all the conditions there, so you can see what this functions was trying to fix. Just try to break your code with all the conditions the code was testing.
Again this is a difference in our POV, in what is render as expected, in this case, it is dependent what is the default font.
Let me clarify because I think you missed my point. I'm not talking about "a bit ugly" I'm talking about the difference between seen text and between seeing a missing glyph question mark. This is not a matter of POV it's a matter of "doesn't look like Windows" vs "doesn't render at all".
Anyway, this change should not affect your example
Have you tried? Constructed such fonts (or found in your system)?
As I mention, I updated the description with still images and I hope it is clear now, the main idea is that
1- We will not change old content fonts for text component when new content added. (it will be more consistent at textrun level)
1- Picking font for any textrun, will always give priority to default font over other fonts, (I think default font is what user expected all(or most) of the content will be rendered with).
Much better, but not taking another look yet because I still think it's broken (due to the reasons mentioned above).
Even without knowing the code, you removed quite a few ifs. They were doing *something*, some of them could be unwanted things, OK, but there must be at least on thing there that makes sense, and it doesn't look like you even tried to break your code based on the conditions that were already there. I gave you two examples that look like they would break. I'm sure there are plenty more.
With your changes the hangul changes wouldn't render at all (given the example I said), no?
of-course it will be rendered with the new change. the only difference is that the dots (..) will be rendered with the default font anyway, and a search for hangul Unicode supporting font will be done.
I think there is a misunderstanding about this change, Searching is still done in this function, we only change the way the first font picked for text-run.
It's been a long time since we discussed it, but apparently we discussed it on IRC immediately after my comment above and all of the concerns raised were addressed. Assuming this is true, I have no more objections.