Hi,
On Tue, Oct 1, 2019 at 4:10 AM Tom Hacohen <[email protected]> wrote:
> Hey,
>
> Sorry for the delay, and thanks for your feedback! Comments are inline.
>
> Just one thing though, while you provided a lot of very valuable
> comments, you haven't solved my issue with splitting interfaces, which
> is the main thing blocking my work ("the advice"). Any comments on that?
>
Yes, we'll need to split Efl.Canvas.Text into interfaces/mixins to solve
this issue. I've created an API ticket (https://phab.enlightenment.org/T8296)
where we can directly discuss the API here as well as how best to split it.
>
>
>
> On 27/09/2019 15:22, Marcel Hollerbach wrote:
> > Hi,
> >
> > after a long read:
>
> Sorry, there's a lot there. :)
>
> > - efl2_text_attribute_factory:
> > - Why having a struct here as handle, i fear that bindings will have
> > a very hard time with this, where you actually pass in a struct to the
> > remove method, and this one is dead after it.
>
> What's your suggested alternative? Anyhow, why would bindings have a
> hard time? Structs have associated free functions and ref/unref, no? I'm
> happy with an alternative, but I'd need a lightweight handle for this.
>
> > - Its also not clear to me how explicit remove plays well with unref.
>
> Remove: removes it from the text object. This means the handle is still
> alive, just disassociated from the text object so it doesn't affect it
> anymore.
>
> Unref: reduces the refcount in case you reffed it before. Used for when
> you kept your own copies.
>
> > - the `insert` method should return a owned struct handle ? Or is
> > the user never really owning any of the structs there ?
>
> It's not owned. If you want to use it, you need to immediately ref it
> after it's returned from there. It's done this way so you can safely
> ignore the return value when you don't want it, but also use it when you
> do.
>
> > - efl2_text_font_properties / style_properties: (I have criticized the
> > names before, i like the new names.) However, hard to comment more, as i
> > do not know much about text property stuff itself.
>
> No worries, these ones are actually less of a problem for me.
>
> > - efl2_text_wrap_properties: Can you document what impact ellipsis and
> > wrap do have ?
>
> As in: you have questions, or just commenting about the lack of docs?
>
> > - efl2_text_markup:
> > - I am not entirely sure how item_factory works here. Is the
> > item_factory knowing to which Efl2.Text_Markup they belong ? If so,
> > shoudnt that be expressed somehow ?
>
> Not sure I understand your question.
>
> > - efl2_text_item_factory:
> > - Again, why having a struct here, same as above applies to this.
>
> Same answer as before. :)
>
> > - efl2_text_raw_editable:
> > - Where does this object belong ? We normally only have objects in
> > efl.canvas / ui / layout. But not in efl. itself.
>
> Happy to rename it, though dunno what to. It's somewhere between canvas
> and ui. It's above canvas as it handles input and deals with X, but
> below UI because it doesn't have a theme.
>
This is a little tricky.
If it's in Efl.Ui namespace then most likely it should inherit from
Efl.Ui.Widget or else it becomes a strange outlier case and is the only
class in the namespace (other than the widget_parts) to not do so.
I think if it's dealing with display servers then it has to be in this
namespace, however. It seems like this is an internal-only class? There's
no explicit description in its documentation, so I'm just speculating. We
do have other internal objects here, so that would make sense.
>
> > - Is it doing undo/redo or only emitting the events ?
>
> At the moment just events, but Ali is going to fix this.
>
> > - efl2_ui_text:
> > - Why are here cnp related events ? Isnt that just a normal
> > insertion / copy operation ? The cut and copy operation could be on a
> > object that handles cnp, the paste operation as well, paired with a
> > changed,user event?
>
> Fair comment, need to re-evaluate that.
>
> >
> > General notes:
> > - you used a lot of ptr(...), you cannot use them when you remove @beta
> > from the file. These files should not use ptr, nor void_ptr.
>
> I know. :(
> I created my branch before there was an alternative (very recently), and
> haven't updated anything yet.
>
> > - You have a few places where you have explicitly x,y,w,h (x,y) in your
> > API, can you replace them with rect (position) handles from eina, that
> > would make it more aligned with the rest of efl.
> >
>
> Could you please provide an example?
>
> Thanks!
>
> --
> Tom
>
> > Greetings,
> > bu5hm4n
> >
> > On 9/19/19 4:15 PM, Tom Hacohen wrote:
> >> Hey,
> >>
> >> As most of you (at least the IRC lurkers) know, I've been recently
> >> working on the text interfaces. Trying to clean them up and stabilise
> >> them.
> >> The discussion and work has been covered on phab at:
> >> https://phab.enlightenment.org/T8151
> >>
> >> And the new (suggested) interfaces are all the files starting with
> >> "efl2_" in my branch:
> >> https://git.enlightenment.org/core/efl.git/tree/?h=devs/tasn/ifaces
> >>
> >> I'd love to get your feedback and let me know if there's anything I've
> >> missed. All feedback is welcomed, including bike shedding.
> >> Some interfaces still have massive FIXMEs at the top, so obviously read
> >> those before replying to avoid repeating what we already know.
> >>
> >>
> >> As for the advice I mentioned in the title: due to composite object
> >> regressions as described in T8184, I'm forced to break up the classes
> >> into interfaces. As discussed at length in the ticket, these interfaces
> >> would have to be very specific to the classes and not really reusable
> >> ("cursor_new" is quite specific, obviously).
> >>
> >> I can either just do as I said in the ticket, and for every class do a
> >> big interface, so Efl.Canvas.Text -> Efl.Canvas.Text +
> >> Efl.Canvas.Text_Interface. This is one way. It's obviously very ugly.
> >> The other way is to split to a lot of smaller, probably 1/2 property
> >> interfaces, which is also ugly and quite inefficient (classes/interfaces
> >> are not free).
> >>
> >> I'd love to get your input, to what interfaces would you split up these
> >> two classes:
> >> 1.
> >>
> https://git.enlightenment.org/core/efl.git/tree/src/lib/evas/canvas/efl2_canvas_text.eo?h=devs/tasn/ifaces
> >>
> >> 2.
> >>
> https://git.enlightenment.org/core/efl.git/tree/src/lib/elementary/efl2_text_raw_editable.eo?h=devs/tasn/ifaces
> >>
> >>
> >>
> >> Thanks a lot for your help and feedback!
> >>
> >> --
> >> Tom
> >>
> >>
> >> _______________________________________________
> >> enlightenment-devel mailing list
> >> [email protected]
> >> https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
> >>
> >
> >
> > _______________________________________________
> > enlightenment-devel mailing list
> > [email protected]
> > https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
>
>
> _______________________________________________
> enlightenment-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
>
_______________________________________________
enlightenment-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
