Page MenuHomePhabricator

Force all apis to be implemented
Open, HighPublic

Description

We were discussing on irc how a caller of a api should no if a api is implemented or not, since in efl a function still may be unimplemented even if it isa interface of the interface that defined that function. We came to the conclusion that it would be a good idea to force every function to be implemented.

Compile time

Solve it at compile time, every function has to be mentioned in one implements section in the inheritance tree. Eolian should error when a function is not implemented in a regular class.

If someone wants to use composite to build up logic in a object, then he could specify that in eolian in the implements section with something like implements { Foo.Bar @composite } where Foo.Bar is a class, eo then enforces somehow that the objects are composited at construction time, (dont know exactly how to solve that exactly)

Runtime

@jpeg your turn! :)

Details

Differential Revisions
D7790: evas_canvas3d: add empty functions for missing APIs
D7760: efl_ui_tab_page: implement missing api
D7783: efl_ui_slider: this does not use anything from Efl.Content
D7752: elm_fileselector: improve documentation
Commits
D8420 / rEFL37c703437a4d: efl.pack_table: inherit efl.pack instead of efl.pack_linear
rEFL23d0076346e6: evas_canvas3d: add empty functions for missing APIs
D7759 / rEFL120e247fa0ac: efl_canvas_layout_part_external: implement missing functions
D7844 / rEFLbd949111be8b: efl_ui_win_part: remove unused interface
D7786 / rEFLaadd9af864a8: efl.file: move 'save' method into separate efl.file_save interface
D7787 / rEFL9aca866fd13b: efl.text_markup: move cursor-related methods to efl.text_markup_interactive
D7804 / rEFL497acc672215: elm_prefs: remove efl.file usage
D7791 / rEFL6dd861fe3241: efl_model_loop: make it abstract
D7802 / rEFL73d3a6396007: efl_net_server_udp: add empty functions
D7784 / rEFL3278e8275919: elm_map: convert paused to animation property
D7788 / rEFL25925f026407: elm_photo: implement remaining efl.file methods
D7781 / rEFLd6992432c8fe: efl_ui_image: implement efl.file.mmap_get
D7779 / rEFLb42eeb4c95c4: Implement missing APIs in elm_photo
D7785 / rEFL956a66c74852: evas_canvas3d_mesh: implement get methods for efl.file file and mmap props
D7782 / rEFL015fbac20da7: ecore: make ecore_audio_out abstract
D7778 / rEFLdb13fbc4940e: evas_canvas3d_primitives: make unused functions empty
D7777 / rEFLb7cd3f4bfd8b: efl_net_dialer_websocker: set cannot be called here
D7757 / rEFL37c115542c6d: efl_ui_pan: implement content interface
D7756 / rEFL2dacf8a69b58: efl_ui_scroller: support the complete efl.content API
D7755 / rEFL68d1579faad5: efl: introduce efl_ui_direction_readonly
D7754 / rEFLc440ee442d98: elm_fileselector_entry: resolve missing interface api by composition
D7753 / rEFL1c27529363fa: elm_fileselector_button: support the whole interface
D7751 / rEFL812543ad004e: elm_code: remove the implementation of efl.access.text
D7749 / rEFLf00ae98a1dc1: elm: add container api for the two objects
D7748 / rEFL1763afb391f5: edje: implement container api
D7741 / rEFLd483302a13bf: efl_ui_image: implement remaining efl.layout.calc methods
D7740 / rEFL70827b4d7e05: efl_ui_layout: implement remaining efl.layout.calc methods
D7739 / rEFL04c2d313d7e9: efl_ui_image: implement the last two APIs from edje
D7729 / rEFL09a83d216163: efl_ui_win: move base and step size hints here
D7715 / rEFLa2162b90ca48: efl: split efl_ui_range into display and interactive
D7713 / rEFLdfb0a2a458d0: ector: remove unused function
D7680 / rEFL270cb385cae3: eolian: make error messages usable
There are a very large number of changes, so older changes are hidden. Show Older Changes

P265 was updated.

I'm going to tackle these since they seem reasonable:

API defined in efl_layout_calc.eo missing in 2
   calc_auto_update_hints:
      efl_ui_layout.eo
      efl_ui_image.eo
   calc_size_min:
      efl_ui_layout.eo
      efl_ui_layout.eo
   calc_parts_extends:
      efl_ui_layout.eo
      efl_ui_image.eo
   calc_force:
      efl_ui_layout.eo
      efl_ui_layout.eo
   calc_freeze:
      efl_ui_image.eo
      efl_ui_image.eo
   calc_thaw:
      efl_ui_image.eo
      efl_ui_image.eo

Nice, i am going to tackle

API defined in efl_container.eo missing in 3
   content_remove:
      efl_ui_box.eo
      efl_ui_table.eo
   content_iterate:
      efl_canvas_layout.eo
      efl_canvas_layout.eo
   content_count:
      efl_canvas_layout.eo
      efl_canvas_layout.eo

Looking through the rest:
efl_player, efl_file, efl_text, efl_i10n, efl_l18n are needing reconsiderations. I would say we resolve everything arround that. Then look at them.
When we are in this stage, we can enable the errors in eolian as warnings, it will print a few but not too much, new API will then be warned on, and we should be picky about not introducing more unimplemented APIs

It seems that P265 is not accurate; the mmap property is shown as not implemented by efl_ui_image, but it definitely is implemented there.

Hermet added a subscriber: Hermet.Jan 25 2019, 2:51 AM
This comment was removed by Hermet.

I propose to change the name of efl_gfx_image_animation_controller to efl_gfx_frame_controller since "frame" could be commonly used then "image",
This interface must be designed to control sort of "scene", just normal "image" or something continous vector data...
Any feedback is welcome.

This task will be progressed on D7769.

Thanks 211064113104702b2c6bd279e9e8a04ee0e8254a :)
But when I try to print all the unimplemented warnings by using the above commit, build error causes not to print all the unimplemented warnings.

To print all the unimplemented warnings, EOLIAN_CLASS_UNIMPLEMENTED_WARN=1 is defined and also make _db_check_implemented() always return EINA_TRUE not to cause build error.
Otherwise, some of unimplemented warnings are not printed because the build error stops build before printing the unimplemented warnings.

BTW, for now, the unimplemented warnings are printed repeatdly (same warnings are printed many times).

bu5hm4n added a comment.EditedJan 25 2019, 4:10 AM

@Jaehyun_Cho please see https://phab.enlightenment.org/T5719#129918. This is the possibility to get the latest warnings in one strucutred file or in the first file, the blank eolian output.

Further more, all revisions added to this task, are already fixing such API problems. P265 <- this is the current state in master, P268 <-this is the state after all those revisions above are applied.

I propose to change the name of efl_gfx_image_animation_controller to efl_gfx_frame_controller since "frame" could be commonly used then "image",
This interface must be designed to control sort of "scene", just normal "image" or something continous vector data...
Any feedback is welcome.

This task will be progressed on D7769.

This sounds fine.

@Hermet The problem i can see in efl_canvas_layout is, that the player is used, even if there is no video, rather just a animation.
My idea: Efl_Animation_Player, which just has playable_get, play_get, play_set, play_speed_set, play_speed_get. which covers everything that can be done with a animation. This would solve the problem for efl_canvas_layout.

What do you think ?

zmike added a comment.Jan 25 2019, 6:04 AM
mmap:
   efl_canvas_snapshot.eo
   efl_ui_image.eo
   evas_canvas3d_mesh.eo
   efl_ui_widget_part_bg.eo
   efl_ui_image.eo
mmap:
    elm_photo.eo
    efl_canvas_proxy.eo
    efl_ui_image.eo
    efl_canvas_scene3d.eo
    efl_ui_image_zoomable.eo
    efl_canvas_video.eo
    efl_io_file.eo
    efl_ui_popup_part_backwall.eo
    evas_canvas3d_mesh.eo
    efl_canvas_snapshot.eo
    efl_ui_win_part.eo
    elm_icon.eo
    efl_ui_image_legacy.eo
    efl_ui_widget_part_bg.eo
    efl_canvas_surface_wayland.eo
    efl_canvas_vg_object.eo
    efl_ui_image_zoomable_legacy.eo
    efl_ui_layout_part_bg.eo
    elm_prefs.eo
    efl_canvas_surface_tbm.eo
    efl_canvas_surface_x11.eo
    efl_ui_image.eo
    efl_ui_widget_part_bg.eo

P265 was updated! Due to a change in eolian we are back to 2434 Cases to check, anyways, we already got ~ 1K resolved since we started!

Putting this on pause on my end for a bit. I think we need the following to make further progress:

  • landing all outstanding patches
  • discussion of T7664

I think we should be in better shape at this point?

@bu5hm4n @zmike @felipealmeida

I think we can separate the cause of this task into 3 cases.

Case 1. The interface is not well designed. (The interface should be separated or deleted)
Case 2. The class should not implement the interface.
Case 3. Both the interface and the class are well designed.

For case1 and case2, we can modify interface and class.

However, for case3, how about just leave it and mark the method with something (e.g. @empty) ?
And the case3 can be handled in language bindings as follows.

//C# example (.cs)
virtual public bool setcontent(Efl.Gfx.Entity) {

throw new NotImplementedException();
...

}

What do you think about it?

bu5hm4n added a comment.EditedJan 29 2019, 8:25 AM

Lets split this up:

First part: We should split up more interfaces in order to get a better usability of them, so they apply to *all* of our usecases, and not just a few. This even might end up with interfaces with only 1 property.

Second part "@empty / erroring in implementations":

I think we should be very carefull with @empty or erroring in implementations. Lets take the example of getting the position of a object, lets say something takes the position of a element because it wants to display a highlight on them. In this case a not implementation position getter would result in a completely miss oriented highlight. However the implementation of the highlight cannot ensure if the position is legal or illegal, thus we should make sure that such getters are really implemented in a meaningfull manner, a setter however, is less dangours in that regard, since the position that you set might be altered anyways, so as long as you respect ownership etc., having a empty setter is fine, a empty getter is *not*. Same applies basically to methods with a return value which is something else than just a success indicator.

tldr: We should only use @empty / error out on setters, never on methods or getters.

Third part: Throwing a exception in case of a unimplemented method does not seem to be a good idea to me, a exception will change the control-flow of a software just becase you called something on a object that does not support the functions that it actaully implemented. This is to me a indicator that then the interface should have been splitted up, since such a thing can *never* work in runtime, and with a splitted interface, it would also not work at compile time, which would make debugging a issue regarding this a lot easier. Doing this will also lower the usability-quality of our API IMO.

For further comments, we should split this up into multiple tasks i think, or the communication will be a bit difficult.

@bu5hm4n @zmike

One question.
Do we need to handle legacy eo files (for example, elm_xxx.eo, evas_xxx.eo, and so on) now - even those will not be bind to other languages ?

zmike added a comment.Jan 30 2019, 5:51 AM

@bu5hm4n @zmike

One question.
Do we need to handle legacy eo files (for example, elm_xxx.eo, evas_xxx.eo, and so on) now - even those will not be bind to other languages ?

No, see T7463

Jaehyun_Cho added a comment.EditedJan 31 2019, 1:53 AM

It seems that many legacy eo files are considered in P265.
(e.g. elm_xxx, evas_xxx, edje_xxx, ecore_xxx)

I think that we can consider other eo files first and then later we can think about the legacy eo files.
(I think it would be fine that we do not consider at all the legacy eo files if we do not have enough time for this.)

The following eo files look like legacy but they are referenced by other new efl interface.
It seems that the following eo files should be renamed or the reference should be removed.

elm_general.eot
elm_interface_scrollable.eo
elm_widget_item.eo
edje_types.eot

zmike added a comment.Jan 31 2019, 7:12 AM

For getting a detailed view to what is going on, please clone https://github.com/marcelhollerbach/efl-utility.

Build the project using meson.

then run EOLIAN_CLASS_UNIMPLEMENTED_WARN=1 ./efl-check <path-to-efl>/src/lib

This will output a huge amount of eolian error data. pipe this into a file using
EOLIAN_CLASS_UNIMPLEMENTED_WARN=1 ./efl-check <path-to-efl>/src/lib 2> ./error

after that you can run the script ´./efl-missing-api ./error´ this will print the structured output to a file.

I created an issue ticket since this doesn't build from git. Also when running efl-check I get eolian: could not parse files. This is on a completely clean efl tree.

zmike added a comment.Jan 31 2019, 8:02 AM

@zmike

Then, why are we handling legacy things now ?

Such as,
https://phab.enlightenment.org/rEFL1c27529363faa90450685140b7fa552fb24ab546
https://phab.enlightenment.org/rEFLdb13fbc4940ee619b1edf47e6bbb745b769a7dcb
....

I just want to check whether I'm missing something.

Eolian syntax cannot be stabilized until all unimplemented apis are resolved. This means that anything using eo (including legacy) must have this resolved.

If legacy code did not use eolian then this would not be an issue; T7463 may be a means to resolving the eolian-related issues, but then there would still be runtime errors that would be undetectable.

@zmike
Ok ~ I got your point well, and also check T7436 together.
Thank you !

zmike added a comment.Feb 1 2019, 6:25 AM

@zmike
Ok ~ I got your point well, and also check T7436 together.
Thank you !

Sorry, this was the wrong ticket. I meant T7463.

In T5719#130622, @zmike wrote:

For getting a detailed view to what is going on, please clone https://github.com/marcelhollerbach/efl-utility.

Build the project using meson.

then run EOLIAN_CLASS_UNIMPLEMENTED_WARN=1 ./efl-check <path-to-efl>/src/lib

This will output a huge amount of eolian error data. pipe this into a file using
EOLIAN_CLASS_UNIMPLEMENTED_WARN=1 ./efl-check <path-to-efl>/src/lib 2> ./error

after that you can run the script ´./efl-missing-api ./error´ this will print the structured output to a file.

I created an issue ticket since this doesn't build from git. Also when running efl-check I get eolian: could not parse files. This is on a completely clean efl tree.

This message is ignored by the script, and cannot be avoided, since eolian errors out on such errors, and does not just warn.
The problem with the dependency is fixed.

zmike added a comment.Feb 1 2019, 11:14 AM

So the error message is normal and occurs at the end of the run? Just checking, since I thought it meant that the run was prematurely aborted.

Yes exactly.

So what is happening is that eolian is trying to parse the classes that result from parsing the .eo files in its search tree. Those classes can have errors like the unimplemented API, so if this orrurse then its printing an error. And after that, it will continue validating the next class. If it recognizes in the end that there was an error, then it will print the message that you refered to above :)