Page MenuHomePhabricator

efl_ui_theme: Introduce Efl.Ui.Theme class
ClosedPublic

Authored by Jaehyun_Cho on Nov 7 2018, 1:35 AM.

Details

Summary

Efl.Ui.Theme class is required to support language bindings.
Efl.Ui.Theme works based on current elm_theme features.

This patch fixes T7357.

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.
Jaehyun_Cho created this revision.Nov 7 2018, 1:35 AM
Jaehyun_Cho requested review of this revision.Nov 7 2018, 1:35 AM
segfaultxavi requested changes to this revision.Nov 7 2018, 3:17 AM

make, make check and make examples all work, including C# bindings.

There are some good comments in the code, thanks a lot!

However, this is adding new API, so I would expect unit tests and examples to come with it.

src/lib/elementary/efl_ui_theme.eo
30

"to be searched"

This revision now requires changes to proceed.Nov 7 2018, 3:17 AM

fixed the documentation typo and rebased

Added examples

Added example source and theme files to elementary.mk file

@segfaultxavi

I added examples for efl_ui_theme.

However, it seems that the unit tests for efl_ui interface has not been prepared yet.

How about adding unit tests for this case later?

Thanks a lot. However, since this is NEW API, could you use only NEW API in the examples?
Please avoid mixing legacy and new API, as then the examples become very difficult to understand, and they cannot be converted to other languages (like C#).
Take a look at the new tutorials if you want to know how to create apps using only the new API:
https://www.enlightenment.org/develop/tutorials/c/start.md

It's OK for me to leave the unit tests for later.

Sorry for the mixed APIs
Updated the examples

Added example executable files to elementary.mk

@segfaultxavi

I updated the examples not to use legacy APIs and the examples are generated and executed correctly :)

segfaultxavi requested changes to this revision.Nov 14 2018, 1:27 AM

Except for the minor inline comments, it looks fine to me.

Please remember to say that this patch fixes T7357.

I would like to wait for somebody with actual experience with Elm to review this patch too.

src/examples/elementary/efl_ui_theme_example_01.c
52

Why do you need edj_path? Wouldn't it be simpler to have a:

#define EXAMPLE_EDJ_FILE_PATH "./efl_ui_theme_example.edj"

and use it both for ecore_file_exists() and efl_ui_theme_extension_add() ?

src/examples/elementary/efl_ui_theme_example_02.c
52

Same comment about edj_path as in the other example.

src/lib/elementary/efl_ui.eot
3

Don't you think Efl.Ui.Theme_Apply_Result is a better name?

I have wanted to change this for a long time... maybe now is the best time to correct this name.

src/lib/elementary/efl_ui_theme.eo
32

Instead of elm_theme_overlay_add() it should say efl_ui_theme_overlay_add(). Also, use an @ sign so an automatic link is created in the documentation.

src/lib/elementary/elm_priv.h
153

Can you use Efl_Ui_Theme *eo_theme; instead of Eo *obj;? I think it is much more clear (and then the comment can be changed to For accessing through the Eo interface).

This revision now requires changes to proceed.Nov 14 2018, 1:27 AM

updated as @segfaultxavi commented

segfaultxavi accepted this revision.Nov 16 2018, 6:00 AM

I have no further comments, but I would wait for another review by someone who knows how theming works :)

This revision is now accepted and ready to land.Nov 16 2018, 6:00 AM

Please give some comments or feedback on this patch :)

If there is no comment or feedback, then I will submit this patch.

Modified the enum name from "failed" to "fail" to be a noun like "success".
(from EFL_UI_THEME_APPLY_RESULT_FAILED to EFL_UI_THEME_APPLY_RESULT_FAIL)

SanghyeonLee accepted this revision.Nov 19 2018, 8:53 PM
SanghyeonLee added a subscriber: SanghyeonLee.

Looks good to me.

This revision was automatically updated to reflect the committed changes.