Page MenuHomePhabricator

Remove efl_canvas_text.eo and keep the only the legacy API for textblock
Closed, ResolvedPublic

Description

As the title says. This interface should be removed and textblock should just work without it. Check if it's used elsewhere in the inheritance tree and if it breaks existing widgets. If so, it should probably be kept, but the .eo file and class name renamed. It should be marked as internal and etc.

I'm going to use this .eo filename and implement this class, so it'll cause a clash if this is not split out.

tasn created this task.Aug 30 2019, 12:31 PM
woohyun added a comment.EditedSep 1 2019, 10:46 PM

I'm confused the work details for this task.

  1. Do you want to remove whole efl_canvas_text.eo file from Master (or your branch)?
    • Currently, it is used by "Efl.Ui.Internal.Text.Interactive" (which is core composite instance for efl_ui_text), and without that we will not have any text widget class till you complete the new classes.
  1. Remove efl_canvas_text interface from evas_object_textblock.c codes.
    • I don't know what is the effect of this work. There are only few use of efl_canvas_text interface inside the textblock codes.

@tasn
Could you make the tasks a bit clear ? I wish you will give a description something like -

1, Remove efl_canvas_text.eo
    (just change it with efl_canvas_temp_text.eo)
2. Make "Efl.Ui.Internal.Text.Interactive" extends 
     this "Efl.Canvas.Text.Text" till the new text_canvas compeletiong
3. ... and so on.
tasn added a comment.Sep 2 2019, 1:09 AM

Everything is in my branch... All the requests I'm making.

  1. Yeah, that would be step two. Remove all the classes (again, they can be kept and renamed to a legacy name if fine with the rest of the EFL people) so I can use these class names. The idea is that existing widgets will just use legacy APIs and that the new one will use the new Eo API I'm working on.
  1. Not quite, there are *a lot*. Just look for "EOLIAN", all of this object is based on the eolian.

I'm happy to further elaborate, but I think you understood it exactly as it is. Another shot at rephrasing:

  1. Delete all of the existing .eo files that depend on Efl.Canvas.Text + everything that depends on it.
  2. Make sure all the legacy APIs still work after those are removed.

Does that make it clearer?

@tasn

I got it for now :)
If I have more questions, while doing this work -
then I'll disturb you again ! :)

I'll make a patch soon on your branch.

tasn added a comment.Sep 2 2019, 8:53 AM

I think it would be easier to just maintain your own branch. So just have your own branch and let me know. It doesn't even need to be based on mine. :)

Thanks!

I'm doing my work in "devs/woohyun/remove_canvas_text".

After doing some works - I will share the direction of my work.

If it looks wrong ~ please share your idea :)

tasn added a comment.Sep 3 2019, 12:54 AM

Sounds great, thanks!

@tasn

I have tried to remove efl_canvas_text.eo perfectly (including its dependency),
but there were lots of dependencies (as you expected) and I failed to do that.

So, I simply changed the name from "efl_canvas_text.eo" to "efl_canvas_textblock.eo" temporarily.

At this point - I'm wondering two things.

  1. Are there any other classes that I need to change more ?
  2. You will not use the implementations of "evas_object_textblock.c" for your new classes. Right ? (I did check efl_canvas_text.c in your branch - so, I assumed you would use this)
  3. I don't know well how to make specific eo class to internal one. Do you have any idea on this ?
tasn added a comment.Sep 3 2019, 6:13 AM

I think what you did is good enough for now. I'm have no problem with keeping a legacy internal .eo file if it's too hard to remove it.

  1. Nothing that comes to mind at the moment.
  2. No, I will not. It's going to be significantly faster to not have to worry about legacy, and to be honest, a lot will change anyway, so I think that in the end, there won't be a lot of shared code anyway. I'll try to share what I can after the fact.
  3. I also have no idea. Take a look at elm (or evas?) I think there are a few files named *internal.eo. No idea if they are really internal. Maybe if we call it evas_textblock_legacy.eo it's obvious it shouldn't be used? No idea, really...

In my branch (devs/woohyun/remove_canvas_text), I updated two commits.

e3b2d3e62a909ae4110e2a0983aa7f90ae1b0cb1 (efl_canvas_text -> efl_canvas_textblock)
6cb887a8bafb8ff225020575679bbbd7abb29305 (efl_canvas_textblock -> evas_textblock_legacy)

Get one or both depending on your preference :)

tasn added a comment.EditedSep 4 2019, 1:27 AM

Thanks, will take a look soon. Hopefully today!
Was much quicker than expected! :)

In T8194#141617, @tasn wrote:

Thanks, will take a look soon. Hopefully today!
Was much quicker than expected! :)

If I'm wrong in any point, let me know ~ I'll try to modify it asap.

tasn closed this task as Resolved.Oct 2 2019, 2:46 AM

Merged, thanks!

(I squashed your commits btw, as they seem to just be renaming in two steps).