Implement TreeIter manually to allow instance creation and storage of i32

[felinira]

This would in theory allow for TreeModel subclassing. I am not completely happy with how it turned out because FromGlibPtrBorrow copies the struct data. I did not really find a better alternative though.

[bilelmoussaoui]

This would in theory allow for TreeModel subclassing.

Is that something you plan to work on? Most of the code is already in #169, it could use some testing & an example

[felinira]

This would in theory allow for TreeModel subclassing.

Is that something you plan to work on? Most of the code is already in #169, it could use some testing & an example

I was looking into it at least. As TreeIter was the biggest stumbling block from that issue I concentrated on that first. I might even have a use case for custom TreeModels. Even though it is a very complex interface and definitely not "fun" to implement ^^

[bilelmoussaoui]

What I would suggest here:

Try to make use of the TreeModel subclassing api in #169 and see what are the use cases you have for TreeIter. That would help defining an API based on a use case. We can iter on it in the future if needed after that.

[felinira]

I don't really know how this API is supposed to be used in C when implementing your own tree model, but like this the implementation is unsound in Rust.

You'd need to find a way to not store any owned data inside the iter but without that potentially causing dangling pointers when used from safe code.

The model is expected to handle the lifetime of the TreeIter. All TreeIters get invalidated when you change the model in any way. If you use GTK_TREE_MODEL_ITERS_PERSIST as a flag a TreeIter is only invalidated when it is actually removed.

You can't really control who has references to your TreeIters, they are not GObjects so they are not reference counted which makes this unsafe to use. I should have seen it earlier that you can't really know who still has references to your TreeIters but didn't really think it through. This whole API is a giant use after free nightmare. So I do come to the (sad) conclusion that it seems to be impossible to store references in the pointer fields.

This also means the way of just taking the TreeIter references from C and transparently casting them is unsafe too. I'm not completely sure how Boxed works and if it catches these kinds of issues but if I am not mistaken even the current auto implementation is unsafe too because if you store a reference to the TreeIter anywhere and the model frees it you have use after free.

So the safest way is probably to make the struct Copy.

What I would suggest here:

Try to make use of the TreeModel subclassing api in #169 and see what are the use cases you have for TreeIter. That would help defining an API based on a use case. We can iter on it in the future if needed after that.

I tried using the indextree crate. The problem is you need some way to store a NodeId or a reference to one of those. As you can't store the struct directly you probably need to create a HashMap or a Vec and store your object there and just store indexes in the TreeIter.

So after thinking about it a bit the only safe way I can think of is just storing an integer in the user_data pointer (or even using the stamp field for it and leaving user_data completely alone). Then you store all the stuff you need elsewhere as this data structure sadly is not really capable of doing anything remotely useful in a safe way.

I will redo the whole thing and just allow it to store integers. At least they can't betray you in the way pointers can^^.

[sdroege]

The stamp probably makes sense to keep, and you could expose the pointers as usizes. The idea from a Rust point of view here would be that you'd have to give each of your items a unique identifier (the usizes) and the stamp would be used to notice when the iterator is stale (you'd keep a stamp internally too and on every change increase it, and on iterator usage check if the iterator has the latest stamp).

For the general memory unsafety here, can you reproduce a invalid memory access or similar with just using the existing Rust bindings around this? That would be bad and would require further investigation there, but also there's limits to what we can do. See https://github.com/gtk-rs/gtk3-rs/issues/333