Hey, On 03/10/2019 13:31, Marcel Hollerbach wrote: >> >>> - 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).
I don't understand the problem you're describing. How can it be freed without the bindings knowing about it if they are holding a ref to it? > > 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. Not sure what you mean by "invalidate", but I'd expect this to be solved with eo objects using refcounts too. > > 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.) Making them normal eo objects is not really an option (as the whole point is them being lightweight). Having them as a unique ID is the same as having them as a pointer. It's just a matter of dealing with it differently internally (is it actually a pointer we deref or some value we analyse). So I'm not sure how is it any different from an API perspective. I'm actually changing this though, as discussed in a few phab tickets over the last week+, so this discussion is less relevant I guess. > >> >>> - 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 ? Yeah, or you can re-attach it to an object. > >> >>> - 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. Wrap does text wrapping and ellipsis does ellipsis. I'm not sure what your question is if you're not just stating that docs should be better. These too are very common and well defined terms. > >>> - 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. It's not the case, item_factories can be used multiple times, in tandem, and probably should, as it should just be a singleton. As above though, this is going to change a bit. > >> >>> - efl2_text_item_factory: >>> - Again, why having a struct here, same as above applies to this. >> >> Same answer as before. :) > > see above ... :P > Right back at ya. >> >>> - 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 ? It's not a layout though, or does efl.layout also include objects that don't layout things. > >> >>> - 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. Ahh, yes, I since understood what you were talking about and fixed it. Thanks. > > 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. Thanks for the comment, I'll fix it! > > 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 >>>> enlightenment-devel@lists.sourceforge.net >>>> https://lists.sourceforge.net/lists/listinfo/enlightenment-devel >>>> >>> >>> >>> _______________________________________________ >>> enlightenment-devel mailing list >>> enlightenment-devel@lists.sourceforge.net >>> https://lists.sourceforge.net/lists/listinfo/enlightenment-devel _______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
