Page MenuHomePhabricator

eolian: Fixed possibility of duplication due to @ingroup auto generation
AbandonedPublic

Authored by jsuya on Oct 1 2018, 11:42 PM.

Details

Summary

When creating docs in an eo class, the ingroup is automatically created with the class name.
but, the automatically generated group name may be different from the defined group (@defgroup).
When you create an ingroup for each function, two ingroups are created in the docs of the actual generated header's document.
This creates two html groups for the same API.

To prevent this, set the following rules.
If ingroup name is not declared, it follows the class name.
If an ingroup name is created in the docs of the class, all the methods follow the ingroup name.
However, if an ingroup name is declared in the docs of a method, it follows that ingroup name

+)
mangled docs check removed.
Because the existing check only checks that ']]' is located immediately after the since keyword.
So I replace it with D7138

Test Plan

Write and compile @ingroup XXX in the eo class docs.
And make doc

Diff Detail

Repository
rEFL core/efl
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 7784
Build 7305: arc lint + arc unit
jsuya created this revision.Oct 1 2018, 11:42 PM
jsuya requested review of this revision.Oct 1 2018, 11:42 PM
jsuya updated this revision to Diff 16990.Oct 1 2018, 11:48 PM

update code

Jaehyun_Cho added a subscriber: id213sin.

@segfaultxavi @q66 @id213sin

I think this patch should be applied to upstream to resolve doxygen ingroup name issue.

What do you think?

It is unclear to me what this commit does. By reading the code, it looks like you are introducing a new keyword to Eolian named @ingroup. Is that true? Please make it clearer, because there is confusion with the Doxygen ingroup.

What do you think, @q66?

Also, the check for the mangled docs should be a separate commit.

jsuya added a comment.Oct 2 2018, 1:30 AM

Hi @segfaultxavi
You are right. This is to support "@ingroup" of doxygen.
Is title a problem? Or is it a problem that prevents it from creating an "@ingroup" twice?

jsuya retitled this revision from eolian: Add @ingorup keyword for documentation to eolian: Fixed possibility of duplication due to @ingroup auto generation.Oct 2 2018, 1:30 AM

Title was OK (except the typo). I just wanted to make sure I understood.

I still would like to hear @q66 opinion and separate the mangled docs fix to another commit.

jsuya added a comment.Oct 2 2018, 5:55 AM

@segfaultxavi
OK I will soon make a commit to check the mangled documentation error by checking the parentheses(docs block?). :)

jsuya updated this revision to Diff 17015.Oct 3 2018, 10:34 PM

update code

jsuya edited the summary of this revision. (Show Details)Oct 3 2018, 10:35 PM
jsuya added a comment.Oct 3 2018, 10:37 PM

@segfaultxavi
As you said, I made a patch for the mangled doc check. Please check D7138

D7138 adds a new check for mangled docs, that is OK, but this commit still does TWO different things, which is not OK.
However, I understand in this code the parsing of the tags and the check for mangled docs was done simultaneously, so it is not easy to separate.

Let's see what @q66 thinks.

q66 added a comment.Oct 4 2018, 6:13 AM

Grouping is a purely legacy Doxygen feature and I don't think it's right to introduce it into docs at all. The generated Doxygen comments in the API are not for actual documentation, but more for IDE hints etc. where the group name does not matter.

jsuya added a comment.Oct 4 2018, 7:27 AM

@q66
This patch was created because I think the problem is a duplicate auto-generated group name.
As you say grouping is a feature of doxygen.
But eolian is making it automatically.
You seem to think this is not a problem.
As far as I know, @ingroup was in old code. However, it was deleted because it was automatically created in eolian.
Some APIs can not be found in the docs.
We need action on this part.

If you think that the documentation of the API is represented in multiple groups is not a problem, we don't need this patch.
But, If you have seen D7085, you will find that the legacy classes written there will require other group name than the one created by eolian.
Give me a your opinion(solution) to the class that needs definition of group name.

@myoungwoon, @Jaehyun_Cho

q66 added a comment.Oct 4 2018, 8:22 AM

Here is the thing; we have new documentation for generated eo API, on dokuwiki. Therefore, for that, doxygen is considered deprecated, and the only reason Eolian generates any Doxygen documentation comments is to help IDEs and other tools with suggestions, it is not intended for actual documentation generation. I am aware of the situation with legacy APIs, which are unfortunately also currently generated using Eolian; however, there is an ongoing investigation on how to deal with legacy APIs and things will very likely change significantly in near future, so there is no need right now to add something like this which would only introduce more APIs to deal with legacy into Eolian, when the ultimate goal is to reduce the amount of legacy stuff in Eolian. So what we need to do will only become clear depending on what path we will take with handling legacy in the EFL, but it will be different from what we have now.

myoungwoon added a comment.EditedOct 4 2018, 8:08 PM

Hi Daniel,

When I heard about dokuwiki, I had conversation with Cedric about Doxygen support.
Currently Tizen had been using Doxygen for API reference. And all native APIs in Tizen also had been using Doxygen to support their API reference documentation.
So I already asked Cedric to support Doxygen after moving to dokuwiki as documentation's backward compatibility.
It means EFL upstream Doxygen documentation from header files which generated by Eolian should include all APIs with right format(page) and grouping Etc like before. We expect that.
Thease days we just started to use the latest Doxygen based API reference and found there are some grouping issues from eo based APIs compare to previous Doxygen documentation.
So jsuya analyzed the issues and made patches.
I know Doxygen is old style for supporting API reference documentation. Many software's API already had been changed to new format(Dokuwiki, md based doc Etc.) Tizen also will change Doxygen later but not now.
Actually, Tizen C# API reference documentation and markdown files are generated by DocFx(https://dotnet.github.io/docfx/).
EFL#(C# based APIs) API reference documentation should be generated by DocFx. (@segfaultxavi - please should know about this for EFL# API documentation)

I hope you will consider our situation nicely.
Thanks.

jsuya added a comment.Oct 4 2018, 10:51 PM

@q66
I think this is a plan, not a solution.

I understand that eolian should create an ingroup based on the class name for other tools.
But the problem with legacy APIs still remain.
(I think the plan to reduce the amount of legacy is not what we are talking about right now.)

Let's talk about using doxygen.
For example, in a document generated by make doc
Can you find ecore_timer_interval_set start with doc/index.html?
(or below list?)
Most are legacy api. The documentation for legacy api is broken.
Probably we will not be able to deperecate all legacy api.
Since we are published following APIs, we must keep backwards compatibility.

Back in problem
One eo class creates eo api and EAPI (lagacy api).
The legacy api should be clearly separable into different groups.
I want to fix this patch as follows: What do you think?

eo docs                                     eo.h                 eo.legacy.h
  1. none -> @ingroup class_name / @ingroup class_name
  2. @ingroup_legacy custom_name on method -> @ingroup class_name / @ingroup custom_name with @ingroup class_name
  3. @ingroup_legacy custom_name on class -> @ingroup class_name / all @ingroup custom_name with @ingroup class_name

or

eo docs                                     eo.h                 eo.legacy.h
  1. none -> @ingroup class_name / @ingroup class_name
  2. @ingroup_legacy custom_name on method -> @ingroup class_name / @ingroup custom_name.
  3. @ingroup_legacy custom_name on class -> @ingroup class_name / all @ingroup custom_name

The point is to support the feature to add ingroups to legacy headers only.

+)
elm_glview_changed_set
elm_glview_rotation_get
elm_glview_size_set
elm_object_item_color_class_color_set
elm_object_item_color_class_color_get
elm_object_item_color_class_color2_set
elm_object_item_color_class_color2_get
elm_object_item_color_class_color3_set
elm_object_item_color_class_color3_get
elm_multibuttonentry_item_next_get
elm_multibuttonentry_item_prev_get
elm_multibuttonentry_item_selected_get
elm_multibuttonentry_item_selected_set
elm_gengrid_item_all_contents_unset
elm_gengrid_item_index_get
elm_gengrid_item_item_class_get
elm_gengrid_item_item_class_update
elm_gengrid_item_next_get
elm_gengrid_item_prev_get
elm_gengrid_item_tooltip_window_mode_get
elm_gengrid_item_tooltip_window_mode_set
elm_gengrid_item_fields_update
elm_genlist_item_all_contents_unset
elm_genlist_item_bring_in
elm_genlist_item_demote
elm_genlist_item_expanded_depth_get
elm_genlist_item_index_get
elm_genlist_item_item_class_get
elm_genlist_item_promote
elm_genlist_item_select_mode_get
elm_genlist_item_select_mode_set
elm_genlist_item_tooltip_window_mode_get
elm_genlist_item_tooltip_window_mode_set
elm_genlist_item_type_get
elm_list_item_bring_in
elm_list_item_next
elm_list_item_object_get
elm_list_item_prev
elm_list_item_selected_get
elm_list_item_separator_get
elm_list_item_separator_set
elm_list_item_show
elm_index_item_priority_set
elm_index_item_selected_set
elm_hoversel_item_icon_set
elm_hoversel_item_icon_get
elm_flipselector_item_prev_get
elm_flipselector_item_next_get
elm_toolbar_item_bring_in
elm_toolbar_item_icon_file_set
elm_toolbar_item_icon_get
elm_toolbar_item_icon_memfile_set
elm_toolbar_item_icon_object_get
elm_toolbar_item_next_get
elm_toolbar_item_object_get
elm_toolbar_item_prev_get
elm_toolbar_item_priority_get
elm_toolbar_item_priority_set
elm_toolbar_item_selected_get
elm_toolbar_item_separator_get
elm_toolbar_item_separator_set
elm_toolbar_item_show
elm_toolbar_item_state_get
elm_toolbar_item_state_next
elm_toolbar_item_state_prev

elm_object_item_access_info_set
elm_object_item_access_object_get
elm_object_item_access_order_get
elm_object_item_access_order_set
elm_object_item_access_order_unset
elm_object_item_access_register
elm_object_item_access_unregister
elm_object_item_cursor_engine_only_get
elm_object_item_cursor_engine_only_set
elm_object_item_cursor_get
elm_object_item_cursor_set
elm_object_item_cursor_style_get
elm_object_item_cursor_style_set
elm_object_item_cursor_unset
elm_object_item_domain_part_text_translatable_set
elm_object_item_domain_translatable_part_text_set
elm_object_item_signal_callback_del
elm_object_item_tooltip_content_cb_set
elm_object_item_tooltip_style_get
elm_object_item_tooltip_style_set
elm_object_item_tooltip_text_set
elm_object_item_tooltip_unset
elm_object_item_tooltip_window_mode_get
elm_object_item_tooltip_window_mode_set
elm_object_item_track
elm_object_item_track_get
elm_object_item_translatable_part_text_get
elm_object_item_untrack
evas_object_text_bidi_delimiters_get
evas_object_text_bidi_delimiters_set
evas_object_text_char_pos_get
evas_object_text_direction_get
evas_object_text_ellipsis_get
evas_object_text_ellipsis_set
evas_object_text_glow_color_get
evas_object_text_glow2_color_get
evas_object_text_last_up_to_pos
evas_object_text_outline_color_get
evas_object_text_shadow_color_get
evas_object_text_style_pad_get
evas_object_textblock_bidi_delimiters_get
evas_object_textblock_bidi_delimiters_set
evas_object_textblock_cursor_get
evas_object_textblock_legacy_newline_get
evas_object_textblock_legacy_newline_set
evas_object_textblock_line_number_geometry_get
evas_object_textblock_replace_char_get
evas_object_textblock_replace_char_set
evas_object_textblock_size_formatted_get
evas_object_textblock_size_native_get
evas_object_textblock_style_get
evas_object_textblock_style_user_peek
evas_object_textblock_style_user_pop
evas_object_textblock_text_markup_set
evas_object_textblock_valign_get
evas_object_textblock_valign_set
evas_textblock_node_format_first_get
evas_textblock_node_format_last_get
evas_textblock_node_format_list_get
evas_textblock_node_format_remove_pair
evas_object_text_ascent_get
evas_object_textblock_obstacle_add
evas_object_textblock_obstacle_del
evas_object_textblock_obstacles_update
evas_object_textblock_style_insets_get
evas_object_text_char_coords_get
evas_object_text_descent_get
evas_object_textgrid_cellrow_get
evas_object_textgrid_cellrow_set
evas_object_textgrid_cell_size_get
evas_object_textgrid_palette_get
evas_object_textgrid_palette_set
evas_object_textgrid_size_get
evas_object_textgrid_size_set
evas_object_textgrid_supported_font_styles_get
evas_object_textgrid_supported_font_styles_set
evas_object_textgrid_update_add
evas_object_text_horiz_advance_get
evas_object_text_inset_get
evas_object_text_max_ascent_get
evas_object_text_max_descent_get
evas_object_text_vert_advance_get

etc...

Hi @jsuya, please let me check that I understood the problem correctly:

ecore_timer_interval_set() does not appear in the autogenerated docs, because its doxygen documentation includes the @ingroup tag for a non-existing group.
Indeed, if we look at efl_loop_timer.eo.legacy.h:25 we see @ingroup Efl_Loop_Timer which is not defined anywhere (there is no @defgroup Efl_Loop_Timer).
This legacy header file is generated from efl_loop_timer.eo, and the @ingroup tags are generated automatically from the class name (Efl_Loop_Timer).

However, the main documentation for the Ecore_Timer class (and some of its methods) is in a diferent doxygen group, defined manually (i.e., not through an EO file) in Ecore_Common.h:3037 as @defgroup Ecore_Timer_Group.

So, the problem is that when @ingroup tags moved from manual to generated from eo files, they changed the group, and nobody noticed until now. Correct?

Here is some data, to help understand the size of the problem:

  • There are 432 unique @defgroup tags (only 2 start with Efl_, because docs for the new API are not meant to use doxygen)
  • In the *.legacy.h files there are 146 unique @ingroup tags (48 start with Efl_, which are the problem)

The two obvious solutions are:

  1. Change the manual files (Ecore_Common.h) so they use the same groups as the autogenerated ones (i.e. @defgroup Efl_Loop_Timer).
  2. Change legacy header generator in eolian so that it uses the @ingroup tags defined in the manual files (i.e. @defgroup Ecore_Timer_Group).

Let's examine both options in more detail:

1. Change manual files

  • This means searching for the @defgroup of each missing legacy group name and change it to use the new group name.
  • For example, replace @defgroup Ecore_Timer_Group in Ecore_Common.h to @defgroup Efl_Loop_Timer.
  • Not all legacy group names need to be changed, only the ones which have been ported to EO files and are causing problems now.
  • The legacy group names will be left a bit disorganized, as some will keep the old prefix (Ecore_) whereas some other groups will use the new prefix (Efl_). Currently these group names are already a bit disorganized, since, out of the 432 groups, 200 include the word _Group in the group name and 232 do not.

2. Change legacy header generator

  • This is the option chosen by this task (D7126), which is more general than D7085.
  • Have we considered using the legacy_prefix eo tag to generate the @ingroup tags? In the case of ecore_timer_interval_set() it works perfectly well, because efl_loop_timer.eo already contains a legacy_prefix: ecore_timer; line, which means that eolian could create the correct @ingroup Ecore_Timer_Group.
  • Option 2b can be to use the legacy_prefix line to generate correct @ingroup tags, and then use option 1 to fix the remaining issues.

I think I prefer option 2b and I can take care of it. I would also clean up a bit the group names and all the other Doxygen problems we have.

Is there anything that I have missed from the analysis? What are your opinions?

FINAL NOTE: Regardless of what we decide, we should turn all "make doc" issues into real errors that abort compilation so this does not happen again.

q66 added a comment.Oct 5 2018, 5:15 AM

If we are to add any sort of temporary fix to support legacy API documentation better, I suggest it is not done in the Eolian library or .eo files, but rather in the legacy header generator, using some kind of non-invasive approach that can be easily phased out once we've dealt with legacy APIs (for example, some kind of mapping file that can be easily expanded and later removed); the thing is, legacy APIs within .eo files are a non-ideal situation that creates a fair amount of burden on Eolian/eo and will likely be moved out into header files sometime in near future, and then there would be no need for any such workaround, because the docs for legacy will be like they used to be in the past. I'm open to having a temporary solution/workaround but not one that goes directly against our future goals.

q66 added a comment.EditedOct 5 2018, 5:16 AM

Option 2b as suggested by Xavi could also help, I strongly suggest looking into implementing it that way and see if it solves the problem for you.

q66 added a comment.Oct 5 2018, 5:21 AM

When I talk of a mapping file, what I have in mind is something like D7085, but generalized, moved out of source code and not doing O(n) lookup for each alias (therefore using a hash table to do lookups instead)

@jsuya what do you think of the analysis above? Should we start implementing Option 2b or do you foresee any problem with it?

To evaluate Option 2b, please check if D7148 fixes some of the missing methods in the documentation. If we apply that patch, manually fixing the rest of the inconsistent doxygen group names should be easier.

jsuya abandoned this revision.Oct 10 2018, 12:03 AM

@segfaultxavi
Sorry for the late reply.
I saw your patch. We need to manually fix the documentation as you say.
There are some issues that this patch does not solve.

A widget with itemclass will generate two html (widget, widget_item).
If we do not show item APIs on one page, we need to provide a link to that page.
For example,maybe a hoversel widget make like this:

diff --git a/doc/index_elm.dox b/doc/index_elm.dox
index 2aa1f4b..ebd17b6 100644
--- a/doc/index_elm.dox
+++ b/doc/index_elm.dox
@@ -127,7 +127,7 @@
  *
  * @image html img/widget/hover/preview-00.png
  * @image latex img/widget/hover/preview-00.eps
- * @li @ref Elm_Hoversel
+ * @li @ref Elm_Hoversel_Group / @ref Elm_Hoversel_Item_Group
  *
  * @image html img/widget/hoversel/preview-00.png
  * @image latex img/widget/hoversel/preview-00.eps
diff --git a/src/lib/elementary/elc_hoversel.h b/src/lib/elementary/elc_hoversel.h
index 523bd98..657b483 100644
--- a/src/lib/elementary/elc_hoversel.h
+++ b/src/lib/elementary/elc_hoversel.h
@@ -1,5 +1,9 @@
 /**
- * @defgroup Elm_Hoversel Hoversel
+ * @defgroup Elm_Hoversel_Item_Group Hoversel Item
+**/
+
+/**
+ * @defgroup Elm_Hoversel_Group Hoversel
  * @ingroup Elementary
  *
  * @image html hoversel_inheritance_tree.

what do you think?

And Many documents that refer to each group are likely to require many modifications.
(Maybe, Some groups want to be displayed on the same page. (I can not give examples at now.))

Anyway
Based on your patch, We will do documentation work that is also available in tizen.
(Maybe when @myoungwoon returns from vacation(next week?), I will talk to him about this.)

Thank you for your help :)