On 10/1/19 10:09 AM, Tom Hacohen 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?
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.
Lets say you have a situation in a binded language where you have stored
a reference to this struct in a variable, someone else frees it. What is
now happening when you access this variable in the binded language ? How
can the binded language protect against that while keeping the idea of
having it as a lightweight handle. (The problem is basically classic use
after free, with 2 people having a ref to the struct).
For normal eo objects this is solved by invalidate etc. etc. If we now
start to use structs like this, we also need a solution to that problem
for the binded languages, which basically invalidates the argument of
them beeing leightweight handles.
My suggested alternative is just making them a normal eo object, OR
making it not a struct pointer you return but rather some unique ID,
which does not have the problem of unprotected accessing (Definitly the
more leightweight option.)
- 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.
Okay, and a handle that is not assosiated with a text object is just
waiting for complete deletion ?
- 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.
Oh okay, that was not clear to me.
- 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?
As in: I do not understand what the different properties do result in.
- 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.
If each item_factory belongs to exactly one Efl2.Text_Markup object.
Then the factory should probebly be owned by the Efl2.Text_Markup object
in order to support correct destruction etc.
- efl2_text_item_factory:
- Again, why having a struct here, same as above applies to this.
Same answer as before. :)
see above ... :P
- 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.
Sounds like a thing for efl.layout ?
- 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?
I think you replaced all of them in the branch, cannot find them right now.
Something else i have spotted while taking a second look:
- you can remove the ptr(...) stuff in iterator<ptr(...)> as structs in
iterators are passed by reference implicitly.
Greetings,
bu5hm4n
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
